Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid crash for import code fixes with dotted require #47433

Merged
merged 6 commits into from Jan 19, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/compiler/binder.ts
Expand Up @@ -3274,15 +3274,15 @@ namespace ts {
return isEnumConst(node)
? bindBlockScopedDeclaration(node, SymbolFlags.ConstEnum, SymbolFlags.ConstEnumExcludes)
: bindBlockScopedDeclaration(node, SymbolFlags.RegularEnum, SymbolFlags.RegularEnumExcludes);
}
}
DanielRosenwasser marked this conversation as resolved.
Show resolved Hide resolved

function bindVariableDeclarationOrBindingElement(node: VariableDeclaration | BindingElement) {
if (inStrictMode) {
checkStrictModeEvalOrArguments(node, node.name);
}

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)) {
Expand Down
4 changes: 2 additions & 2 deletions src/compiler/checker.ts
Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
}
Expand Down
5 changes: 5 additions & 0 deletions src/compiler/types.ts
Expand Up @@ -4668,6 +4668,11 @@ namespace ts {
readonly initializer: RequireOrImportCall;
}

/* @internal */
export interface AccessedOrBareRequireVariableDeclaration extends VariableDeclaration {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I prefer RequireVariableDeclarationOrExpression

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd agree (less easy to miss if you're doing completion and were about to use the old one, too).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s not a “declaration or expression” though—it’s always a variable declaration, and the initializer of that variable declaration is always an expression. The thing that changes is whether that expression is a special call expression or a property access expression off that special call expression. I don’t like either of these names and don’t have a better suggestion ready, but I don’t understand what RequireVariableDeclarationOrExpression is supposed to mean.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm going to go with VariableDeclarationInitializedTo<T> with CallExpression and AccessExpression | CallExpression.

readonly initializer: RequireOrImportCall | AccessExpression;
}

/* @internal */
export interface RequireVariableStatement extends VariableStatement {
readonly declarationList: RequireVariableDeclarationList;
Expand Down
17 changes: 15 additions & 2 deletions src/compiler/utilities.ts
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion src/services/findAllReferences.ts
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion src/services/goToDefinition.ts
Expand Up @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion src/services/importTracker.ts
Expand Up @@ -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;
}
Expand Down
21 changes: 21 additions & 0 deletions tests/cases/fourslash/importFixesWithExistingDottedRequire.ts
@@ -0,0 +1,21 @@
/// <reference path="./fourslash.ts" />

// @module: commonjs
// @checkJs: true

// @Filename: ./library.js
//// module.exports.aaa = function() {}
//// module.exports.bbb = function() {}

// @Filename: ./foo.js
//// var aaa = require("./library.js").aaa;
//// aaa();
//// /*$*/bbb

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" },
]);