Fix a crash on invalid found by fuzzing. #3404
Conversation
When a `namespace` keyword has no `;` following it, we recover by building a parse tree `Namespace` node from the `Namespace` token (as there isn't a `;` token). Allow this correspondence on errors. Also teach the diagnostics in this case to avoid the end-of-file token as that's almost always going to be a less meaningful location. Instead, we can point at the introducer which should at least be in the code that led to the error.
| CARBON_PARSE_NODE_KIND_CHILD_COUNT(Namespace, 2, | ||
| CARBON_TOKEN(Semi) | ||
| CARBON_IF_ERROR(CARBON_TOKEN(Namespace))) |
There was a problem hiding this comment.
Can you please add a test of:
namespace "x"
In this case, won't the location be the string literal token, and still crash? Maybe you should use CARBON_ANY_TOKEN here?
There was a problem hiding this comment.
Good idea, added a bunch of tests.
Interestingly, none of them crash. The parser ends up reusing the namespace token reliably here it seems? Happy to add more tests or make another change though if useful.
There was a problem hiding this comment.
Thanks, I'm not clear on what the crash was caused by now, maybe I'll look separately.
| @@ -27,7 +27,13 @@ static auto HandleDeclNameAndParams(Context& context, State after_name) | |||
| CARBON_DIAGNOSTIC(ExpectedDeclName, Error, | |||
| "`{0}` introducer should be followed by a name.", | |||
| Lex::TokenKind); | |||
| context.emitter().Emit(*context.position(), ExpectedDeclName, | |||
| Lex::Token location = *context.position(); | |||
| if (context.tokens().GetKind(location) == Lex::TokenKind::EndOfFile) { | |||
There was a problem hiding this comment.
I should note too, there are probably a lot of places that we could get into logic like this. If you want to get started on this more generally, a context helper function would probably be suitable (e.g., PositionForDiagnostic with a default).
There was a problem hiding this comment.
Definitely agree that we'll likely want something like this eventually.
That said, I'm not super confident on what it will look like yet... I worry that the code I've used here is somewhat specific to this diagnostic and we might need more interesting logic for others. My preference would be to wait until we have a few cases to look at before trying to generalize if that's OK?
| @@ -27,7 +27,13 @@ static auto HandleDeclNameAndParams(Context& context, State after_name) | |||
| CARBON_DIAGNOSTIC(ExpectedDeclName, Error, | |||
| "`{0}` introducer should be followed by a name.", | |||
| Lex::TokenKind); | |||
| context.emitter().Emit(*context.position(), ExpectedDeclName, | |||
| Lex::Token location = *context.position(); | |||
| if (context.tokens().GetKind(location) == Lex::TokenKind::EndOfFile) { | |||
Note #3486 rewrites the macro behavior, and is already approved: so this PR is only for the changed enforcement during error. Also, #3493 already changed several things to allow any token while this PR was awaiting review, but this still changes enforcement for `For` and `If`. This was brought up on [#toolchain](https://discord.com/channels/655572317891461132/655578254970716160/1182066616456970251), and I think this any-on-error approach gets at least some support. We could try setting it to the introducer, but it's quite possible we want it to be something like the token which led to the parse error, rather than a static token. That leads to a conclusion that, most typically, we'll expect arbitrary tokens when error conditions may lead to tokens which aren't the expected token. A couple related, recent `CARBON_IF_ERROR` crash fixes can be found in #3404 and #3424. Something like #3404 would've been needed regardless because `namespace` didn't have `CARBON_IF_ERROR` before, although I might've missed the underlying issue with declarations because only `namespace` had a relevant test (that is, if #3404 had added `CARBON_ANY_TOKEN_ON_ERROR`, I wouldn't have had a crash in #3462). #3424 would've been avoided with this change because there was a `CARBON_IF_ERROR`, and it was just too restrictive.
When a
namespacekeyword has no;following it, we recover by building a parse treeNamespacenode from theNamespacetoken (as there isn't a;token). Allow this correspondence on errors.Also teach the diagnostics in this case to avoid the end-of-file token as that's almost always going to be a less meaningful location. Instead, we can point at the introducer which should at least be in the code that led to the error.