Skip to content
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

On invalid parse nodes, if the token may vary, allow any token. #3484

Merged
merged 1 commit into from
Dec 13, 2023

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented Dec 8, 2023

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, 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.

@jonmeow
Copy link
Contributor Author

jonmeow commented Dec 8, 2023

(note, dependencies in stack)

@jonmeow jonmeow force-pushed the any-token branch 3 times, most recently from 29528fe to 8c3e648 Compare December 11, 2023 19:38
@jonmeow jonmeow force-pushed the any-token branch 2 times, most recently from a6d5c70 to 2d927d6 Compare December 12, 2023 01:01
@jonmeow
Copy link
Contributor Author

jonmeow commented Dec 12, 2023

Note, some changes here overlapped with #3493, which would've incidentally fixed the namespace crash. However, since #3493 still didn't change everything do CARBON_IF_ERROR(CARBON_ANY_TOKEN), this is still a question.

Copy link
Contributor

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

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

I think this makes sense. The prior behaviour doesn't seem especially useful to me, given that if you wanted the for or if token you can easily find them.

@jonmeow jonmeow force-pushed the any-token branch 2 times, most recently from 014095c to 5eb5ad9 Compare December 13, 2023 20:19
@jonmeow jonmeow added this pull request to the merge queue Dec 13, 2023
Merged via the queue into carbon-language:trunk with commit 550559e Dec 13, 2023
6 checks passed
@jonmeow jonmeow deleted the any-token branch December 13, 2023 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants