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

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 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

@@           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.

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=

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 😨.

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.

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