Skip to content

Fix import assertion occurrences crash #47535

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

Merged
merged 1 commit into from
Jan 20, 2022

Conversation

weswigham
Copy link
Member

This also makes import assertion parsing more generous, parsing any expression for an import assertion value, and then only later issuing an error on non-string-literal expressions.

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jan 20, 2022
@@ -526,7 +526,7 @@ namespace ts.FindAllReferences {
function getTextSpan(node: Node, sourceFile: SourceFile, endNode?: Node): TextSpan {
let start = node.getStart(sourceFile);
let end = (endNode || node).getEnd();
if (isStringLiteralLike(node)) {
if (isStringLiteralLike(node) && (end - start) > 2) {
Copy link
Member

@DanielRosenwasser DanielRosenwasser Jan 20, 2022

Choose a reason for hiding this comment

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

What's this guarding on? Unterminated string at EOF?

Copy link
Member Author

Choose a reason for hiding this comment

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

Response here

But generally, sort of, yeah - short ill-formed strings without quotes.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Jan 20, 2022

Do you have the old stack-trace so we can track any possible duplicates?

@@ -7281,7 +7281,7 @@ namespace ts {
const pos = getNodePos();
const name = tokenIsIdentifierOrKeyword(token()) ? parseIdentifierName() : parseLiteralLikeNode(SyntaxKind.StringLiteral) as StringLiteral;
parseExpected(SyntaxKind.ColonToken);
const value = parseLiteralLikeNode(SyntaxKind.StringLiteral) as StringLiteral;
const value = parseAssignmentExpressionOrHigher();
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 changed this to parseAssignmentExpressionOrHigher since it's what objects parse out for values, so covers the most likely set of expressions people may mistakenly put here (which is to say: any expression except comma expressions). There are other common "mistakes" that people may make for import assertions - passing an arbitrary expression instead of an object-like-thing, using computed property names or shorthand or rest properties... But handling those gracefully would require more involved alterations to how we parse import assertions.

@@ -526,7 +526,7 @@ namespace ts.FindAllReferences {
function getTextSpan(node: Node, sourceFile: SourceFile, endNode?: Node): TextSpan {
let start = node.getStart(sourceFile);
let end = (endNode || node).getEnd();
if (isStringLiteralLike(node)) {
if (isStringLiteralLike(node) && (end - start) > 2) {
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 line alone is technically the minimal fix to stop the crash, but it doesn't really get at the root cause (forcibly parsing a non-string-thing as a string) and instead treats the symptom (stopping adjusting the span bounds for strings shorter than a minimum quoted string length). I'm of a mind to keep this, since I'm pretty sure getTextSpan crashes on empty string literals without it, too (because createTextSpan demands a nonzero nonnegative span length), though I don't know of a repro for that case.

@weswigham
Copy link
Member Author

Do you have the old stack-trace so we can track any possible duplicates?

Something like

Error: length < 0
      at createTextSpan (src\compiler\utilitiesPublic.ts:98:19)
      at Object.createTextSpanFromBounds (src\compiler\utilitiesPublic.ts:105:16)
      at getTextSpan (src\services\findAllReferences.ts:534:16)
      at entryToDocumentSpan (src\services\findAllReferences.ts:411:30)
      at toHighlightSpan (src\services\findAllReferences.ts:505:30)
      at Array.map (<anonymous>)
      at getSemanticDocumentHighlights (src\services\documentHighlights.ts:34:58)
      at Object.getDocumentHighlights (src\services\documentHighlights.ts:19:20) 
      at getDocumentHighlights (src\services\services.ts:1761:39)
      at Object.getOccurrencesAtPosition (src\services\services.ts:1743:17)      

however this may not fix all instances of that assertion being triggered, as it's a generic assertion failure crash for any zero-or-less-length text spans; so any time span math happens on ill-formed nodes, this stack trace is liable to appear. I'm hoping, as I said here that this should defensively remove the assertion crash in most cases by eliminating the string literal span math when it's obviously wrong, however odd span length may still filter in via other means (eg, from bad initial node positions and lengths).

@weswigham weswigham merged commit bae0f50 into microsoft:main Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants