New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: handle conditional breaks in nested switch statement cases #4937
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov Report
@@ 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
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
4b68421
to
0726171
Compare
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Thanks for sharing your process for fixing this issue, I learned something from it. And your solution is really better and more professional. |
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. |
This PR has been released as part of rollup@3.20.4. You can test it via |
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
resolves #4930
Description
This PR add a new
InclusionContext
propexistedBroken
. That way, theSwitchCase
could return a rightbrokenFlow
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.