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

Better try statement tree shaking #3166

Merged
merged 8 commits into from Oct 18, 2019

Conversation

lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Oct 16, 2019

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 #3161
Resolves #3162
Resolves #3163
Resolves #3164

Description

This will no longer include a try-statement if the only side-effect can be found in the catch block. Also it will properly consider treeshake.tryCatchDeoptimization when checking for side-effects in the try block: If the optimization is off, the block is checked for side-effects, otherwise we check if it is non-empty.

// with treeshake.tryCatchDeoptimization: false this will be removed
try {
  const x = 'ignored';
} catch {
  console.log('side-effect');
}

Furthermore, this improves the switch statement rendering logic to prevent trailing empty lines. Also, trailing side-effect-free switch cases will be tree-shaken UNLESS there is a default case that is not the last case. I.e.

// input
switch (value) {
  case 1:
    console.log('effect);
    break;
  case 2: // these cases are safe to be removed
    const x = 'ignored';
    break;
  default:
}

switch (value) {
  default:
  case 1:
    console.log('effect);
    break;
  case 2: // we cannot remove these cases, otherwise they would trigger a side-effect
  case 3:
}

// output
switch (value) {
  case 1:
    console.log('effect);
    break;
}

switch (value) {
  default:
  case 1:
    console.log('effect);
    break;
  case 2: // we cannot remove these cases, otherwise they would trigger a side-effect
  case 3:
}

Lastly, continue statements are now properly differentiated from break statements inside switch statements to fix the issue mentioned here: #3164 (comment)

@codecov
Copy link

codecov bot commented Oct 16, 2019

Codecov Report

Merging #3166 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3166      +/-   ##
==========================================
+ Coverage   90.12%   90.17%   +0.05%     
==========================================
  Files         165      165              
  Lines        5873     5896      +23     
  Branches     1789     1794       +5     
==========================================
+ Hits         5293     5317      +24     
  Misses        350      350              
+ Partials      230      229       -1
Impacted Files Coverage Δ
src/ast/nodes/shared/FunctionNode.ts 100% <ø> (ø) ⬆️
src/ast/ExecutionContext.ts 100% <ø> (ø) ⬆️
src/ast/nodes/ArrowFunctionExpression.ts 100% <ø> (ø) ⬆️
src/ast/nodes/WhileStatement.ts 100% <100%> (ø) ⬆️
src/ast/nodes/SwitchCase.ts 100% <100%> (ø) ⬆️
src/ast/nodes/TryStatement.ts 100% <100%> (+6.66%) ⬆️
src/ast/nodes/ForStatement.ts 100% <100%> (ø) ⬆️
src/ast/nodes/ContinueStatement.ts 100% <100%> (ø) ⬆️
src/ast/nodes/ForInStatement.ts 100% <100%> (ø) ⬆️
src/ast/nodes/SwitchStatement.ts 100% <100%> (ø) ⬆️
... and 3 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 4fd918f...36bc84f. Read the comment docs.

@lukastaegert lukastaegert force-pushed the better-try-statement-tree-shaking branch from 416ce23 to 88d871f Compare October 17, 2019 05:20
@@ -3,6 +3,7 @@ import { PathTracker } from './utils/PathTracker';
import ThisVariable from './variables/ThisVariable';

interface ExecutionContextIgnore {
breakAndContinue: boolean;
breakStatements: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming this isn't a public interface, rather than having the overlap of concerns between breakAndContinue and breakStatements, I'd suggest to simplify this and have distinct breakStatements and continueStatements. Yes, the meaning of the terms would change and would require some more refactoring, but there'd be less cognitive overload.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was considering this and expect this version to be slightly more performant than the alternative in real world scenarios (loops without breaks are more common than loops with breaks and that way, you only need to check both flags for break statements while otherwise, you need to set and reset both flags for each kind of loop, which also results in more code) but I guess you are right that the advantage is negligible.

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