Refactor NodeKind to take advantage of a parse node only having one token kind. #3486
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
(also a change with dependencies) |
jonmeow
force-pushed
the
single-token
branch
3 times, most recently
from
977cfce
to
eed2070
Compare
zygoloid
approved these changes
Dec 11, 2023
toolchain/parse/node_kind.def
Outdated
|
|
||
| // An invalid parse. Used to balance the parse tree. Always has an error. | ||
| CARBON_PARSE_NODE_KIND_CHILD_COUNT(InvalidParse, 0, CARBON_ANY_TOKEN_ON_ERROR) | ||
| CARBON_PARSE_NODE_KIND_CHILD_COUNT(InvalidParse, 0, CARBON_ONLY_IF_VALID(Error)) |
There was a problem hiding this comment.
This is confusing :)
Can we drop the CARBON_ONLY_IF_VALID here?
toolchain/parse/node_kind.def
Outdated
|
|
||
| // An empty declaration, such as `;`. | ||
| CARBON_PARSE_NODE_KIND_CHILD_COUNT(EmptyDecl, 0, | ||
| CARBON_TOKEN(Semi) CARBON_ANY_TOKEN_ON_ERROR) | ||
| CARBON_PARSE_NODE_KIND_CHILD_COUNT(EmptyDecl, 0, CARBON_ONLY_IF_VALID(Semi)) |
There was a problem hiding this comment.
I'd find this more readable without the "ONLY" in the macro name. To me, this reads as "the parse node will only correspond to a semicolon if it's valid (and otherwise will not be a semicolon)", not "the parse node will correspond to a semicolon, but that's only guaranteed if it's valid (and otherwise nothing is guaranteed)".
jonmeow
force-pushed
the
single-token
branch
2 times, most recently
from
6a73add
to
2f3232b
Compare
jonmeow
force-pushed
the
single-token
branch
from
2f3232b
to
3bbb708
Compare
github-merge-queue bot
pushed a commit
that referenced
this pull request
Dec 13, 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](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.
jonmeow
force-pushed
the
single-token
branch
from
3bbb708
to
a4ce888
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This builds on the series of changes to NodeKinds, aiming to simplify the NodeKind implementation a little, also making it clearer that there's a single associated token for each parse node (or, for placeholders/invalid parses, not validated).
Note that prior to the relevant changes, there were nodes with multiple tokens. This change is also locking in the approach of one token per parse node, by refactoring macros to stop supporting multiple.