Services debug failure in presence of unclosed JSX fragment

[DanielRosenwasser]

Maybe related to #38317.

In a .tsx file:

let x = <></Foo>;

Hover over Foo, or click on Foo to get document highlights

[Trace  - 21:32:41.378] <semantic> Response received: quickinfo (12). Request took 2 ms. Success: false . Message: Error processing request. Debug Failure. Did not expect JsxClosingFragment to have an Identifier in its trivia
Error: Debug Failure. Did not expect JsxClosingFragment to have an Identifier in its trivia
    at addSyntheticNodes (c:\Users\drosen\.vscode-insiders\extensions\ms-vscode.vscode-typescript-next-4.4.20210512\node_modules\typescript\lib\tsserver.js:152168:30)
    at createChildren (c:\Users\drosen\.vscode-insiders\extensions\ms-vscode.vscode-typescript-next-4.4.20210512\node_modules\typescript\lib\tsserver.js:152157:9)
    at NodeObject.getChildren (c:\Users\drosen\.vscode-insiders\extensions\ms-vscode.vscode-typescript-next-4.4.20210512\node_modules\typescript\lib\tsserver.js:152101:56)
    at getTokenAtPositionWorker (c:\Users\drosen\.vscode-insiders\extensions\ms-vscode.vscode-typescript-next-4.4.20210512\node_modules\typescript\lib\tsserver.js:119030:43)
    at getTouchingToken (c:\Users\drosen\.vscode-insiders\extensions\ms-vscode.vscode-typescript-next-4.4.20210512\node_modules\typescript\lib\tsserver.js:119017:16)
    at Object.getTouchingPropertyName (c:\Users\drosen\.vscode-insiders\extensions\ms-vscode.vscode-typescript-next-4.4.20210512\node_modules\typescript\lib\tsserver.js:119009:16)
    at Proxy.getQuickInfoAtPosition (c:\Users\drosen\.vscode-insiders\extensions\ms-vscode.vscode-typescript-next-4.4.20210512\node_modules\typescript\lib\tsserver.js:153343:27)
    at IOSession.Session.getQuickInfoWorker (c:\Users\drosen\.vscode-insiders\extensions\ms-vscode.vscode-typescript-next-4.4.20210512\node_modules\typescript\lib\tsserver.js:164421:62)
    at Session.handlers.ts.Map.ts.getEntries._a.<computed> (c:\Users\drosen\.vscode-insiders\extensions\ms-vscode.vscode-typescript-next-4.4.20210512\node_modules\typescript\lib\tsserver.js:163331:61)
    at c:\Users\drosen\.vscode-insiders\extensions\ms-vscode.vscode-typescript-next-4.4.20210512\node_modules\typescript\lib\tsserver.js:165173:88
    at IOSession.Session.executeWithRequestId (c:\Users\drosen\.vscode-insiders\extensions\ms-vscode.vscode-typescript-next-4.4.20210512\node_modules\typescript\lib\tsserver.js:165164:28)
    at IOSession.Session.executeCommand (c:\Users\drosen\.vscode-insiders\extensions\ms-vscode.vscode-typescript-next-4.4.20210512\node_modules\typescript\lib\tsserver.js:165173:33)
    at IOSession.Session.onMessage (c:\Users\drosen\.vscode-insiders\extensions\ms-vscode.vscode-typescript-next-4.4.20210512\node_modules\typescript\lib\tsserver.js:165199:35)
    at Interface.<anonymous> (c:\Users\drosen\.vscode-insiders\extensions\ms-vscode.vscode-typescript-next-4.4.20210512\node_modules\typescript\lib\tsserver.js:167801:31)
    at Interface.emit (events.js:315:20)
    at Interface._onLine (readline.js:337:10)
    at Interface._normalWrite (readline.js:482:12)
    at Socket.ondata (readline.js:194:10)
    at Socket.emit (events.js:315:20)
    at addChunk (internal/streams/readable.js:309:12)
    at readableAddChunk (internal/streams/readable.js:284:9)
    at Socket.Readable.push (internal/streams/readable.js:223:10)
    at Pipe.onStreamRead (internal/stream_base_commons.js:188:23)

[DanielRosenwasser]

Moving this off the backlog - some version of this hits over 20k machines a day.

[DanielRosenwasser]

I think conceptually, what's happening "makes sense". As a recovery tactic for JSX fragments (<>...</>) in the following case

