Tracks side effects of thenables

[lukastaegert]

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

Description

This handles side-effects that happen because async functions and Promise utility functions access the then property of thenables. This happens by deoptimizing the global Promise helpers for now until we find a better solution. Async functions are also mostly deoptimized except if their return value has a side effect free then property, the same for awaited values.

While this may seem drastic, I would expect that most promise chains contain side effects anyway as they wrap some IO or other external process so not many people would notice this. It also fixes the last remaining issue we had with es6shim.

[github-actions]

Thank you for your contribution! ❤️

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

npm install rollup/rollup#gh-4102-promise-side-effects

or load it into the REPL:
https://rollupjs.org/repl/?pr=4115

[codecov]

Codecov Report

Merging #4115 (d529119) into master (c0ba3ee) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4115   +/-   ##
=======================================
  Coverage   98.11%   98.11%           
=======================================
  Files         201      201           
  Lines        7061     7066    +5     
  Branches     2064     2066    +2     
=======================================
+ Hits         6928     6933    +5     
  Misses         64       64           
  Partials       69       69           

Impacted Files	Coverage Δ
src/ast/nodes/shared/knownGlobals.ts	92.00% <ø> (ø)
src/ast/nodes/AwaitExpression.ts	100.00% <100.00%> (ø)
src/ast/nodes/shared/FunctionNode.ts	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c0ba3ee...d529119. Read the comment docs.

[kzc]

This happens by deoptimizing the global Promise helpers for now until we find a better solution.

+1

Correctness is always the way to go.

[stefcameron]

@lukastaegert Hi, 👋 , unfortunately, this is again another patch that breaks our builds by introducing unnecessary code from what Rollup now thinks is a side effect.

[stefcameron]

@lukastaegert Hi, 👋 , unfortunately, this is again another patch that breaks our builds by introducing unnecessary code from what Rollup now thinks is a side effect.

Sorry, spoke too soon. It's either #4111, #4112, or #4113 in the 2.50.4 patch that's causing an unintended inclusion of a library that does not have any side effects.

[stefcameron]

Based on the title, and some of our code, I'm going to guess it's #4113 so I will continue this over on #4113.

[shellscape]

@stefcameron ahoy hoy 👋 really appreciate the enthusiasm, but please use the Edit Reply capability rather than multiple replies in quick succession.

[stefcameron]

Sorry for the noise, just not fun having a build break as a result of a patch update that suddenly treats existing code as having side effects when it doesn't, but I was able to figure out what the issue was. I'm still not sure if this is Rollup incorrectly identifying something as being side effectful, or if it's Rollup finding existing code may have side effects it didn't realize it may have had in past versions (since I understand that can be hard to determine), so I'm not sure how to repro.

[lukastaegert]

Sorry for the noise, just not fun having a build break as a result of a patch update

In general I hope you will understand that we cannot make guarantees for the exact generated code even in patch releases. As we fix bugs, code may be rearranged or even some "wrong" code may be included.

For your particular issue, there was indeed still some room for improvement as the new deoptimization did not respect propertyReadSideEffects: false. Maybe #4119 resolves your issue.

[stefcameron]

@lukastaegert Of course, I understand it's hard, there are no guarantees, and I appreciate your work nonetheless. I maintain a few OSS packages myself, albeit smaller ones than Rollup, and it's hard when you're serving "the world". Still, it's a patch, and in Semver, that does come with a level of hope of non-breakage, and I've had Rollup patches fail on me before due to bugs introduced.

This time, though, it's not so clear if it's a Rollup bug, or if it's a result of Rollup fixing a bug and now correctly identifying new code that may have side effects when it wasn't before. I've been able to work around (or with) the issue.