Fixes for break label and continue label parsing#60910
Conversation
Fixes for the new parsing for labeled `break` / `continue` which is broken or surprising in a few ways: * The syntax `break f()` was allowed but parsed as `(break f (tuple))` rather than `(break (call f))`. This is very surprising, so introduce a condition forcing the label to be followed by whitespace. * The trailing token error handling was somehow deleted (but replaced with a comment yay AI slop :-/ ?), allowing things like `break + x` to parse as `(call-i (break) + x)`. We didn't have tests for this, so fix this bug and beef up the tests. * It doesn't use `bump()` with `remap_kind=K"Identifier"` so the label can be a contextual keyword. This is inconsistent with the rest of the parser's treatment of identifier-like things in identifier position. Also clean up other AI slop, deleting redundant comments and factoring break and continue parsing back into a single block as they share the same logic for label validity and recovery. Currently this parsing doesn't allow a few things as labels that we allow in other locations as plain identifiers. For consistency we might want to consider allowing the following as labels: * Operator symbols like `+` * Word operators `where` `in`, `as` * `var""` identifiers (?)
|
Just my two cents, but to me it is a feature that |
|
The win64 failure seems like an unrelated issue with the
Yes I'm also ok with not supporting operators so I didn't try to generalize in this PR. |
| "begin break end" => "(block (break))" | ||
| "a ? continue : c" => "(? a (continue) c)" | ||
| "begin continue end" => "(block (continue))" | ||
| "break:x" => "(call-i (break) : x)" # range colon allowed |
There was a problem hiding this comment.
I think we should disallow this one in 1.14 syntax - needs to work in 1.13 syntax of course, but i think it's too confusing otherwise and we may want it for more complicated labels in the future.
There was a problem hiding this comment.
Yeah this one is pretty weird.
It seems it was added along with the fix of #9358 but there's no sign in the commit message for why the colon was allowed as punctuation. Maybe it was just a bug.
There was a problem hiding this comment.
Although that does raise an interesting point. What about:
label = :foo
:(@label $label begin break $label end)
I think we should probably allow interpolation here.
There was a problem hiding this comment.
Oh ok I think I see. The fix of #9358 probably added : so that ternary could work, but that should have been combined with (not range-colon-enabled) to restrict it to ternaries and not ranges. But that was never added.
So there's another subtle bug to fix here: it was correct to add a test on range_colon_enabled but we actually needed the opposite !range_colon_enabled. The normal_context() at the top of parse_resword undoes our ability to test for this so will need a little refactor. Also we should gate that parsing change on syntax version.
Disallow I think
These should probably be allowed. |
3 participants
Fixes for the new parsing for labeled
break/continuewhich is broken or surprising in a few ways:break f()was allowed but parsed as(break f (tuple))rather than(break (call f)). This is very surprising, so introduce a condition forcing the label to be followed by whitespace.break + xto parse as(call-i (break) + x). We didn't have tests for this, so fix this bug and beef up the tests.bump()withremap_kind=K"Identifier"so the label can be a contextual keyword. This is inconsistent with the rest of the parser's treatment of identifier-like things in identifier position.Also clean up other AI slop written by Claude in #60481: delete redundant comments and factoring break and continue parsing back into a single block as they share the same logic for label validity and error recovery.
Currently this parsing doesn't allow a few things as labels that we allow in other locations as plain identifiers. For consistency we might want to consider allowing the following as labels which are currently allowed in macros like
@label+wherein,asvar""identifiers (?)I didn't change these yet, however.