From fce282b5bd7a15a09ee19238018dbc3871365cf5 Mon Sep 17 00:00:00 2001 From: Daniel Rosenwasser Date: Thu, 13 Jan 2022 21:54:16 +0000 Subject: [PATCH 1/6] Add failing test. --- .../importFixesWithExistingDottedRequire.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 tests/cases/fourslash/importFixesWithExistingDottedRequire.ts diff --git a/tests/cases/fourslash/importFixesWithExistingDottedRequire.ts b/tests/cases/fourslash/importFixesWithExistingDottedRequire.ts new file mode 100644 index 0000000000000..6c9d07c1c53b1 --- /dev/null +++ b/tests/cases/fourslash/importFixesWithExistingDottedRequire.ts @@ -0,0 +1,14 @@ +/// + +// @allowJs: true +// @module: commonjs +// @Filename: /library.js +//// export function aaa() {} +//// export function bbb() {} + +// @Filename: /foo.js +//// var a = require("./library").aaa; +//// bbb/**/ + +goTo.marker(); +verify.codeFixAvailable(); From ea2c290ba8d71ce4949bf400004199d4f7fe088b Mon Sep 17 00:00:00 2001 From: Daniel Rosenwasser Date: Thu, 13 Jan 2022 22:49:41 +0000 Subject: [PATCH 2/6] Update failing test. --- .../importFixesWithExistingDottedRequire.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/cases/fourslash/importFixesWithExistingDottedRequire.ts b/tests/cases/fourslash/importFixesWithExistingDottedRequire.ts index 6c9d07c1c53b1..fcfaafa4d11c8 100644 --- a/tests/cases/fourslash/importFixesWithExistingDottedRequire.ts +++ b/tests/cases/fourslash/importFixesWithExistingDottedRequire.ts @@ -1,14 +1,14 @@ /// // @allowJs: true -// @module: commonjs // @Filename: /library.js -//// export function aaa() {} -//// export function bbb() {} +//// module.exports.aaa = function() {} +//// module.exports.bbb = function() {} // @Filename: /foo.js -//// var a = require("./library").aaa; -//// bbb/**/ +//// var aaa = require("./library.js").aaa; +//// aaa(); +//// /**/bbb goTo.marker(); -verify.codeFixAvailable(); +verify.getAndApplyCodeFix(ts.Diagnostics.Cannot_find_name_0.code); \ No newline at end of file From d9549483949276f0550c4b430b312957ee304cf7 Mon Sep 17 00:00:00 2001 From: Daniel Rosenwasser Date: Fri, 14 Jan 2022 01:00:42 +0000 Subject: [PATCH 3/6] Finalized failing test case. --- .../importFixesWithExistingDottedRequire.ts | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/tests/cases/fourslash/importFixesWithExistingDottedRequire.ts b/tests/cases/fourslash/importFixesWithExistingDottedRequire.ts index fcfaafa4d11c8..8dae9bdac8aed 100644 --- a/tests/cases/fourslash/importFixesWithExistingDottedRequire.ts +++ b/tests/cases/fourslash/importFixesWithExistingDottedRequire.ts @@ -1,14 +1,21 @@ /// -// @allowJs: true -// @Filename: /library.js +// @module: commonjs +// @checkJs: true + +// @Filename: ./library.js //// module.exports.aaa = function() {} //// module.exports.bbb = function() {} -// @Filename: /foo.js +// @Filename: ./foo.js //// var aaa = require("./library.js").aaa; //// aaa(); -//// /**/bbb +//// /*$*/bbb -goTo.marker(); -verify.getAndApplyCodeFix(ts.Diagnostics.Cannot_find_name_0.code); \ No newline at end of file +goTo.marker("$") +verify.codeFixAvailable([ + { description: "Import 'bbb' from module \"./library.js\"" }, + { description: "Ignore this error message" }, + { description: "Disable checking for this file" }, + { description: "Convert to ES module" }, +]); From f476e8426bc1dffe5ef82e672779337f3cb031dd Mon Sep 17 00:00:00 2001 From: Daniel Rosenwasser Date: Fri, 14 Jan 2022 01:06:45 +0000 Subject: [PATCH 4/6] Separate our usages of utilities that expect variables initialized to require(...) and require(...).foo. --- src/compiler/binder.ts | 4 ++-- src/compiler/checker.ts | 4 ++-- src/compiler/types.ts | 5 +++++ src/compiler/utilities.ts | 17 +++++++++++++++-- src/services/findAllReferences.ts | 2 +- src/services/goToDefinition.ts | 2 +- src/services/importTracker.ts | 2 +- 7 files changed, 27 insertions(+), 9 deletions(-) diff --git a/src/compiler/binder.ts b/src/compiler/binder.ts index eefc41fdaf8b5..4ae6a2bd31bf9 100644 --- a/src/compiler/binder.ts +++ b/src/compiler/binder.ts @@ -3274,7 +3274,7 @@ namespace ts { return isEnumConst(node) ? bindBlockScopedDeclaration(node, SymbolFlags.ConstEnum, SymbolFlags.ConstEnumExcludes) : bindBlockScopedDeclaration(node, SymbolFlags.RegularEnum, SymbolFlags.RegularEnumExcludes); - } + } function bindVariableDeclarationOrBindingElement(node: VariableDeclaration | BindingElement) { if (inStrictMode) { @@ -3282,7 +3282,7 @@ namespace ts { } if (!isBindingPattern(node.name)) { - if (isInJSFile(node) && isRequireVariableDeclaration(node) && !getJSDocTypeTag(node)) { + if (isInJSFile(node) && isAccessedOrBareRequireVariableDeclaration(node) && !getJSDocTypeTag(node)) { declareSymbolAndAddToSymbolTable(node as Declaration, SymbolFlags.Alias, SymbolFlags.AliasExcludes); } else if (isBlockOrCatchScoped(node)) { diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 578313bc7993c..84352d74cbfe6 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -2596,7 +2596,7 @@ namespace ts { && isAliasableOrJsExpression(node.parent.right) || node.kind === SyntaxKind.ShorthandPropertyAssignment || node.kind === SyntaxKind.PropertyAssignment && isAliasableOrJsExpression((node as PropertyAssignment).initializer) - || isRequireVariableDeclaration(node); + || isAccessedOrBareRequireVariableDeclaration(node); } function isAliasableOrJsExpression(e: Expression) { @@ -36984,7 +36984,7 @@ namespace ts { } // For a commonjs `const x = require`, validate the alias and exit const symbol = getSymbolOfNode(node); - if (symbol.flags & SymbolFlags.Alias && isRequireVariableDeclaration(node)) { + if (symbol.flags & SymbolFlags.Alias && isAccessedOrBareRequireVariableDeclaration(node)) { checkAliasSymbol(node); return; } diff --git a/src/compiler/types.ts b/src/compiler/types.ts index dcea2e7e38526..224fb27ef919a 100644 --- a/src/compiler/types.ts +++ b/src/compiler/types.ts @@ -4668,6 +4668,11 @@ namespace ts { readonly initializer: RequireOrImportCall; } + /* @internal */ + export interface AccessedOrBareRequireVariableDeclaration extends VariableDeclaration { + readonly initializer: RequireOrImportCall | AccessExpression; + } + /* @internal */ export interface RequireVariableStatement extends VariableStatement { readonly declarationList: RequireVariableDeclarationList; diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index 98d89d4612824..635bb02765aa5 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -2046,7 +2046,7 @@ namespace ts { } export function getExternalModuleRequireArgument(node: Node) { - return isRequireVariableDeclaration(node) && (getLeftmostAccessExpression(node.initializer) as CallExpression).arguments[0] as StringLiteral; + return isAccessedOrBareRequireVariableDeclaration(node) && (getLeftmostAccessExpression(node.initializer) as CallExpression).arguments[0] as StringLiteral; } export function isInternalModuleImportEqualsDeclaration(node: Node): node is ImportEqualsDeclaration { @@ -2114,10 +2114,23 @@ namespace ts { * This function does not test if the node is in a JavaScript file or not. */ export function isRequireVariableDeclaration(node: Node): node is RequireVariableDeclaration { + return isVariableDeclarationInitializedWithRequireHelper(node, /*allowAccessedRequire*/ false); + } + + /** + * Like `isRequireVariableDeclaration` but allows things like `require("...").foo.bar` or `require("...")["baz"]`. + */ + export function isAccessedOrBareRequireVariableDeclaration(node: Node): node is AccessedOrBareRequireVariableDeclaration { + return isVariableDeclarationInitializedWithRequireHelper(node, /*allowAccessedRequire*/ true); + } + + function isVariableDeclarationInitializedWithRequireHelper(node: Node, allowAccessedRequire: boolean) { if (node.kind === SyntaxKind.BindingElement) { node = node.parent.parent; } - return isVariableDeclaration(node) && !!node.initializer && isRequireCall(getLeftmostAccessExpression(node.initializer), /*requireStringLiteralLikeArgument*/ true); + return isVariableDeclaration(node) && + !!node.initializer && + isRequireCall(allowAccessedRequire ? getLeftmostAccessExpression(node.initializer) : node.initializer, /*requireStringLiteralLikeArgument*/ true); } export function isRequireVariableStatement(node: Node): node is RequireVariableStatement { diff --git a/src/services/findAllReferences.ts b/src/services/findAllReferences.ts index aebc52b2fb0f8..7dc65cee20689 100644 --- a/src/services/findAllReferences.ts +++ b/src/services/findAllReferences.ts @@ -1531,7 +1531,7 @@ namespace ts.FindAllReferences { // Use the parent symbol if the location is commonjs require syntax on javascript files only. if (isInJSFile(referenceLocation) && referenceLocation.parent.kind === SyntaxKind.BindingElement - && isRequireVariableDeclaration(referenceLocation.parent)) { + && isAccessedOrBareRequireVariableDeclaration(referenceLocation.parent)) { referenceSymbol = referenceLocation.parent.symbol; // The parent will not have a symbol if it's an ObjectBindingPattern (when destructuring is used). In // this case, just skip it, since the bound identifiers are not an alias of the import. diff --git a/src/services/goToDefinition.ts b/src/services/goToDefinition.ts index 9ae8dda40f84c..f73e54c6419d1 100644 --- a/src/services/goToDefinition.ts +++ b/src/services/goToDefinition.ts @@ -290,7 +290,7 @@ namespace ts.GoToDefinition { return declaration.parent.kind === SyntaxKind.NamedImports; case SyntaxKind.BindingElement: case SyntaxKind.VariableDeclaration: - return isInJSFile(declaration) && isRequireVariableDeclaration(declaration); + return isInJSFile(declaration) && isAccessedOrBareRequireVariableDeclaration(declaration); default: return false; } diff --git a/src/services/importTracker.ts b/src/services/importTracker.ts index d3c95854f9bbe..dc069f5e0b657 100644 --- a/src/services/importTracker.ts +++ b/src/services/importTracker.ts @@ -621,7 +621,7 @@ namespace ts.FindAllReferences { Debug.assert((parent as ImportClause | NamespaceImport).name === node); return true; case SyntaxKind.BindingElement: - return isInJSFile(node) && isRequireVariableDeclaration(parent); + return isInJSFile(node) && isAccessedOrBareRequireVariableDeclaration(parent); default: return false; } From 9f0810c26d965a7ff24b9332a2a52f672aa89a68 Mon Sep 17 00:00:00 2001 From: Daniel Rosenwasser Date: Wed, 19 Jan 2022 21:17:53 +0000 Subject: [PATCH 5/6] Renamed types and utilities, removed accidental indentation. --- src/compiler/binder.ts | 4 ++-- src/compiler/checker.ts | 4 ++-- src/compiler/types.ts | 8 +++----- src/compiler/utilities.ts | 4 ++-- src/services/findAllReferences.ts | 2 +- src/services/goToDefinition.ts | 2 +- src/services/importTracker.ts | 2 +- 7 files changed, 12 insertions(+), 14 deletions(-) diff --git a/src/compiler/binder.ts b/src/compiler/binder.ts index 4ae6a2bd31bf9..5e4ed7af65031 100644 --- a/src/compiler/binder.ts +++ b/src/compiler/binder.ts @@ -3274,7 +3274,7 @@ namespace ts { return isEnumConst(node) ? bindBlockScopedDeclaration(node, SymbolFlags.ConstEnum, SymbolFlags.ConstEnumExcludes) : bindBlockScopedDeclaration(node, SymbolFlags.RegularEnum, SymbolFlags.RegularEnumExcludes); - } + } function bindVariableDeclarationOrBindingElement(node: VariableDeclaration | BindingElement) { if (inStrictMode) { @@ -3282,7 +3282,7 @@ namespace ts { } if (!isBindingPattern(node.name)) { - if (isInJSFile(node) && isAccessedOrBareRequireVariableDeclaration(node) && !getJSDocTypeTag(node)) { + if (isInJSFile(node) && isVariableDeclarationInitializedToBareOrAccessedRequire(node) && !getJSDocTypeTag(node)) { declareSymbolAndAddToSymbolTable(node as Declaration, SymbolFlags.Alias, SymbolFlags.AliasExcludes); } else if (isBlockOrCatchScoped(node)) { diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 84352d74cbfe6..1a8b1d0fc50e4 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -2596,7 +2596,7 @@ namespace ts { && isAliasableOrJsExpression(node.parent.right) || node.kind === SyntaxKind.ShorthandPropertyAssignment || node.kind === SyntaxKind.PropertyAssignment && isAliasableOrJsExpression((node as PropertyAssignment).initializer) - || isAccessedOrBareRequireVariableDeclaration(node); + || isVariableDeclarationInitializedToBareOrAccessedRequire(node); } function isAliasableOrJsExpression(e: Expression) { @@ -36984,7 +36984,7 @@ namespace ts { } // For a commonjs `const x = require`, validate the alias and exit const symbol = getSymbolOfNode(node); - if (symbol.flags & SymbolFlags.Alias && isAccessedOrBareRequireVariableDeclaration(node)) { + if (symbol.flags & SymbolFlags.Alias && isVariableDeclarationInitializedToBareOrAccessedRequire(node)) { checkAliasSymbol(node); return; } diff --git a/src/compiler/types.ts b/src/compiler/types.ts index 224fb27ef919a..fe326922e3c4f 100644 --- a/src/compiler/types.ts +++ b/src/compiler/types.ts @@ -4664,14 +4664,12 @@ namespace ts { export type RequireOrImportCall = CallExpression & { expression: Identifier, arguments: [StringLiteralLike] }; /* @internal */ - export interface RequireVariableDeclaration extends VariableDeclaration { - readonly initializer: RequireOrImportCall; + export interface VariableDeclarationInitializedTo extends VariableDeclaration { + readonly initializer: T; } /* @internal */ - export interface AccessedOrBareRequireVariableDeclaration extends VariableDeclaration { - readonly initializer: RequireOrImportCall | AccessExpression; - } + export type RequireVariableDeclaration = VariableDeclarationInitializedTo; /* @internal */ export interface RequireVariableStatement extends VariableStatement { diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index 635bb02765aa5..1e3f5035e2b75 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -2046,7 +2046,7 @@ namespace ts { } export function getExternalModuleRequireArgument(node: Node) { - return isAccessedOrBareRequireVariableDeclaration(node) && (getLeftmostAccessExpression(node.initializer) as CallExpression).arguments[0] as StringLiteral; + return isVariableDeclarationInitializedToBareOrAccessedRequire(node) && (getLeftmostAccessExpression(node.initializer) as CallExpression).arguments[0] as StringLiteral; } export function isInternalModuleImportEqualsDeclaration(node: Node): node is ImportEqualsDeclaration { @@ -2120,7 +2120,7 @@ namespace ts { /** * Like `isRequireVariableDeclaration` but allows things like `require("...").foo.bar` or `require("...")["baz"]`. */ - export function isAccessedOrBareRequireVariableDeclaration(node: Node): node is AccessedOrBareRequireVariableDeclaration { + export function isVariableDeclarationInitializedToBareOrAccessedRequire(node: Node): node is VariableDeclarationInitializedTo { return isVariableDeclarationInitializedWithRequireHelper(node, /*allowAccessedRequire*/ true); } diff --git a/src/services/findAllReferences.ts b/src/services/findAllReferences.ts index 7dc65cee20689..9f6baf0cc73c4 100644 --- a/src/services/findAllReferences.ts +++ b/src/services/findAllReferences.ts @@ -1531,7 +1531,7 @@ namespace ts.FindAllReferences { // Use the parent symbol if the location is commonjs require syntax on javascript files only. if (isInJSFile(referenceLocation) && referenceLocation.parent.kind === SyntaxKind.BindingElement - && isAccessedOrBareRequireVariableDeclaration(referenceLocation.parent)) { + && isVariableDeclarationInitializedToBareOrAccessedRequire(referenceLocation.parent)) { referenceSymbol = referenceLocation.parent.symbol; // The parent will not have a symbol if it's an ObjectBindingPattern (when destructuring is used). In // this case, just skip it, since the bound identifiers are not an alias of the import. diff --git a/src/services/goToDefinition.ts b/src/services/goToDefinition.ts index f73e54c6419d1..bc1b2f64d5ab6 100644 --- a/src/services/goToDefinition.ts +++ b/src/services/goToDefinition.ts @@ -290,7 +290,7 @@ namespace ts.GoToDefinition { return declaration.parent.kind === SyntaxKind.NamedImports; case SyntaxKind.BindingElement: case SyntaxKind.VariableDeclaration: - return isInJSFile(declaration) && isAccessedOrBareRequireVariableDeclaration(declaration); + return isInJSFile(declaration) && isVariableDeclarationInitializedToBareOrAccessedRequire(declaration); default: return false; } diff --git a/src/services/importTracker.ts b/src/services/importTracker.ts index dc069f5e0b657..7bc8ffc36947e 100644 --- a/src/services/importTracker.ts +++ b/src/services/importTracker.ts @@ -621,7 +621,7 @@ namespace ts.FindAllReferences { Debug.assert((parent as ImportClause | NamespaceImport).name === node); return true; case SyntaxKind.BindingElement: - return isInJSFile(node) && isAccessedOrBareRequireVariableDeclaration(parent); + return isInJSFile(node) && isVariableDeclarationInitializedToBareOrAccessedRequire(parent); default: return false; } From bf708bfde75c72923c6221bbb1e1177345806909 Mon Sep 17 00:00:00 2001 From: Daniel Rosenwasser Date: Wed, 19 Jan 2022 21:26:20 +0000 Subject: [PATCH 6/6] Renamed both utilitiy functions uniformly. --- src/compiler/types.ts | 7 ++----- src/compiler/utilities.ts | 6 +++--- src/services/codefixes/importFixes.ts | 2 +- 3 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/compiler/types.ts b/src/compiler/types.ts index fe326922e3c4f..32b316137b3f6 100644 --- a/src/compiler/types.ts +++ b/src/compiler/types.ts @@ -4639,7 +4639,7 @@ namespace ts { export type AnyImportSyntax = ImportDeclaration | ImportEqualsDeclaration; /* @internal */ - export type AnyImportOrRequire = AnyImportSyntax | RequireVariableDeclaration; + export type AnyImportOrRequire = AnyImportSyntax | VariableDeclarationInitializedTo; /* @internal */ export type AnyImportOrRequireStatement = AnyImportSyntax | RequireVariableStatement; @@ -4668,9 +4668,6 @@ namespace ts { readonly initializer: T; } - /* @internal */ - export type RequireVariableDeclaration = VariableDeclarationInitializedTo; - /* @internal */ export interface RequireVariableStatement extends VariableStatement { readonly declarationList: RequireVariableDeclarationList; @@ -4678,7 +4675,7 @@ namespace ts { /* @internal */ export interface RequireVariableDeclarationList extends VariableDeclarationList { - readonly declarations: NodeArray; + readonly declarations: NodeArray>; } /* @internal */ diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index 1e3f5035e2b75..d634192b9365b 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -2113,12 +2113,12 @@ namespace ts { * Returns true if the node is a VariableDeclaration initialized to a require call (see `isRequireCall`). * This function does not test if the node is in a JavaScript file or not. */ - export function isRequireVariableDeclaration(node: Node): node is RequireVariableDeclaration { + export function isVariableDeclarationInitializedToRequire(node: Node): node is VariableDeclarationInitializedTo { return isVariableDeclarationInitializedWithRequireHelper(node, /*allowAccessedRequire*/ false); } /** - * Like `isRequireVariableDeclaration` but allows things like `require("...").foo.bar` or `require("...")["baz"]`. + * Like {@link isVariableDeclarationInitializedToRequire} but allows things like `require("...").foo.bar` or `require("...")["baz"]`. */ export function isVariableDeclarationInitializedToBareOrAccessedRequire(node: Node): node is VariableDeclarationInitializedTo { return isVariableDeclarationInitializedWithRequireHelper(node, /*allowAccessedRequire*/ true); @@ -2136,7 +2136,7 @@ namespace ts { export function isRequireVariableStatement(node: Node): node is RequireVariableStatement { return isVariableStatement(node) && node.declarationList.declarations.length > 0 - && every(node.declarationList.declarations, decl => isRequireVariableDeclaration(decl)); + && every(node.declarationList.declarations, decl => isVariableDeclarationInitializedToRequire(decl)); } export function isSingleOrDoubleQuote(charCode: number) { diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index 6284cb6775569..bf78d043e1084 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -535,7 +535,7 @@ namespace ts.codefix { const importKind = getImportKind(importingFile, exportKind, compilerOptions); return mapDefined(importingFile.imports, (moduleSpecifier): FixAddToExistingImportInfo | undefined => { const i = importFromModuleSpecifier(moduleSpecifier); - if (isRequireVariableDeclaration(i.parent)) { + if (isVariableDeclarationInitializedToRequire(i.parent)) { return checker.resolveExternalModuleName(moduleSpecifier) === moduleSymbol ? { declaration: i.parent, importKind, symbol, targetFlags } : undefined; } if (i.kind === SyntaxKind.ImportDeclaration || i.kind === SyntaxKind.ImportEqualsDeclaration) {