Skip to content

Commit

Permalink
Cherry-pick PR #47433 into release-4.5 (#47515)
Browse files Browse the repository at this point in the history
Component commits:
fce282b Add failing test.

ea2c290 Update failing test.

d954948 Finalized failing test case.

f476e84 Separate our usages of utilities that expect variables initialized to require(...) and require(...).foo.

9f0810c Renamed types and utilities, removed accidental indentation.

bf708bf Renamed both utilitiy functions uniformly.

Co-authored-by: Daniel Rosenwasser <drosen@microsoft.com>
  • Loading branch information
TypeScript Bot and DanielRosenwasser authored Jan 19, 2022
1 parent 8a8048f commit 8450901
Show file tree
Hide file tree
Showing 9 changed files with 49 additions and 15 deletions.
2 changes: 1 addition & 1 deletion src/compiler/binder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3288,7 +3288,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
Original file line number Diff line number Diff line change
Expand Up @@ -2592,7 +2592,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 @@ -36831,7 +36831,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
Original file line number Diff line number Diff line change
Expand Up @@ -4637,7 +4637,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 @@ -4662,8 +4662,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 @@ -4673,7 +4673,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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
@@ -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" },
]);

0 comments on commit 8450901

Please sign in to comment.