Parsing impl...as
#3473
Parsing impl...as
#3473
Conversation
toolchain/parse/handle_impl.cpp
Outdated
| auto state = context.PopState(); | ||
| if (state.has_error) { | ||
| context.ReturnErrorOnState(); | ||
| return; |
There was a problem hiding this comment.
In this state, you've encountered a forall token. Why do you return here instead of letting the AddNode call occur, adding a parse node for the error token?
(note too, with the return, using state.has_error below is equivalent to /*has_error=*/false, which might be clearer)
There was a problem hiding this comment.
I'm pretty uncertain about how to handle errors in the parser, could you perhaps suggest some code? I'm particularly uncertain about what can be assumed if state.has_error is true. When I try moving the context.AddNode call before the if (state.has_error), I get a parse validation error since the ImplForall node is missing its expected child. I assume filling in the missing child is what you mean by "adding a parse node for the error token"? I took a stab at it, but I'm not sure what is expected here. I also deleted the return; on error, but that is just a guess.
It would be really helpful to add a section to https://docs.google.com/document/d/1RRYMm42osyqhI2LyjrjockYCutQ5dOf8Abu50kTrkX0/edit?resourcekey=0-kHyqOESbOHmzZphUbtLrTw#heading=h.d6o1d67xs8xu about what to do about errors in the parse stage:
- what does
state.has_errormean? when should it be set? what can you assume when it has been set by another function? how should you handle it? - what invariants need to be maintained when there are errors?
- what are common patterns?
There was a problem hiding this comment.
I'd meant just dropping the return.
I'd expected though that ParamListAsImplicit would always add a node, even in error cases. That may be a consistency issue. You adding an InvalidParse node is an unintended workaround.
Anyways, current state is fine, though it's probably worth someone (maybe me) changing ParamList behavior there.
Regarding the rest, jotted down some notes: https://docs.google.com/document/d/1RRYMm42osyqhI2LyjrjockYCutQ5dOf8Abu50kTrkX0/edit?resourcekey=0-kHyqOESbOHmzZphUbtLrTw#heading=h.f0xq7he13pm
There was a problem hiding this comment.
Based on the observed behavior in the test, it seems that ParamListAsImplicit sets error == true on the parent state if it did not output a node. If ParamListAsImplicit does output a node, that node has the error state set to true, but the parent state is left alone.
toolchain/parse/handle_impl.cpp
Outdated
|
|
||
| default: | ||
| context.PushState(State::ImplAs); | ||
| context.PushStateForExpr(PrecedenceGroup::ForImplAs()); |
There was a problem hiding this comment.
I was expecting to see ForType here. Do we intend to have different rules for the type operands of impl T1 as T2 than for let x: T1?
There was a problem hiding this comment.
I tried ForType, but that is currently defined as equivalent to ForTopLevelExpr(), which means it will include the as afterwards.
toolchain/parse/handle_impl.cpp
Outdated
| if (context.PositionKind() == Lex::TokenKind::As) { | ||
| context.AddLeafNode(NodeKind::ImplAs, context.Consume()); | ||
| context.PushState(State::Expr); | ||
| } else { | ||
| context.PushState(State::ImplAs); | ||
| context.PushStateForExpr(PrecedenceGroup::ForImplAs()); | ||
| } |
There was a problem hiding this comment.
Can this share code with HandleImplAfterIntroducer?
toolchain/parse/handle_impl.cpp
Outdated
| auto state = context.PopState(); | ||
| if (state.has_error) { | ||
| context.ReturnErrorOnState(); | ||
| return; |
There was a problem hiding this comment.
I'd meant just dropping the return.
I'd expected though that ParamListAsImplicit would always add a node, even in error cases. That may be a consistency issue. You adding an InvalidParse node is an unintended workaround.
Anyways, current state is fine, though it's probably worth someone (maybe me) changing ParamList behavior there.
Regarding the rest, jotted down some notes: https://docs.google.com/document/d/1RRYMm42osyqhI2LyjrjockYCutQ5dOf8Abu50kTrkX0/edit?resourcekey=0-kHyqOESbOHmzZphUbtLrTw#heading=h.f0xq7he13pm
|
FYI, I had to make some changes after merging. The trickiest one is now that |
|
LGTM, todo's seem reasonable for that. |
No description provided.