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.

Sign up for GitHub

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

Jump to bottom

Better try statement tree shaking #3166

Merged
merged 8 commits into from Oct 18, 2019
Merged

Better try statement tree shaking #3166

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4372c7c
Do not consider catch clause side-effects, better respect false try-c…
lukastaegert Oct 16, 2019
9a5bb13
Suppress trailing blank lines in switch statements
lukastaegert Oct 16, 2019
3dee511
Slightly improve coverage
lukastaegert Oct 16, 2019
ef98fa4
Omit side-effect free cases in switch-statements
lukastaegert Oct 16, 2019
34269b0
Improve coverage
lukastaegert Oct 16, 2019
7f01ea8
Differentiate between break and continue inside switch statements
lukastaegert Oct 17, 2019
88d871f
Update dependencies. Only log that the license has updated when it
lukastaegert Oct 17, 2019
36bc84f
Separate break and continue statements in execution context
lukastaegert Oct 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/ast/ExecutionContext.ts
View file
Open in desktop
Expand Up @@ -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.

labels: Set<string>;
returnAwaitYield: boolean;
Expand Down Expand Up @@ -39,6 +40,7 @@ export function createHasEffectsContext(): HasEffectsContext {
breakFlow: BREAKFLOW_NONE,
called: new PathTracker(),
ignore: {
breakAndContinue: false,
breakStatements: false,
labels: new Set(),
returnAwaitYield: false
Expand Down
1 change: 1 addition & 0 deletions src/ast/nodes/ArrowFunctionExpression.ts
View file
Open in desktop
Expand Up @@ -58,6 +58,7 @@ export default class ArrowFunctionExpression extends NodeBase {
}
const { ignore, breakFlow } = context;
context.ignore = {
breakAndContinue: false,
breakStatements: false,
labels: new Set(),
returnAwaitYield: true
Expand Down
6 changes: 5 additions & 1 deletion src/ast/nodes/BreakStatement.ts
View file
Open in desktop
Expand Up @@ -8,7 +8,11 @@ export default class BreakStatement extends StatementBase {
type!: NodeType.tBreakStatement;

hasEffects(context: HasEffectsContext) {
if (!(this.label ? context.ignore.labels.has(this.label.name) : context.ignore.breakStatements))
if (
!(this.label
? context.ignore.labels.has(this.label.name)
: context.ignore.breakStatements || context.ignore.breakAndContinue)
)
return true;
context.breakFlow = new Set([this.label && this.label.name]);
return false;
Expand Down
4 changes: 3 additions & 1 deletion src/ast/nodes/ContinueStatement.ts
View file
Open in desktop
Expand Up @@ -8,7 +8,9 @@ export default class ContinueStatement extends StatementBase {
type!: NodeType.tContinueStatement;

hasEffects(context: HasEffectsContext) {
if (!(this.label ? context.ignore.labels.has(this.label.name) : context.ignore.breakStatements))
if (
!(this.label ? context.ignore.labels.has(this.label.name) : context.ignore.breakAndContinue)
)
return true;
context.breakFlow = new Set([this.label && this.label.name]);
return false;
Expand Down
6 changes: 3 additions & 3 deletions src/ast/nodes/DoWhileStatement.ts
View file
Open in desktop
Expand Up @@ -11,11 +11,11 @@ export default class DoWhileStatement extends StatementBase {
if (this.test.hasEffects(context)) return true;
const {
breakFlow,
ignore: { breakStatements }
ignore: { breakAndContinue }
} = context;
context.ignore.breakStatements = true;
context.ignore.breakAndContinue = true;
if (this.body.hasEffects(context)) return true;
context.ignore.breakStatements = breakStatements;
context.ignore.breakAndContinue = breakAndContinue;
if (context.breakFlow instanceof Set && context.breakFlow.has(null)) {
context.breakFlow = breakFlow;
}
Expand Down
6 changes: 3 additions & 3 deletions src/ast/nodes/ForInStatement.ts
View file
Open in desktop
Expand Up @@ -36,11 +36,11 @@ export default class ForInStatement extends StatementBase {
return true;
const {
breakFlow,
ignore: { breakStatements }
ignore: { breakAndContinue }
} = context;
context.ignore.breakStatements = true;
context.ignore.breakAndContinue = true;
if (this.body.hasEffects(context)) return true;
context.ignore.breakStatements = breakStatements;
context.ignore.breakAndContinue = breakAndContinue;
context.breakFlow = breakFlow;
return false;
}
Expand Down
6 changes: 3 additions & 3 deletions src/ast/nodes/ForStatement.ts
View file
Open in desktop
Expand Up @@ -27,11 +27,11 @@ export default class ForStatement extends StatementBase {
return true;
const {
breakFlow,
ignore: { breakStatements }
ignore: { breakAndContinue }
} = context;
context.ignore.breakStatements = true;
context.ignore.breakAndContinue = true;
if (this.body.hasEffects(context)) return true;
context.ignore.breakStatements = breakStatements;
context.ignore.breakAndContinue = breakAndContinue;
context.breakFlow = breakFlow;
return false;
}
Expand Down
14 changes: 12 additions & 2 deletions src/ast/nodes/SwitchCase.ts
View file
Open in desktop
@@ -1,6 +1,7 @@
import MagicString from 'magic-string';
import {
findFirstOccurrenceOutsideComment,
NodeRenderOptions,
RenderOptions,
renderStatementList
} from '../../utils/renderHelpers';
Expand All @@ -10,6 +11,7 @@ import { ExpressionNode, IncludeChildren, NodeBase, StatementNode } from './shar

export default class SwitchCase extends NodeBase {
consequent!: StatementNode[];
needsBoundaries!: true;
test!: ExpressionNode | null;
type!: NodeType.tSwitchCase;

Expand All @@ -31,16 +33,24 @@ export default class SwitchCase extends NodeBase {
}
}

render(code: MagicString, options: RenderOptions) {
render(code: MagicString, options: RenderOptions, nodeRenderOptions?: NodeRenderOptions) {
if (this.consequent.length) {
this.test && this.test.render(code, options);
const testEnd = this.test
? this.test.end
: findFirstOccurrenceOutsideComment(code.original, 'default', this.start) + 7;
const consequentStart = findFirstOccurrenceOutsideComment(code.original, ':', testEnd) + 1;
renderStatementList(this.consequent, code, consequentStart, this.end, options);
renderStatementList(
this.consequent,
code,
consequentStart,
(nodeRenderOptions as NodeRenderOptions).end as number,
options
);
} else {
super.render(code, options);
}
}
}

SwitchCase.prototype.needsBoundaries = true;
52 changes: 42 additions & 10 deletions src/ast/nodes/SwitchStatement.ts
View file
Open in desktop
@@ -1,7 +1,10 @@
import MagicString from 'magic-string';
import { RenderOptions, renderStatementList } from '../../utils/renderHelpers';
import {
BreakFlow,
BREAKFLOW_ERROR_RETURN,
BREAKFLOW_NONE,
createHasEffectsContext,
HasEffectsContext,
InclusionContext
} from '../ExecutionContext';
Expand Down Expand Up @@ -34,6 +37,8 @@ export default class SwitchStatement extends StatementBase {
discriminant!: ExpressionNode;
type!: NodeType.tSwitchStatement;

private defaultCase!: number | null;

createScope(parentScope: Scope) {
this.scope = new BlockScope(parentScope);
}
Expand All @@ -44,16 +49,14 @@ export default class SwitchStatement extends StatementBase {
breakFlow,
ignore: { breakStatements }
} = context;
let hasDefault = false;
let minBreakFlow: BreakFlow | false = BREAKFLOW_ERROR_RETURN;
context.ignore.breakStatements = true;
for (const switchCase of this.cases) {
if (switchCase.hasEffects(context)) return true;
if (switchCase.test === null) hasDefault = true;
minBreakFlow = getMinBreakflowAfterCase(minBreakFlow, context);
context.breakFlow = breakFlow;
}
if (hasDefault && !(minBreakFlow instanceof Set && minBreakFlow.has(null))) {
if (this.defaultCase !== null && !(minBreakFlow instanceof Set && minBreakFlow.has(null))) {
context.breakFlow = minBreakFlow;
}
context.ignore.breakStatements = breakStatements;
Expand All @@ -64,16 +67,45 @@ export default class SwitchStatement extends StatementBase {
this.included = true;
this.discriminant.include(context, includeChildrenRecursively);
const { breakFlow } = context;
let hasDefault = false;
let minBreakFlow: BreakFlow | false = BREAKFLOW_ERROR_RETURN;
for (const switchCase of this.cases) {
if (switchCase.test === null) hasDefault = true;
switchCase.include(context, includeChildrenRecursively);
minBreakFlow = getMinBreakflowAfterCase(minBreakFlow, context);
context.breakFlow = breakFlow;
let isCaseIncluded =
includeChildrenRecursively ||
(this.defaultCase !== null && this.defaultCase < this.cases.length - 1);
for (let caseIndex = this.cases.length - 1; caseIndex >= 0; caseIndex--) {
const switchCase = this.cases[caseIndex];
if (switchCase.included) {
isCaseIncluded = true;
}
if (!isCaseIncluded) {
const hasEffectsContext = createHasEffectsContext();
hasEffectsContext.ignore.breakStatements = true;
isCaseIncluded = switchCase.hasEffects(hasEffectsContext);
}
if (isCaseIncluded) {
switchCase.include(context, includeChildrenRecursively);
minBreakFlow = getMinBreakflowAfterCase(minBreakFlow, context);
context.breakFlow = breakFlow;
}
}
if (hasDefault && !(minBreakFlow instanceof Set && minBreakFlow.has(null))) {
if (this.defaultCase !== null && !(minBreakFlow instanceof Set && minBreakFlow.has(null))) {
context.breakFlow = minBreakFlow;
}
}

initialise() {
for (let caseIndex = 0; caseIndex < this.cases.length; caseIndex++) {
if (this.cases[caseIndex].test === null) {
this.defaultCase = caseIndex;
return;
}
}
this.defaultCase = null;
}

render(code: MagicString, options: RenderOptions) {
this.discriminant.render(code, options);
if (this.cases.length > 0) {
renderStatementList(this.cases, code, this.cases[0].start, this.end - 1, options);
}
}
}
5 changes: 3 additions & 2 deletions src/ast/nodes/TryStatement.ts
View file
Open in desktop
Expand Up @@ -14,8 +14,9 @@ export default class TryStatement extends StatementBase {

hasEffects(context: HasEffectsContext): boolean {
return (
this.block.body.length > 0 ||
(this.handler !== null && this.handler.hasEffects(context)) ||
(this.context.tryCatchDeoptimization
? this.block.body.length > 0
: this.block.hasEffects(context)) ||
(this.finalizer !== null && this.finalizer.hasEffects(context))
);
}
Expand Down
6 changes: 3 additions & 3 deletions src/ast/nodes/WhileStatement.ts
View file
Open in desktop
Expand Up @@ -11,11 +11,11 @@ export default class WhileStatement extends StatementBase {
if (this.test.hasEffects(context)) return true;
const {
breakFlow,
ignore: { breakStatements }
ignore: { breakAndContinue }
} = context;
context.ignore.breakStatements = true;
context.ignore.breakAndContinue = true;
if (this.body.hasEffects(context)) return true;
context.ignore.breakStatements = breakStatements;
context.ignore.breakAndContinue = breakAndContinue;
context.breakFlow = breakFlow;
return false;
}
Expand Down
1 change: 1 addition & 0 deletions src/ast/nodes/shared/FunctionNode.ts
View file
Open in desktop
Expand Up @@ -74,6 +74,7 @@ export default class FunctionNode extends NodeBase {
);
const { breakFlow, ignore } = context;
context.ignore = {
breakAndContinue: false,
breakStatements: false,
labels: new Set(),
returnAwaitYield: true
Expand Down
11 changes: 0 additions & 11 deletions test/form/samples/break-control-flow/break-statement-labels-switch/_expected.js
View file
Open in desktop
Expand Up @@ -3,15 +3,12 @@ function returnAll() {
case 1:
console.log('retained');
return;

case 2:
console.log('retained');
return;

default:
console.log('retained');
return;

}
}

Expand All @@ -25,10 +22,8 @@ function returnNoDefault() {
switch (globalThis.unknown) {
case 1:
return;

case 2:
return;

}
console.log('retained');
}
Expand All @@ -39,13 +34,10 @@ function returnSomeBreak() {
switch (globalThis.unknown) {
case 1:
return;

case 2:
break;

default:
return;

}
console.log('retained');
}
Expand All @@ -64,13 +56,10 @@ function returnBreakDifferentLabels() {
switch (globalThis.unknown) {
case 1:
break outer;

case 2:
break inner;

default:
break outer;

}
}
console.log('retained');
Expand Down
3 changes: 0 additions & 3 deletions test/form/samples/break-control-flow/switch-errors/_expected.js
View file
Open in desktop
@@ -1,10 +1,8 @@
switch (globalThis.unknown) {
case 1:
throw new Error();

case 2:
throw new Error();

case 3:
console.log('retained');
default:
Expand All @@ -22,5 +20,4 @@ switch (globalThis.unknown) {
case 3:
var hoisted3;
default:

}
1 change: 0 additions & 1 deletion test/form/samples/render-removed-statements/_expected.js
View file
Open in desktop
Expand Up @@ -46,7 +46,6 @@ switch (globalThis.unknown) {
case 3: // retained
// lead retained
console.log(2); // trail retained

case 4: /* lead retained */ console.log('3'); // trail retained
default: // retained
/* lead retained */
Expand Down
3 changes: 1 addition & 2 deletions test/form/samples/side-effects-switch-statements/_config.js
View file
Open in desktop
@@ -1,4 +1,3 @@
module.exports = {
description: 'switch statements should be correctly tree-shaken',
options: { output: { name: 'myBundle' } }
description: 'switch statements should be correctly tree-shaken'
};