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
[WIP] handle assignments in dead code #3212
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,11 @@ | ||
import MagicString from 'magic-string'; | ||
import { findFirstOccurrenceOutsideComment, RenderOptions } from '../../utils/renderHelpers'; | ||
import { getSystemExportStatement } from '../../utils/systemJsRendering'; | ||
import { HasEffectsContext } from '../ExecutionContext'; | ||
import { HasEffectsContext, InclusionContext } from '../ExecutionContext'; | ||
import { EMPTY_PATH, ObjectPath, UNKNOWN_PATH } from '../utils/PathTracker'; | ||
import Variable from '../variables/Variable'; | ||
import * as NodeType from './NodeType'; | ||
import { ExpressionNode, NodeBase } from './shared/Node'; | ||
import { ExpressionNode, IncludeChildren, NodeBase } from './shared/Node'; | ||
import { PatternNode } from './shared/Pattern'; | ||
|
||
export default class AssignmentExpression extends NodeBase { | ||
|
@@ -26,15 +26,10 @@ export default class AssignmentExpression extends NodeBase { | |
| '**='; | ||
right!: ExpressionNode; | ||
type!: NodeType.tAssignmentExpression; | ||
|
||
bind() { | ||
super.bind(); | ||
this.left.deoptimizePath(EMPTY_PATH); | ||
// We cannot propagate mutations of the new binding to the old binding with certainty | ||
this.right.deoptimizePath(UNKNOWN_PATH); | ||
} | ||
private deoptimized = false; | ||
|
||
hasEffects(context: HasEffectsContext): boolean { | ||
if (!this.deoptimized) this.applyDeoptimizations(); | ||
return ( | ||
this.right.hasEffects(context) || | ||
this.left.hasEffects(context) || | ||
|
@@ -46,6 +41,13 @@ export default class AssignmentExpression extends NodeBase { | |
return path.length > 0 && this.right.hasEffectsWhenAccessedAtPath(path, context); | ||
} | ||
|
||
include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void { | ||
if (!this.deoptimized) this.applyDeoptimizations(); | ||
this.included = true; | ||
this.left.include(context, includeChildrenRecursively); | ||
this.right.include(context, includeChildrenRecursively); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar reasons, we could also call |
||
|
||
render(code: MagicString, options: RenderOptions) { | ||
this.left.render(code, options); | ||
this.right.render(code, options); | ||
|
@@ -79,4 +81,10 @@ export default class AssignmentExpression extends NodeBase { | |
} | ||
} | ||
} | ||
|
||
private applyDeoptimizations() { | ||
this.deoptimized = true; | ||
this.left.deoptimizePath(EMPTY_PATH); | ||
this.right.deoptimizePath(UNKNOWN_PATH); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -8,27 +8,26 @@ import { LiteralValueOrUnknown, UnknownValue } from '../values'; | |||||||||||
import * as NodeType from './NodeType'; | ||||||||||||
import { ExpressionNode, IncludeChildren, StatementBase, StatementNode } from './shared/Node'; | ||||||||||||
|
||||||||||||
const unset = Symbol('unset'); | ||||||||||||
|
||||||||||||
export default class IfStatement extends StatementBase implements DeoptimizableEntity { | ||||||||||||
alternate!: StatementNode | null; | ||||||||||||
consequent!: StatementNode; | ||||||||||||
test!: ExpressionNode; | ||||||||||||
type!: NodeType.tIfStatement; | ||||||||||||
|
||||||||||||
private testValue: LiteralValueOrUnknown; | ||||||||||||
|
||||||||||||
bind() { | ||||||||||||
super.bind(); | ||||||||||||
// ensure the testValue is set for the tree-shaking passes | ||||||||||||
this.testValue = this.test.getLiteralValueAtPath(EMPTY_PATH, SHARED_RECURSION_TRACKER, this); | ||||||||||||
} | ||||||||||||
private testValue: LiteralValueOrUnknown | typeof unset = unset; | ||||||||||||
|
||||||||||||
deoptimizeCache() { | ||||||||||||
this.testValue = UnknownValue; | ||||||||||||
} | ||||||||||||
|
||||||||||||
hasEffects(context: HasEffectsContext): boolean { | ||||||||||||
if (this.test.hasEffects(context)) return true; | ||||||||||||
if (this.testValue === UnknownValue) { | ||||||||||||
if (this.test.hasEffects(context)) { | ||||||||||||
return true; | ||||||||||||
} | ||||||||||||
const testValue = this.getTestValue(); | ||||||||||||
if (testValue === UnknownValue) { | ||||||||||||
const { brokenFlow } = context; | ||||||||||||
if (this.consequent.hasEffects(context)) return true; | ||||||||||||
const consequentBrokenFlow = context.brokenFlow; | ||||||||||||
|
@@ -39,7 +38,7 @@ export default class IfStatement extends StatementBase implements DeoptimizableE | |||||||||||
context.brokenFlow < consequentBrokenFlow ? context.brokenFlow : consequentBrokenFlow; | ||||||||||||
return false; | ||||||||||||
} | ||||||||||||
return this.testValue | ||||||||||||
return testValue | ||||||||||||
? this.consequent.hasEffects(context) | ||||||||||||
: this.alternate !== null && this.alternate.hasEffects(context); | ||||||||||||
} | ||||||||||||
|
@@ -48,22 +47,24 @@ export default class IfStatement extends StatementBase implements DeoptimizableE | |||||||||||
this.included = true; | ||||||||||||
if (includeChildrenRecursively) { | ||||||||||||
this.includeRecursively(includeChildrenRecursively, context); | ||||||||||||
} else if (this.testValue === UnknownValue) { | ||||||||||||
this.includeUnknownTest(context); | ||||||||||||
} else { | ||||||||||||
this.includeKnownTest(context); | ||||||||||||
const testValue = this.getTestValue(); | ||||||||||||
if (testValue === UnknownValue) { | ||||||||||||
this.includeUnknownTest(context); | ||||||||||||
} else { | ||||||||||||
this.includeKnownTest(context, testValue); | ||||||||||||
} | ||||||||||||
} | ||||||||||||
} | ||||||||||||
|
||||||||||||
render(code: MagicString, options: RenderOptions) { | ||||||||||||
// Note that unknown test values are always included | ||||||||||||
const testValue = this.getTestValue(); | ||||||||||||
if ( | ||||||||||||
!this.test.included && | ||||||||||||
(this.testValue | ||||||||||||
? this.alternate === null || !this.alternate.included | ||||||||||||
: !this.consequent.included) | ||||||||||||
(testValue ? this.alternate === null || !this.alternate.included : !this.consequent.included) | ||||||||||||
) { | ||||||||||||
const singleRetainedBranch = (this.testValue ? this.consequent : this.alternate)!; | ||||||||||||
const singleRetainedBranch = (testValue ? this.consequent : this.alternate)!; | ||||||||||||
code.remove(this.start, singleRetainedBranch.start); | ||||||||||||
code.remove(singleRetainedBranch.end, this.end); | ||||||||||||
removeAnnotations(this, code); | ||||||||||||
|
@@ -72,7 +73,7 @@ export default class IfStatement extends StatementBase implements DeoptimizableE | |||||||||||
if (this.test.included) { | ||||||||||||
this.test.render(code, options); | ||||||||||||
} else { | ||||||||||||
code.overwrite(this.test.start, this.test.end, this.testValue ? 'true' : 'false'); | ||||||||||||
code.overwrite(this.test.start, this.test.end, testValue ? 'true' : 'false'); | ||||||||||||
} | ||||||||||||
if (this.consequent.included) { | ||||||||||||
this.consequent.render(code, options); | ||||||||||||
|
@@ -89,14 +90,25 @@ export default class IfStatement extends StatementBase implements DeoptimizableE | |||||||||||
} | ||||||||||||
} | ||||||||||||
|
||||||||||||
private includeKnownTest(context: InclusionContext) { | ||||||||||||
private getTestValue(): LiteralValueOrUnknown { | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The changes in this file make sense but I do not think they actually solve any issue; on the other hand, they add a lot of additional function calls in various places. Usually, the deoptimization logic should fix it if the test value is later reassigned. The reason why it was not working is because there is a bug in rollup/src/ast/nodes/MemberExpression.ts Lines 114 to 118 in f9b9a7c
expressionsToBeDeoptimized but also set this.propertyKey = UnknownKey . With that change, I can revert all changes in IfStatement and still have all tests green.
So how do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||
if (this.testValue === unset) { | ||||||||||||
return (this.testValue = this.test.getLiteralValueAtPath( | ||||||||||||
EMPTY_PATH, | ||||||||||||
SHARED_RECURSION_TRACKER, | ||||||||||||
this | ||||||||||||
)); | ||||||||||||
} | ||||||||||||
return this.testValue; | ||||||||||||
} | ||||||||||||
|
||||||||||||
private includeKnownTest(context: InclusionContext, testValue: LiteralValueOrUnknown) { | ||||||||||||
if (this.test.shouldBeIncluded(context)) { | ||||||||||||
this.test.include(context, false); | ||||||||||||
} | ||||||||||||
if (this.testValue && this.consequent.shouldBeIncluded(context)) { | ||||||||||||
if (testValue && this.consequent.shouldBeIncluded(context)) { | ||||||||||||
this.consequent.include(context, false); | ||||||||||||
} | ||||||||||||
if (this.alternate !== null && !this.testValue && this.alternate.shouldBeIncluded(context)) { | ||||||||||||
if (this.alternate !== null && !testValue && this.alternate.shouldBeIncluded(context)) { | ||||||||||||
this.alternate.include(context, false); | ||||||||||||
} | ||||||||||||
} | ||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
module.exports = { | ||
description: 'only deoptimizes assigned variables when the assignment is included' | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
console.log('retained'); | ||
|
||
console.log('retained'); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
const foo = { toggled: false }; | ||
const bar = { toggled: false }; | ||
|
||
if (foo.toggled) { | ||
foo.toggled = bar; | ||
} | ||
|
||
if (foo.toggled) console.log('this should be removed'); | ||
else console.log('retained'); | ||
|
||
if (bar.toggled) console.log('this should be removed'); | ||
else console.log('retained'); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
module.exports = { | ||
description: 'makes sure the assignee is deoptimized' | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
const flags = { updated: false }; | ||
let toBeUpdated = {}; | ||
toBeUpdated = flags; | ||
toBeUpdated.updated = true; | ||
if (!flags.updated) throw new Error('Update was not tracked'); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
module.exports = { | ||
description: 'makes sure the assignment target is deoptimized' | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
let updated = false; | ||
updated = true; | ||
if (!updated) throw new Error('Update was not tracked'); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
module.exports = { | ||
description: 'tracks assigments nested in expressions that are included for other reasons' | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
let updated = false; | ||
assert.ok(!updated) || (updated = true); | ||
if (!updated) throw new Error('Update was not tracked'); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
module.exports = { | ||
description: 'tracks assigments included via try-catch-deoptimization' | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
let updated = false; | ||
try { | ||
updated = true; | ||
} catch (err) {} | ||
|
||
if (!updated) throw new Error('Update was not tracked'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I extracted the check because
hasEffects
andinclude
are called quite often and the deoptimizations will only be necessary once. This would save the cost of one function call, but the wins are probably small so we could also move the check back if you prefer.