fix: use a new includedLabels in the body of the LabeledStatement

[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 #5292

Description

[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	Dec 17, 2023 4:44am

[github-actions]

Thank you for your contribution! ❤️

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

npm install rollup/rollup#xiaopi-fix-5292

Notice: Ensure you have installed Rust nightly. If you haven't installed it yet, please first see https://www.rust-lang.org/tools/install to learn how to download Rustup and install Rust, then see https://rust-lang.github.io/rustup/concepts/channels.html to learn how to install Rust nightly.

or load it into the REPL:
https://rollup-n7uzbgb2d-rollup-js.vercel.app/repl/?pr=5297

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4081802) 98.82% compared to head (0c6efc1) 98.82%.

❗ Current head 0c6efc1 differs from pull request most recent head a0c0ad0. Consider uploading reports for the commit a0c0ad0 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5297   +/-   ##
=======================================
  Coverage   98.82%   98.82%           
=======================================
  Files         232      232           
  Lines        8968     8974    +6     
  Branches     2338     2338           
=======================================
+ Hits         8863     8869    +6     
  Misses         46       46           
  Partials       59       59           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lukastaegert]

I think the solution makes sense. There is one thing that worries me, though, but I could not produce an error from this: Usually, the label logic should be symmetric between hasEffect and include, which is no longer the case here.
The other thing is that I do not understand why I need a switch statement to create the error and cannot just create it with nested block scopes and functions.

[TrickyPi]

The other thing is that I do not understand why I need a switch statement to create the error and cannot just create it with nested block scopes and functions.

I was curious about it before debugging. After debugging, I think the reason is the order of BreakStatement and LabeledStatement. If the BreakStatement comes before LabeledStatement, it will cause this problem. Otherwise, it works. see https://rollupjs.org/repl/?version=4.9.0&shareable=JTdCJTIyZXhhbXBsZSUyMiUzQW51bGwlMkMlMjJtb2R1bGVzJTIyJTNBJTVCJTdCJTIyY29kZSUyMiUzQSUyMiU1Q25vdXRlciUzQSUyMCU3QiU1Q24lNUN0c3dpdGNoJTIwKGdsb2JhbFRoaXMudW5rbm93biklMjAlN0IlNUNuJTIwJTIwJTIwJTIwY2FzZSUyMDIlM0ElNUNuJTVDdCU1Q3QlNUN0YnJlYWslMjBvdXRlciUzQiU1Q24lNUN0JTVDdGNhc2UlMjAxJTNBJTVDbiU1Q3QlNUN0JTVDdCgoKSUyMCUzRCUzRSUyMCU3QiU1Q24lNUN0JTVDdCU1Q3QlNUN0b3V0ZXIlM0ElMjBjb25zb2xlLmxvZygncmV0YWluZWQnKSUzQiU1Q24lNUN0JTVDdCU1Q3QlN0QpKCklM0IlNUNuJTVDdCU3RCU1Q24lN0QlMjIlMkMlMjJpc0VudHJ5JTIyJTNBdHJ1ZSUyQyUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTdEJTVEJTJDJTIyb3B0aW9ucyUyMiUzQSU3QiUyMm91dHB1dCUyMiUzQSU3QiUyMmZvcm1hdCUyMiUzQSUyMmVzJTIyJTdEJTJDJTIydHJlZXNoYWtlJTIyJTNBdHJ1ZSU3RCU3RA==
Notice: the last SwitchCase is first executed in the include logic.

So why we cannot just create it with nested block scopes and functions? becuase we have no way to place the BreakStatement before the LabeledStatement in that case.

