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

Fix crash from inaccurate type guard implementation #49252

Merged
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/compiler/binder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3291,7 +3291,12 @@ namespace ts {
}

if (!isBindingPattern(node.name)) {
if (isInJSFile(node) && isVariableDeclarationInitializedToBareOrAccessedRequire(node) && !getJSDocTypeTag(node) && !(getCombinedModifierFlags(node) & ModifierFlags.Export)) {
const possibleVariableDecl = node.kind === SyntaxKind.VariableDeclaration ? node : node.parent.parent;
if (isInJSFile(node) &&
isVariableDeclarationInitializedToBareOrAccessedRequire(possibleVariableDecl) &&
!getJSDocTypeTag(node) &&
!(getCombinedModifierFlags(node) & ModifierFlags.Export)
) {
declareSymbolAndAddToSymbolTable(node as Declaration, SymbolFlags.Alias, SymbolFlags.AliasExcludes);
}
else if (isBlockOrCatchScoped(node)) {
Expand Down
9 changes: 5 additions & 4 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2641,7 +2641,8 @@ namespace ts {
&& isAliasableOrJsExpression(node.parent.right)
|| node.kind === SyntaxKind.ShorthandPropertyAssignment
|| node.kind === SyntaxKind.PropertyAssignment && isAliasableOrJsExpression((node as PropertyAssignment).initializer)
|| isVariableDeclarationInitializedToBareOrAccessedRequire(node);
|| node.kind === SyntaxKind.VariableDeclaration && isVariableDeclarationInitializedToBareOrAccessedRequire(node)
|| node.kind === SyntaxKind.BindingElement && isVariableDeclarationInitializedToBareOrAccessedRequire(node.parent.parent);
}

function isAliasableOrJsExpression(e: Expression) {
Expand Down Expand Up @@ -37819,8 +37820,8 @@ namespace ts {
}
// For a commonjs `const x = require`, validate the alias and exit
const symbol = getSymbolOfNode(node);
if (symbol.flags & SymbolFlags.Alias && isVariableDeclarationInitializedToBareOrAccessedRequire(node)) {
checkAliasSymbol(node);
if (symbol.flags & SymbolFlags.Alias && isVariableDeclarationInitializedToBareOrAccessedRequire(node.kind === SyntaxKind.BindingElement ? node.parent.parent : node)) {
checkAliasSymbol(node as BindingElement | VariableDeclaration);
return;
}

Expand Down Expand Up @@ -40684,7 +40685,7 @@ namespace ts {
return true;
}

function checkAliasSymbol(node: ImportEqualsDeclaration | VariableDeclaration | ImportClause | NamespaceImport | ImportSpecifier | ExportSpecifier | NamespaceExport) {
function checkAliasSymbol(node: ImportEqualsDeclaration | VariableDeclaration | ImportClause | NamespaceImport | ImportSpecifier | ExportSpecifier | NamespaceExport | BindingElement) {
let symbol = getSymbolOfNode(node);
const target = resolveAlias(symbol);

Expand Down
3 changes: 0 additions & 3 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2197,9 +2197,6 @@ namespace ts {
}

function isVariableDeclarationInitializedWithRequireHelper(node: Node, allowAccessedRequire: boolean) {
if (node.kind === SyntaxKind.BindingElement) {
node = node.parent.parent;
}
Comment on lines -2200 to -2202
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the problem code; other changes are accounting for the deletion. The issue is that this function gets consumed exclusively by functions whose return type is a predicate of node being some sort of VariableDeclaration, so this defeats that type predicate.

return isVariableDeclaration(node) &&
!!node.initializer &&
isRequireCall(allowAccessedRequire ? getLeftmostAccessExpression(node.initializer) : node.initializer, /*requireStringLiteralLikeArgument*/ true);
Expand Down
2 changes: 1 addition & 1 deletion src/services/findAllReferences.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1601,7 +1601,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
&& isVariableDeclarationInitializedToBareOrAccessedRequire(referenceLocation.parent)) {
&& isVariableDeclarationInitializedToBareOrAccessedRequire(referenceLocation.parent.parent.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/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) && isVariableDeclarationInitializedToBareOrAccessedRequire(parent);
return isInJSFile(node) && isVariableDeclarationInitializedToBareOrAccessedRequire(parent.parent.parent);
default:
return false;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/// <reference path="../fourslash.ts" />

// @allowJs: true

// @Filename: /index.js
//// const { blah/**/ } = require("unresolved");

verify.goToSourceDefinition("", []);