From fc1f88c74039e958002b4b01597137a51fef8e05 Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Tue, 10 Dec 2019 06:37:47 +0100 Subject: [PATCH 1/5] Avoid some unnecessary deoptimizations --- src/ast/nodes/BinaryExpression.ts | 9 +- src/ast/nodes/CallExpression.ts | 86 ++++++------- src/ast/nodes/ConditionalExpression.ts | 57 +++++---- src/ast/nodes/IfStatement.ts | 5 +- src/ast/nodes/LogicalExpression.ts | 58 +++++---- src/ast/nodes/MemberExpression.ts | 38 +++--- src/ast/nodes/ObjectExpression.ts | 114 +++++++++--------- src/ast/nodes/Property.ts | 39 +++--- src/ast/utils/PathTracker.ts | 2 +- test/cli/index.js | 2 +- .../_config.js | 3 + .../_expected.js | 27 +++++ .../main.js | 47 ++++++++ 13 files changed, 294 insertions(+), 193 deletions(-) create mode 100644 test/form/samples/avoid-unnecessary-conditional-deopt/_config.js create mode 100644 test/form/samples/avoid-unnecessary-conditional-deopt/_expected.js create mode 100644 test/form/samples/avoid-unnecessary-conditional-deopt/main.js diff --git a/src/ast/nodes/BinaryExpression.ts b/src/ast/nodes/BinaryExpression.ts index 9cf3f5d76bf..eb85497e630 100644 --- a/src/ast/nodes/BinaryExpression.ts +++ b/src/ast/nodes/BinaryExpression.ts @@ -1,6 +1,11 @@ import { DeoptimizableEntity } from '../DeoptimizableEntity'; import { HasEffectsContext } from '../ExecutionContext'; -import { EMPTY_IMMUTABLE_TRACKER, EMPTY_PATH, ObjectPath, PathTracker } from '../utils/PathTracker'; +import { + EMPTY_PATH, + ObjectPath, + PathTracker, + SHARED_RECURSION_TRACKER +} from '../utils/PathTracker'; import { LiteralValueOrUnknown, UnknownValue } from '../values'; import ExpressionStatement from './ExpressionStatement'; import { LiteralValue } from './Literal'; @@ -68,7 +73,7 @@ export default class BinaryExpression extends NodeBase implements DeoptimizableE if ( this.operator === '+' && this.parent instanceof ExpressionStatement && - this.left.getLiteralValueAtPath(EMPTY_PATH, EMPTY_IMMUTABLE_TRACKER, this) === '' + this.left.getLiteralValueAtPath(EMPTY_PATH, SHARED_RECURSION_TRACKER, this) === '' ) return true; return super.hasEffects(context); diff --git a/src/ast/nodes/CallExpression.ts b/src/ast/nodes/CallExpression.ts index d89e4b9ecdc..e510d5e1ab7 100644 --- a/src/ast/nodes/CallExpression.ts +++ b/src/ast/nodes/CallExpression.ts @@ -9,10 +9,10 @@ import { CallOptions } from '../CallOptions'; import { DeoptimizableEntity } from '../DeoptimizableEntity'; import { HasEffectsContext, InclusionContext } from '../ExecutionContext'; import { - EMPTY_IMMUTABLE_TRACKER, EMPTY_PATH, ObjectPath, PathTracker, + SHARED_RECURSION_TRACKER, UNKNOWN_PATH } from '../utils/PathTracker'; import { LiteralValueOrUnknown, UNKNOWN_EXPRESSION, UnknownValue } from '../values'; @@ -30,9 +30,9 @@ export default class CallExpression extends NodeBase implements DeoptimizableEnt type!: NodeType.tCallExpression; private callOptions!: CallOptions; - // We collect deoptimization information if returnExpression !== UNKNOWN_EXPRESSION private expressionsToBeDeoptimized: DeoptimizableEntity[] = []; private returnExpression: ExpressionEntity | null = null; + private wasPathDeoptmizedWhileOptimized = false; bind() { super.bind(); @@ -60,13 +60,9 @@ export default class CallExpression extends NodeBase implements DeoptimizableEnt ); } } - if (this.returnExpression === null) { - this.returnExpression = this.callee.getReturnExpressionWhenCalledAtPath( - EMPTY_PATH, - EMPTY_IMMUTABLE_TRACKER, - this - ); - } + // ensure the returnExpression is set for the tree-shaking passes + this.getReturnExpression(SHARED_RECURSION_TRACKER); + // This deoptimizes "this" for non-namespace calls until we have a better solution if (this.callee instanceof MemberExpression && !this.callee.variable) { this.callee.object.deoptimizePath(UNKNOWN_PATH); } @@ -78,8 +74,19 @@ export default class CallExpression extends NodeBase implements DeoptimizableEnt deoptimizeCache() { if (this.returnExpression !== UNKNOWN_EXPRESSION) { - this.returnExpression = UNKNOWN_EXPRESSION; - for (const expression of this.expressionsToBeDeoptimized) { + this.returnExpression = null; + const returnExpression = this.getReturnExpression(SHARED_RECURSION_TRACKER); + const expressionsToBeDeoptimized = this.expressionsToBeDeoptimized; + if (returnExpression !== UNKNOWN_EXPRESSION) { + // We need to replace here because is possible new expressions are added + // while we are deoptimizing the old ones + this.expressionsToBeDeoptimized = []; + if (this.wasPathDeoptmizedWhileOptimized) { + returnExpression.deoptimizePath(UNKNOWN_PATH); + this.wasPathDeoptmizedWhileOptimized = false; + } + } + for (const expression of expressionsToBeDeoptimized) { expression.deoptimizeCache(); } } @@ -90,14 +97,11 @@ export default class CallExpression extends NodeBase implements DeoptimizableEnt const trackedEntities = this.context.deoptimizationTracker.getEntities(path); if (trackedEntities.has(this)) return; trackedEntities.add(this); - if (this.returnExpression === null) { - this.returnExpression = this.callee.getReturnExpressionWhenCalledAtPath( - EMPTY_PATH, - EMPTY_IMMUTABLE_TRACKER, - this - ); + const returnExpression = this.getReturnExpression(SHARED_RECURSION_TRACKER); + if (returnExpression !== UNKNOWN_EXPRESSION) { + this.wasPathDeoptmizedWhileOptimized = true; + returnExpression.deoptimizePath(path); } - this.returnExpression.deoptimizePath(path); } getLiteralValueAtPath( @@ -105,24 +109,18 @@ export default class CallExpression extends NodeBase implements DeoptimizableEnt recursionTracker: PathTracker, origin: DeoptimizableEntity ): LiteralValueOrUnknown { - if (this.returnExpression === null) { - this.returnExpression = this.callee.getReturnExpressionWhenCalledAtPath( - EMPTY_PATH, - recursionTracker, - this - ); - } - if (this.returnExpression === UNKNOWN_EXPRESSION) { + const returnExpression = this.getReturnExpression(recursionTracker); + if (returnExpression === UNKNOWN_EXPRESSION) { return UnknownValue; } const trackedEntities = recursionTracker.getEntities(path); - if (trackedEntities.has(this.returnExpression)) { + if (trackedEntities.has(returnExpression)) { return UnknownValue; } this.expressionsToBeDeoptimized.push(origin); - trackedEntities.add(this.returnExpression); - const value = this.returnExpression.getLiteralValueAtPath(path, recursionTracker, origin); - trackedEntities.delete(this.returnExpression); + trackedEntities.add(returnExpression); + const value = returnExpression.getLiteralValueAtPath(path, recursionTracker, origin); + trackedEntities.delete(returnExpression); return value; } @@ -131,28 +129,22 @@ export default class CallExpression extends NodeBase implements DeoptimizableEnt recursionTracker: PathTracker, origin: DeoptimizableEntity ) { - if (this.returnExpression === null) { - this.returnExpression = this.callee.getReturnExpressionWhenCalledAtPath( - EMPTY_PATH, - recursionTracker, - this - ); - } + const returnExpression = this.getReturnExpression(recursionTracker); if (this.returnExpression === UNKNOWN_EXPRESSION) { return UNKNOWN_EXPRESSION; } const trackedEntities = recursionTracker.getEntities(path); - if (trackedEntities.has(this.returnExpression)) { + if (trackedEntities.has(returnExpression)) { return UNKNOWN_EXPRESSION; } this.expressionsToBeDeoptimized.push(origin); - trackedEntities.add(this.returnExpression); - const value = this.returnExpression.getReturnExpressionWhenCalledAtPath( + trackedEntities.add(returnExpression); + const value = returnExpression.getReturnExpressionWhenCalledAtPath( path, recursionTracker, origin ); - trackedEntities.delete(this.returnExpression); + trackedEntities.delete(returnExpression); return value; } @@ -272,4 +264,16 @@ export default class CallExpression extends NodeBase implements DeoptimizableEnt code.prependLeft(this.end, ')'); } } + + private getReturnExpression(recursionTracker: PathTracker): ExpressionEntity { + if (this.returnExpression === null) { + this.returnExpression = UNKNOWN_EXPRESSION; + return (this.returnExpression = this.callee.getReturnExpressionWhenCalledAtPath( + EMPTY_PATH, + recursionTracker, + this + )); + } + return this.returnExpression; + } } diff --git a/src/ast/nodes/ConditionalExpression.ts b/src/ast/nodes/ConditionalExpression.ts index 630cc08754f..052a76702c5 100644 --- a/src/ast/nodes/ConditionalExpression.ts +++ b/src/ast/nodes/ConditionalExpression.ts @@ -11,10 +11,10 @@ import { CallOptions } from '../CallOptions'; import { DeoptimizableEntity } from '../DeoptimizableEntity'; import { HasEffectsContext, InclusionContext } from '../ExecutionContext'; import { - EMPTY_IMMUTABLE_TRACKER, EMPTY_PATH, ObjectPath, PathTracker, + SHARED_RECURSION_TRACKER, UNKNOWN_PATH } from '../utils/PathTracker'; import { LiteralValueOrUnknown, UnknownValue } from '../values'; @@ -30,23 +30,24 @@ export default class ConditionalExpression extends NodeBase implements Deoptimiz test!: ExpressionNode; type!: NodeType.tConditionalExpression; - // We collect deoptimization information if usedBranch !== null private expressionsToBeDeoptimized: DeoptimizableEntity[] = []; private isBranchResolutionAnalysed = false; - private unusedBranch: ExpressionNode | null = null; private usedBranch: ExpressionNode | null = null; + private wasPathDeoptimizedWhileOptimized = false; bind() { super.bind(); - if (!this.isBranchResolutionAnalysed) this.analyseBranchResolution(); + // ensure the usedBranch is set for the tree-shaking passes + this.getUsedBranch(); } deoptimizeCache() { if (this.usedBranch !== null) { - // We did not track if there were reassignments to the previous branch. - // Also, the return value might need to be reassigned. + const unusedBranch = this.usedBranch === this.consequent ? this.alternate : this.consequent; this.usedBranch = null; - (this.unusedBranch as ExpressionNode).deoptimizePath(UNKNOWN_PATH); + if (this.wasPathDeoptimizedWhileOptimized) { + unusedBranch.deoptimizePath(UNKNOWN_PATH); + } for (const expression of this.expressionsToBeDeoptimized) { expression.deoptimizeCache(); } @@ -55,12 +56,13 @@ export default class ConditionalExpression extends NodeBase implements Deoptimiz deoptimizePath(path: ObjectPath) { if (path.length > 0) { - if (!this.isBranchResolutionAnalysed) this.analyseBranchResolution(); - if (this.usedBranch === null) { + const usedBranch = this.getUsedBranch(); + if (usedBranch === null) { this.consequent.deoptimizePath(path); this.alternate.deoptimizePath(path); } else { - this.usedBranch.deoptimizePath(path); + this.wasPathDeoptimizedWhileOptimized = true; + usedBranch.deoptimizePath(path); } } } @@ -70,10 +72,10 @@ export default class ConditionalExpression extends NodeBase implements Deoptimiz recursionTracker: PathTracker, origin: DeoptimizableEntity ): LiteralValueOrUnknown { - if (!this.isBranchResolutionAnalysed) this.analyseBranchResolution(); - if (this.usedBranch === null) return UnknownValue; + const usedBranch = this.getUsedBranch(); + if (usedBranch === null) return UnknownValue; this.expressionsToBeDeoptimized.push(origin); - return this.usedBranch.getLiteralValueAtPath(path, recursionTracker, origin); + return usedBranch.getLiteralValueAtPath(path, recursionTracker, origin); } getReturnExpressionWhenCalledAtPath( @@ -81,14 +83,14 @@ export default class ConditionalExpression extends NodeBase implements Deoptimiz recursionTracker: PathTracker, origin: DeoptimizableEntity ): ExpressionEntity { - if (!this.isBranchResolutionAnalysed) this.analyseBranchResolution(); - if (this.usedBranch === null) + const usedBranch = this.getUsedBranch(); + if (usedBranch === null) return new MultiExpression([ this.consequent.getReturnExpressionWhenCalledAtPath(path, recursionTracker, origin), this.alternate.getReturnExpressionWhenCalledAtPath(path, recursionTracker, origin) ]); this.expressionsToBeDeoptimized.push(origin); - return this.usedBranch.getReturnExpressionWhenCalledAtPath(path, recursionTracker, origin); + return usedBranch.getReturnExpressionWhenCalledAtPath(path, recursionTracker, origin); } hasEffects(context: HasEffectsContext): boolean { @@ -162,14 +164,14 @@ export default class ConditionalExpression extends NodeBase implements Deoptimiz ? findFirstOccurrenceOutsideComment(code.original, '?', this.test.end) : colonPos) + 1; if (preventASI) { - removeLineBreaks(code, inclusionStart, (this.usedBranch as ExpressionNode).start); + removeLineBreaks(code, inclusionStart, this.usedBranch!.start); } code.remove(this.start, inclusionStart); if (this.consequent.included) { code.remove(colonPos, this.end); } removeAnnotations(this, code); - (this.usedBranch as ExpressionNode).render(code, options, { + this.usedBranch!.render(code, options, { isCalleeOfRenderedParent: renderedParentType ? isCalleeOfRenderedParent : (this.parent as CallExpression).callee === this, @@ -180,17 +182,14 @@ export default class ConditionalExpression extends NodeBase implements Deoptimiz } } - private analyseBranchResolution() { - this.isBranchResolutionAnalysed = true; - const testValue = this.test.getLiteralValueAtPath(EMPTY_PATH, EMPTY_IMMUTABLE_TRACKER, this); - if (testValue !== UnknownValue) { - if (testValue) { - this.usedBranch = this.consequent; - this.unusedBranch = this.alternate; - } else { - this.usedBranch = this.alternate; - this.unusedBranch = this.consequent; - } + private getUsedBranch() { + if (this.isBranchResolutionAnalysed) { + return this.usedBranch; } + this.isBranchResolutionAnalysed = true; + const testValue = this.test.getLiteralValueAtPath(EMPTY_PATH, SHARED_RECURSION_TRACKER, this); + return testValue === UnknownValue + ? null + : (this.usedBranch = testValue ? this.consequent : this.alternate); } } diff --git a/src/ast/nodes/IfStatement.ts b/src/ast/nodes/IfStatement.ts index 5b91d33d2c6..1edcc537e04 100644 --- a/src/ast/nodes/IfStatement.ts +++ b/src/ast/nodes/IfStatement.ts @@ -3,7 +3,7 @@ import { RenderOptions } from '../../utils/renderHelpers'; import { removeAnnotations } from '../../utils/treeshakeNode'; import { DeoptimizableEntity } from '../DeoptimizableEntity'; import { BROKEN_FLOW_NONE, HasEffectsContext, InclusionContext } from '../ExecutionContext'; -import { EMPTY_IMMUTABLE_TRACKER, EMPTY_PATH } from '../utils/PathTracker'; +import { EMPTY_PATH, SHARED_RECURSION_TRACKER } from '../utils/PathTracker'; import { LiteralValueOrUnknown, UnknownValue } from '../values'; import * as NodeType from './NodeType'; import { ExpressionNode, IncludeChildren, StatementBase, StatementNode } from './shared/Node'; @@ -18,7 +18,8 @@ export default class IfStatement extends StatementBase implements DeoptimizableE bind() { super.bind(); - this.testValue = this.test.getLiteralValueAtPath(EMPTY_PATH, EMPTY_IMMUTABLE_TRACKER, this); + // ensure the testValue is set for the tree-shaking passes + this.testValue = this.test.getLiteralValueAtPath(EMPTY_PATH, SHARED_RECURSION_TRACKER, this); } deoptimizeCache() { diff --git a/src/ast/nodes/LogicalExpression.ts b/src/ast/nodes/LogicalExpression.ts index b0c52d59a14..22804239d72 100644 --- a/src/ast/nodes/LogicalExpression.ts +++ b/src/ast/nodes/LogicalExpression.ts @@ -11,10 +11,10 @@ import { CallOptions } from '../CallOptions'; import { DeoptimizableEntity } from '../DeoptimizableEntity'; import { HasEffectsContext, InclusionContext } from '../ExecutionContext'; import { - EMPTY_IMMUTABLE_TRACKER, EMPTY_PATH, ObjectPath, PathTracker, + SHARED_RECURSION_TRACKER, UNKNOWN_PATH } from '../utils/PathTracker'; import { LiteralValueOrUnknown, UnknownValue } from '../values'; @@ -37,18 +37,20 @@ export default class LogicalExpression extends NodeBase implements Deoptimizable private isBranchResolutionAnalysed = false; private unusedBranch: ExpressionNode | null = null; private usedBranch: ExpressionNode | null = null; + private wasPathDeoptimizedWhileOptimized = false; bind() { super.bind(); - if (!this.isBranchResolutionAnalysed) this.analyseBranchResolution(); + // ensure the usedBranch is set for the tree-shaking passes + this.getUsedBranch(); } deoptimizeCache() { if (this.usedBranch !== null) { - // We did not track if there were reassignments to any of the branches. - // Also, the return values might need reassignment. this.usedBranch = null; - (this.unusedBranch as ExpressionNode).deoptimizePath(UNKNOWN_PATH); + if (this.wasPathDeoptimizedWhileOptimized) { + this.unusedBranch!.deoptimizePath(UNKNOWN_PATH); + } for (const expression of this.expressionsToBeDeoptimized) { expression.deoptimizeCache(); } @@ -57,12 +59,13 @@ export default class LogicalExpression extends NodeBase implements Deoptimizable deoptimizePath(path: ObjectPath) { if (path.length > 0) { - if (!this.isBranchResolutionAnalysed) this.analyseBranchResolution(); - if (this.usedBranch === null) { + const usedBranch = this.getUsedBranch(); + if (usedBranch === null) { this.left.deoptimizePath(path); this.right.deoptimizePath(path); } else { - this.usedBranch.deoptimizePath(path); + this.wasPathDeoptimizedWhileOptimized = true; + usedBranch.deoptimizePath(path); } } } @@ -72,10 +75,10 @@ export default class LogicalExpression extends NodeBase implements Deoptimizable recursionTracker: PathTracker, origin: DeoptimizableEntity ): LiteralValueOrUnknown { - if (!this.isBranchResolutionAnalysed) this.analyseBranchResolution(); - if (this.usedBranch === null) return UnknownValue; + const usedBranch = this.getUsedBranch(); + if (usedBranch === null) return UnknownValue; this.expressionsToBeDeoptimized.push(origin); - return this.usedBranch.getLiteralValueAtPath(path, recursionTracker, origin); + return usedBranch.getLiteralValueAtPath(path, recursionTracker, origin); } getReturnExpressionWhenCalledAtPath( @@ -83,14 +86,14 @@ export default class LogicalExpression extends NodeBase implements Deoptimizable recursionTracker: PathTracker, origin: DeoptimizableEntity ): ExpressionEntity { - if (!this.isBranchResolutionAnalysed) this.analyseBranchResolution(); - if (this.usedBranch === null) + const usedBranch = this.getUsedBranch(); + if (usedBranch === null) return new MultiExpression([ this.left.getReturnExpressionWhenCalledAtPath(path, recursionTracker, origin), this.right.getReturnExpressionWhenCalledAtPath(path, recursionTracker, origin) ]); this.expressionsToBeDeoptimized.push(origin); - return this.usedBranch.getReturnExpressionWhenCalledAtPath(path, recursionTracker, origin); + return usedBranch.getReturnExpressionWhenCalledAtPath(path, recursionTracker, origin); } hasEffects(context: HasEffectsContext): boolean { @@ -141,7 +144,7 @@ export default class LogicalExpression extends NodeBase implements Deoptimizable if ( includeChildrenRecursively || this.usedBranch === null || - (this.unusedBranch as ExpressionNode).shouldBeIncluded(context) + this.unusedBranch!.shouldBeIncluded(context) ) { this.left.include(context, includeChildrenRecursively); this.right.include(context, includeChildrenRecursively); @@ -170,7 +173,7 @@ export default class LogicalExpression extends NodeBase implements Deoptimizable code.remove(operatorPos, this.end); } removeAnnotations(this, code); - (this.usedBranch as ExpressionNode).render(code, options, { + this.usedBranch!.render(code, options, { isCalleeOfRenderedParent: renderedParentType ? isCalleeOfRenderedParent : (this.parent as CallExpression).callee === this, @@ -181,17 +184,22 @@ export default class LogicalExpression extends NodeBase implements Deoptimizable } } - private analyseBranchResolution() { - this.isBranchResolutionAnalysed = true; - const leftValue = this.left.getLiteralValueAtPath(EMPTY_PATH, EMPTY_IMMUTABLE_TRACKER, this); - if (leftValue !== UnknownValue) { - if (this.operator === '||' ? leftValue : !leftValue) { - this.usedBranch = this.left; - this.unusedBranch = this.right; + private getUsedBranch() { + if (!this.isBranchResolutionAnalysed) { + this.isBranchResolutionAnalysed = true; + const leftValue = this.left.getLiteralValueAtPath(EMPTY_PATH, SHARED_RECURSION_TRACKER, this); + if (leftValue === UnknownValue) { + return null; } else { - this.usedBranch = this.right; - this.unusedBranch = this.left; + if (this.operator === '||' ? leftValue : !leftValue) { + this.usedBranch = this.left; + this.unusedBranch = this.right; + } else { + this.usedBranch = this.right; + this.unusedBranch = this.left; + } } } + return this.usedBranch; } } diff --git a/src/ast/nodes/MemberExpression.ts b/src/ast/nodes/MemberExpression.ts index 39adbedc28e..8de9672fe15 100644 --- a/src/ast/nodes/MemberExpression.ts +++ b/src/ast/nodes/MemberExpression.ts @@ -6,11 +6,11 @@ import { CallOptions } from '../CallOptions'; import { DeoptimizableEntity } from '../DeoptimizableEntity'; import { HasEffectsContext, InclusionContext } from '../ExecutionContext'; import { - EMPTY_IMMUTABLE_TRACKER, EMPTY_PATH, ObjectPath, ObjectPathKey, PathTracker, + SHARED_RECURSION_TRACKER, UNKNOWN_PATH, UnknownKey } from '../utils/PathTracker'; @@ -78,8 +78,8 @@ export default class MemberExpression extends NodeBase implements DeoptimizableE private bound = false; private expressionsToBeDeoptimized: DeoptimizableEntity[] = []; - private hasDeoptimizedPath = false; private replacement: string | null = null; + private wasPathDeoptimizedWhileOptimized = false; addExportedVariables(): void {} @@ -109,13 +109,14 @@ export default class MemberExpression extends NodeBase implements DeoptimizableE } } else { super.bind(); - if (this.propertyKey === null) this.analysePropertyKey(); + // ensure the propertyKey is set for the tree-shaking passes + this.getPropertyKey(); } } deoptimizeCache() { this.propertyKey = UnknownKey; - if (this.hasDeoptimizedPath) { + if (this.wasPathDeoptimizedWhileOptimized) { this.object.deoptimizePath(UNKNOWN_PATH); } for (const expression of this.expressionsToBeDeoptimized) { @@ -129,12 +130,12 @@ export default class MemberExpression extends NodeBase implements DeoptimizableE if (this.variable) { this.variable.deoptimizePath(path); } else { - if (this.propertyKey === null) this.analysePropertyKey(); - if (this.propertyKey === UnknownKey) { + const propertyKey = this.getPropertyKey(); + if (propertyKey === UnknownKey) { this.object.deoptimizePath(UNKNOWN_PATH); } else { - this.hasDeoptimizedPath = true; - this.object.deoptimizePath([this.propertyKey as ObjectPathKey, ...path]); + this.wasPathDeoptimizedWhileOptimized = true; + this.object.deoptimizePath([propertyKey, ...path]); } } } @@ -148,10 +149,9 @@ export default class MemberExpression extends NodeBase implements DeoptimizableE if (this.variable !== null) { return this.variable.getLiteralValueAtPath(path, recursionTracker, origin); } - if (this.propertyKey === null) this.analysePropertyKey(); this.expressionsToBeDeoptimized.push(origin); return this.object.getLiteralValueAtPath( - [this.propertyKey as ObjectPathKey, ...path], + [this.getPropertyKey(), ...path], recursionTracker, origin ); @@ -166,10 +166,9 @@ export default class MemberExpression extends NodeBase implements DeoptimizableE if (this.variable !== null) { return this.variable.getReturnExpressionWhenCalledAtPath(path, recursionTracker, origin); } - if (this.propertyKey === null) this.analysePropertyKey(); this.expressionsToBeDeoptimized.push(origin); return this.object.getReturnExpressionWhenCalledAtPath( - [this.propertyKey as ObjectPathKey, ...path], + [this.getPropertyKey(), ...path], recursionTracker, origin ); @@ -265,12 +264,6 @@ export default class MemberExpression extends NodeBase implements DeoptimizableE } } - private analysePropertyKey() { - this.propertyKey = UnknownKey; - const value = this.property.getLiteralValueAtPath(EMPTY_PATH, EMPTY_IMMUTABLE_TRACKER, this); - this.propertyKey = value === UnknownValue ? UnknownKey : String(value); - } - private disallowNamespaceReassignment() { if ( this.object instanceof Identifier && @@ -286,6 +279,15 @@ export default class MemberExpression extends NodeBase implements DeoptimizableE } } + private getPropertyKey(): ObjectPathKey { + if (this.propertyKey === null) { + this.propertyKey = UnknownKey; + const value = this.property.getLiteralValueAtPath(EMPTY_PATH, SHARED_RECURSION_TRACKER, this); + return (this.propertyKey = value === UnknownValue ? UnknownKey : String(value)); + } + return this.propertyKey; + } + private resolveNamespaceVariables( baseVariable: Variable, path: PathWithPositions diff --git a/src/ast/nodes/ObjectExpression.ts b/src/ast/nodes/ObjectExpression.ts index eca3e4d8c3d..8fde9e7c8cd 100644 --- a/src/ast/nodes/ObjectExpression.ts +++ b/src/ast/nodes/ObjectExpression.ts @@ -5,10 +5,10 @@ import { CallOptions } from '../CallOptions'; import { DeoptimizableEntity } from '../DeoptimizableEntity'; import { HasEffectsContext } from '../ExecutionContext'; import { - EMPTY_IMMUTABLE_TRACKER, EMPTY_PATH, ObjectPath, PathTracker, + SHARED_RECURSION_TRACKER, UNKNOWN_PATH } from '../utils/PathTracker'; import { @@ -32,7 +32,7 @@ interface PropertyMap { exactMatchRead: Property | null; exactMatchWrite: Property | null; propertiesRead: (Property | SpreadElement)[]; - propertiesSet: Property[]; + propertiesWrite: Property[]; }; } @@ -51,7 +51,8 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE bind() { super.bind(); - if (this.propertyMap === null) this.buildPropertyMap(); + // ensure the propertyMap is set for the tree-shaking passes + this.getPropertyMap(); } // We could also track this per-property but this would quickly become much more complex @@ -61,7 +62,7 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE deoptimizePath(path: ObjectPath) { if (this.hasUnknownDeoptimizedProperty) return; - if (this.propertyMap === null) this.buildPropertyMap(); + const propertyMap = this.getPropertyMap(); if (path.length === 0) { this.deoptimizeAllProperties(); return; @@ -87,8 +88,8 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE } const subPath = path.length === 1 ? UNKNOWN_PATH : path.slice(1); for (const property of typeof key === 'string' - ? (this.propertyMap as PropertyMap)[key] - ? (this.propertyMap as PropertyMap)[key].propertiesRead + ? propertyMap[key] + ? propertyMap[key].propertiesRead : [] : this.properties) { property.deoptimizePath(subPath); @@ -100,7 +101,7 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE recursionTracker: PathTracker, origin: DeoptimizableEntity ): LiteralValueOrUnknown { - if (this.propertyMap === null) this.buildPropertyMap(); + const propertyMap = this.getPropertyMap(); const key = path[0]; if ( @@ -113,7 +114,7 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE if ( path.length === 1 && - !(this.propertyMap as PropertyMap)[key] && + !propertyMap[key] && !objectMembers[key] && this.unmatchablePropertiesRead.length === 0 ) { @@ -127,9 +128,9 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE } if ( - !(this.propertyMap as PropertyMap)[key] || - (this.propertyMap as PropertyMap)[key].exactMatchRead === null || - (this.propertyMap as PropertyMap)[key].propertiesRead.length > 1 + !propertyMap[key] || + propertyMap[key].exactMatchRead === null || + propertyMap[key].propertiesRead.length > 1 ) { return UnknownValue; } @@ -140,8 +141,11 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE } else { this.expressionsToBeDeoptimized.set(key, [origin]); } - return ((this.propertyMap as PropertyMap)[key] - .exactMatchRead as Property).getLiteralValueAtPath(path.slice(1), recursionTracker, origin); + return (propertyMap[key].exactMatchRead as Property).getLiteralValueAtPath( + path.slice(1), + recursionTracker, + origin + ); } getReturnExpressionWhenCalledAtPath( @@ -149,7 +153,7 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE recursionTracker: PathTracker, origin: DeoptimizableEntity ): ExpressionEntity { - if (this.propertyMap === null) this.buildPropertyMap(); + const propertyMap = this.getPropertyMap(); const key = path[0]; if ( @@ -164,15 +168,14 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE path.length === 1 && objectMembers[key] && this.unmatchablePropertiesRead.length === 0 && - (!(this.propertyMap as PropertyMap)[key] || - (this.propertyMap as PropertyMap)[key].exactMatchRead === null) + (!propertyMap[key] || propertyMap[key].exactMatchRead === null) ) return getMemberReturnExpressionWhenCalled(objectMembers, key); if ( - !(this.propertyMap as PropertyMap)[key] || - (this.propertyMap as PropertyMap)[key].exactMatchRead === null || - (this.propertyMap as PropertyMap)[key].propertiesRead.length > 1 + !propertyMap[key] || + propertyMap[key].exactMatchRead === null || + propertyMap[key].propertiesRead.length > 1 ) return UNKNOWN_EXPRESSION; @@ -182,8 +185,7 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE } else { this.expressionsToBeDeoptimized.set(key, [origin]); } - return ((this.propertyMap as PropertyMap)[key] - .exactMatchRead as Property).getReturnExpressionWhenCalledAtPath( + return (propertyMap[key].exactMatchRead as Property).getReturnExpressionWhenCalledAtPath( path.slice(1), recursionTracker, origin @@ -193,21 +195,22 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE hasEffectsWhenAccessedAtPath(path: ObjectPath, context: HasEffectsContext) { if (path.length === 0) return false; const key = path[0]; + const propertyMap = this.propertyMap!; if ( path.length > 1 && (this.hasUnknownDeoptimizedProperty || typeof key !== 'string' || this.deoptimizedPaths.has(key) || - !(this.propertyMap as PropertyMap)[key] || - (this.propertyMap as PropertyMap)[key].exactMatchRead === null) + !propertyMap[key] || + propertyMap[key].exactMatchRead === null) ) return true; const subPath = path.slice(1); for (const property of typeof key !== 'string' ? this.properties - : (this.propertyMap as PropertyMap)[key] - ? (this.propertyMap as PropertyMap)[key].propertiesRead + : propertyMap[key] + ? propertyMap[key].propertiesRead : []) { if (property.hasEffectsWhenAccessedAtPath(subPath, context)) return true; } @@ -217,13 +220,14 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE hasEffectsWhenAssignedAtPath(path: ObjectPath, context: HasEffectsContext) { if (path.length === 0) return false; const key = path[0]; + const propertyMap = this.propertyMap!; if ( path.length > 1 && (this.hasUnknownDeoptimizedProperty || typeof key !== 'string' || this.deoptimizedPaths.has(key) || - !(this.propertyMap as PropertyMap)[key] || - (this.propertyMap as PropertyMap)[key].exactMatchRead === null) + !propertyMap[key] || + propertyMap[key].exactMatchRead === null) ) return true; @@ -231,9 +235,9 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE for (const property of typeof key !== 'string' ? this.properties : path.length > 1 - ? (this.propertyMap as PropertyMap)[key].propertiesRead - : (this.propertyMap as PropertyMap)[key] - ? (this.propertyMap as PropertyMap)[key].propertiesSet + ? propertyMap[key].propertiesRead + : propertyMap[key] + ? propertyMap[key].propertiesWrite : []) { if (property.hasEffectsWhenAssignedAtPath(subPath, context)) return true; } @@ -251,15 +255,13 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE this.hasUnknownDeoptimizedProperty || typeof key !== 'string' || this.deoptimizedPaths.has(key) || - ((this.propertyMap as PropertyMap)[key] - ? !(this.propertyMap as PropertyMap)[key].exactMatchRead + (this.propertyMap![key] + ? !this.propertyMap![key].exactMatchRead : path.length > 1 || !objectMembers[key]) ) return true; const subPath = path.slice(1); - for (const property of (this.propertyMap as PropertyMap)[key] - ? (this.propertyMap as PropertyMap)[key].propertiesRead - : []) { + for (const property of this.propertyMap![key] ? this.propertyMap![key].propertiesRead : []) { if (property.hasEffectsWhenCalledAtPath(subPath, callOptions, context)) return true; } if (path.length === 1 && objectMembers[key]) @@ -279,8 +281,23 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE } } - private buildPropertyMap() { - this.propertyMap = Object.create(null); + private deoptimizeAllProperties() { + this.hasUnknownDeoptimizedProperty = true; + for (const property of this.properties) { + property.deoptimizePath(UNKNOWN_PATH); + } + for (const expressionsToBeDeoptimized of this.expressionsToBeDeoptimized.values()) { + for (const expression of expressionsToBeDeoptimized) { + expression.deoptimizeCache(); + } + } + } + + private getPropertyMap(): PropertyMap { + if (this.propertyMap !== null) { + return this.propertyMap; + } + const propertyMap = (this.propertyMap = Object.create(null)); for (let index = this.properties.length - 1; index >= 0; index--) { const property = this.properties[index]; if (property instanceof SpreadElement) { @@ -293,7 +310,7 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE if (property.computed) { const keyValue = property.key.getLiteralValueAtPath( EMPTY_PATH, - EMPTY_IMMUTABLE_TRACKER, + SHARED_RECURSION_TRACKER, this ); if (keyValue === UnknownValue) { @@ -310,13 +327,13 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE } else { key = String((property.key as Literal).value); } - const propertyMapProperty = (this.propertyMap as PropertyMap)[key]; + const propertyMapProperty = propertyMap[key]; if (!propertyMapProperty) { - (this.propertyMap as PropertyMap)[key] = { + propertyMap[key] = { exactMatchRead: isRead ? property : null, exactMatchWrite: isWrite ? property : null, propertiesRead: isRead ? [property, ...this.unmatchablePropertiesRead] : [], - propertiesSet: isWrite && !isRead ? [property, ...this.unmatchablePropertiesWrite] : [] + propertiesWrite: isWrite && !isRead ? [property, ...this.unmatchablePropertiesWrite] : [] }; continue; } @@ -326,20 +343,9 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE } if (isWrite && !isRead && propertyMapProperty.exactMatchWrite === null) { propertyMapProperty.exactMatchWrite = property; - propertyMapProperty.propertiesSet.push(property, ...this.unmatchablePropertiesWrite); - } - } - } - - private deoptimizeAllProperties() { - this.hasUnknownDeoptimizedProperty = true; - for (const property of this.properties) { - property.deoptimizePath(UNKNOWN_PATH); - } - for (const expressionsToBeDeoptimized of this.expressionsToBeDeoptimized.values()) { - for (const expression of expressionsToBeDeoptimized) { - expression.deoptimizeCache(); + propertyMapProperty.propertiesWrite.push(property, ...this.unmatchablePropertiesWrite); } } + return propertyMap; } } diff --git a/src/ast/nodes/Property.ts b/src/ast/nodes/Property.ts index 290ae0da0de..c2a4b44410a 100644 --- a/src/ast/nodes/Property.ts +++ b/src/ast/nodes/Property.ts @@ -4,10 +4,10 @@ import { CallOptions, NO_ARGS } from '../CallOptions'; import { DeoptimizableEntity } from '../DeoptimizableEntity'; import { HasEffectsContext } from '../ExecutionContext'; import { - EMPTY_IMMUTABLE_TRACKER, EMPTY_PATH, ObjectPath, PathTracker, + SHARED_RECURSION_TRACKER, UnknownKey } from '../utils/PathTracker'; import { LiteralValueOrUnknown, UNKNOWN_EXPRESSION } from '../values'; @@ -30,7 +30,10 @@ export default class Property extends NodeBase implements DeoptimizableEntity { bind() { super.bind(); - if (this.kind === 'get' && this.returnExpression === null) this.updateReturnExpression(); + if (this.kind === 'get') { + // ensure the returnExpression is set for the tree-shaking passes + this.getReturnExpression(); + } if (this.declarationInit !== null) { this.declarationInit.deoptimizePath([UnknownKey, UnknownKey]); } @@ -41,7 +44,7 @@ export default class Property extends NodeBase implements DeoptimizableEntity { return this.value.declare(kind, UNKNOWN_EXPRESSION); } - deoptimizeCache() { + deoptimizeCache(): void { // As getter properties directly receive their values from function expressions that always // have a fixed return value, there is no known situation where a getter is deoptimized. throw new Error('Unexpected deoptimization'); @@ -50,8 +53,7 @@ export default class Property extends NodeBase implements DeoptimizableEntity { deoptimizePath(path: ObjectPath) { if (this.kind === 'get') { if (path.length > 0) { - if (this.returnExpression === null) this.updateReturnExpression(); - (this.returnExpression as ExpressionEntity).deoptimizePath(path); + this.getReturnExpression().deoptimizePath(path); } } else if (this.kind !== 'set') { this.value.deoptimizePath(path); @@ -64,12 +66,7 @@ export default class Property extends NodeBase implements DeoptimizableEntity { origin: DeoptimizableEntity ): LiteralValueOrUnknown { if (this.kind === 'get') { - if (this.returnExpression === null) this.updateReturnExpression(); - return (this.returnExpression as ExpressionEntity).getLiteralValueAtPath( - path, - recursionTracker, - origin - ); + return this.getReturnExpression().getLiteralValueAtPath(path, recursionTracker, origin); } return this.value.getLiteralValueAtPath(path, recursionTracker, origin); } @@ -80,8 +77,7 @@ export default class Property extends NodeBase implements DeoptimizableEntity { origin: DeoptimizableEntity ): ExpressionEntity { if (this.kind === 'get') { - if (this.returnExpression === null) this.updateReturnExpression(); - return (this.returnExpression as ExpressionEntity).getReturnExpressionWhenCalledAtPath( + return this.getReturnExpression().getReturnExpressionWhenCalledAtPath( path, recursionTracker, origin @@ -162,12 +158,15 @@ export default class Property extends NodeBase implements DeoptimizableEntity { this.value.render(code, options, { isShorthandProperty: this.shorthand }); } - private updateReturnExpression() { - this.returnExpression = UNKNOWN_EXPRESSION; - this.returnExpression = this.value.getReturnExpressionWhenCalledAtPath( - EMPTY_PATH, - EMPTY_IMMUTABLE_TRACKER, - this - ); + private getReturnExpression(): ExpressionEntity { + if (this.returnExpression === null) { + this.returnExpression = UNKNOWN_EXPRESSION; + return (this.returnExpression = this.value.getReturnExpressionWhenCalledAtPath( + EMPTY_PATH, + SHARED_RECURSION_TRACKER, + this + )); + } + return this.returnExpression; } } diff --git a/src/ast/utils/PathTracker.ts b/src/ast/utils/PathTracker.ts index 7deba3346d5..f4a47acea1d 100644 --- a/src/ast/utils/PathTracker.ts +++ b/src/ast/utils/PathTracker.ts @@ -28,4 +28,4 @@ export class PathTracker { } } -export const EMPTY_IMMUTABLE_TRACKER = new PathTracker(); +export const SHARED_RECURSION_TRACKER = new PathTracker(); diff --git a/test/cli/index.js b/test/cli/index.js index 7b25e47623f..3bef6e75a37 100644 --- a/test/cli/index.js +++ b/test/cli/index.js @@ -28,7 +28,7 @@ runTestSuiteWithSamples( const childProcess = exec( command, - { timeout: 40000, env: Object.assign({ FORCE_COLOR: '0' }, process.env, config.env) }, + { timeout: 40000, env: Object.assign({}, process.env, { FORCE_COLOR: '0' }, config.env) }, (err, code, stderr) => { if (err && !err.killed) { if (config.error) { diff --git a/test/form/samples/avoid-unnecessary-conditional-deopt/_config.js b/test/form/samples/avoid-unnecessary-conditional-deopt/_config.js new file mode 100644 index 00000000000..2c7ad61c32a --- /dev/null +++ b/test/form/samples/avoid-unnecessary-conditional-deopt/_config.js @@ -0,0 +1,3 @@ +module.exports = { + description: 'avoids unnecessary deoptimizations of conditional expressions' +}; diff --git a/test/form/samples/avoid-unnecessary-conditional-deopt/_expected.js b/test/form/samples/avoid-unnecessary-conditional-deopt/_expected.js new file mode 100644 index 00000000000..92a9ac36f0f --- /dev/null +++ b/test/form/samples/avoid-unnecessary-conditional-deopt/_expected.js @@ -0,0 +1,27 @@ +console.log('not modified'); + +let x2 = false; +modifyX2(); +const obj2 = {}; +(x2 ? obj2 : {}).modified = true; + +if (obj2.modified) console.log('modified'); +else console.log('not modified'); + +function modifyX2() { + x2 = true; +} + +console.log('not modified'); + +let x4 = false; +modifyX4(); +const obj4 = {}; +(x4 && obj4).modified = true; + +if (obj4.modified) console.log('modified'); +else console.log('not modified'); + +function modifyX4() { + x4 = true; +} diff --git a/test/form/samples/avoid-unnecessary-conditional-deopt/main.js b/test/form/samples/avoid-unnecessary-conditional-deopt/main.js new file mode 100644 index 00000000000..c00050cbaf4 --- /dev/null +++ b/test/form/samples/avoid-unnecessary-conditional-deopt/main.js @@ -0,0 +1,47 @@ +let x1 = false; +modifyX1(); +const obj1 = {}; +x1 ? obj1 : {}; + +if (obj1.modified) console.log('should not happen'); +else console.log('not modified'); + +function modifyX1() { + x1 = true; +} + +let x2 = false; +modifyX2(); +const obj2 = {}; +(x2 ? obj2 : {}).modified = true; + +if (obj2.modified) console.log('modified'); +else console.log('not modified'); + +function modifyX2() { + x2 = true; +} + +let x3 = false; +modifyX3(); +const obj3 = {}; +x3 && obj3; + +if (obj3.modified) console.log('should not happen'); +else console.log('not modified'); + +function modifyX3() { + x3 = true; +} + +let x4 = false; +modifyX4(); +const obj4 = {}; +(x4 && obj4).modified = true; + +if (obj4.modified) console.log('modified'); +else console.log('not modified'); + +function modifyX4() { + x4 = true; +} From 8f2cf9cb9ceb6685625624f940eca2ac9120bd49 Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Tue, 10 Dec 2019 08:19:17 +0100 Subject: [PATCH 2/5] Improve coverage --- src/ast/nodes/Property.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/ast/nodes/Property.ts b/src/ast/nodes/Property.ts index c2a4b44410a..8d43496dc81 100644 --- a/src/ast/nodes/Property.ts +++ b/src/ast/nodes/Property.ts @@ -52,10 +52,8 @@ export default class Property extends NodeBase implements DeoptimizableEntity { deoptimizePath(path: ObjectPath) { if (this.kind === 'get') { - if (path.length > 0) { - this.getReturnExpression().deoptimizePath(path); - } - } else if (this.kind !== 'set') { + this.getReturnExpression().deoptimizePath(path); + } else { this.value.deoptimizePath(path); } } From 451a6da82ae34e252d50ebc8e29afe58e3cf7811 Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Tue, 10 Dec 2019 09:53:42 +0100 Subject: [PATCH 3/5] Improve coverage --- src/ast/nodes/ObjectExpression.ts | 15 ++++------ .../invalid-property-assignments/_config.js | 6 ++++ .../invalid-property-assignments/main.js | 30 +++++++++++++++++++ 3 files changed, 41 insertions(+), 10 deletions(-) create mode 100644 test/function/samples/invalid-property-assignments/_config.js create mode 100644 test/function/samples/invalid-property-assignments/main.js diff --git a/src/ast/nodes/ObjectExpression.ts b/src/ast/nodes/ObjectExpression.ts index 8fde9e7c8cd..e6e70a1c908 100644 --- a/src/ast/nodes/ObjectExpression.ts +++ b/src/ast/nodes/ObjectExpression.ts @@ -63,10 +63,6 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE deoptimizePath(path: ObjectPath) { if (this.hasUnknownDeoptimizedProperty) return; const propertyMap = this.getPropertyMap(); - if (path.length === 0) { - this.deoptimizeAllProperties(); - return; - } const key = path[0]; if (path.length === 1) { if (typeof key !== 'string') { @@ -218,18 +214,17 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE } hasEffectsWhenAssignedAtPath(path: ObjectPath, context: HasEffectsContext) { - if (path.length === 0) return false; const key = path[0]; const propertyMap = this.propertyMap!; if ( path.length > 1 && (this.hasUnknownDeoptimizedProperty || - typeof key !== 'string' || - this.deoptimizedPaths.has(key) || - !propertyMap[key] || - propertyMap[key].exactMatchRead === null) - ) + this.deoptimizedPaths.has(key as string) || + !propertyMap[key as string] || + propertyMap[key as string].exactMatchRead === null) + ) { return true; + } const subPath = path.slice(1); for (const property of typeof key !== 'string' diff --git a/test/function/samples/invalid-property-assignments/_config.js b/test/function/samples/invalid-property-assignments/_config.js new file mode 100644 index 00000000000..e33b01d0ee7 --- /dev/null +++ b/test/function/samples/invalid-property-assignments/_config.js @@ -0,0 +1,6 @@ +module.exports = { + description: 'includes invalid property assignments', + options: { + treeshake: { propertyReadSideEffects: false, tryCatchDeoptimization: false } + } +}; diff --git a/test/function/samples/invalid-property-assignments/main.js b/test/function/samples/invalid-property-assignments/main.js new file mode 100644 index 00000000000..b1a443e0650 --- /dev/null +++ b/test/function/samples/invalid-property-assignments/main.js @@ -0,0 +1,30 @@ +function assertThrows(callback, description) { + let thrown = false; + try { + callback(); + } catch (err) { + thrown = true; + } + assert.ok(thrown, description); +} + +assertThrows(() => { + const obj = { first: { second: 0} }; + obj[globalThis.unknown].second = 1; +}, 'sub-property of unknown property'); + +assertThrows(() => { + const obj = { first: { second: 0} }; + delete obj.first; + obj.first.second = 1; +}, 'sub-property of reassigned property'); + +assertThrows(() => { + const obj = {}; + obj.first.second = 1; +}, 'sub-property of missing property'); + +assertThrows(() => { + const obj4 = { set first(value) {} }; + obj4.first.second = 1; +}, 'sub-property of property without getter'); From 5995bd0d63749ee18055f0c6a8b0299507a18339 Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Wed, 11 Dec 2019 07:43:18 +0100 Subject: [PATCH 4/5] Improve coverage --- src/ast/nodes/LogicalExpression.ts | 16 ++++---- src/ast/nodes/ObjectExpression.ts | 12 +++--- src/ast/nodes/Property.ts | 8 ++-- .../recursive-literal-values/_config.js | 3 ++ .../recursive-literal-values/_expected.js | 9 ++++ .../samples/recursive-literal-values/main.js | 9 ++++ .../invalid-property-assignments/_config.js | 7 +++- .../invalid-property-assignments/main.js | 2 +- .../samples/invalid-property-calls/_config.js | 11 +++++ .../samples/invalid-property-calls/main.js | 41 +++++++++++++++++++ 10 files changed, 97 insertions(+), 21 deletions(-) create mode 100644 test/form/samples/recursive-literal-values/_config.js create mode 100644 test/form/samples/recursive-literal-values/_expected.js create mode 100644 test/form/samples/recursive-literal-values/main.js create mode 100644 test/function/samples/invalid-property-calls/_config.js create mode 100644 test/function/samples/invalid-property-calls/main.js diff --git a/src/ast/nodes/LogicalExpression.ts b/src/ast/nodes/LogicalExpression.ts index 22804239d72..8dd2a628a4e 100644 --- a/src/ast/nodes/LogicalExpression.ts +++ b/src/ast/nodes/LogicalExpression.ts @@ -58,15 +58,13 @@ export default class LogicalExpression extends NodeBase implements Deoptimizable } deoptimizePath(path: ObjectPath) { - if (path.length > 0) { - const usedBranch = this.getUsedBranch(); - if (usedBranch === null) { - this.left.deoptimizePath(path); - this.right.deoptimizePath(path); - } else { - this.wasPathDeoptimizedWhileOptimized = true; - usedBranch.deoptimizePath(path); - } + const usedBranch = this.getUsedBranch(); + if (usedBranch === null) { + this.left.deoptimizePath(path); + this.right.deoptimizePath(path); + } else { + this.wasPathDeoptimizedWhileOptimized = true; + usedBranch.deoptimizePath(path); } } diff --git a/src/ast/nodes/ObjectExpression.ts b/src/ast/nodes/ObjectExpression.ts index e6e70a1c908..338b8b546ac 100644 --- a/src/ast/nodes/ObjectExpression.ts +++ b/src/ast/nodes/ObjectExpression.ts @@ -246,18 +246,20 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE ): boolean { const key = path[0]; if ( - path.length === 0 || - this.hasUnknownDeoptimizedProperty || typeof key !== 'string' || + this.hasUnknownDeoptimizedProperty || this.deoptimizedPaths.has(key) || (this.propertyMap![key] ? !this.propertyMap![key].exactMatchRead : path.length > 1 || !objectMembers[key]) - ) + ) { return true; + } const subPath = path.slice(1); - for (const property of this.propertyMap![key] ? this.propertyMap![key].propertiesRead : []) { - if (property.hasEffectsWhenCalledAtPath(subPath, callOptions, context)) return true; + if (this.propertyMap![key]) { + for (const property of this.propertyMap![key].propertiesRead) { + if (property.hasEffectsWhenCalledAtPath(subPath, callOptions, context)) return true; + } } if (path.length === 1 && objectMembers[key]) return hasMemberEffectWhenCalled(objectMembers, key, this.included, callOptions, context); diff --git a/src/ast/nodes/Property.ts b/src/ast/nodes/Property.ts index 8d43496dc81..6354bc9522b 100644 --- a/src/ast/nodes/Property.ts +++ b/src/ast/nodes/Property.ts @@ -44,11 +44,9 @@ export default class Property extends NodeBase implements DeoptimizableEntity { return this.value.declare(kind, UNKNOWN_EXPRESSION); } - deoptimizeCache(): void { - // As getter properties directly receive their values from function expressions that always - // have a fixed return value, there is no known situation where a getter is deoptimized. - throw new Error('Unexpected deoptimization'); - } + // As getter properties directly receive their values from function expressions that always + // have a fixed return value, there is no known situation where a getter is deoptimized. + deoptimizeCache(): void {} deoptimizePath(path: ObjectPath) { if (this.kind === 'get') { diff --git a/test/form/samples/recursive-literal-values/_config.js b/test/form/samples/recursive-literal-values/_config.js new file mode 100644 index 00000000000..490e29b384d --- /dev/null +++ b/test/form/samples/recursive-literal-values/_config.js @@ -0,0 +1,3 @@ +module.exports = { + description: 'do not fail for literal values from recursive return values' +}; diff --git a/test/form/samples/recursive-literal-values/_expected.js b/test/form/samples/recursive-literal-values/_expected.js new file mode 100644 index 00000000000..cb9c794cd36 --- /dev/null +++ b/test/form/samples/recursive-literal-values/_expected.js @@ -0,0 +1,9 @@ +function foo() { + return foo() +} + +if (foo()) { + console.log('A'); +} else { + console.log('B'); +} diff --git a/test/form/samples/recursive-literal-values/main.js b/test/form/samples/recursive-literal-values/main.js new file mode 100644 index 00000000000..c4416573e2c --- /dev/null +++ b/test/form/samples/recursive-literal-values/main.js @@ -0,0 +1,9 @@ +function foo() { + return foo() +} + +if (foo()) { + console.log('A') +} else { + console.log('B') +} diff --git a/test/function/samples/invalid-property-assignments/_config.js b/test/function/samples/invalid-property-assignments/_config.js index e33b01d0ee7..77c1bcbb785 100644 --- a/test/function/samples/invalid-property-assignments/_config.js +++ b/test/function/samples/invalid-property-assignments/_config.js @@ -1,6 +1,11 @@ module.exports = { description: 'includes invalid property assignments', + context: { globalOther: 'other' }, options: { - treeshake: { propertyReadSideEffects: false, tryCatchDeoptimization: false } + treeshake: { + propertyReadSideEffects: false, + tryCatchDeoptimization: false, + unknownGlobalSideEffects: false + } } }; diff --git a/test/function/samples/invalid-property-assignments/main.js b/test/function/samples/invalid-property-assignments/main.js index b1a443e0650..9641ce13662 100644 --- a/test/function/samples/invalid-property-assignments/main.js +++ b/test/function/samples/invalid-property-assignments/main.js @@ -10,7 +10,7 @@ function assertThrows(callback, description) { assertThrows(() => { const obj = { first: { second: 0} }; - obj[globalThis.unknown].second = 1; + obj[globalOther].second = 1; }, 'sub-property of unknown property'); assertThrows(() => { diff --git a/test/function/samples/invalid-property-calls/_config.js b/test/function/samples/invalid-property-calls/_config.js new file mode 100644 index 00000000000..b9a1c9c33f3 --- /dev/null +++ b/test/function/samples/invalid-property-calls/_config.js @@ -0,0 +1,11 @@ +module.exports = { + description: 'includes invalid property calls', + context: { globalFirst: 'first', globalOther: 'other' }, + options: { + treeshake: { + propertyReadSideEffects: false, + tryCatchDeoptimization: false, + unknownGlobalSideEffects: false + } + } +}; diff --git a/test/function/samples/invalid-property-calls/main.js b/test/function/samples/invalid-property-calls/main.js new file mode 100644 index 00000000000..5c4771e149b --- /dev/null +++ b/test/function/samples/invalid-property-calls/main.js @@ -0,0 +1,41 @@ +function assertThrows(callback, description) { + let thrown = false; + try { + callback(); + } catch (err) { + thrown = true; + } + assert.ok(thrown, description); +} + +assertThrows(() => { + const obj = { first: () => {} }; + obj[globalOther](); +}, 'unknown missing property'); + +assertThrows(() => { + const obj = { first: () => {} }; + delete obj[globalFirst]; + obj.first(); +}, 'known property that might be missing'); + +assertThrows(() => { + const obj = { first: () => {} }; + delete obj.first; + obj.first.second(); +}, 'known property that is missing'); + +assertThrows(() => { + const obj = { set first(value) {} }; + obj.first.second(); +}, 'property without getter'); + +assertThrows(() => { + const obj = {}; + obj.hasOwnProperty.second('first'); +}, 'sub-property of object method'); + +assertThrows(() => { + const obj = {}; + obj.first(); +}, 'missing property'); From 685bc89fead35d03dde58e9f0f0bda7cdedd8e0a Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Wed, 11 Dec 2019 08:39:32 +0100 Subject: [PATCH 5/5] Fix vulnerability --- package-lock.json | 125 ++++++++++++++---- package.json | 6 +- .../samples/supports-core-js/_expected.js | 31 +++-- 3 files changed, 121 insertions(+), 41 deletions(-) diff --git a/package-lock.json b/package-lock.json index 7c19f007274..d5541e39395 100644 --- a/package-lock.json +++ b/package-lock.json @@ -245,9 +245,9 @@ "dev": true }, "@types/node": { - "version": "12.12.14", - "resolved": "https://registry.npmjs.org/@types/node/-/node-12.12.14.tgz", - "integrity": "sha512-u/SJDyXwuihpwjXy7hOOghagLEV1KdAST6syfnOk6QZAMzZuWZqXy5aYYZbh8Jdpd4escVFP0MvftHNDb9pruA==" + "version": "12.12.17", + "resolved": "https://registry.npmjs.org/@types/node/-/node-12.12.17.tgz", + "integrity": "sha512-Is+l3mcHvs47sKy+afn2O1rV4ldZFU7W8101cNlOd+MRbjM4Onida8jSZnJdTe/0Pcf25g9BNIUsuugmE6puHA==" }, "@types/normalize-package-data": { "version": "2.4.0", @@ -527,6 +527,54 @@ "integrity": "sha1-qJS3XUvE9s1nnvMkSp/Y9Gri1Cg=", "dev": true }, + "array.prototype.flat": { + "version": "1.2.2", + "resolved": "https://registry.npmjs.org/array.prototype.flat/-/array.prototype.flat-1.2.2.tgz", + "integrity": "sha512-VXjh7lAL4KXKF2hY4FnEW9eRW6IhdvFW1sN/JwLbmECbCgACCnBHNyP3lFiYuttr0jxRN9Bsc5+G27dMseSWqQ==", + "dev": true, + "requires": { + "define-properties": "^1.1.3", + "es-abstract": "^1.15.0", + "function-bind": "^1.1.1" + }, + "dependencies": { + "es-abstract": { + "version": "1.16.3", + "resolved": "https://registry.npmjs.org/es-abstract/-/es-abstract-1.16.3.tgz", + "integrity": "sha512-WtY7Fx5LiOnSYgF5eg/1T+GONaGmpvpPdCpSnYij+U2gDTL0UPfWrhDw7b2IYb+9NQJsYpCA0wOQvZfsd6YwRw==", + "dev": true, + "requires": { + "es-to-primitive": "^1.2.1", + "function-bind": "^1.1.1", + "has": "^1.0.3", + "has-symbols": "^1.0.1", + "is-callable": "^1.1.4", + "is-regex": "^1.0.4", + "object-inspect": "^1.7.0", + "object-keys": "^1.1.1", + "string.prototype.trimleft": "^2.1.0", + "string.prototype.trimright": "^2.1.0" + } + }, + "es-to-primitive": { + "version": "1.2.1", + "resolved": "https://registry.npmjs.org/es-to-primitive/-/es-to-primitive-1.2.1.tgz", + "integrity": "sha512-QCOllgZJtaUo9miYBcLChTUaHNjJF3PYs1VidD7AwiEj1kYxKeQTctLAezAOH5ZKRH0g2IgPn6KwB4IT8iRpvA==", + "dev": true, + "requires": { + "is-callable": "^1.1.4", + "is-date-object": "^1.0.1", + "is-symbol": "^1.0.2" + } + }, + "has-symbols": { + "version": "1.0.1", + "resolved": "https://registry.npmjs.org/has-symbols/-/has-symbols-1.0.1.tgz", + "integrity": "sha512-PLcsoqu++dmEIZB+6totNFKq/7Do+Z0u4oT0zKOJNl3lYK6vGwwu2hjHs+68OEZbTjiUE9bgOABXbP/GvrS0Kg==", + "dev": true + } + } + }, "assign-symbols": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/assign-symbols/-/assign-symbols-1.0.0.tgz", @@ -1096,9 +1144,9 @@ "dev": true }, "core-js": { - "version": "3.4.7", - "resolved": "https://registry.npmjs.org/core-js/-/core-js-3.4.7.tgz", - "integrity": "sha512-qaPVGw30J1wQ0GR3GvoPqlGf9GZfKKF4kFC7kiHlcsPTqH3txrs9crCp3ZiMAXuSenhz89Jnl4GZs/67S5VOSg==", + "version": "3.4.8", + "resolved": "https://registry.npmjs.org/core-js/-/core-js-3.4.8.tgz", + "integrity": "sha512-b+BBmCZmVgho8KnBUOXpvlqEMguko+0P+kXCwD4vIprsXC6ht1qgPxtb1OK6XgSlrySF71wkwBQ0Hv695bk9gQ==", "dev": true }, "core-util-is": { @@ -1609,32 +1657,33 @@ } }, "eslint-module-utils": { - "version": "2.4.1", - "resolved": "https://registry.npmjs.org/eslint-module-utils/-/eslint-module-utils-2.4.1.tgz", - "integrity": "sha512-H6DOj+ejw7Tesdgbfs4jeS4YMFrT8uI8xwd1gtQqXssaR0EQ26L+2O/w6wkYFy2MymON0fTwHmXBvvfLNZVZEw==", + "version": "2.5.0", + "resolved": "https://registry.npmjs.org/eslint-module-utils/-/eslint-module-utils-2.5.0.tgz", + "integrity": "sha512-kCo8pZaNz2dsAW7nCUjuVoI11EBXXpIzfNxmaoLhXoRDOnqXLC4iSGVRdZPhOitfbdEfMEfKOiENaK6wDPZEGw==", "dev": true, "requires": { - "debug": "^2.6.8", + "debug": "^2.6.9", "pkg-dir": "^2.0.0" } }, "eslint-plugin-import": { - "version": "2.18.2", - "resolved": "https://registry.npmjs.org/eslint-plugin-import/-/eslint-plugin-import-2.18.2.tgz", - "integrity": "sha512-5ohpsHAiUBRNaBWAF08izwUGlbrJoJJ+W9/TBwsGoR1MnlgfwMIKrFeSjWbt6moabiXW9xNvtFz+97KHRfI4HQ==", + "version": "2.19.1", + "resolved": "https://registry.npmjs.org/eslint-plugin-import/-/eslint-plugin-import-2.19.1.tgz", + "integrity": "sha512-x68131aKoCZlCae7rDXKSAQmbT5DQuManyXo2sK6fJJ0aK5CWAkv6A6HJZGgqC8IhjQxYPgo6/IY4Oz8AFsbBw==", "dev": true, "requires": { "array-includes": "^3.0.3", + "array.prototype.flat": "^1.2.1", "contains-path": "^0.1.0", "debug": "^2.6.9", "doctrine": "1.5.0", "eslint-import-resolver-node": "^0.3.2", - "eslint-module-utils": "^2.4.0", + "eslint-module-utils": "^2.4.1", "has": "^1.0.3", "minimatch": "^3.0.4", "object.values": "^1.1.0", "read-pkg-up": "^2.0.0", - "resolve": "^1.11.0" + "resolve": "^1.12.0" }, "dependencies": { "doctrine": { @@ -1648,9 +1697,9 @@ } }, "resolve": { - "version": "1.12.0", - "resolved": "https://registry.npmjs.org/resolve/-/resolve-1.12.0.tgz", - "integrity": "sha512-B/dOmuoAik5bKcD6s6nXDCjzUKnaDvdkRyAk6rsmsKLipWj4797iothd7jmmUhWTfinVMU+wc56rYKsit2Qy4w==", + "version": "1.13.1", + "resolved": "https://registry.npmjs.org/resolve/-/resolve-1.13.1.tgz", + "integrity": "sha512-CxqObCX8K8YtAhOBRg+lrcdn+LK+WYOS8tSjqSFbjtrI5PnS63QPhZl4+yKfrU9tdsbMu9Anr/amegT87M9Z6w==", "dev": true, "requires": { "path-parse": "^1.0.6" @@ -4730,6 +4779,12 @@ } } }, + "object-inspect": { + "version": "1.7.0", + "resolved": "https://registry.npmjs.org/object-inspect/-/object-inspect-1.7.0.tgz", + "integrity": "sha512-a7pEHdh1xKIAgTySUGgLMx/xwDZskN1Ud6egYYN3EdRW4ZMPNEDUTF+hwy2LUC+Bl+SyLXANnwz/jyh/qutKUw==", + "dev": true + }, "object-keys": { "version": "1.1.1", "resolved": "https://registry.npmjs.org/object-keys/-/object-keys-1.1.1.tgz", @@ -5427,9 +5482,9 @@ } }, "rollup": { - "version": "1.27.8", - "resolved": "https://registry.npmjs.org/rollup/-/rollup-1.27.8.tgz", - "integrity": "sha512-EVoEV5rAWl+5clnGznt1KY8PeVkzVQh/R0d2s3gHEkN7gfoyC4JmvIVuCtPbYE8NM5Ep/g+nAmvKXBjzaqTsHA==", + "version": "1.27.9", + "resolved": "https://registry.npmjs.org/rollup/-/rollup-1.27.9.tgz", + "integrity": "sha512-8AfW4cJTPZfG6EXWwT/ujL4owUsDI1Xl8J1t+hvK4wDX81F5I4IbwP9gvGbHzxnV19fnU4rRABZQwZSX9J402Q==", "dev": true, "requires": { "@types/estree": "*", @@ -5579,7 +5634,7 @@ "@babel/code-frame": "^7.0.0", "jest-worker": "^24.6.0", "rollup-pluginutils": "^2.8.1", - "serialize-javascript": "^1.7.0", + "serialize-javascript": "^2.1.1", "terser": "^4.1.0" } }, @@ -5695,9 +5750,9 @@ "dev": true }, "serialize-javascript": { - "version": "1.9.1", - "resolved": "https://registry.npmjs.org/serialize-javascript/-/serialize-javascript-1.9.1.tgz", - "integrity": "sha512-0Vb/54WJ6k5v8sSWN09S0ora+Hnr+cX40r9F170nT+mSkaxltoE/7R3OrIdBSUv1OoiobH1QoWQbCnAO+e8J1A==", + "version": "2.1.2", + "resolved": "https://registry.npmjs.org/serialize-javascript/-/serialize-javascript-2.1.2.tgz", + "integrity": "sha512-rs9OggEUF0V4jUSecXazOYsLfu7OGK2qIn3c7IPBiffz32XniEp/TX9Xmc9LQfK2nQ2QKHvZ2oygKUGU0lG4jQ==", "dev": true }, "set-blocking": { @@ -6078,6 +6133,26 @@ "strip-ansi": "^4.0.0" } }, + "string.prototype.trimleft": { + "version": "2.1.0", + "resolved": "https://registry.npmjs.org/string.prototype.trimleft/-/string.prototype.trimleft-2.1.0.tgz", + "integrity": "sha512-FJ6b7EgdKxxbDxc79cOlok6Afd++TTs5szo+zJTUyow3ycrRfJVE2pq3vcN53XexvKZu/DJMDfeI/qMiZTrjTw==", + "dev": true, + "requires": { + "define-properties": "^1.1.3", + "function-bind": "^1.1.1" + } + }, + "string.prototype.trimright": { + "version": "2.1.0", + "resolved": "https://registry.npmjs.org/string.prototype.trimright/-/string.prototype.trimright-2.1.0.tgz", + "integrity": "sha512-fXZTSV55dNBwv16uw+hh5jkghxSnc5oHq+5K/gXgizHwAvMetdAJlHqqoFC1FSDVPYWLkAKl2cxpUT41sV7nSg==", + "dev": true, + "requires": { + "define-properties": "^1.1.3", + "function-bind": "^1.1.1" + } + }, "string_decoder": { "version": "1.1.1", "resolved": "https://registry.npmjs.org/string_decoder/-/string_decoder-1.1.1.tgz", diff --git a/package.json b/package.json index 95a29f5ba3f..9770aada196 100644 --- a/package.json +++ b/package.json @@ -80,13 +80,13 @@ "chokidar": "^2.1.8", "codecov": "^3.6.1", "console-group": "^0.3.3", - "core-js": "^3.4.7", + "core-js": "^3.4.8", "cross-os": "^1.3.0", "date-time": "^3.1.0", "es5-shim": "^4.5.13", "es6-shim": "^0.35.5", "eslint": "^6.7.2", - "eslint-plugin-import": "^2.18.2", + "eslint-plugin-import": "^2.19.1", "execa": "^3.4.0", "fixturify": "^1.2.0", "hash.js": "^1.1.7", @@ -105,7 +105,7 @@ "pretty-ms": "^5.1.0", "require-relative": "^0.8.7", "requirejs": "^2.3.6", - "rollup": "^1.27.8", + "rollup": "^1.27.9", "rollup-plugin-alias": "^2.2.0", "rollup-plugin-buble": "^0.19.8", "rollup-plugin-commonjs": "^10.1.0", diff --git a/test/form/samples/supports-core-js/_expected.js b/test/form/samples/supports-core-js/_expected.js index 8f7e614d827..0ee43f2e7be 100644 --- a/test/form/samples/supports-core-js/_expected.js +++ b/test/form/samples/supports-core-js/_expected.js @@ -185,33 +185,38 @@ var setGlobal = function (key, value) { } return value; }; -var isPure = false; - var SHARED = '__core-js_shared__'; var store = global_1[SHARED] || setGlobal(SHARED, {}); var sharedStore = store; +var functionToString = Function.toString; + +// this helper broken in `3.4.1-3.4.4`, so we can't use `shared` helper +if (typeof sharedStore.inspectSource != 'function') { + sharedStore.inspectSource = function (it) { + return functionToString.call(it); + }; +} + +var inspectSource = sharedStore.inspectSource; + +var WeakMap = global_1.WeakMap; + +var nativeWeakMap = typeof WeakMap === 'function' && /native code/.test(inspectSource(WeakMap)); + +var isPure = false; + var shared = createCommonjsModule(function (module) { (module.exports = function (key, value) { return sharedStore[key] || (sharedStore[key] = value !== undefined ? value : {}); })('versions', []).push({ - version: '3.4.7', + version: '3.4.8', mode: 'global', copyright: '© 2019 Denis Pushkarev (zloirock.ru)' }); }); -var functionToString = Function.toString; - -var inspectSource = shared('inspectSource', function (it) { - return functionToString.call(it); -}); - -var WeakMap = global_1.WeakMap; - -var nativeWeakMap = typeof WeakMap === 'function' && /native code/.test(inspectSource(WeakMap)); - var id = 0; var postfix = Math.random();