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

Refactor side effect handling for property interactions #4522

Merged
merged 13 commits into from Jun 7, 2022

Conversation

lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Jun 7, 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:

Description

This will refactor and simplify the mechanism for checking property side effects (read, write, call) to use a single function hasEffectsOnInteractionAtPath that uses "interaction" objects as parameters that are also used by deoptimizeThisOnInteractionAtPath (formerly "eventAtPath).
These interaction objects have a type to determine if it is a read, write or call. They also have a thisArg that is used for deoptimizations but may also be used for other purposes in the future. Besides depending on the interaction type, they may also specify args. For writes, args is an array with a single element to make it easy for setters to convert writes to calls.
While this is mostly a refactoring, it will hopefully make the algorithm slightly more approachable, but it will more importantly lay the ground work for some future improvements.

Also, some subtle bugs were fixed as setters did not properly deoptimize their this value when called in updated expressions or for-in/of loops Previous version REPLFIxed REPL

@github-actions
Copy link

github-actions bot commented Jun 7, 2022

Thank you for your contribution! ❤️

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

npm install rollup/rollup#refactor-interactions

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

@codecov
Copy link

codecov bot commented Jun 7, 2022

Codecov Report

Merging #4522 (605c701) into master (35e7657) will increase coverage by 0.12%.
The diff coverage is 99.36%.

@@            Coverage Diff             @@
##           master    #4522      +/-   ##
==========================================
+ Coverage   98.76%   98.89%   +0.12%     
==========================================
  Files         209      208       -1     
  Lines        7395     7332      -63     
  Branches     2108     2094      -14     
==========================================
- Hits         7304     7251      -53     
+ Misses         32       26       -6     
+ Partials       59       55       -4     
Impacted Files Coverage Δ
src/ast/nodes/VariableDeclaration.ts 98.00% <ø> (ø)
src/ast/nodes/shared/MethodTypes.ts 92.85% <84.61%> (+3.96%) ⬆️
src/ast/NodeInteractions.ts 100.00% <100.00%> (ø)
src/ast/nodes/ArrayExpression.ts 100.00% <100.00%> (ø)
src/ast/nodes/ArrayPattern.ts 100.00% <100.00%> (ø)
src/ast/nodes/ArrowFunctionExpression.ts 94.73% <100.00%> (+0.29%) ⬆️
src/ast/nodes/AssignmentExpression.ts 100.00% <100.00%> (ø)
src/ast/nodes/AssignmentPattern.ts 100.00% <100.00%> (+11.11%) ⬆️
src/ast/nodes/BinaryExpression.ts 100.00% <100.00%> (ø)
src/ast/nodes/CallExpression.ts 100.00% <100.00%> (ø)
... and 40 more

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 35e7657...605c701. Read the comment docs.

@lukastaegert lukastaegert merged commit c443303 into master Jun 7, 2022
@lukastaegert lukastaegert deleted the refactor-interactions branch June 7, 2022 14:37
pos777 pushed a commit to pos777/rollup that referenced this pull request Jun 18, 2022
* Rename event -> interaction

* Refactor MemberExpression assignment logic to prepare for new interactions

* Use objects for interactions

* Add additional interaction parameters

* Pick "this"-argument from interaction

* Use interaction for getReturnExpression

* Replace dedicated methods with hasEffectsOnInteractionAtPath

* Fix accessor handling for loops and updated expressions

* Simplify assignment handling

* Change assignment to use args property

* Simplify ObjectEntity effect handling

* Cache remaining interactions that may be cached

* Improve coverage
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