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 all 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
2 changes: 1 addition & 1 deletion src/compiler/binder.ts
Expand Up @@ -3282,7 +3282,7 @@ namespace ts {
}

if (!isBindingPattern(node.name)) {
if (isInJSFile(node) && isRequireVariableDeclaration(node) && !getJSDocTypeTag(node)) {
if (isInJSFile(node) && isVariableDeclarationInitializedToBareOrAccessedRequire(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);
|| isVariableDeclarationInitializedToBareOrAccessedRequire(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 && isVariableDeclarationInitializedToBareOrAccessedRequire(node)) {
checkAliasSymbol(node);
return;
}
Expand Down
8 changes: 4 additions & 4 deletions src/compiler/types.ts
Expand Up @@ -4639,7 +4639,7 @@ namespace ts {
export type AnyImportSyntax = ImportDeclaration | ImportEqualsDeclaration;

/* @internal */
export type AnyImportOrRequire = AnyImportSyntax | RequireVariableDeclaration;
export type AnyImportOrRequire = AnyImportSyntax | VariableDeclarationInitializedTo<RequireOrImportCall>;

/* @internal */
export type AnyImportOrRequireStatement = AnyImportSyntax | RequireVariableStatement;
Expand All @@ -4664,8 +4664,8 @@ namespace ts {
export type RequireOrImportCall = CallExpression & { expression: Identifier, arguments: [StringLiteralLike] };

/* @internal */
export interface RequireVariableDeclaration extends VariableDeclaration {
readonly initializer: RequireOrImportCall;
export interface VariableDeclarationInitializedTo<T extends Expression> extends VariableDeclaration {
readonly initializer: T;
}

/* @internal */
Expand All @@ -4675,7 +4675,7 @@ namespace ts {

/* @internal */
export interface RequireVariableDeclarationList extends VariableDeclarationList {
readonly declarations: NodeArray<RequireVariableDeclaration>;
readonly declarations: NodeArray<VariableDeclarationInitializedTo<RequireOrImportCall>>;
}

/* @internal */
Expand Down
21 changes: 17 additions & 4 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 isVariableDeclarationInitializedToBareOrAccessedRequire(node) && (getLeftmostAccessExpression(node.initializer) as CallExpression).arguments[0] as StringLiteral;
}

export function isInternalModuleImportEqualsDeclaration(node: Node): node is ImportEqualsDeclaration {
Expand Down Expand Up @@ -2113,17 +2113,30 @@ 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<RequireOrImportCall> {
return isVariableDeclarationInitializedWithRequireHelper(node, /*allowAccessedRequire*/ false);
}

/**
* Like {@link isVariableDeclarationInitializedToRequire} but allows things like `require("...").foo.bar` or `require("...")["baz"]`.
*/
export function isVariableDeclarationInitializedToBareOrAccessedRequire(node: Node): node is VariableDeclarationInitializedTo<RequireOrImportCall | AccessExpression> {
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 {
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) {
Expand Down
2 changes: 1 addition & 1 deletion src/services/codefixes/importFixes.ts
Expand Up @@ -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) {
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)) {
&& 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.
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) && isVariableDeclarationInitializedToBareOrAccessedRequire(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) && isVariableDeclarationInitializedToBareOrAccessedRequire(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" },
]);