Refactor switch support in NodePath#getCompletionRecords
#13030
Conversation
JLHwung
added
PR: Bug Fix 🐛
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/44945/ |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 232af53:
|
| ) { | ||
| markCompletionsAs(lastNormalCompletions, BREAK_COMPLETION); | ||
| completions = completions.concat(lastNormalCompletions); | ||
| statementCompletions.forEach(c => c.path.remove()); |
There was a problem hiding this comment.
I know that this was already the previous behavior, but we should add a comment explaining that this function mutates the AST. I didn't expect it, given the function name.
JLHwung
force-pushed
the
refactor-get-completion-records
branch
from
881d512
to
bb89512
Compare
| shouldPopulateBreak: false, | ||
| inCaseClause: false, | ||
| }); | ||
| const paths = []; |
There was a problem hiding this comment.
this can just be records.map(r => r.path) right?
|
|
||
| type Completion = { | ||
| path: NodePath; | ||
| type: 0 | 1; |
There was a problem hiding this comment.
Should this be:
| type: 0 | 1; | |
| type: NORMAL_COMPLETION | BREAK_COMPLETION; |
?
Also, any reason for not using an enum?
There was a problem hiding this comment.
I wish we could use const enum here, one of few TypeScript features Babel does not support. 😔
There was a problem hiding this comment.
We could make it
| type: 0 | 1; | |
| type: typeof NORMAL_COMPLETION | typeof BREAK_COMPLETION; |
?
(I don't have a preference)
There was a problem hiding this comment.
The typeof operator does not account for the value of the const binding, so typeof NORMAL_COMPLETION is number.
There was a problem hiding this comment.
Ok technically we could add as const to the declarations, but let's keep 0 | 1 at this point 😂
Co-authored-by: Henry Zhu <hi@henryzoo.com>
Co-authored-by: Brian Ng <bng412@gmail.com>
JLHwung
force-pushed
the
refactor-get-completion-records
branch
from
01f68ad
to
232af53
Compare
|
@JLHwung Is this ready to be merged? |
github-actions
bot
added
the
outdated
do-expressionsThis PR reimplements switch support in
NodePath#getCompletionRecordsfirst introduced in #10070. It aims for unifying how we handle the AST structures inside switch and versa. A new tag (normalorbreak) is attributed to every completion record so we can correctly handle break completion in switch case clauses. The new implementation now can also handle the broken but disabled tests indo-expressions.This PR also addressed how we handle declaration in completion records. I am aware that @bakkot is proposing to forbid do expression blocks which end in declaration, which means we could further throw an early error. But given that
NodePath#getCompletionRecordsis also used in popularNodePath#replaceWith, it will be great if we can correctly handle such scenarios.Known issues:
This PR does not support LabeledStatements, neither does the
mainbranch, though I think we could support labeled statement based on current approach.I recommend reviewing this PR commit by commit since the real diff is not that readable.