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

Handle getters on functions and improve property deoptimization #4493

Merged
merged 8 commits into from May 13, 2022

Conversation

lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented May 11, 2022

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:
Closes #4492

Description

This PR supersedes #4492 as it reimplements the logic in a nicer way. At the core, this PR does two things:

  • Use the full object handling logic for functions and arrow functions. This allows to properly track the creation of getters on those: before - after
  • Differentiate assignment handling so that Rollup does no longer assume that reassigning an unknown property can create a getter. before - after
  • Add special handling to Object.defineProperty so that those functions are not counted as side effects unless the mutated object is used. Note that this ignores some errors that could be thrown by those functions, but this should not cause issues as long as code that relies on those errors wraps them in try-catch, see also #4492 .

Architecturally, the last two points are achieved by adding a new type of "unknown property key" in the logic, UnknownNonAccessorKey, that works similar to UnknownIntegerKey in that it is treated as unknown but we do not assume that reassignments to that key can create setters or getters and that we do not assume there will be setter side effects when reassigning that key.
For the function improvements, we now use ObjectEntity as a shared base for all object-like literals, i.e. objects, classes, functions (including arrow functions) and arrays. This will also make it easier to avoid code duplication for future features.

@github-actions
Copy link

@github-actions github-actions bot commented May 11, 2022

Thank you for your contribution! ❤️

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

npm install rollup/rollup#function-getter-side-effects

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

@codecov
Copy link

@codecov codecov bot commented May 11, 2022

Codecov Report

Merging #4493 (9ecae34) into master (8c6e0f3) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4493      +/-   ##
==========================================
+ Coverage   98.72%   98.74%   +0.02%     
==========================================
  Files         206      207       +1     
  Lines        7351     7343       -8     
  Branches     2086     2082       -4     
==========================================
- Hits         7257     7251       -6     
+ Misses         34       33       -1     
+ Partials       60       59       -1     
Impacted Files Coverage Δ
src/ast/nodes/ArrowFunctionExpression.ts 100.00% <100.00%> (ø)
src/ast/nodes/MemberExpression.ts 100.00% <100.00%> (ø)
src/ast/nodes/shared/FunctionBase.ts 100.00% <100.00%> (ø)
src/ast/nodes/shared/FunctionNode.ts 100.00% <100.00%> (ø)
src/ast/nodes/shared/ObjectEntity.ts 100.00% <100.00%> (ø)
src/ast/nodes/shared/knownGlobals.ts 100.00% <100.00%> (+8.00%) ⬆️
src/ast/utils/PathTracker.ts 100.00% <100.00%> (ø)
src/ast/variables/GlobalVariable.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 8c6e0f3...9ecae34. Read the comment docs.

@lukastaegert lukastaegert merged commit f3a1fa3 into master May 13, 2022
12 checks passed
@lukastaegert lukastaegert deleted the function-getter-side-effects branch May 13, 2022
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.

None yet

1 participant