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

Conversation

DanielRosenwasser
Copy link
Member

@DanielRosenwasser DanielRosenwasser commented Jan 14, 2022

Fixes #47428.

Okay, here's the summary:

First, we have a type predicate in our services layer that determines whether something is an import statement or a variable initialized with a require call.

const importKindPredicate: (node: Node) => node is AnyImportOrRequireStatement = decl.kind === SyntaxKind.VariableStatement ? isRequireVariableStatement : isAnyImportSyntax;

That type predicate is wrong because it takes a dependency on isRequireVariableStatement...

export function isRequireVariableStatement(node: Node): node is RequireVariableStatement {
return isVariableStatement(node)
&& node.declarationList.declarations.length > 0
&& every(node.declarationList.declarations, decl => isRequireVariableDeclaration(decl));
}

which relies on isRequireVariableDeclaration...

export function isRequireVariableDeclaration(node: Node): node is RequireVariableDeclaration {
if (node.kind === SyntaxKind.BindingElement) {
node = node.parent.parent;
}
return isVariableDeclaration(node) && !!node.initializer && isRequireCall(getLeftmostAccessExpression(node.initializer), /*requireStringLiteralLikeArgument*/ true);
}

which returns true a variable like in const foo = require("yadda").foo.

Because the type predicate is wrong, we eventually end up crashing because we think we have a CallExpression on the initializer and can't access the first argument, resulting in:

TypeError: Cannot read property '0' of undefined

So my solution here is to split up the usages of the function - some for when a consumer can tolerate the property access version (require("yadda").foo), and some for tolerating the bare require("yadda") version. isRequireVariableStatement can safely be changed to rely on the bare version because it's only called by the type predicate function.

This at the very least avoids crashing for code-fixes without impacting go-to-definition, and other functionality in the type-checker itself.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jan 14, 2022
@DanielRosenwasser DanielRosenwasser changed the title Import code fix with dotted require Avoid crash for import code fixes with dotted require Jan 14, 2022
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

The fix seems good, I just don't like the new name at all.
I suggested a slightly less bad name, although it's still not good.

@@ -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.

src/compiler/binder.ts Outdated Show resolved Hide resolved
@@ -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'd agree (less easy to miss if you're doing completion and were about to use the old one, too).

@typescript-bot typescript-bot added For Milestone Bug PRs that fix a bug with a specific milestone and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jan 19, 2022
@DanielRosenwasser
Copy link
Member Author

I've renamed both functions uniformly so you can't miss them, though I've left isRequireVariableStatement alone. That can be a follow-up if people feel strongly enough about it.

Copy link
Member

@jakebailey jakebailey left a comment

Choose a reason for hiding this comment

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

Seems okay to me, but someone else may object to the naming, of course.

@DanielRosenwasser
Copy link
Member Author

@typescript-bot cherry-pick this to release-4.5

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 19, 2022

Heya @DanielRosenwasser, I've started to run the task to cherry-pick this into release-4.5 on this PR at bf708bf. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

Hey @DanielRosenwasser, I've opened #47515 for you.

typescript-bot pushed a commit to typescript-bot/TypeScript that referenced this pull request Jan 19, 2022
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.
@DanielRosenwasser DanielRosenwasser merged commit ad5ca67 into main Jan 19, 2022
@DanielRosenwasser DanielRosenwasser deleted the importCodeFixWithDottedRequire branch January 19, 2022 23:05
DanielRosenwasser added a commit that referenced this pull request Jan 19, 2022
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash in completionEntryDetails with existing require("...").foo
5 participants