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 1 commit
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
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
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'
};
9 changes: 9 additions & 0 deletions test/form/samples/side-effects-switch-statements/_expected.js
View file
Open in desktop
Expand Up @@ -44,3 +44,12 @@ switch (globalThis.unknown) {
break;
case 'bar':
}

for (var i = 0; i < 4; i++) {
switch (i) {
case 0:
case 1:
continue;
}
effect();
}
9 changes: 9 additions & 0 deletions test/form/samples/side-effects-switch-statements/main.js
View file
Open in desktop
Expand Up @@ -65,3 +65,12 @@ switch (globalThis.unknown) {
case 'bar':
noEffect();
}

for (var i = 0; i < 4; i++) {
switch (i) {
case 0:
case 1:
continue;
}
effect();
}
3 changes: 3 additions & 0 deletions test/function/samples/switch-statement-nested-continue/_config.js
View file
Open in desktop
@@ -0,0 +1,3 @@
module.exports = {
description: 'treats continue different from break in switch statements'
};
25 changes: 25 additions & 0 deletions test/function/samples/switch-statement-nested-continue/main.js
View file
Open in desktop
@@ -0,0 +1,25 @@
function getValueContinue() {
for (var i = 0; i < 4; i++) {
switch (i) {
case 0:
case 1:
continue;
}
return i;
}
}

assert.strictEqual(getValueContinue(), 2);

function getValueBreak() {
for (var i = 0; i < 4; i++) {
switch (i) {
case 0:
case 1:
break;
}
return i;
}
}

assert.strictEqual(getValueBreak(), 0);