BTW, the If conditional can also produce this error, see https://rollupjs.org/repl/?version=4.9.0&shareable=JTdCJTIyZXhhbXBsZSUyMiUzQW51bGwlMkMlMjJtb2R1bGVzJTIyJTNBJTVCJTdCJTIyY29kZSUyMiUzQSUyMm91dGVyJTNBJTIwJTdCJTVDbiU1Q3RpZiUyMChnbG9iYWxUaGlzLnVua25vd24pJTIwJTdCJTVDbiU1Q3QlNUN0YnJlYWslMjBvdXRlciUzQiU1Q24lNUN0JTdEJTIwZWxzZSUyMCU3QiU1Q24lNUN0JTVDdCgoKSUyMCUzRCUzRSUyMCU3QiU1Q24lNUN0JTVDdCU1Q3RvdXRlciUzQSUyMGNvbnNvbGUubG9nKCdyZXRhaW5lZCcpJTNCJTVDbiU1Q3QlNUN0JTdEKSgpJTNCJTVDbiU1Q3QlN0QlNUNuJTdEJTIyJTJDJTIyaXNFbnRyeSUyMiUzQXRydWUlMkMlMjJuYW1lJTIyJTNBJTIybWFpbi5qcyUyMiU3RCU1RCUyQyUyMm9wdGlvbnMlMjIlM0ElN0IlMjJvdXRwdXQlMjIlM0ElN0IlMjJmb3JtYXQlMjIlM0ElMjJlcyUyMiU3RCUyQyUyMnRyZWVzaGFrZSUyMiUzQXRydWUlN0QlN0Q=

[TrickyPi]

There is one thing that worries me, though, but I could not produce an error from this:

Yeah, I also feel a bit worried about this 😨.

[lukastaegert]

Here is a situation where the asymmetry between hasEffects and include retains code that has no side effects: https://rollup-4yijtklk8-rollup-js.vercel.app/repl/?pr=5297&shareable=JTdCJTIyZXhhbXBsZSUyMiUzQW51bGwlMkMlMjJtb2R1bGVzJTIyJTNBJTVCJTdCJTIyY29kZSUyMiUzQSUyMm91dGVyJTNBJTIwJTdCJTVDbiU1Q3RpZiUyMChnbG9iYWxUaGlzLnVua25vd24pJTIwJTdCJTVDbiU1Q3QlNUN0YnJlYWslMjBvdXRlciUzQiU1Q24lNUN0JTdEJTIwZWxzZSUyMCU3QiU1Q24lNUN0JTVDdCgoKSUyMCUzRCUzRSUyMCU3QiU1Q24lNUN0JTVDdCU1Q3Rpbm5lciUzQSUyMCU3QiU1Q24lNUN0JTVDdCU1Q3QlNUN0b3V0ZXIlM0ElMjAlN0IlNUNuJTVDdCU1Q3QlNUN0JTVDdCU1Q3RicmVhayUyMGlubmVyJTNCJTVDbiU1Q3QlNUN0JTVDdCU1Q3QlN0QlNUNuJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTIwJTJGJTJGJTIwUmVtb3ZlJTIwdGhpcyUyMGxpbmUlMjBhbmQlMjBldmVyeXRoaW5nJTIwaXMlMjByZW1vdmVkJTVDbiU1Q3QlNUN0JTVDdCU1Q3Rsb2coJ2VmZmVjdCcpJTNCJTVDbiU1Q3QlNUN0JTVDdCU3RCU1Q24lNUN0JTVDdCU3RCkoKSUzQiU1Q24lNUN0JTdEJTVDbiU3RCUyMiUyQyUyMmlzRW50cnklMjIlM0F0cnVlJTJDJTIybmFtZSUyMiUzQSUyMm1haW4uanMlMjIlN0QlNUQlMkMlMjJvcHRpb25zJTIyJTNBJTdCJTIyb3V0cHV0JTIyJTNBJTdCJTIyZm9ybWF0JTIyJTNBJTIyZXMlMjIlN0QlMkMlMjJ0cmVlc2hha2UlMjIlM0F0cnVlJTdEJTdE

This is easily fixed by adding the same logic to hasEffects that uses a fresh Set of included labels when checking effects of children.

If you add this test and fix, I will merge this because essentially, your solution IS the right one.

[github-actions]

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