From 6d116ae19e7c3e84377f0657b20b26bf31aa45e6 Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Tue, 6 Aug 2024 18:22:44 -0700 Subject: [PATCH 1/4] Fix accidental relative imports from non namespace barrels --- .../fixMissingTypeAnnotationOnExports.ts | 21 +++++++++---------- src/services/pasteEdits.ts | 16 +++++++------- src/services/refactors/moveToFile.ts | 10 +++++---- src/services/stringCompletions.ts | 3 +-- 4 files changed, 24 insertions(+), 26 deletions(-) diff --git a/src/services/codefixes/fixMissingTypeAnnotationOnExports.ts b/src/services/codefixes/fixMissingTypeAnnotationOnExports.ts index db117935dea2d..deea661f2e72a 100644 --- a/src/services/codefixes/fixMissingTypeAnnotationOnExports.ts +++ b/src/services/codefixes/fixMissingTypeAnnotationOnExports.ts @@ -1,3 +1,12 @@ +import { + createCodeFixAction, + createCombinedCodeActions, + createImportAdder, + eachDiagnostic, + registerCodeFix, + typePredicateToAutoImportableTypeNode, + typeToAutoImportableTypeNode, +} from "../_namespaces/ts.codefix.js"; import { ArrayBindingPattern, ArrayLiteralExpression, @@ -97,17 +106,7 @@ import { VariableStatement, walkUpParenthesizedExpressions, } from "../_namespaces/ts.js"; - -import { - createCodeFixAction, - createCombinedCodeActions, - createImportAdder, - eachDiagnostic, - registerCodeFix, - typePredicateToAutoImportableTypeNode, - typeToAutoImportableTypeNode, -} from "../_namespaces/ts.codefix.js"; -import { getIdentifierForNode } from "../refactors/helpers.js"; +import { getIdentifierForNode } from "../_namespaces/ts.refactor.js"; const fixId = "fixMissingTypeAnnotationOnExports"; diff --git a/src/services/pasteEdits.ts b/src/services/pasteEdits.ts index f3b350816781e..e39a51bf45076 100644 --- a/src/services/pasteEdits.ts +++ b/src/services/pasteEdits.ts @@ -1,22 +1,20 @@ -import { findIndex } from "../compiler/core.js"; import { CancellationToken, - Program, - SourceFile, - Statement, - SymbolFlags, - TextRange, - UserPreferences, -} from "../compiler/types.js"; -import { codefix, Debug, fileShouldUseJavaScriptRequire, + findIndex, forEachChild, formatting, getQuotePreference, isIdentifier, + Program, + SourceFile, + Statement, + SymbolFlags, textChanges, + TextRange, + UserPreferences, } from "./_namespaces/ts.js"; import { addTargetFileImports } from "./refactors/helpers.js"; import { diff --git a/src/services/refactors/moveToFile.ts b/src/services/refactors/moveToFile.ts index da6d806a06f30..85c040013edcb 100644 --- a/src/services/refactors/moveToFile.ts +++ b/src/services/refactors/moveToFile.ts @@ -1,4 +1,3 @@ -import { getModuleSpecifier } from "../../compiler/_namespaces/ts.moduleSpecifiers.js"; import { ApplicableRefactorInfo, arrayFrom, @@ -118,6 +117,7 @@ import { ModifierLike, ModuleDeclaration, ModuleKind, + moduleSpecifiers, moduleSpecifierToValidIdentifier, NamedImportBindings, Node, @@ -157,8 +157,10 @@ import { VariableDeclarationList, VariableStatement, } from "../_namespaces/ts.js"; -import { addTargetFileImports } from "../_namespaces/ts.refactor.js"; -import { registerRefactor } from "../refactorProvider.js"; +import { + addTargetFileImports, + registerRefactor, +} from "../_namespaces/ts.refactor.js"; const refactorNameForMoveToFile = "Move to file"; const description = getLocaleSpecificMessage(Diagnostics.Move_to_file); @@ -358,7 +360,7 @@ function updateImportsInOtherFiles( if (getStringComparer(!program.useCaseSensitiveFileNames())(pathToTargetFileWithExtension, sourceFile.fileName) === Comparison.EqualTo) return; - const newModuleSpecifier = getModuleSpecifier(program.getCompilerOptions(), sourceFile, sourceFile.fileName, pathToTargetFileWithExtension, createModuleSpecifierResolutionHost(program, host)); + const newModuleSpecifier = moduleSpecifiers.getModuleSpecifier(program.getCompilerOptions(), sourceFile, sourceFile.fileName, pathToTargetFileWithExtension, createModuleSpecifierResolutionHost(program, host)); const newImportDeclaration = filterImport(importNode, makeStringLiteral(newModuleSpecifier, quotePreference), shouldMove); if (newImportDeclaration) changes.insertNodeAfter(sourceFile, statement, newImportDeclaration); diff --git a/src/services/stringCompletions.ts b/src/services/stringCompletions.ts index a26959d57f2dc..667cdf24ca8aa 100644 --- a/src/services/stringCompletions.ts +++ b/src/services/stringCompletions.ts @@ -1,4 +1,3 @@ -import { getModuleSpecifierPreferences } from "../compiler/moduleSpecifiers.js"; import { CompletionKind, createCompletionDetails, @@ -823,7 +822,7 @@ function getFilenameWithExtensionOption(name: string, program: Program, extensio return { name, extension: tryGetExtensionFromPath(name) }; } - let allowedEndings = getModuleSpecifierPreferences( + let allowedEndings = moduleSpecifiers.getModuleSpecifierPreferences( { importModuleSpecifierEnding: extensionOptions.endingPreference }, program, program.getCompilerOptions(), From 2a3f287c7aae6dca16d6ae2baaab77295235c05c Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Wed, 7 Aug 2024 17:21:42 -0700 Subject: [PATCH 2/4] Add a lint rule based on the DT rule with the same vibe --- eslint.config.mjs | 8 +++ scripts/eslint/rules/no-direct-import.cjs | 86 +++++++++++++++++++++++ 2 files changed, 94 insertions(+) create mode 100644 scripts/eslint/rules/no-direct-import.cjs diff --git a/eslint.config.mjs b/eslint.config.mjs index 13729d3ee83be..f2566c3b631f8 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -210,12 +210,20 @@ export default tseslint.config( { name: "clearImmediate" }, { name: "performance" }, ], + "local/no-direct-import": "error", }, }, { files: ["src/harness/**", "src/testRunner/**"], rules: { "no-restricted-globals": "off", + "local/no-direct-import": "off", + }, + }, + { + files: ["src/**/_namespaces/**"], + rules: { + "local/no-direct-import": "off", }, }, { diff --git a/scripts/eslint/rules/no-direct-import.cjs b/scripts/eslint/rules/no-direct-import.cjs new file mode 100644 index 0000000000000..6e16c3e284129 --- /dev/null +++ b/scripts/eslint/rules/no-direct-import.cjs @@ -0,0 +1,86 @@ +const { createRule } = require("./utils.cjs"); +const path = require("path"); + +/** @import { TSESTree } from "@typescript-eslint/utils" */ +void 0; + +module.exports = createRule({ + name: "no-direct-import", + meta: { + docs: { + description: ``, + }, + messages: { + noDirectImport: `This import relatively references another project; did you mean to import from a local _namespace module?`, + }, + schema: [], + type: "problem", + }, + defaultOptions: [], + + create(context) { + const containingFileName = context.filename; + const containingDirectory = path.dirname(containingFileName); + + /** @type {(node: TSESTree.ImportDeclaration | TSESTree.TSImportEqualsDeclaration) => void} */ + const check = node => { + let source; + if (node.type === "TSImportEqualsDeclaration") { + const moduleReference = node.moduleReference; + if ( + moduleReference.type === "TSExternalModuleReference" + && moduleReference.expression.type === "Literal" + && typeof moduleReference.expression.value === "string" + ) { + source = moduleReference.expression; + } + } + else { + source = node.source; + } + + if (!source) { + return; + } + + const p = source.value; + + if (p.endsWith("../typescript/typescript.js")) { + // These appear in place of public API imports. + return; + } + + // The below is similar to https://github.com/microsoft/DefinitelyTyped-tools/blob/main/packages/eslint-plugin/src/rules/no-bad-reference.ts + + // Perf trick; if a path doesn't have ".." anywhere, then it can't have resolved + // up and out of a package dir so we can skip this work. + if (p.includes("..")) { + const parts = p.split("/"); + let cwd = containingDirectory; + for (const part of parts) { + if (part === "" || part === ".") { + continue; + } + if (part === "..") { + cwd = path.dirname(cwd); + } + else { + cwd = path.join(cwd, part); + } + + if (path.basename(cwd) === "src") { + context.report({ + messageId: "noDirectImport", + node: source, + }); + } + } + } + }; + + return { + ImportDeclaration: check, + TSImportEqualsDeclaration: check, + }; + }, +}); From 126e1684bd8e2594fe7bb77d0ee70bf6017edeb1 Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Wed, 7 Aug 2024 17:24:16 -0700 Subject: [PATCH 3/4] Refactor --- scripts/eslint/rules/no-direct-import.cjs | 51 ++++++++++------------- 1 file changed, 23 insertions(+), 28 deletions(-) diff --git a/scripts/eslint/rules/no-direct-import.cjs b/scripts/eslint/rules/no-direct-import.cjs index 6e16c3e284129..6bd1706f47488 100644 --- a/scripts/eslint/rules/no-direct-import.cjs +++ b/scripts/eslint/rules/no-direct-import.cjs @@ -39,41 +39,36 @@ module.exports = createRule({ source = node.source; } - if (!source) { - return; - } + if (!source) return; const p = source.value; - if (p.endsWith("../typescript/typescript.js")) { - // These appear in place of public API imports. - return; - } + // These appear in place of public API imports. + if (p.endsWith("../typescript/typescript.js")) return; // The below is similar to https://github.com/microsoft/DefinitelyTyped-tools/blob/main/packages/eslint-plugin/src/rules/no-bad-reference.ts - // Perf trick; if a path doesn't have ".." anywhere, then it can't have resolved - // up and out of a package dir so we can skip this work. - if (p.includes("..")) { - const parts = p.split("/"); - let cwd = containingDirectory; - for (const part of parts) { - if (part === "" || part === ".") { - continue; - } - if (part === "..") { - cwd = path.dirname(cwd); - } - else { - cwd = path.join(cwd, part); - } + // Any relative path that goes to the wrong place will contain "..". + if (!p.includes("..")) return; + + const parts = p.split("/"); + let cwd = containingDirectory; + for (const part of parts) { + if (part === "" || part === ".") { + continue; + } + if (part === "..") { + cwd = path.dirname(cwd); + } + else { + cwd = path.join(cwd, part); + } - if (path.basename(cwd) === "src") { - context.report({ - messageId: "noDirectImport", - node: source, - }); - } + if (path.basename(cwd) === "src") { + context.report({ + messageId: "noDirectImport", + node: source, + }); } } }; From 21821db2ad8bd4c1250569962d4e29361a7d8ae8 Mon Sep 17 00:00:00 2001 From: Jake Bailey <5341706+jakebailey@users.noreply.github.com> Date: Wed, 7 Aug 2024 18:42:36 -0700 Subject: [PATCH 4/4] fmt --- scripts/eslint/rules/no-direct-import.cjs | 162 +++++++++++----------- 1 file changed, 81 insertions(+), 81 deletions(-) diff --git a/scripts/eslint/rules/no-direct-import.cjs b/scripts/eslint/rules/no-direct-import.cjs index 6bd1706f47488..614e67db30be3 100644 --- a/scripts/eslint/rules/no-direct-import.cjs +++ b/scripts/eslint/rules/no-direct-import.cjs @@ -1,81 +1,81 @@ -const { createRule } = require("./utils.cjs"); -const path = require("path"); - -/** @import { TSESTree } from "@typescript-eslint/utils" */ -void 0; - -module.exports = createRule({ - name: "no-direct-import", - meta: { - docs: { - description: ``, - }, - messages: { - noDirectImport: `This import relatively references another project; did you mean to import from a local _namespace module?`, - }, - schema: [], - type: "problem", - }, - defaultOptions: [], - - create(context) { - const containingFileName = context.filename; - const containingDirectory = path.dirname(containingFileName); - - /** @type {(node: TSESTree.ImportDeclaration | TSESTree.TSImportEqualsDeclaration) => void} */ - const check = node => { - let source; - if (node.type === "TSImportEqualsDeclaration") { - const moduleReference = node.moduleReference; - if ( - moduleReference.type === "TSExternalModuleReference" - && moduleReference.expression.type === "Literal" - && typeof moduleReference.expression.value === "string" - ) { - source = moduleReference.expression; - } - } - else { - source = node.source; - } - - if (!source) return; - - const p = source.value; - - // These appear in place of public API imports. - if (p.endsWith("../typescript/typescript.js")) return; - - // The below is similar to https://github.com/microsoft/DefinitelyTyped-tools/blob/main/packages/eslint-plugin/src/rules/no-bad-reference.ts - - // Any relative path that goes to the wrong place will contain "..". - if (!p.includes("..")) return; - - const parts = p.split("/"); - let cwd = containingDirectory; - for (const part of parts) { - if (part === "" || part === ".") { - continue; - } - if (part === "..") { - cwd = path.dirname(cwd); - } - else { - cwd = path.join(cwd, part); - } - - if (path.basename(cwd) === "src") { - context.report({ - messageId: "noDirectImport", - node: source, - }); - } - } - }; - - return { - ImportDeclaration: check, - TSImportEqualsDeclaration: check, - }; - }, -}); +const { createRule } = require("./utils.cjs"); +const path = require("path"); + +/** @import { TSESTree } from "@typescript-eslint/utils" */ +void 0; + +module.exports = createRule({ + name: "no-direct-import", + meta: { + docs: { + description: ``, + }, + messages: { + noDirectImport: `This import relatively references another project; did you mean to import from a local _namespace module?`, + }, + schema: [], + type: "problem", + }, + defaultOptions: [], + + create(context) { + const containingFileName = context.filename; + const containingDirectory = path.dirname(containingFileName); + + /** @type {(node: TSESTree.ImportDeclaration | TSESTree.TSImportEqualsDeclaration) => void} */ + const check = node => { + let source; + if (node.type === "TSImportEqualsDeclaration") { + const moduleReference = node.moduleReference; + if ( + moduleReference.type === "TSExternalModuleReference" + && moduleReference.expression.type === "Literal" + && typeof moduleReference.expression.value === "string" + ) { + source = moduleReference.expression; + } + } + else { + source = node.source; + } + + if (!source) return; + + const p = source.value; + + // These appear in place of public API imports. + if (p.endsWith("../typescript/typescript.js")) return; + + // The below is similar to https://github.com/microsoft/DefinitelyTyped-tools/blob/main/packages/eslint-plugin/src/rules/no-bad-reference.ts + + // Any relative path that goes to the wrong place will contain "..". + if (!p.includes("..")) return; + + const parts = p.split("/"); + let cwd = containingDirectory; + for (const part of parts) { + if (part === "" || part === ".") { + continue; + } + if (part === "..") { + cwd = path.dirname(cwd); + } + else { + cwd = path.join(cwd, part); + } + + if (path.basename(cwd) === "src") { + context.report({ + messageId: "noDirectImport", + node: source, + }); + } + } + }; + + return { + ImportDeclaration: check, + TSImportEqualsDeclaration: check, + }; + }, +});