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

Tree shake parameter defaults #4498

Merged
merged 15 commits into from May 19, 2022
Merged

Tree shake parameter defaults #4498

merged 15 commits into from May 19, 2022

Conversation

lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented May 13, 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:
Resolves #4466

Description

This allows tree-shaking for unnecessary default parameters in functions. This includes arrow functions but does not include nested defaults in destructuring patterns REPL

// input
const foo = (a = 'removed', { b = 'unused but included' } = 'removed') =>
  console.log(a,b);
foo('a', 'b');

// output
const foo = (a, { b = 'unused but included' }) =>
  console.log(a,b);
foo('a', 'b');

While the feature may sound simple at first glance, it was actually quite complex. The main problem is that JavaScript does not make a difference between not passing a parameter to a function and passing the value undefined: In both cases, the default value will be used.

Therefore, there are quite some subtleties involved which in the end I resolved by extending the mechanism I created for omitting unneeded parameters. This mechanism was removing arguments from function calls if the corresponding parameter was unused. Now, it will also see if parameter defaults need to be included. Care was taken to ensure no side effects are omitted REPL

// input
const foo = (a = 'removed because unused') => effect();
foo();

const bar = (a = effect('included because triggered')) => {};
// It does not make a difference if you pass nothing or undefined
bar(undefined);

const baz = (a = effect('removed')) => {};
baz('value');

// output
const foo = (a) => effect();
foo();

const bar = (a = effect('included because triggered')) => {};
// It does not make a difference if you pass nothing or undefined
bar();

const baz = (a) => {};
baz();

It is very important to track the instance where we "lose track" of how a function is used, e.g. because it is exported, passed to an unknown function etc. In those cases, all necessary defaults are included REPL

// input
export const foo = (a = 'included') => console.log(a);

// output
const foo = (a = 'included') => console.log(a);

export { foo };

@lukastaegert lukastaegert force-pushed the tree-shake-parameter-defaults branch from ae3658b to df42bb5 Compare May 13, 2022
@github-actions
Copy link

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

Thank you for your contribution! ❤️

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

npm install rollup/rollup#tree-shake-parameter-defaults

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

@lukastaegert lukastaegert force-pushed the tree-shake-parameter-defaults branch 2 times, most recently from 4406233 to 115d3b2 Compare May 14, 2022
* Use an inclusion parameter instead of separate methods to handle this
* Make call argument inclusion path aware so that in the future, we can use
  this mechanism to determine possible values for parameters for deoptimization
* Support parameter default treeshaking for functions, object and class methods,
  functions in array tuples and template tags
* Use full CallExpression logic for TaggedTemplateExpression
@lukastaegert lukastaegert force-pushed the tree-shake-parameter-defaults branch from 115d3b2 to f450178 Compare May 14, 2022
@lukastaegert lukastaegert force-pushed the tree-shake-parameter-defaults branch from f450178 to 1888f1e Compare May 14, 2022
@codecov
Copy link

@codecov codecov bot commented May 14, 2022

Codecov Report

Merging #4498 (d035821) into master (05d294f) will increase coverage by 0.01%.
The diff coverage is 98.40%.

@@            Coverage Diff             @@
##           master    #4498      +/-   ##
==========================================
+ Coverage   98.74%   98.76%   +0.01%     
==========================================
  Files         207      208       +1     
  Lines        7357     7433      +76     
  Branches     2084     2118      +34     
==========================================
+ Hits         7265     7341      +76     
- Misses         33       34       +1     
+ Partials       59       58       -1     
Impacted Files Coverage Δ
src/ast/nodes/ArrowFunctionExpression.ts 100.00% <ø> (ø)
src/ast/nodes/AssignmentExpression.ts 100.00% <ø> (ø)
src/ast/nodes/AwaitExpression.ts 100.00% <ø> (ø)
src/ast/nodes/ExpressionStatement.ts 87.50% <ø> (ø)
src/ast/nodes/Property.ts 100.00% <ø> (ø)
src/ast/nodes/SequenceExpression.ts 97.14% <ø> (ø)
src/ast/nodes/SpreadElement.ts 100.00% <ø> (ø)
src/ast/nodes/shared/MethodTypes.ts 89.65% <ø> (+6.89%) ⬆️
src/ast/nodes/shared/ObjectMember.ts 100.00% <ø> (+10.00%) ⬆️
src/ast/scopes/FunctionScope.ts 100.00% <ø> (ø)
... and 42 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 05d294f...d035821. Read the comment docs.

@lukastaegert lukastaegert marked this pull request as ready for review May 17, 2022
@lukastaegert
Copy link
Member Author

@lukastaegert lukastaegert commented May 17, 2022

This is ready for review now

@lukastaegert lukastaegert merged commit 4624917 into master May 19, 2022
11 of 12 checks passed
@lukastaegert lukastaegert deleted the tree-shake-parameter-defaults branch May 19, 2022
lukastaegert added a commit that referenced this issue May 19, 2022
@bitjson
Copy link
Contributor

@bitjson bitjson commented May 20, 2022

For anyone else following – this seems to be the issue behind the revert: #4505

@lukastaegert
Copy link
Member Author

@lukastaegert lukastaegert commented May 21, 2022

There were actually two more issues. So far so I identified the causes but could not find time to fix stuff yet du to family things, so I reverted to get more time without people reporting the same issues again.

@lukastaegert
Copy link
Member Author

@lukastaegert lukastaegert commented May 26, 2022

There is a new attempt that does this more conservatively at #4510

lukastaegert added a commit that referenced this issue May 27, 2022
* Recreate parameter tree-shaking but keep boolean return values

* Properly include parameters when tree-shaking is disabled

Also improve side effect detection for unused parameter defaults

* Make null checks more compact

* Include super class parameter defaults

* Include return value parameter defaults

* Limit feature to top-level functions

* Remove path from deoptimization signature

* Ensure class methods are deoptimized

* Add comment

* Class fields need Node 12

* Improve coverage

* Improve coverage
@lukastaegert
Copy link
Member Author

@lukastaegert lukastaegert commented Jun 1, 2022

To everyone following this, I decided to roll back the feature for good as it turned out to be far too brittle with far too much overhead for my liking at the moment, see #4520

pos777 pushed a commit to pos777/rollup that referenced this issue Jun 18, 2022
* Recreate parameter tree-shaking but keep boolean return values

* Properly include parameters when tree-shaking is disabled

Also improve side effect detection for unused parameter defaults

* Make null checks more compact

* Include super class parameter defaults

* Include return value parameter defaults

* Limit feature to top-level functions

* Remove path from deoptimization signature

* Ensure class methods are deoptimized

* Add comment

* Class fields need Node 12

* Improve coverage

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

2 participants