Error recovery for case #5277
< 8000 div class="gh-header-actions mt-0 mb-3 mb-md-2 ml-1 flex-md-order-1 flex-shrink-0 d-flex flex-items-center gap-1">Error recovery for case #5277
Conversation
10b796b to
b7bc0f3
Compare
There was a problem hiding this comment.
I think this is reasonable, but I have concerns about:
rewind_if_dedentednow appearing in rules that aren't actually errors, which seems like scope creep.- various things that don't seem to be used or changes that appear spurious.
Will stamp once those concerns are cleared up.
| { | ||
| auto &case_body = $4; | ||
| auto &else_ = case_body->els; | ||
| $$ = driver.build.case_match(self, $1, $2, | ||
| &case_body->whens, | ||
| else_ ? else_->tok : nullptr, | ||
| else_ ? else_->nod : nullptr, $5); | ||
| driver.rewind_if_dedented($1, $5); |
There was a problem hiding this comment.
It's not obvious to me why we need to alter the behavior of the pattern-matching case here, since this was existing non-error code. Shouldn't this be removed?
| } | ||
| | kCASE expr_value opt_terms p_case_body kEND |
There was a problem hiding this comment.
This change seems unnecessary.
There was a problem hiding this comment.
It was the only production in this file that didn’t have its pipe character aligned so I fixed it.
| @@ -1856,6 +1856,7 @@ lbrace_cmd_block_start: | |||
| &case_body->whens, | |||
| else_ ? else_->tok : nullptr, | |||
| else_ ? else_->nod : nullptr, $5); | |||
| driver.rewind_if_dedented($1, $5); | |||
There was a problem hiding this comment.
Why are we rewinding in existing productions?
| @@ -1865,15 +1866,40 @@ lbrace_cmd_block_start: | |||
| &case_body->whens, | |||
| else_ ? else_->tok : nullptr, | |||
| else_ ? else_->nod : nullptr, $4); | |||
| driver.rewind_if_dedented($1, $4); | |||
There was a problem hiding this comment.
Why are we rewinding in existing productions?
|
I've fixed the small changes. Thanks for pointing those out. To answer all the "Why are we rewinding in existing productions?" questions: That is the whole point of the change. Calling
|
|
We have a policy of testing changes to Sorbet against Stripe's codebase before Stripe employees can see the build results here: → https://go/builds/bui_L7diw9SiuA6mWC |
6c5e833 to
ae88a6d
Compare
Motivation
Sorry, I didn't go through the work of making this one into small commits.
Test plan
See included automated tests.