fix: handle conditional breaks in nested switch statement cases

[TrickyPi]

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

List any relevant issue numbers:
resolves #4930

Description

This PR add a new InclusionContext prop existedBroken. That way, the SwitchCase could return a right brokenFlow when it's consequent contain a break statement in the if condition. I'm not sure about this solution is good enough, looking forward to your thoughts.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
rollup	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 16, 2023 3:48pm

[codecov]

Codecov Report

Merging #4937 (d25e759) into master (ab33645) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4937      +/-   ##
==========================================
- Coverage   98.98%   98.98%   -0.01%     
==========================================
  Files         221      222       +1     
  Lines        8067     8047      -20     
  Branches     2214     2208       -6     
==========================================
- Hits         7985     7965      -20     
  Misses         28       28              
  Partials       54       54              

Impacted Files	Coverage Δ
src/ast/ExecutionContext.ts	100.00% <ø> (ø)
src/ast/nodes/BreakStatement.ts	100.00% <100.00%> (ø)
src/ast/nodes/ContinueStatement.ts	100.00% <100.00%> (ø)
src/ast/nodes/DoWhileStatement.ts	100.00% <100.00%> (ø)
src/ast/nodes/ForInStatement.ts	100.00% <100.00%> (ø)
src/ast/nodes/ForOfStatement.ts	100.00% <100.00%> (ø)
src/ast/nodes/ForStatement.ts	100.00% <100.00%> (ø)
src/ast/nodes/IfStatement.ts	100.00% <100.00%> (ø)
src/ast/nodes/ReturnStatement.ts	100.00% <100.00%> (ø)
src/ast/nodes/SwitchStatement.ts	100.00% <100.00%> (ø)
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[github-actions]

Thank you for your contribution! ❤️

You can try out this pull request locally by installing Rollup via

npm install TrickyPi/rollup#fix/switch-4930

or load it into the REPL:
https://deploy-preview-4937--rollupjs.netlify.app/repl/?pr=4937

[lukastaegert]

Thank you very much for this one, especially with the comprehensive test. I had to think a while for this one. While your solution definitely works, it felt strange to me to build special handling in both SwitchCase and IfStatement as the underlying problem seemed to be of a more general nature. My worry is that it is hard to figure out if we overlooked other scenarios and may therefore be harder to maintain in the future.
While thinking about this, I slightly modified the problem to use a labeled break and alas, this already worked flawlessly: https://rollupjs.org/repl/?version=3.20.2&shareable=JTdCJTIyZXhhbXBsZSUyMiUzQW51bGwlMkMlMjJtb2R1bGVzJTIyJTNBJTVCJTdCJTIyY29kZSUyMiUzQSUyMmZ1bmN0aW9uJTIwaXNzdWUob2JqKSUyMCU3QiU1Q24lMjAlMjBzd2l0Y2glMjAob2JqLmZpZWxkMSklMjAlN0IlNUNuJTIwJTIwJTIwJTIwY2FzZSUyMCU1QyUyMmJheiU1QyUyMiUzQSU1Q24lMjAlMjAlMjAlMjAlMjAlMjBicmVha0xhYmVsJTNBJTIwJTdCJTVDbiUyMCUyMCUyMCUyMCUyMCUyMCUyMCUyMHN3aXRjaCUyMChvYmouZmllbGQyKSUyMCU3QiU1Q24lMjAlMjAlMjAlMjAlMjAlMjAlMjAlMjAlMjAlMjBjYXNlJTIwJTVDJTIydmFsdWUlNUMlMjIlM0ElMjAlN0IlNUNuJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwaWYlMjAob2JqLmZpZWxkMSklMjAlN0IlNUNuJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwYnJlYWslMjBicmVha0xhYmVsJTNCJTVDbiUyMCUyMCUyMCUyMCUyMCUyMCUyMCUyMCUyMCUyMCUyMCUyMCU3RCU1Q24lMjAlMjAlMjAlMjAlMjAlMjAlMjAlMjAlMjAlMjAlMjAlMjB0aHJvdyUyMG5ldyUyMEVycm9yKCU2MGVycm9yJTIwMSU2MCklM0IlNUNuJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTdEJTVDbiUyMCUyMCUyMCUyMCUyMCUyMCUyMCUyMCUyMCUyMGRlZmF1bHQlM0ElNUNuJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwdGhyb3clMjBuZXclMjBFcnJvciglNjBlcnJvciUyMDIlNjApJTNCJTVDbiUyMCUyMCUyMCUyMCUyMCUyMCUyMCUyMCU3RCU1Q24lMjAlMjAlMjAlMjAlMjAlMjAlN0QlNUNuJTIwJTIwJTIwJTIwJTIwJTIwYnJlYWslM0IlMjAlMkYlMkYlMjBUaGlzJTIwc2hvdWxkbid0JTIwYmUlMjByZW1vdmVkJTVDbiUyMCUyMCUyMCUyMGRlZmF1bHQlM0ElNUNuJTIwJTIwJTIwJTIwJTIwJTIwdGhyb3clMjBuZXclMjBFcnJvciglNUMlMjJlcnJvciUyMDMlNUMlMjIpJTNCJTVDbiUyMCUyMCU3RCU1Q24lN0QlNUNuaXNzdWUoJTdCJTIwZmllbGQxJTNBJTIwJTVDJTIyYmF6JTVDJTIyJTJDJTIwZmllbGQyJTNBJTIwJTVDJTIydmFsdWUlNUMlMjIlMjAlN0QpJTNCJTIyJTJDJTIyaXNFbnRyeSUyMiUzQXRydWUlMkMlMjJuYW1lJTIyJTNBJTIybWFpbi5qcyUyMiU3RCU1RCUyQyUyMm9wdGlvbnMlMjIlM0ElN0IlN0QlN0Q=
Why is that? Because a labeled break does not use the special BROKEN_FLOW_BREAK_CONTINUE value but the same value as errors and returns. To figure out if this was a label, the LabeledStatement looks at the context.includedLabels. If the correct label is present, then we know that there was somewhere a break statement inside the labeled statement that used the correct label.
Now couldn't we just reuse this logic for breaks and continue? My first attempt was to use special label symbols for breaks and continues and have each switch statement or loop check and clean up those labels. However this cleaning up proved to be slightly cumbersome, so instead I added two flags that represent those states. I pushed this to your branch, please have a look if this makes sense to you.

[TrickyPi]

Thanks for sharing your process for fixing this issue, I learned something from it. And your solution is really better and more professional.

[TrickyPi]

BTW, the current PR title doesn't match the current code changes. When merging this PR, please correct the default commit message which may be the current PR title.

[rollup-bot]

This PR has been released as part of rollup@3.20.4. You can test it via npm install rollup.