Skip to content
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

Tracks side effects of thenables #4115

Merged
merged 2 commits into from May 30, 2021
Merged

Conversation

lukastaegert
Copy link
Member

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

github-actions bot commented May 30, 2021

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

codecov bot commented May 30, 2021

Codecov Report

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

Impacted file tree graph

@@           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
Copy link
Contributor

kzc commented May 30, 2021

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

+1

Correctness is always the way to go.

@lukastaegert lukastaegert merged commit e9c7495 into master May 30, 2021
@lukastaegert lukastaegert deleted the gh-4102-promise-side-effects branch May 30, 2021 18:42
@stefcameron
Copy link

@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
Copy link

@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
Copy link

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
Copy link
Contributor

shellscape commented Jun 1, 2021

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

@stefcameron
Copy link

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
Copy link
Member Author

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

thenables in object literals erroneously dropped with accompanying async/await/Promise
4 participants