Modify debug assertion to avoid crashing on SyntaxList #47500
Conversation
typescript-bot
added
Author: Team
For Backlog Bug
src/services/utilities.ts
Outdated
| */ | ||
| function findRightmostChildNodeWithTokens(children: Node[], exclusiveStartPosition: number, sourceFile: SourceFile): Node | undefined { | ||
| function findRightmostChildNodeWithTokens(n: Node, exclusiveStartPosition: number, sourceFile: SourceFile): Node | undefined { | ||
| const children = n.getChildren(sourceFile); |
There was a problem hiding this comment.
Not sure if I should bother leaving children as a parameter to save a little bit of processing; seemed more clear to me to just pass down the parent node and get the children, rather than passing the parent with its children (all call sites technically have the list).
There was a problem hiding this comment.
It looks like n is only needed for its kind and that kind is only needed for an assert so, personally, I'd add a new parameter for the kind to make it clear that it's "extra".
src/services/utilities.ts
Outdated
| for (let i = exclusiveStartPosition - 1; i >= 0; i--) { | ||
| const child = children[i]; | ||
|
|
||
| if (isWhiteSpaceOnlyJsxText(child)) { | ||
| Debug.assert(i > 0, "`JsxText` tokens should not be the first child of `JsxElement | JsxSelfClosingElement`"); | ||
| if (i === 0 && isWhiteSpaceOnlyJsxText(child)) { |
There was a problem hiding this comment.
Can you explain why the index check moved into the if? That seems like a potentially big behavior change and I'm having trouble reasoning about its effect.
There was a problem hiding this comment.
Shoot, you're right! I was trying to be too clever and optimize the assert, but I completely tunnel visioned away the code below. Will revert.
| if (isWhiteSpaceOnlyJsxText(child)) { | ||
| Debug.assert(i > 0, "`JsxText` tokens should not be the first child of `JsxElement | JsxSelfClosingElement`"); | ||
| if (i === 0 && isWhiteSpaceOnlyJsxText(child)) { | ||
| Debug.assert(!(isJsxElement(n) || isJsxSelfClosingElement(n)), "`JsxText` tokens should not be the first child of `JsxElement | JsxSelfClosingElement`"); | ||
| } | ||
| else if (nodeHasTokens(children[i], sourceFile)) { |
There was a problem hiding this comment.
Out of scope, but should children[i] be child here and below?
There was a problem hiding this comment.
Yeah, this seems like an oops. I can fix this, if desired. Shouldn't change anything.
There was a problem hiding this comment.
Maybe as a follow-up.
src/services/utilities.ts
Outdated
| if (parentKind === SyntaxKind.JsxText || parentKind === SyntaxKind.JsxSelfClosingElement) { | ||
| Debug.assert(i > 0, "`JsxText` tokens should not be the first child of `JsxElement | JsxSelfClosingElement`"); |
There was a problem hiding this comment.
I might just fold the i > 0 into the above condition and turn this into a Debug.fail, but this is fine.
There was a problem hiding this comment.
Yeah, that's a reasonable change.
|
@typescript-bot cherry-pick this to release-4.5 |
|
Heya @DanielRosenwasser, I've started to run the task to cherry-pick this into |
|
Hey @DanielRosenwasser, I've opened #47514 for you. |
Fixes #47446
Fixes #21815
Fixes #46946