<>
</div>

we always parse out a tag name, but we don't store it in the tree. Then later, we try to populate each child node with its trivia.

That said, I don't know what the right fix is. Storing the name in the tree?

[gabritto]

I'm currently investigating this bug by accident, because the fuzzer detected this crash and it's getting in the way of me reproing another crash.
Edit: this same assertion causes the crash I got, but it's a different bug because it happens when parsing weird jsdoc, which seems way less frequent than this.

[jakebailey]

Yeah, I was working on this one too. Not sure what the "good" fix would be here. Having followed the code, I don't think the recovery is all that perfect. Basically all it does is eat the next token, see if that was an identifier, then issue an error, then try and close with the >, but of course the token could also be Foo.Bar.Baz or something and it wouldn't recover there well either. That on top of the fact that the tokens just don't get parsed and become trivia.

I almost wonder what this debug assertion gets us.

[jakebailey]

I almost wonder what this debug assertion gets us.

Just to clarify this, it seems like the services code is trying to be clever to detect parser bugs, but effectively ends up re-encoding parser rules that may or may not be right without context (which this part of the code does not have).

In this case, I don't think that the assertion is really correct; it's only failing because it happens to be an identifier, but what if I stuck in 1234? No debug fail there, but that token is similarly stranded.

[jakebailey]

@gabritto says @andrewbranch says (phew) that there are some actual reasons to have it, so I'll see if I can get the parser to do better here instead of ragging on our debug asserts any more. 😄

[andrewbranch]

Let’s just say you’re not the first to propose removing the assertion in the first few months of being on the team 😅

[DanielRosenwasser]

After speaking to @CyrusNajmabadi outside GitHub about this, I am kind of convinced that the assertion is not really guaranteeing anything useful (though it would be worth looking into when the assertion was added).

Ultimately, "skipped" tokens are something our trees should be able to hold onto. There are only 2 cases I think this assertion was meant to catch:

• Accidentally rescanning JSX text. There was a fairly common bug around accidentally rescanning or skipping trivia in the content of a JSX text node.
• Handling "specialized scans" (e.g. scanJsxToken, scanTemplateToken, etc.) where the result is not stored in the tree. If we do that, we end up "hydrating" the tree with incorrect child tokens.

Just to clarify this, it seems like the services code is trying to be clever to detect parser bugs, but effectively ends up re-encoding parser rules that may or may not be right without context (which this part of the code does not have).

I think that's exactly the concern in the latter point above.

In this case, I don't think that the assertion is really correct; it's only failing because it happens to be an identifier, but what if I stuck in 1234? No debug fail there, but that token is similarly stranded.

Let’s just say you’re not the first to propose removing the assertion in the first few months of being on the team 😅

I think all of us have come to the same conclusion. I think the fix is to remove the assertion.

[DanielRosenwasser]

Though now I've re-read - if there are good reasons to keep it, let's discuss them here.

[andrewbranch]

I sort of agree in theory, but I suspect removing the assertion would just move the crash to a dozen different places where LS operations naturally assume that an identifier is going to be predictably parented into the parse tree. It’s usually not hard to change the parser recovery so it doesn’t drop identifiers.

[jakebailey]

My point about Foo.Bar.Baz isn't right; I didn't notice that it's actually calling parseJsxElementName (it's an argument with a side effect, bleh). It definitely is odd when I stick a number there, though, as it doesn't actually bother to go all the way to > (but, at least there's no trivia); I suspect things get weird if you accidentally stuck JSX attributes there too.

I'm interested in how this would be fixed to not drop the node though; it seems like the only thing that can be done is to actually stick a tag name into this fragment, which seems odd.

The opening fragment just doesn't have anything fancy; it just only lets you type <>; if the same thing were done for </> then maybe it'd avoid the problem, but I get the feeling that'd be worse recovery. (EDIT: definitely does.)

[andrewbranch]

Recovery of this sort is usually done by making up tokens that don’t exist instead of dropping tokens that do exist. I’m not sure what the best route for this one would be without looking at the parser code, but my first instinct would be <></> Foo > [empty identifier]

[jakebailey]

Thanks, that's helpful.

[gabritto]

so we're not getting rid of the assertion then?

[jakebailey]

Seems like no; I'm going to see if I can do what Andrew suggested by inventing things to recover instead and we'll see, I guess.