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

Do not tear apart declarations in loop or if bodies #3947

Merged
merged 1 commit into from Feb 2, 2021

Conversation

@lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Feb 2, 2021

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

As it turns out, partially tree-shaking a declaration while keeping side-effects, i.e.

// input
var foo = 1, unused = sideEffect(), bar = 2;

// output
var foo = 1; sideEffect(); var bar = 2;

will wreak havoc if the declaration is the body of a loop or if statement, e.g.

// input
if (condition) var foo = 1, unused = sideEffect(), bar = 2;

// broken output
if (condition) var foo = 1; sideEffect(); var bar = 2;

as it will replace the conditional side-effect with an unconditional one. Similar things apply to loops. The solution is to always include the identifier together with the initialiser in such a situation.

@rollup-bot
Copy link
Collaborator

@rollup-bot rollup-bot commented Feb 2, 2021

Thank you for your contribution! ❤️

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

npm install rollup/rollup#partially-tree-shaken-loop-body-declarations

or load it into the REPL:
https://rollupjs.org/repl/?circleci=14182

@codecov
Copy link

@codecov codecov bot commented Feb 2, 2021

Codecov Report

Merging #3947 (6911a37) into master (683cf48) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3947      +/-   ##
==========================================
- Coverage   97.18%   97.18%   -0.01%     
==========================================
  Files         188      188              
  Lines        6681     6678       -3     
  Branches     1946     1948       +2     
==========================================
- Hits         6493     6490       -3     
  Misses         99       99              
  Partials       89       89              
Impacted Files Coverage Δ
src/ast/nodes/VariableDeclarator.ts 100.00% <ø> (ø)
src/ast/nodes/shared/Node.ts 100.00% <ø> (ø)
src/ast/nodes/DoWhileStatement.ts 100.00% <100.00%> (ø)
src/ast/nodes/ForInStatement.ts 100.00% <100.00%> (ø)
src/ast/nodes/ForOfStatement.ts 100.00% <100.00%> (ø)
src/ast/nodes/ForStatement.ts 100.00% <100.00%> (ø)
src/ast/nodes/IfStatement.ts 100.00% <100.00%> (ø)
src/ast/nodes/VariableDeclaration.ts 98.00% <100.00%> (+0.02%) ⬆️
src/ast/nodes/WhileStatement.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 683cf48...6911a37. Read the comment docs.

@lukastaegert lukastaegert force-pushed the partially-tree-shaken-loop-body-declarations branch from d447972 to 6911a37 Feb 2, 2021
@lukastaegert lukastaegert merged commit 6705fdc into master Feb 2, 2021
8 of 9 checks passed
8 of 9 checks passed
@github-actions
14 (Windows)
Details
@github-actions
12 (Windows)
Details
@github-actions
10 (Windows)
Details
@codecov
codecov/project 97.18% (-0.01%) compared to 683cf48
Details
ci/circleci: analysis Your tests passed on CircleCI!
Details
ci/circleci: node-v10-latest Your tests passed on CircleCI!
Details
ci/circleci: node-v12-latest Your tests passed on CircleCI!
Details
ci/circleci: node-v14-latest Your tests passed on CircleCI!
Details
@codecov
codecov/patch 100.00% of diff hit (target 97.18%)
Details
@lukastaegert lukastaegert deleted the partially-tree-shaken-loop-body-declarations branch Feb 2, 2021
@@ -49,11 +49,11 @@ export default class ForInStatement extends StatementBase {

include(context: InclusionContext, includeChildrenRecursively: IncludeChildren) {
this.included = true;
this.left.includeAllDeclaredVariables(context, includeChildrenRecursively);
this.left.include(context, includeChildrenRecursively || true);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants