Avoid translating implicit rest nodes in parameters #408
Conversation
| if (paramsNode->rest != nullptr) | ||
| params.emplace_back(translate(paramsNode->rest)); | ||
| auto prismRestNode = paramsNode->rest; | ||
| // Implicit rest node in parameters don't need to be translated |
There was a problem hiding this comment.
Could you please elaborate on the "why"? What was the behaviour before?
There was a problem hiding this comment.
Previously it simply entered the unreachable function and crash. And Sorbet parser's parse tree doesn't have a corresponding node for it. So I'm not sure how to expand the comment more 😅
There was a problem hiding this comment.
Or do you think I should update it to be can NOT be translated?
There was a problem hiding this comment.
Ah I see. Yes "cannot" fit better than "don't need to be" (which implies "can be, but isn't")
Playing around with this, it seems that handles it incorrectly. Found this bug report: sorbet#6407
So I'd suggest explaining that it's a known limitation:
| // Implicit rest node in parameters don't need to be translated | |
| // Known limitation: Sorbet doesn't handle implicit rest nodes for restructuring arguments to blocks. | |
| // https://github.com/sorbet/sorbet/issues/6407 |
st0012
force-pushed
the
handle-implicit-rest-node-in-parameters
branch
from
cfccfce to
5db265d
Compare
|
Closing in favor of #409 |
Motivation
Similar to the case in array pattern translation, Sorbet's parser doesn't care about the implicit rest node in parameters either. So the translator should skip its translation.
Addressed the issue mentioned in https://github.com/sorbet/sorbet/pull/8485/files#r1943210573
Test plan
See included automated tests.