fix(parser): allow excess tokens after expected EOF

[arendjr]

Summary

Fixes #5610.

This may be a somewhat controversial fix, so I'm going to need some advice on this one.

While investigating why the parser was crashing on the plugin in #5610, I noticed there was a panic inside SyntaxKind::to_string() which only happened when the syntax kind was an EOL token. This could happen because both the GritQL and the GraphQL parsers have a p.expect(EOF) statement at the end of their parser.

So my intuition was to fix this so that SyntaxKind::to_string() can handle this gracefully and I've updated the codegen accordingly.

It all worked out, and the panic is resolved now. But then I looked at the snapshot, and I see these excess tokens behind the EOF in the CST. I know this is not what we want, but then again... it seems we handle it gracefully now and the alternative would probably involve much more complex fixes in both these parsers. What do you think, @ematipico ?

Test Plan

Test case added.

[ematipico]

I'm a bit conflicted by the fix. It does the job, but I think it will have some impact on other tools like formatter or linter. I'm not sure though, why don't you add the same code in the grit formatter test suite, and see the result?

Also, it's a symptom that the parser isn't recovering as expected. Ideally, any non-valid syntax should be placed inside a bogus node, even a generic one.

With this fix, we're allowing our parser to not recover anymore properly, and as a "last resort" we allow adding everything to the EOF token. That's probably why we didn't allow that.

[codspeed-hq]

CodSpeed Performance Report

Merging #5621 will not alter performance

_{Comparing arendjr:allow-excess-tokens-after-expected-eof (7871f1d) with main (634682c)}

Summary

✅ 95 untouched benchmarks

[arendjr]

Yeah, I agree. I think the fix is still an improvement (almost everything is better than crashing), but ideally we try to avoid these situations. Maybe let's keep this fix, but try to enhance the Grit error recovery for this test case before merging?

[arendjr]

At least the error recovery is fixed too now, but I'd be tempted to keep the original fix as a "defence in depth" in case there similar issues lurking still.

[ematipico]

Looks good! As a nice to have, I wonder if we could log some kind of warning in the screenshot in case some content falls into the EOF token. This should help maintainers understand that they shouldn't have such cases

[arendjr]

Good idea! I'll try that in a separate PR 👍

[d0422]

Hello @arendjr,
This is d0422 — I previously posted a Q/A about this issue on Discord.
I was wondering if there’s a release planned related to this issue, so I wanted to reach out and ask!

[arendjr]

@d0422 It will be in the upcoming 2.0 release, and we also have bi-weekly preview releases if you want it faster. They’re announced in the #releases channel on Discord.