From 4dac40c4bef6ab4568a1e169db2932929f867ab2 Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Sun, 8 May 2022 16:47:17 +0200 Subject: [PATCH 1/8] Do not make Object.defineProperty/ies a side effect by default --- src/ast/Entity.ts | 7 +++- src/ast/nodes/ArrayExpression.ts | 8 +++-- src/ast/nodes/Identifier.ts | 8 +++-- src/ast/nodes/ObjectExpression.ts | 8 +++-- src/ast/nodes/shared/ClassNode.ts | 8 +++-- src/ast/nodes/shared/Expression.ts | 6 +++- src/ast/nodes/shared/ObjectEntity.ts | 15 ++++++-- src/ast/nodes/shared/knownGlobals.ts | 35 ++++++++++--------- src/ast/variables/GlobalVariable.ts | 25 ++++++++++--- src/ast/variables/LocalVariable.ts | 8 +++-- src/ast/variables/ThisVariable.ts | 8 +++-- .../samples/object-define-property/_config.js | 3 ++ .../object-define-property/_expected.js | 7 ++++ .../samples/object-define-property/main.js | 20 +++++++++++ 14 files changed, 129 insertions(+), 37 deletions(-) create mode 100644 test/form/samples/object-define-property/_config.js create mode 100644 test/form/samples/object-define-property/_expected.js create mode 100644 test/form/samples/object-define-property/main.js diff --git a/src/ast/Entity.ts b/src/ast/Entity.ts index 4447be9f109..32bd4f51ea8 100644 --- a/src/ast/Entity.ts +++ b/src/ast/Entity.ts @@ -12,5 +12,10 @@ export interface WritableEntity extends Entity { * expression of this node is reassigned as well. */ deoptimizePath(path: ObjectPath): void; - hasEffectsWhenAssignedAtPath(path: ObjectPath, context: HasEffectsContext): boolean; + + hasEffectsWhenAssignedAtPath( + path: ObjectPath, + context: HasEffectsContext, + ignoreAccessors?: boolean + ): boolean; } diff --git a/src/ast/nodes/ArrayExpression.ts b/src/ast/nodes/ArrayExpression.ts index 406687d99fb..f79a82df6ad 100644 --- a/src/ast/nodes/ArrayExpression.ts +++ b/src/ast/nodes/ArrayExpression.ts @@ -60,8 +60,12 @@ export default class ArrayExpression extends NodeBase { return this.getObjectEntity().hasEffectsWhenAccessedAtPath(path, context); } - hasEffectsWhenAssignedAtPath(path: ObjectPath, context: HasEffectsContext): boolean { - return this.getObjectEntity().hasEffectsWhenAssignedAtPath(path, context); + hasEffectsWhenAssignedAtPath( + path: ObjectPath, + context: HasEffectsContext, + ignoreAccessors: boolean + ): boolean { + return this.getObjectEntity().hasEffectsWhenAssignedAtPath(path, context, ignoreAccessors); } hasEffectsWhenCalledAtPath( diff --git a/src/ast/nodes/Identifier.ts b/src/ast/nodes/Identifier.ts index 89965f79287..b786581b493 100644 --- a/src/ast/nodes/Identifier.ts +++ b/src/ast/nodes/Identifier.ts @@ -144,13 +144,17 @@ export default class Identifier extends NodeBase implements PatternNode { ); } - hasEffectsWhenAssignedAtPath(path: ObjectPath, context: HasEffectsContext): boolean { + hasEffectsWhenAssignedAtPath( + path: ObjectPath, + context: HasEffectsContext, + ignoreAccessors: boolean + ): boolean { return ( !this.variable || (path.length > 0 ? this.getVariableRespectingTDZ() : this.variable - ).hasEffectsWhenAssignedAtPath(path, context) + ).hasEffectsWhenAssignedAtPath(path, context, ignoreAccessors) ); } diff --git a/src/ast/nodes/ObjectExpression.ts b/src/ast/nodes/ObjectExpression.ts index 61ff336a6c1..9b3399adbd3 100644 --- a/src/ast/nodes/ObjectExpression.ts +++ b/src/ast/nodes/ObjectExpression.ts @@ -79,8 +79,12 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE return this.getObjectEntity().hasEffectsWhenAccessedAtPath(path, context); } - hasEffectsWhenAssignedAtPath(path: ObjectPath, context: HasEffectsContext): boolean { - return this.getObjectEntity().hasEffectsWhenAssignedAtPath(path, context); + hasEffectsWhenAssignedAtPath( + path: ObjectPath, + context: HasEffectsContext, + ignoreAccessors: boolean + ): boolean { + return this.getObjectEntity().hasEffectsWhenAssignedAtPath(path, context, ignoreAccessors); } hasEffectsWhenCalledAtPath( diff --git a/src/ast/nodes/shared/ClassNode.ts b/src/ast/nodes/shared/ClassNode.ts index f8823593b89..3bf932041b3 100644 --- a/src/ast/nodes/shared/ClassNode.ts +++ b/src/ast/nodes/shared/ClassNode.ts @@ -86,8 +86,12 @@ export default class ClassNode extends NodeBase implements DeoptimizableEntity { return this.getObjectEntity().hasEffectsWhenAccessedAtPath(path, context); } - hasEffectsWhenAssignedAtPath(path: ObjectPath, context: HasEffectsContext): boolean { - return this.getObjectEntity().hasEffectsWhenAssignedAtPath(path, context); + hasEffectsWhenAssignedAtPath( + path: ObjectPath, + context: HasEffectsContext, + ignoreAccessors: boolean + ): boolean { + return this.getObjectEntity().hasEffectsWhenAssignedAtPath(path, context, ignoreAccessors); } hasEffectsWhenCalledAtPath( diff --git a/src/ast/nodes/shared/Expression.ts b/src/ast/nodes/shared/Expression.ts index 60764af727c..01a46c7745f 100644 --- a/src/ast/nodes/shared/Expression.ts +++ b/src/ast/nodes/shared/Expression.ts @@ -52,7 +52,11 @@ export class ExpressionEntity implements WritableEntity { return true; } - hasEffectsWhenAssignedAtPath(_path: ObjectPath, _context: HasEffectsContext): boolean { + hasEffectsWhenAssignedAtPath( + _path: ObjectPath, + _context: HasEffectsContext, + _ignoreAccessors?: boolean + ): boolean { return true; } diff --git a/src/ast/nodes/shared/ObjectEntity.ts b/src/ast/nodes/shared/ObjectEntity.ts index ad84a57eb36..bb382bc38a4 100644 --- a/src/ast/nodes/shared/ObjectEntity.ts +++ b/src/ast/nodes/shared/ObjectEntity.ts @@ -302,7 +302,11 @@ export class ObjectEntity extends ExpressionEntity { return false; } - hasEffectsWhenAssignedAtPath(path: ObjectPath, context: HasEffectsContext): boolean { + hasEffectsWhenAssignedAtPath( + path: ObjectPath, + context: HasEffectsContext, + ignoreAccessors: boolean + ): boolean { const [key, ...subPath] = path; if (path.length > 1) { if (typeof key !== 'string') { @@ -310,14 +314,19 @@ export class ObjectEntity extends ExpressionEntity { } const expressionAtPath = this.getMemberExpression(key); if (expressionAtPath) { - return expressionAtPath.hasEffectsWhenAssignedAtPath(subPath, context); + return expressionAtPath.hasEffectsWhenAssignedAtPath(subPath, context, ignoreAccessors); } if (this.prototypeExpression) { - return this.prototypeExpression.hasEffectsWhenAssignedAtPath(path, context); + return this.prototypeExpression.hasEffectsWhenAssignedAtPath( + path, + context, + ignoreAccessors + ); } return true; } + if (ignoreAccessors) return false; if (this.hasUnknownDeoptimizedProperty) return true; // We do not need to test for unknown properties as in that case, hasUnknownDeoptimizedProperty is true if (typeof key === 'string') { diff --git a/src/ast/nodes/shared/knownGlobals.ts b/src/ast/nodes/shared/knownGlobals.ts index b628fd3924f..3b8c05979f0 100644 --- a/src/ast/nodes/shared/knownGlobals.ts +++ b/src/ast/nodes/shared/knownGlobals.ts @@ -5,7 +5,11 @@ import type { ObjectPath } from '../../utils/PathTracker'; const ValueProperties = Symbol('Value Properties'); interface ValueDescription { - pure: boolean; + // Denotes a proper function except for the side effects listed explicitly + function: boolean; + // This assumes that mutation itself cannot have side effects and that this + // does not trigger setters or getters + mutatesArg1: boolean; } interface GlobalDescription { @@ -14,8 +18,8 @@ interface GlobalDescription { __proto__: null; } -const PURE: ValueDescription = { pure: true }; -const IMPURE: ValueDescription = { pure: false }; +const PURE: ValueDescription = { function: true, mutatesArg1: false }; +const IMPURE: ValueDescription = { function: false, mutatesArg1: false }; // We use shortened variables to reduce file size here /* OBJECT */ @@ -30,6 +34,12 @@ const PF: GlobalDescription = { [ValueProperties]: PURE }; +/* FUNCTION THAT MUTATES FIRST ARG WITHOUT TRIGGERING ACCESSORS */ +const MUTATES_ARG_WITHOUT_ACCESSOR: GlobalDescription = { + __proto__: null, + [ValueProperties]: { function: true, mutatesArg1: true } +}; + /* CONSTRUCTOR */ const C: GlobalDescription = { __proto__: null, @@ -173,6 +183,11 @@ const knownGlobals: GlobalDescription = { __proto__: null, [ValueProperties]: PURE, create: PF, + // Technically those can throw in certain situations, but we ignore this as + // code that relies on this will hopefully wrap this in a try-catch, which + // deoptimizes everything anyway + defineProperty: MUTATES_ARG_WITHOUT_ACCESSOR, + defineProperties: MUTATES_ARG_WITHOUT_ACCESSOR, getOwnPropertyDescriptor: PF, getOwnPropertyNames: PF, getOwnPropertySymbols: PF, @@ -847,7 +862,7 @@ for (const global of ['window', 'global', 'self', 'globalThis']) { knownGlobals[global] = knownGlobals; } -function getGlobalAtPath(path: ObjectPath): ValueDescription | null { +export function getGlobalAtPath(path: ObjectPath): ValueDescription | null { let currentGlobal: GlobalDescription | null = knownGlobals; for (const pathSegment of path) { if (typeof pathSegment !== 'string') { @@ -860,15 +875,3 @@ function getGlobalAtPath(path: ObjectPath): ValueDescription | null { } return currentGlobal[ValueProperties]; } - -export function isPureGlobal(path: ObjectPath): boolean { - const globalAtPath = getGlobalAtPath(path); - return globalAtPath !== null && globalAtPath.pure; -} - -export function isGlobalMember(path: ObjectPath): boolean { - if (path.length === 1) { - return path[0] === 'undefined' || getGlobalAtPath(path) !== null; - } - return getGlobalAtPath(path.slice(0, -1)) !== null; -} diff --git a/src/ast/variables/GlobalVariable.ts b/src/ast/variables/GlobalVariable.ts index 7ac5c466962..160d0825fd9 100644 --- a/src/ast/variables/GlobalVariable.ts +++ b/src/ast/variables/GlobalVariable.ts @@ -1,4 +1,7 @@ -import { isGlobalMember, isPureGlobal } from '../nodes/shared/knownGlobals'; +import { CallOptions } from '../CallOptions'; +import { HasEffectsContext } from '../ExecutionContext'; +import { getGlobalAtPath } from '../nodes/shared/knownGlobals'; +import { UNKNOWN_PATH } from '../utils/PathTracker'; import type { ObjectPath } from '../utils/PathTracker'; import Variable from './Variable'; @@ -6,10 +9,24 @@ export default class GlobalVariable extends Variable { isReassigned = true; hasEffectsWhenAccessedAtPath(path: ObjectPath): boolean { - return !isGlobalMember([this.name, ...path]); + if (path.length === 0) { + return this.name !== 'undefined' && getGlobalAtPath([this.name]) === null; + } + return getGlobalAtPath([this.name, ...path].slice(0, -1)) === null; } - hasEffectsWhenCalledAtPath(path: ObjectPath): boolean { - return !isPureGlobal([this.name, ...path]); + hasEffectsWhenCalledAtPath( + path: ObjectPath, + callOptions: CallOptions, + context: HasEffectsContext + ): boolean { + const globalAtPath = getGlobalAtPath([this.name, ...path]); + return ( + globalAtPath === null || + !globalAtPath.function || + (globalAtPath.mutatesArg1 && + (!callOptions.args.length || + callOptions.args[0].hasEffectsWhenAssignedAtPath(UNKNOWN_PATH, context, true))) + ); } } diff --git a/src/ast/variables/LocalVariable.ts b/src/ast/variables/LocalVariable.ts index a90613a2825..2ebbd5dd91b 100644 --- a/src/ast/variables/LocalVariable.ts +++ b/src/ast/variables/LocalVariable.ts @@ -150,13 +150,17 @@ export default class LocalVariable extends Variable { this.init.hasEffectsWhenAccessedAtPath(path, context))!; } - hasEffectsWhenAssignedAtPath(path: ObjectPath, context: HasEffectsContext): boolean { + hasEffectsWhenAssignedAtPath( + path: ObjectPath, + context: HasEffectsContext, + ignoreAccessors: boolean + ): boolean { if (this.included) return true; if (path.length === 0) return false; if (this.isReassigned) return true; return (this.init && !context.assigned.trackEntityAtPathAndGetIfTracked(path, this) && - this.init.hasEffectsWhenAssignedAtPath(path, context))!; + this.init.hasEffectsWhenAssignedAtPath(path, context, ignoreAccessors))!; } hasEffectsWhenCalledAtPath( diff --git a/src/ast/variables/ThisVariable.ts b/src/ast/variables/ThisVariable.ts index 7c4cba21045..c783be89803 100644 --- a/src/ast/variables/ThisVariable.ts +++ b/src/ast/variables/ThisVariable.ts @@ -73,10 +73,14 @@ export default class ThisVariable extends LocalVariable { ); } - hasEffectsWhenAssignedAtPath(path: ObjectPath, context: HasEffectsContext): boolean { + hasEffectsWhenAssignedAtPath( + path: ObjectPath, + context: HasEffectsContext, + ignoreAccessors: boolean + ): boolean { return ( this.getInit(context).hasEffectsWhenAssignedAtPath(path, context) || - super.hasEffectsWhenAssignedAtPath(path, context) + super.hasEffectsWhenAssignedAtPath(path, context, ignoreAccessors) ); } diff --git a/test/form/samples/object-define-property/_config.js b/test/form/samples/object-define-property/_config.js new file mode 100644 index 00000000000..406267436f3 --- /dev/null +++ b/test/form/samples/object-define-property/_config.js @@ -0,0 +1,3 @@ +module.exports = { + description: 'allows globals to have parameter mutation side effects' +}; diff --git a/test/form/samples/object-define-property/_expected.js b/test/form/samples/object-define-property/_expected.js new file mode 100644 index 00000000000..58141e185ae --- /dev/null +++ b/test/form/samples/object-define-property/_expected.js @@ -0,0 +1,7 @@ +const retained1 = {}; +Object.defineProperty(retained1, 'foo', { value: true }); +console.log(retained1); + +const retained2 = {}; +Object.defineProperties(retained2, { bar: { value: true } }); +console.log(retained2); diff --git a/test/form/samples/object-define-property/main.js b/test/form/samples/object-define-property/main.js new file mode 100644 index 00000000000..0b23bfb3f22 --- /dev/null +++ b/test/form/samples/object-define-property/main.js @@ -0,0 +1,20 @@ +const removed = {}; +Object.defineProperty(removed, 'foo', { value: true }); +Object.defineProperties(removed, { bar: { value: true } }); + +const retained1 = {}; +Object.defineProperty(retained1, 'foo', { value: true }); +console.log(retained1); + +const retained2 = {}; +Object.defineProperties(retained2, { bar: { value: true } }); +console.log(retained2); + +const removed2 = []; +Object.defineProperty(removed2, 'foo', { value: true }); + +class removed3 {} +Object.defineProperty(removed3, 'foo', { value: true }); + +function removed4() {} +Object.defineProperty(removed4, 'foo', { value: true }); From 3c188679c79336ac05aceb42c35100a5307da55c Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Mon, 9 May 2022 06:55:10 +0200 Subject: [PATCH 2/8] Detect side effects for getters on functions --- src/ast/nodes/shared/FunctionNode.ts | 102 ++++++++++++------ src/ast/nodes/shared/ObjectEntity.ts | 2 + .../function-getter-side-effects/_config.js | 3 + .../function-getter-side-effects/main.js | 17 +++ 4 files changed, 91 insertions(+), 33 deletions(-) create mode 100644 test/function/samples/function-getter-side-effects/_config.js create mode 100644 test/function/samples/function-getter-side-effects/main.js diff --git a/src/ast/nodes/shared/FunctionNode.ts b/src/ast/nodes/shared/FunctionNode.ts index 1ad65e74b46..6b6a861715e 100644 --- a/src/ast/nodes/shared/FunctionNode.ts +++ b/src/ast/nodes/shared/FunctionNode.ts @@ -1,5 +1,6 @@ import type { NormalizedTreeshakingOptions } from '../../../rollup/types'; import { type CallOptions, NO_ARGS } from '../../CallOptions'; +import { DeoptimizableEntity } from '../../DeoptimizableEntity'; import { BROKEN_FLOW_NONE, type HasEffectsContext, @@ -7,12 +8,12 @@ import { } from '../../ExecutionContext'; import { EVENT_CALLED, type NodeEvent } from '../../NodeEvents'; import FunctionScope from '../../scopes/FunctionScope'; -import { type ObjectPath, UNKNOWN_PATH, UnknownKey } from '../../utils/PathTracker'; +import { type ObjectPath, PathTracker, UNKNOWN_PATH, UnknownKey } from '../../utils/PathTracker'; import BlockStatement from '../BlockStatement'; import Identifier, { type IdentifierWithVariable } from '../Identifier'; import RestElement from '../RestElement'; import type SpreadElement from '../SpreadElement'; -import { type ExpressionEntity, UNKNOWN_EXPRESSION } from './Expression'; +import { type ExpressionEntity, LiteralValueOrUnknown, UNKNOWN_EXPRESSION } from './Expression'; import { type ExpressionNode, type GenericEsTreeNode, @@ -23,6 +24,7 @@ import { ObjectEntity } from './ObjectEntity'; import { OBJECT_PROTOTYPE } from './ObjectPrototype'; import type { PatternNode } from './Pattern'; +// TODO Lukas create shared base with arrow functions export default class FunctionNode extends NodeBase { declare async: boolean; declare body: BlockStatement; @@ -31,44 +33,60 @@ export default class FunctionNode extends NodeBase { declare preventChildBlockScope: true; declare scope: FunctionScope; private deoptimizedReturn = false; - private isPrototypeDeoptimized = false; + private objectEntity: ObjectEntity | null = null; createScope(parentScope: FunctionScope): void { this.scope = new FunctionScope(parentScope, this.context); } deoptimizePath(path: ObjectPath): void { - if (path.length === 1) { - if (path[0] === 'prototype') { - this.isPrototypeDeoptimized = true; - } else if (path[0] === UnknownKey) { - this.isPrototypeDeoptimized = true; - - // A reassignment of UNKNOWN_PATH is considered equivalent to having lost track - // which means the return expression needs to be reassigned as well - this.scope.getReturnExpression().deoptimizePath(UNKNOWN_PATH); - } + this.getObjectEntity().deoptimizePath(path); + if (path.length === 1 && path[0] === UnknownKey) { + // A reassignment of UNKNOWN_PATH is considered equivalent to having lost track + // which means the return expression needs to be reassigned as well + this.scope.getReturnExpression().deoptimizePath(UNKNOWN_PATH); } } - // TODO for completeness, we should also track other events here deoptimizeThisOnEventAtPath( event: NodeEvent, path: ObjectPath, - thisParameter: ExpressionEntity + thisParameter: ExpressionEntity, + recursionTracker: PathTracker ): void { - if (event === EVENT_CALLED) { - if (path.length > 0) { - thisParameter.deoptimizePath(UNKNOWN_PATH); - } else { - this.scope.thisVariable.addEntityToBeDeoptimized(thisParameter); - } + if (path.length > 0) { + this.getObjectEntity().deoptimizeThisOnEventAtPath( + event, + path, + thisParameter, + recursionTracker + ); + } else if (event === EVENT_CALLED) { + this.scope.thisVariable.addEntityToBeDeoptimized(thisParameter); } } - getReturnExpressionWhenCalledAtPath(path: ObjectPath): ExpressionEntity { - if (path.length !== 0) { - return UNKNOWN_EXPRESSION; + getLiteralValueAtPath( + path: ObjectPath, + recursionTracker: PathTracker, + origin: DeoptimizableEntity + ): LiteralValueOrUnknown { + return this.getObjectEntity().getLiteralValueAtPath(path, recursionTracker, origin); + } + + getReturnExpressionWhenCalledAtPath( + path: ObjectPath, + callOptions: CallOptions, + recursionTracker: PathTracker, + origin: DeoptimizableEntity + ): ExpressionEntity { + if (path.length > 0) { + return this.getObjectEntity().getReturnExpressionWhenCalledAtPath( + path, + callOptions, + recursionTracker, + origin + ); } if (this.async) { if (!this.deoptimizedReturn) { @@ -85,16 +103,16 @@ export default class FunctionNode extends NodeBase { return this.id !== null && this.id.hasEffects(); } - hasEffectsWhenAccessedAtPath(path: ObjectPath): boolean { - if (path.length <= 1) return false; - return path.length > 2 || path[0] !== 'prototype' || this.isPrototypeDeoptimized; + hasEffectsWhenAccessedAtPath(path: ObjectPath, context: HasEffectsContext): boolean { + return this.getObjectEntity().hasEffectsWhenAccessedAtPath(path, context); } - hasEffectsWhenAssignedAtPath(path: ObjectPath): boolean { - if (path.length <= 1) { - return false; - } - return path.length > 2 || path[0] !== 'prototype' || this.isPrototypeDeoptimized; + hasEffectsWhenAssignedAtPath( + path: ObjectPath, + context: HasEffectsContext, + ignoreAccessors: boolean + ): boolean { + return this.getObjectEntity().hasEffectsWhenAssignedAtPath(path, context, ignoreAccessors); } hasEffectsWhenCalledAtPath( @@ -102,7 +120,9 @@ export default class FunctionNode extends NodeBase { callOptions: CallOptions, context: HasEffectsContext ): boolean { - if (path.length > 0) return true; + if (path.length > 0) { + return this.getObjectEntity().hasEffectsWhenCalledAtPath(path, callOptions, context); + } if (this.async) { const { propertyReadSideEffects } = this.context.options .treeshake as NormalizedTreeshakingOptions; @@ -185,6 +205,22 @@ export default class FunctionNode extends NodeBase { this.body = new BlockStatement(esTreeNode.body, this, this.scope.hoistedBodyVarScope); super.parseNode(esTreeNode); } + + private getObjectEntity(): ObjectEntity { + if (this.objectEntity !== null) { + return this.objectEntity; + } + return (this.objectEntity = new ObjectEntity( + [ + { + key: 'prototype', + kind: 'init', + property: new ObjectEntity([], OBJECT_PROTOTYPE) + } + ], + OBJECT_PROTOTYPE + )); + } } FunctionNode.prototype.preventChildBlockScope = true; diff --git a/src/ast/nodes/shared/ObjectEntity.ts b/src/ast/nodes/shared/ObjectEntity.ts index bb382bc38a4..379c6c8e347 100644 --- a/src/ast/nodes/shared/ObjectEntity.ts +++ b/src/ast/nodes/shared/ObjectEntity.ts @@ -96,6 +96,7 @@ export class ObjectEntity extends ExpressionEntity { this.deoptimizeCachedIntegerEntities(); } + // Assumption: If only a specific path is deoptimized, no accessors are created deoptimizePath(path: ObjectPath): void { if (this.hasUnknownDeoptimizedProperty || this.immutable) return; const key = path[0]; @@ -137,6 +138,7 @@ export class ObjectEntity extends ExpressionEntity { thisParameter: ExpressionEntity, recursionTracker: PathTracker ): void { + if (path.length === 0) return; const [key, ...subPath] = path; if ( diff --git a/test/function/samples/function-getter-side-effects/_config.js b/test/function/samples/function-getter-side-effects/_config.js new file mode 100644 index 00000000000..904e3d0e679 --- /dev/null +++ b/test/function/samples/function-getter-side-effects/_config.js @@ -0,0 +1,3 @@ +module.exports = { + description: 'respects getters created on functions' +}; diff --git a/test/function/samples/function-getter-side-effects/main.js b/test/function/samples/function-getter-side-effects/main.js new file mode 100644 index 00000000000..4fca02d3838 --- /dev/null +++ b/test/function/samples/function-getter-side-effects/main.js @@ -0,0 +1,17 @@ +let funDeclEffect = false; +function funDecl() {} +Object.defineProperty(funDecl, 'foo', { get() { funDeclEffect = true; }}); +funDecl.foo; +assert.ok(funDeclEffect, 'function declaration'); + +let funExpEffect = false; +const funExp = function () {}; +Object.defineProperty(funExp, 'foo', { get() { funExpEffect = true }}); +funExp.foo; +assert.ok(funExpEffect, 'function expression'); + +let arrowEffect = false; +const arrow = function () {}; +Object.defineProperty(arrow, 'foo', { get() { arrowEffect = true }}); +arrow.foo; +assert.ok(arrowEffect, 'arrow function'); From ce156ea2f43e868669d449b0b8d3ffd25bab2be7 Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Tue, 10 May 2022 07:15:55 +0200 Subject: [PATCH 3/8] Less deoptimization for non-accessor assignments --- src/ast/nodes/MemberExpression.ts | 7 +-- src/ast/nodes/shared/ObjectEntity.ts | 46 +++++++++++++------ src/ast/utils/PathTracker.ts | 9 +++- .../samples/for-loop-assignment/_config.js | 4 ++ .../samples/for-loop-assignment/_expected.js | 1 + test/form/samples/for-loop-assignment/main.js | 5 ++ .../function-iterable-prototype/_config.js | 4 ++ .../function-iterable-prototype/_expected.js | 1 + .../function-iterable-prototype/main.js | 5 ++ 9 files changed, 65 insertions(+), 17 deletions(-) create mode 100644 test/form/samples/for-loop-assignment/_config.js create mode 100644 test/form/samples/for-loop-assignment/_expected.js create mode 100644 test/form/samples/for-loop-assignment/main.js create mode 100644 test/form/samples/function-iterable-prototype/_config.js create mode 100644 test/form/samples/function-iterable-prototype/_expected.js create mode 100644 test/form/samples/function-iterable-prototype/main.js diff --git a/src/ast/nodes/MemberExpression.ts b/src/ast/nodes/MemberExpression.ts index 4666bfdc9da..dbbfd933575 100644 --- a/src/ast/nodes/MemberExpression.ts +++ b/src/ast/nodes/MemberExpression.ts @@ -14,7 +14,8 @@ import { type PathTracker, SHARED_RECURSION_TRACKER, UNKNOWN_PATH, - UnknownKey + UnknownKey, + UnknownNonAccessorKey } from '../utils/PathTracker'; import ExternalVariable from '../variables/ExternalVariable'; import type NamespaceVariable from '../variables/NamespaceVariable'; @@ -378,9 +379,9 @@ export default class MemberExpression extends NodeBase implements DeoptimizableE private getPropertyKey(): ObjectPathKey { if (this.propertyKey === null) { - this.propertyKey = UnknownKey; + this.propertyKey = UnknownNonAccessorKey; const value = this.property.getLiteralValueAtPath(EMPTY_PATH, SHARED_RECURSION_TRACKER, this); - return (this.propertyKey = value === UnknownValue ? UnknownKey : String(value)); + return (this.propertyKey = value === UnknownValue ? UnknownNonAccessorKey : String(value)); } return this.propertyKey; } diff --git a/src/ast/nodes/shared/ObjectEntity.ts b/src/ast/nodes/shared/ObjectEntity.ts index 379c6c8e347..176ae669ef5 100644 --- a/src/ast/nodes/shared/ObjectEntity.ts +++ b/src/ast/nodes/shared/ObjectEntity.ts @@ -9,7 +9,8 @@ import { UNKNOWN_INTEGER_PATH, UNKNOWN_PATH, UnknownInteger, - UnknownKey + UnknownKey, + UnknownNonAccessorKey } from '../../utils/PathTracker'; import { ExpressionEntity, @@ -35,6 +36,7 @@ export class ObjectEntity extends ExpressionEntity { private readonly expressionsToBeDeoptimizedByKey: Record = Object.create(null); private readonly gettersByKey: PropertyMap = Object.create(null); + private hasLostTrack = false; private hasUnknownDeoptimizedInteger = false; private hasUnknownDeoptimizedProperty = false; private readonly propertiesAndGettersByKey: PropertyMap = Object.create(null); @@ -64,11 +66,17 @@ export class ObjectEntity extends ExpressionEntity { } } - deoptimizeAllProperties(): void { - if (this.hasUnknownDeoptimizedProperty) { + // TODO Lukas check other usages + deoptimizeAllProperties(noAccessors?: boolean): void { + const isDeoptimized = this.hasLostTrack || this.hasUnknownDeoptimizedProperty; + if (noAccessors) { + this.hasUnknownDeoptimizedProperty = true; + } else { + this.hasLostTrack = true; + } + if (isDeoptimized) { return; } - this.hasUnknownDeoptimizedProperty = true; for (const properties of Object.values(this.propertiesAndGettersByKey).concat( Object.values(this.settersByKey) )) { @@ -82,7 +90,11 @@ export class ObjectEntity extends ExpressionEntity { } deoptimizeIntegerProperties(): void { - if (this.hasUnknownDeoptimizedProperty || this.hasUnknownDeoptimizedInteger) { + if ( + this.hasLostTrack || + this.hasUnknownDeoptimizedProperty || + this.hasUnknownDeoptimizedInteger + ) { return; } this.hasUnknownDeoptimizedInteger = true; @@ -98,14 +110,16 @@ export class ObjectEntity extends ExpressionEntity { // Assumption: If only a specific path is deoptimized, no accessors are created deoptimizePath(path: ObjectPath): void { - if (this.hasUnknownDeoptimizedProperty || this.immutable) return; + if (this.hasLostTrack || this.immutable) { + return; + } const key = path[0]; if (path.length === 1) { if (typeof key !== 'string') { if (key === UnknownInteger) { return this.deoptimizeIntegerProperties(); } - return this.deoptimizeAllProperties(); + return this.deoptimizeAllProperties(key === UnknownNonAccessorKey); } if (!this.deoptimizedPaths[key]) { this.deoptimizedPaths[key] = true; @@ -142,11 +156,11 @@ export class ObjectEntity extends ExpressionEntity { const [key, ...subPath] = path; if ( - this.hasUnknownDeoptimizedProperty || + this.hasLostTrack || // single paths that are deoptimized will not become getters or setters ((event === EVENT_CALLED || path.length > 1) && - typeof key === 'string' && - this.deoptimizedPaths[key]) + (this.hasUnknownDeoptimizedProperty || + (typeof key === 'string' && this.deoptimizedPaths[key]))) ) { thisParameter.deoptimizePath(UNKNOWN_PATH); return; @@ -275,7 +289,7 @@ export class ObjectEntity extends ExpressionEntity { return true; } - if (this.hasUnknownDeoptimizedProperty) return true; + if (this.hasLostTrack) return true; if (typeof key === 'string') { if (this.propertiesAndGettersByKey[key]) { const getters = this.gettersByKey[key]; @@ -329,8 +343,7 @@ export class ObjectEntity extends ExpressionEntity { } if (ignoreAccessors) return false; - if (this.hasUnknownDeoptimizedProperty) return true; - // We do not need to test for unknown properties as in that case, hasUnknownDeoptimizedProperty is true + if (this.hasLostTrack) return true; if (typeof key === 'string') { if (this.propertiesAndSettersByKey[key]) { const setters = this.settersByKey[key]; @@ -346,6 +359,12 @@ export class ObjectEntity extends ExpressionEntity { return true; } } + } else { + for (const setters of Object.values(this.settersByKey).concat([this.unmatchableSetters])) { + for (const setter of setters) { + if (setter.hasEffectsWhenAssignedAtPath(subPath, context)) return true; + } + } } if (this.prototypeExpression) { return this.prototypeExpression.hasEffectsWhenAssignedAtPath(path, context); @@ -445,6 +464,7 @@ export class ObjectEntity extends ExpressionEntity { private getMemberExpression(key: ObjectPathKey): ExpressionEntity | null { if ( + this.hasLostTrack || this.hasUnknownDeoptimizedProperty || typeof key !== 'string' || (this.hasUnknownDeoptimizedInteger && INTEGER_REG_EXP.test(key)) || diff --git a/src/ast/utils/PathTracker.ts b/src/ast/utils/PathTracker.ts index 1ffa0554c82..68f33736fe7 100644 --- a/src/ast/utils/PathTracker.ts +++ b/src/ast/utils/PathTracker.ts @@ -2,8 +2,13 @@ import { getOrCreate } from '../../utils/getOrCreate'; import type { Entity } from '../Entity'; export const UnknownKey = Symbol('Unknown Key'); +export const UnknownNonAccessorKey = Symbol('Unknown Non-Accessor Key'); export const UnknownInteger = Symbol('Unknown Integer'); -export type ObjectPathKey = string | typeof UnknownKey | typeof UnknownInteger; +export type ObjectPathKey = + | string + | typeof UnknownKey + | typeof UnknownNonAccessorKey + | typeof UnknownInteger; export type ObjectPath = ObjectPathKey[]; export const EMPTY_PATH: ObjectPath = []; @@ -16,6 +21,7 @@ interface EntityPaths { [EntitiesKey]: Set; [UnknownInteger]?: EntityPaths; [UnknownKey]?: EntityPaths; + [UnknownNonAccessorKey]?: EntityPaths; } export class PathTracker { @@ -62,6 +68,7 @@ interface DiscriminatedEntityPaths { [EntitiesKey]: Map>; [UnknownInteger]?: DiscriminatedEntityPaths; [UnknownKey]?: DiscriminatedEntityPaths; + [UnknownNonAccessorKey]?: DiscriminatedEntityPaths; } export class DiscriminatedPathTracker { diff --git a/test/form/samples/for-loop-assignment/_config.js b/test/form/samples/for-loop-assignment/_config.js new file mode 100644 index 00000000000..120cbe0c1b7 --- /dev/null +++ b/test/form/samples/for-loop-assignment/_config.js @@ -0,0 +1,4 @@ +module.exports = { + description: 'removes assignments with computed indexes in for loops', + expectedWarnings: ['EMPTY_BUNDLE'] +}; diff --git a/test/form/samples/for-loop-assignment/_expected.js b/test/form/samples/for-loop-assignment/_expected.js new file mode 100644 index 00000000000..8b137891791 --- /dev/null +++ b/test/form/samples/for-loop-assignment/_expected.js @@ -0,0 +1 @@ + diff --git a/test/form/samples/for-loop-assignment/main.js b/test/form/samples/for-loop-assignment/main.js new file mode 100644 index 00000000000..948cb07f092 --- /dev/null +++ b/test/form/samples/for-loop-assignment/main.js @@ -0,0 +1,5 @@ +const lut = []; + +for (let i = 0; i < 256; i++) { + lut[i] = i < 16 ? '0' : ''; +} diff --git a/test/form/samples/function-iterable-prototype/_config.js b/test/form/samples/function-iterable-prototype/_config.js new file mode 100644 index 00000000000..a4bea7d5c41 --- /dev/null +++ b/test/form/samples/function-iterable-prototype/_config.js @@ -0,0 +1,4 @@ +module.exports = { + description: 'Removes unused functions where the prototype is iterable', + expectedWarnings: ['EMPTY_BUNDLE'] +}; diff --git a/test/form/samples/function-iterable-prototype/_expected.js b/test/form/samples/function-iterable-prototype/_expected.js new file mode 100644 index 00000000000..8b137891791 --- /dev/null +++ b/test/form/samples/function-iterable-prototype/_expected.js @@ -0,0 +1 @@ + diff --git a/test/form/samples/function-iterable-prototype/main.js b/test/form/samples/function-iterable-prototype/main.js new file mode 100644 index 00000000000..fd07b376992 --- /dev/null +++ b/test/form/samples/function-iterable-prototype/main.js @@ -0,0 +1,5 @@ +function AsyncGenerator(gen) {} + +AsyncGenerator.prototype[Symbol.asyncIterator] = function () { + return this; +}; From 51000b54a3046f85339726dbe32517ea46735a25 Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Tue, 10 May 2022 14:43:34 +0200 Subject: [PATCH 4/8] Use ObjectEntity for arrow function properties --- src/ast/nodes/ArrowFunctionExpression.ts | 85 +++++++++++++++---- src/ast/nodes/shared/ObjectEntity.ts | 1 - .../samples/object-define-property/main.js | 3 + 3 files changed, 73 insertions(+), 16 deletions(-) diff --git a/src/ast/nodes/ArrowFunctionExpression.ts b/src/ast/nodes/ArrowFunctionExpression.ts index 7a6a51522cc..d4e7e75c9ac 100644 --- a/src/ast/nodes/ArrowFunctionExpression.ts +++ b/src/ast/nodes/ArrowFunctionExpression.ts @@ -1,25 +1,33 @@ import type { NormalizedTreeshakingOptions } from '../../rollup/types'; import { type CallOptions, NO_ARGS } from '../CallOptions'; +import { DeoptimizableEntity } from '../DeoptimizableEntity'; import { BROKEN_FLOW_NONE, type HasEffectsContext, type InclusionContext } from '../ExecutionContext'; +import { NodeEvent } from '../NodeEvents'; import ReturnValueScope from '../scopes/ReturnValueScope'; import type Scope from '../scopes/Scope'; -import { type ObjectPath, UNKNOWN_PATH, UnknownKey } from '../utils/PathTracker'; +import { type ObjectPath, PathTracker, UNKNOWN_PATH, UnknownKey } from '../utils/PathTracker'; import BlockStatement from './BlockStatement'; import Identifier from './Identifier'; import * as NodeType from './NodeType'; import RestElement from './RestElement'; import type SpreadElement from './SpreadElement'; -import { type ExpressionEntity, UNKNOWN_EXPRESSION } from './shared/Expression'; +import { + type ExpressionEntity, + LiteralValueOrUnknown, + UNKNOWN_EXPRESSION +} from './shared/Expression'; import { type ExpressionNode, type GenericEsTreeNode, type IncludeChildren, NodeBase } from './shared/Node'; +import { ObjectEntity } from './shared/ObjectEntity'; +import { OBJECT_PROTOTYPE } from './shared/ObjectPrototype'; import type { PatternNode } from './shared/Pattern'; export default class ArrowFunctionExpression extends NodeBase { @@ -30,25 +38,59 @@ export default class ArrowFunctionExpression extends NodeBase { declare scope: ReturnValueScope; declare type: NodeType.tArrowFunctionExpression; private deoptimizedReturn = false; + private objectEntity: ObjectEntity | null = null; createScope(parentScope: Scope): void { this.scope = new ReturnValueScope(parentScope, this.context); } deoptimizePath(path: ObjectPath): void { - // A reassignment of UNKNOWN_PATH is considered equivalent to having lost track - // which means the return expression needs to be reassigned + this.getObjectEntity().deoptimizePath(path); if (path.length === 1 && path[0] === UnknownKey) { + // A reassignment of UNKNOWN_PATH is considered equivalent to having lost track + // which means the return expression needs to be reassigned this.scope.getReturnExpression().deoptimizePath(UNKNOWN_PATH); } } - // Arrow functions do not mutate their context - deoptimizeThisOnEventAtPath(): void {} + deoptimizeThisOnEventAtPath( + event: NodeEvent, + path: ObjectPath, + thisParameter: ExpressionEntity, + recursionTracker: PathTracker + ): void { + // Arrow functions do not mutate their context, but their properties may + if (path.length > 0) { + this.getObjectEntity().deoptimizeThisOnEventAtPath( + event, + path, + thisParameter, + recursionTracker + ); + } + } - getReturnExpressionWhenCalledAtPath(path: ObjectPath): ExpressionEntity { - if (path.length !== 0) { - return UNKNOWN_EXPRESSION; + getLiteralValueAtPath( + path: ObjectPath, + recursionTracker: PathTracker, + origin: DeoptimizableEntity + ): LiteralValueOrUnknown { + return this.getObjectEntity().getLiteralValueAtPath(path, recursionTracker, origin); + } + + getReturnExpressionWhenCalledAtPath( + path: ObjectPath, + callOptions: CallOptions, + recursionTracker: PathTracker, + origin: DeoptimizableEntity + ): ExpressionEntity { + if (path.length > 0) { + return this.getObjectEntity().getReturnExpressionWhenCalledAtPath( + path, + callOptions, + recursionTracker, + origin + ); } if (this.async) { if (!this.deoptimizedReturn) { @@ -65,20 +107,26 @@ export default class ArrowFunctionExpression extends NodeBase { return false; } - hasEffectsWhenAccessedAtPath(path: ObjectPath): boolean { - return path.length > 1; + hasEffectsWhenAccessedAtPath(path: ObjectPath, context: HasEffectsContext): boolean { + return this.getObjectEntity().hasEffectsWhenAccessedAtPath(path, context); } - hasEffectsWhenAssignedAtPath(path: ObjectPath): boolean { - return path.length > 1; + hasEffectsWhenAssignedAtPath( + path: ObjectPath, + context: HasEffectsContext, + ignoreAccessors: boolean + ): boolean { + return this.getObjectEntity().hasEffectsWhenAssignedAtPath(path, context, ignoreAccessors); } hasEffectsWhenCalledAtPath( path: ObjectPath, - _callOptions: CallOptions, + callOptions: CallOptions, context: HasEffectsContext ): boolean { - if (path.length > 0) return true; + if (path.length > 0) { + return this.getObjectEntity().hasEffectsWhenCalledAtPath(path, callOptions, context); + } if (this.async) { const { propertyReadSideEffects } = this.context.options .treeshake as NormalizedTreeshakingOptions; @@ -150,6 +198,13 @@ export default class ArrowFunctionExpression extends NodeBase { } super.parseNode(esTreeNode); } + + private getObjectEntity(): ObjectEntity { + if (this.objectEntity !== null) { + return this.objectEntity; + } + return (this.objectEntity = new ObjectEntity([], OBJECT_PROTOTYPE)); + } } ArrowFunctionExpression.prototype.preventChildBlockScope = true; diff --git a/src/ast/nodes/shared/ObjectEntity.ts b/src/ast/nodes/shared/ObjectEntity.ts index 176ae669ef5..d49ce08b4d3 100644 --- a/src/ast/nodes/shared/ObjectEntity.ts +++ b/src/ast/nodes/shared/ObjectEntity.ts @@ -66,7 +66,6 @@ export class ObjectEntity extends ExpressionEntity { } } - // TODO Lukas check other usages deoptimizeAllProperties(noAccessors?: boolean): void { const isDeoptimized = this.hasLostTrack || this.hasUnknownDeoptimizedProperty; if (noAccessors) { diff --git a/test/form/samples/object-define-property/main.js b/test/form/samples/object-define-property/main.js index 0b23bfb3f22..ee9e5d9df05 100644 --- a/test/form/samples/object-define-property/main.js +++ b/test/form/samples/object-define-property/main.js @@ -18,3 +18,6 @@ Object.defineProperty(removed3, 'foo', { value: true }); function removed4() {} Object.defineProperty(removed4, 'foo', { value: true }); + +const removed5 = () => {}; +Object.defineProperty(removed5, 'foo', { value: true }); From 1a9cb9250e58deb7bc7c41eee9ddd8e604f7218f Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Tue, 10 May 2022 15:42:26 +0200 Subject: [PATCH 5/8] Share code between functions and arrow functions --- src/ast/Entity.ts | 1 + src/ast/nodes/ArrowFunctionExpression.ts | 164 ++------------------- src/ast/nodes/shared/FunctionBase.ts | 179 +++++++++++++++++++++++ src/ast/nodes/shared/FunctionNode.ts | 150 ++----------------- 4 files changed, 205 insertions(+), 289 deletions(-) create mode 100644 src/ast/nodes/shared/FunctionBase.ts diff --git a/src/ast/Entity.ts b/src/ast/Entity.ts index 32bd4f51ea8..c93a1002ce7 100644 --- a/src/ast/Entity.ts +++ b/src/ast/Entity.ts @@ -16,6 +16,7 @@ export interface WritableEntity extends Entity { hasEffectsWhenAssignedAtPath( path: ObjectPath, context: HasEffectsContext, + // TODO Lukas use the new key value instead? ignoreAccessors?: boolean ): boolean; } diff --git a/src/ast/nodes/ArrowFunctionExpression.ts b/src/ast/nodes/ArrowFunctionExpression.ts index d4e7e75c9ac..9f90349b429 100644 --- a/src/ast/nodes/ArrowFunctionExpression.ts +++ b/src/ast/nodes/ArrowFunctionExpression.ts @@ -1,152 +1,40 @@ -import type { NormalizedTreeshakingOptions } from '../../rollup/types'; -import { type CallOptions, NO_ARGS } from '../CallOptions'; -import { DeoptimizableEntity } from '../DeoptimizableEntity'; -import { - BROKEN_FLOW_NONE, - type HasEffectsContext, - type InclusionContext -} from '../ExecutionContext'; -import { NodeEvent } from '../NodeEvents'; +import { type CallOptions } from '../CallOptions'; +import { type HasEffectsContext, InclusionContext } from '../ExecutionContext'; import ReturnValueScope from '../scopes/ReturnValueScope'; import type Scope from '../scopes/Scope'; -import { type ObjectPath, PathTracker, UNKNOWN_PATH, UnknownKey } from '../utils/PathTracker'; +import { type ObjectPath } from '../utils/PathTracker'; import BlockStatement from './BlockStatement'; import Identifier from './Identifier'; import * as NodeType from './NodeType'; -import RestElement from './RestElement'; -import type SpreadElement from './SpreadElement'; -import { - type ExpressionEntity, - LiteralValueOrUnknown, - UNKNOWN_EXPRESSION -} from './shared/Expression'; -import { - type ExpressionNode, - type GenericEsTreeNode, - type IncludeChildren, - NodeBase -} from './shared/Node'; +import FunctionBase from './shared/FunctionBase'; +import { type ExpressionNode, IncludeChildren } from './shared/Node'; import { ObjectEntity } from './shared/ObjectEntity'; import { OBJECT_PROTOTYPE } from './shared/ObjectPrototype'; import type { PatternNode } from './shared/Pattern'; -export default class ArrowFunctionExpression extends NodeBase { +export default class ArrowFunctionExpression extends FunctionBase { declare async: boolean; declare body: BlockStatement | ExpressionNode; declare params: readonly PatternNode[]; declare preventChildBlockScope: true; declare scope: ReturnValueScope; declare type: NodeType.tArrowFunctionExpression; - private deoptimizedReturn = false; - private objectEntity: ObjectEntity | null = null; + protected objectEntity: ObjectEntity | null = null; createScope(parentScope: Scope): void { this.scope = new ReturnValueScope(parentScope, this.context); } - deoptimizePath(path: ObjectPath): void { - this.getObjectEntity().deoptimizePath(path); - if (path.length === 1 && path[0] === UnknownKey) { - // A reassignment of UNKNOWN_PATH is considered equivalent to having lost track - // which means the return expression needs to be reassigned - this.scope.getReturnExpression().deoptimizePath(UNKNOWN_PATH); - } - } - - deoptimizeThisOnEventAtPath( - event: NodeEvent, - path: ObjectPath, - thisParameter: ExpressionEntity, - recursionTracker: PathTracker - ): void { - // Arrow functions do not mutate their context, but their properties may - if (path.length > 0) { - this.getObjectEntity().deoptimizeThisOnEventAtPath( - event, - path, - thisParameter, - recursionTracker - ); - } - } - - getLiteralValueAtPath( - path: ObjectPath, - recursionTracker: PathTracker, - origin: DeoptimizableEntity - ): LiteralValueOrUnknown { - return this.getObjectEntity().getLiteralValueAtPath(path, recursionTracker, origin); - } - - getReturnExpressionWhenCalledAtPath( - path: ObjectPath, - callOptions: CallOptions, - recursionTracker: PathTracker, - origin: DeoptimizableEntity - ): ExpressionEntity { - if (path.length > 0) { - return this.getObjectEntity().getReturnExpressionWhenCalledAtPath( - path, - callOptions, - recursionTracker, - origin - ); - } - if (this.async) { - if (!this.deoptimizedReturn) { - this.deoptimizedReturn = true; - this.scope.getReturnExpression().deoptimizePath(UNKNOWN_PATH); - this.context.requestTreeshakingPass(); - } - return UNKNOWN_EXPRESSION; - } - return this.scope.getReturnExpression(); - } - hasEffects(): boolean { return false; } - hasEffectsWhenAccessedAtPath(path: ObjectPath, context: HasEffectsContext): boolean { - return this.getObjectEntity().hasEffectsWhenAccessedAtPath(path, context); - } - - hasEffectsWhenAssignedAtPath( - path: ObjectPath, - context: HasEffectsContext, - ignoreAccessors: boolean - ): boolean { - return this.getObjectEntity().hasEffectsWhenAssignedAtPath(path, context, ignoreAccessors); - } - hasEffectsWhenCalledAtPath( path: ObjectPath, callOptions: CallOptions, context: HasEffectsContext ): boolean { - if (path.length > 0) { - return this.getObjectEntity().hasEffectsWhenCalledAtPath(path, callOptions, context); - } - if (this.async) { - const { propertyReadSideEffects } = this.context.options - .treeshake as NormalizedTreeshakingOptions; - const returnExpression = this.scope.getReturnExpression(); - if ( - returnExpression.hasEffectsWhenCalledAtPath( - ['then'], - { args: NO_ARGS, thisParam: null, withNew: false }, - context - ) || - (propertyReadSideEffects && - (propertyReadSideEffects === 'always' || - returnExpression.hasEffectsWhenAccessedAtPath(['then'], context))) - ) { - return true; - } - } - for (const param of this.params) { - if (param.hasEffects(context)) return true; - } + if (super.hasEffectsWhenCalledAtPath(path, callOptions, context)) return true; const { ignore, brokenFlow } = context; context.ignore = { breaks: false, @@ -161,50 +49,18 @@ export default class ArrowFunctionExpression extends NodeBase { } include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void { - this.included = true; + super.include(context, includeChildrenRecursively); for (const param of this.params) { if (!(param instanceof Identifier)) { param.include(context, includeChildrenRecursively); } } - const { brokenFlow } = context; - context.brokenFlow = BROKEN_FLOW_NONE; - this.body.include(context, includeChildrenRecursively); - context.brokenFlow = brokenFlow; } - includeCallArguments( - context: InclusionContext, - args: readonly (ExpressionNode | SpreadElement)[] - ): void { - this.scope.includeCallArguments(context, args); - } - - initialise(): void { - this.scope.addParameterVariables( - this.params.map(param => param.declare('parameter', UNKNOWN_EXPRESSION)), - this.params[this.params.length - 1] instanceof RestElement - ); - if (this.body instanceof BlockStatement) { - this.body.addImplicitReturnExpressionToScope(); - } else { - this.scope.addReturnExpression(this.body); - } - } - - parseNode(esTreeNode: GenericEsTreeNode): void { - if (esTreeNode.body.type === NodeType.BlockStatement) { - this.body = new BlockStatement(esTreeNode.body, this, this.scope.hoistedBodyVarScope); - } - super.parseNode(esTreeNode); - } - - private getObjectEntity(): ObjectEntity { + protected getObjectEntity(): ObjectEntity { if (this.objectEntity !== null) { return this.objectEntity; } return (this.objectEntity = new ObjectEntity([], OBJECT_PROTOTYPE)); } } - -ArrowFunctionExpression.prototype.preventChildBlockScope = true; diff --git a/src/ast/nodes/shared/FunctionBase.ts b/src/ast/nodes/shared/FunctionBase.ts new file mode 100644 index 00000000000..eb900b9f6b1 --- /dev/null +++ b/src/ast/nodes/shared/FunctionBase.ts @@ -0,0 +1,179 @@ +import type { NormalizedTreeshakingOptions } from '../../../rollup/types'; +import { type CallOptions, NO_ARGS } from '../../CallOptions'; +import { DeoptimizableEntity } from '../../DeoptimizableEntity'; +import { + BROKEN_FLOW_NONE, + type HasEffectsContext, + type InclusionContext +} from '../../ExecutionContext'; +import { NodeEvent } from '../../NodeEvents'; +import ReturnValueScope from '../../scopes/ReturnValueScope'; +import { type ObjectPath, PathTracker, UNKNOWN_PATH, UnknownKey } from '../../utils/PathTracker'; +import BlockStatement from '../BlockStatement'; +import * as NodeType from '../NodeType'; +import RestElement from '../RestElement'; +import type SpreadElement from '../SpreadElement'; +import { type ExpressionEntity, LiteralValueOrUnknown, UNKNOWN_EXPRESSION } from './Expression'; +import { + type ExpressionNode, + type GenericEsTreeNode, + type IncludeChildren, + NodeBase +} from './Node'; +import { ObjectEntity } from './ObjectEntity'; +import { OBJECT_PROTOTYPE } from './ObjectPrototype'; +import type { PatternNode } from './Pattern'; + +export default class FunctionBase extends NodeBase { + declare async: boolean; + declare body: BlockStatement | ExpressionNode; + declare params: readonly PatternNode[]; + declare preventChildBlockScope: true; + declare scope: ReturnValueScope; + protected objectEntity: ObjectEntity | null = null; + private deoptimizedReturn = false; + + deoptimizePath(path: ObjectPath): void { + this.getObjectEntity().deoptimizePath(path); + if (path.length === 1 && path[0] === UnknownKey) { + // A reassignment of UNKNOWN_PATH is considered equivalent to having lost track + // which means the return expression needs to be reassigned + this.scope.getReturnExpression().deoptimizePath(UNKNOWN_PATH); + } + } + + deoptimizeThisOnEventAtPath( + event: NodeEvent, + path: ObjectPath, + thisParameter: ExpressionEntity, + recursionTracker: PathTracker + ): void { + if (path.length > 0) { + this.getObjectEntity().deoptimizeThisOnEventAtPath( + event, + path, + thisParameter, + recursionTracker + ); + } + } + + getLiteralValueAtPath( + path: ObjectPath, + recursionTracker: PathTracker, + origin: DeoptimizableEntity + ): LiteralValueOrUnknown { + return this.getObjectEntity().getLiteralValueAtPath(path, recursionTracker, origin); + } + + getReturnExpressionWhenCalledAtPath( + path: ObjectPath, + callOptions: CallOptions, + recursionTracker: PathTracker, + origin: DeoptimizableEntity + ): ExpressionEntity { + if (path.length > 0) { + return this.getObjectEntity().getReturnExpressionWhenCalledAtPath( + path, + callOptions, + recursionTracker, + origin + ); + } + if (this.async) { + if (!this.deoptimizedReturn) { + this.deoptimizedReturn = true; + this.scope.getReturnExpression().deoptimizePath(UNKNOWN_PATH); + this.context.requestTreeshakingPass(); + } + return UNKNOWN_EXPRESSION; + } + return this.scope.getReturnExpression(); + } + + hasEffectsWhenAccessedAtPath(path: ObjectPath, context: HasEffectsContext): boolean { + return this.getObjectEntity().hasEffectsWhenAccessedAtPath(path, context); + } + + hasEffectsWhenAssignedAtPath( + path: ObjectPath, + context: HasEffectsContext, + ignoreAccessors: boolean + ): boolean { + return this.getObjectEntity().hasEffectsWhenAssignedAtPath(path, context, ignoreAccessors); + } + + hasEffectsWhenCalledAtPath( + path: ObjectPath, + callOptions: CallOptions, + context: HasEffectsContext + ): boolean { + if (path.length > 0) { + return this.getObjectEntity().hasEffectsWhenCalledAtPath(path, callOptions, context); + } + if (this.async) { + const { propertyReadSideEffects } = this.context.options + .treeshake as NormalizedTreeshakingOptions; + const returnExpression = this.scope.getReturnExpression(); + if ( + returnExpression.hasEffectsWhenCalledAtPath( + ['then'], + { args: NO_ARGS, thisParam: null, withNew: false }, + context + ) || + (propertyReadSideEffects && + (propertyReadSideEffects === 'always' || + returnExpression.hasEffectsWhenAccessedAtPath(['then'], context))) + ) { + return true; + } + } + for (const param of this.params) { + if (param.hasEffects(context)) return true; + } + return false; + } + + include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void { + this.included = true; + const { brokenFlow } = context; + context.brokenFlow = BROKEN_FLOW_NONE; + this.body.include(context, includeChildrenRecursively); + context.brokenFlow = brokenFlow; + } + + includeCallArguments( + context: InclusionContext, + args: readonly (ExpressionNode | SpreadElement)[] + ): void { + this.scope.includeCallArguments(context, args); + } + + initialise(): void { + this.scope.addParameterVariables( + this.params.map(param => param.declare('parameter', UNKNOWN_EXPRESSION)), + this.params[this.params.length - 1] instanceof RestElement + ); + if (this.body instanceof BlockStatement) { + this.body.addImplicitReturnExpressionToScope(); + } else { + this.scope.addReturnExpression(this.body); + } + } + + parseNode(esTreeNode: GenericEsTreeNode): void { + if (esTreeNode.body.type === NodeType.BlockStatement) { + this.body = new BlockStatement(esTreeNode.body, this, this.scope.hoistedBodyVarScope); + } + super.parseNode(esTreeNode); + } + + protected getObjectEntity(): ObjectEntity { + if (this.objectEntity !== null) { + return this.objectEntity; + } + return (this.objectEntity = new ObjectEntity([], OBJECT_PROTOTYPE)); + } +} + +FunctionBase.prototype.preventChildBlockScope = true; diff --git a/src/ast/nodes/shared/FunctionNode.ts b/src/ast/nodes/shared/FunctionNode.ts index 6b6a861715e..c11a007cbc2 100644 --- a/src/ast/nodes/shared/FunctionNode.ts +++ b/src/ast/nodes/shared/FunctionNode.ts @@ -1,148 +1,52 @@ -import type { NormalizedTreeshakingOptions } from '../../../rollup/types'; -import { type CallOptions, NO_ARGS } from '../../CallOptions'; -import { DeoptimizableEntity } from '../../DeoptimizableEntity'; -import { - BROKEN_FLOW_NONE, - type HasEffectsContext, - type InclusionContext -} from '../../ExecutionContext'; +import { type CallOptions } from '../../CallOptions'; +import { type HasEffectsContext, type InclusionContext } from '../../ExecutionContext'; import { EVENT_CALLED, type NodeEvent } from '../../NodeEvents'; import FunctionScope from '../../scopes/FunctionScope'; -import { type ObjectPath, PathTracker, UNKNOWN_PATH, UnknownKey } from '../../utils/PathTracker'; +import { type ObjectPath, PathTracker } from '../../utils/PathTracker'; import BlockStatement from '../BlockStatement'; import Identifier, { type IdentifierWithVariable } from '../Identifier'; -import RestElement from '../RestElement'; -import type SpreadElement from '../SpreadElement'; -import { type ExpressionEntity, LiteralValueOrUnknown, UNKNOWN_EXPRESSION } from './Expression'; -import { - type ExpressionNode, - type GenericEsTreeNode, - type IncludeChildren, - NodeBase -} from './Node'; +import { type ExpressionEntity, UNKNOWN_EXPRESSION } from './Expression'; +import FunctionBase from './FunctionBase'; +import { type IncludeChildren } from './Node'; import { ObjectEntity } from './ObjectEntity'; import { OBJECT_PROTOTYPE } from './ObjectPrototype'; import type { PatternNode } from './Pattern'; -// TODO Lukas create shared base with arrow functions -export default class FunctionNode extends NodeBase { +export default class FunctionNode extends FunctionBase { declare async: boolean; declare body: BlockStatement; declare id: IdentifierWithVariable | null; declare params: readonly PatternNode[]; declare preventChildBlockScope: true; declare scope: FunctionScope; - private deoptimizedReturn = false; - private objectEntity: ObjectEntity | null = null; + protected objectEntity: ObjectEntity | null = null; createScope(parentScope: FunctionScope): void { this.scope = new FunctionScope(parentScope, this.context); } - deoptimizePath(path: ObjectPath): void { - this.getObjectEntity().deoptimizePath(path); - if (path.length === 1 && path[0] === UnknownKey) { - // A reassignment of UNKNOWN_PATH is considered equivalent to having lost track - // which means the return expression needs to be reassigned as well - this.scope.getReturnExpression().deoptimizePath(UNKNOWN_PATH); - } - } - deoptimizeThisOnEventAtPath( event: NodeEvent, path: ObjectPath, thisParameter: ExpressionEntity, recursionTracker: PathTracker ): void { - if (path.length > 0) { - this.getObjectEntity().deoptimizeThisOnEventAtPath( - event, - path, - thisParameter, - recursionTracker - ); - } else if (event === EVENT_CALLED) { + super.deoptimizeThisOnEventAtPath(event, path, thisParameter, recursionTracker); + if (event === EVENT_CALLED && path.length === 0) { this.scope.thisVariable.addEntityToBeDeoptimized(thisParameter); } } - getLiteralValueAtPath( - path: ObjectPath, - recursionTracker: PathTracker, - origin: DeoptimizableEntity - ): LiteralValueOrUnknown { - return this.getObjectEntity().getLiteralValueAtPath(path, recursionTracker, origin); - } - - getReturnExpressionWhenCalledAtPath( - path: ObjectPath, - callOptions: CallOptions, - recursionTracker: PathTracker, - origin: DeoptimizableEntity - ): ExpressionEntity { - if (path.length > 0) { - return this.getObjectEntity().getReturnExpressionWhenCalledAtPath( - path, - callOptions, - recursionTracker, - origin - ); - } - if (this.async) { - if (!this.deoptimizedReturn) { - this.deoptimizedReturn = true; - this.scope.getReturnExpression().deoptimizePath(UNKNOWN_PATH); - this.context.requestTreeshakingPass(); - } - return UNKNOWN_EXPRESSION; - } - return this.scope.getReturnExpression(); - } - hasEffects(): boolean { return this.id !== null && this.id.hasEffects(); } - hasEffectsWhenAccessedAtPath(path: ObjectPath, context: HasEffectsContext): boolean { - return this.getObjectEntity().hasEffectsWhenAccessedAtPath(path, context); - } - - hasEffectsWhenAssignedAtPath( - path: ObjectPath, - context: HasEffectsContext, - ignoreAccessors: boolean - ): boolean { - return this.getObjectEntity().hasEffectsWhenAssignedAtPath(path, context, ignoreAccessors); - } - hasEffectsWhenCalledAtPath( path: ObjectPath, callOptions: CallOptions, context: HasEffectsContext ): boolean { - if (path.length > 0) { - return this.getObjectEntity().hasEffectsWhenCalledAtPath(path, callOptions, context); - } - if (this.async) { - const { propertyReadSideEffects } = this.context.options - .treeshake as NormalizedTreeshakingOptions; - const returnExpression = this.scope.getReturnExpression(); - if ( - returnExpression.hasEffectsWhenCalledAtPath( - ['then'], - { args: NO_ARGS, thisParam: null, withNew: false }, - context - ) || - (propertyReadSideEffects && - (propertyReadSideEffects === 'always' || - returnExpression.hasEffectsWhenAccessedAtPath(['then'], context))) - ) { - return true; - } - } - for (const param of this.params) { - if (param.hasEffects(context)) return true; - } + if (super.hasEffectsWhenCalledAtPath(path, callOptions, context)) return true; const thisInit = context.replacedVariableInits.get(this.scope.thisVariable); context.replacedVariableInits.set( this.scope.thisVariable, @@ -169,7 +73,7 @@ export default class FunctionNode extends NodeBase { } include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void { - this.included = true; + super.include(context, includeChildrenRecursively); if (this.id) this.id.include(); const hasArguments = this.scope.argumentsVariable.included; for (const param of this.params) { @@ -177,36 +81,14 @@ export default class FunctionNode extends NodeBase { param.include(context, includeChildrenRecursively); } } - const { brokenFlow } = context; - context.brokenFlow = BROKEN_FLOW_NONE; - this.body.include(context, includeChildrenRecursively); - context.brokenFlow = brokenFlow; - } - - includeCallArguments( - context: InclusionContext, - args: readonly (ExpressionNode | SpreadElement)[] - ): void { - this.scope.includeCallArguments(context, args); } initialise(): void { - if (this.id !== null) { - this.id.declare('function', this); - } - this.scope.addParameterVariables( - this.params.map(param => param.declare('parameter', UNKNOWN_EXPRESSION)), - this.params[this.params.length - 1] instanceof RestElement - ); - this.body.addImplicitReturnExpressionToScope(); - } - - parseNode(esTreeNode: GenericEsTreeNode): void { - this.body = new BlockStatement(esTreeNode.body, this, this.scope.hoistedBodyVarScope); - super.parseNode(esTreeNode); + super.initialise(); + this.id?.declare('function', this); } - private getObjectEntity(): ObjectEntity { + protected getObjectEntity(): ObjectEntity { if (this.objectEntity !== null) { return this.objectEntity; } @@ -222,5 +104,3 @@ export default class FunctionNode extends NodeBase { )); } } - -FunctionNode.prototype.preventChildBlockScope = true; From 47a4de272fc54a4812402cfdee6a8e5bb019a629 Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Tue, 10 May 2022 15:52:04 +0200 Subject: [PATCH 6/8] Use new path key for defineProperty side effects --- src/ast/Entity.ts | 7 +------ src/ast/nodes/ArrayExpression.ts | 8 ++------ src/ast/nodes/Identifier.ts | 8 ++------ src/ast/nodes/ObjectExpression.ts | 8 ++------ src/ast/nodes/shared/ClassNode.ts | 8 ++------ src/ast/nodes/shared/Expression.ts | 6 +----- src/ast/nodes/shared/FunctionBase.ts | 8 ++------ src/ast/nodes/shared/ObjectEntity.ts | 16 ++++------------ src/ast/utils/PathTracker.ts | 6 ++++++ src/ast/variables/GlobalVariable.ts | 4 ++-- src/ast/variables/LocalVariable.ts | 8 ++------ src/ast/variables/ThisVariable.ts | 8 ++------ 12 files changed, 28 insertions(+), 67 deletions(-) diff --git a/src/ast/Entity.ts b/src/ast/Entity.ts index c93a1002ce7..7ab643b7008 100644 --- a/src/ast/Entity.ts +++ b/src/ast/Entity.ts @@ -13,10 +13,5 @@ export interface WritableEntity extends Entity { */ deoptimizePath(path: ObjectPath): void; - hasEffectsWhenAssignedAtPath( - path: ObjectPath, - context: HasEffectsContext, - // TODO Lukas use the new key value instead? - ignoreAccessors?: boolean - ): boolean; + hasEffectsWhenAssignedAtPath(path: ObjectPath, context: HasEffectsContext): boolean; } diff --git a/src/ast/nodes/ArrayExpression.ts b/src/ast/nodes/ArrayExpression.ts index f79a82df6ad..406687d99fb 100644 --- a/src/ast/nodes/ArrayExpression.ts +++ b/src/ast/nodes/ArrayExpression.ts @@ -60,12 +60,8 @@ export default class ArrayExpression extends NodeBase { return this.getObjectEntity().hasEffectsWhenAccessedAtPath(path, context); } - hasEffectsWhenAssignedAtPath( - path: ObjectPath, - context: HasEffectsContext, - ignoreAccessors: boolean - ): boolean { - return this.getObjectEntity().hasEffectsWhenAssignedAtPath(path, context, ignoreAccessors); + hasEffectsWhenAssignedAtPath(path: ObjectPath, context: HasEffectsContext): boolean { + return this.getObjectEntity().hasEffectsWhenAssignedAtPath(path, context); } hasEffectsWhenCalledAtPath( diff --git a/src/ast/nodes/Identifier.ts b/src/ast/nodes/Identifier.ts index b786581b493..89965f79287 100644 --- a/src/ast/nodes/Identifier.ts +++ b/src/ast/nodes/Identifier.ts @@ -144,17 +144,13 @@ export default class Identifier extends NodeBase implements PatternNode { ); } - hasEffectsWhenAssignedAtPath( - path: ObjectPath, - context: HasEffectsContext, - ignoreAccessors: boolean - ): boolean { + hasEffectsWhenAssignedAtPath(path: ObjectPath, context: HasEffectsContext): boolean { return ( !this.variable || (path.length > 0 ? this.getVariableRespectingTDZ() : this.variable - ).hasEffectsWhenAssignedAtPath(path, context, ignoreAccessors) + ).hasEffectsWhenAssignedAtPath(path, context) ); } diff --git a/src/ast/nodes/ObjectExpression.ts b/src/ast/nodes/ObjectExpression.ts index 9b3399adbd3..61ff336a6c1 100644 --- a/src/ast/nodes/ObjectExpression.ts +++ b/src/ast/nodes/ObjectExpression.ts @@ -79,12 +79,8 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE return this.getObjectEntity().hasEffectsWhenAccessedAtPath(path, context); } - hasEffectsWhenAssignedAtPath( - path: ObjectPath, - context: HasEffectsContext, - ignoreAccessors: boolean - ): boolean { - return this.getObjectEntity().hasEffectsWhenAssignedAtPath(path, context, ignoreAccessors); + hasEffectsWhenAssignedAtPath(path: ObjectPath, context: HasEffectsContext): boolean { + return this.getObjectEntity().hasEffectsWhenAssignedAtPath(path, context); } hasEffectsWhenCalledAtPath( diff --git a/src/ast/nodes/shared/ClassNode.ts b/src/ast/nodes/shared/ClassNode.ts index 3bf932041b3..f8823593b89 100644 --- a/src/ast/nodes/shared/ClassNode.ts +++ b/src/ast/nodes/shared/ClassNode.ts @@ -86,12 +86,8 @@ export default class ClassNode extends NodeBase implements DeoptimizableEntity { return this.getObjectEntity().hasEffectsWhenAccessedAtPath(path, context); } - hasEffectsWhenAssignedAtPath( - path: ObjectPath, - context: HasEffectsContext, - ignoreAccessors: boolean - ): boolean { - return this.getObjectEntity().hasEffectsWhenAssignedAtPath(path, context, ignoreAccessors); + hasEffectsWhenAssignedAtPath(path: ObjectPath, context: HasEffectsContext): boolean { + return this.getObjectEntity().hasEffectsWhenAssignedAtPath(path, context); } hasEffectsWhenCalledAtPath( diff --git a/src/ast/nodes/shared/Expression.ts b/src/ast/nodes/shared/Expression.ts index 01a46c7745f..60764af727c 100644 --- a/src/ast/nodes/shared/Expression.ts +++ b/src/ast/nodes/shared/Expression.ts @@ -52,11 +52,7 @@ export class ExpressionEntity implements WritableEntity { return true; } - hasEffectsWhenAssignedAtPath( - _path: ObjectPath, - _context: HasEffectsContext, - _ignoreAccessors?: boolean - ): boolean { + hasEffectsWhenAssignedAtPath(_path: ObjectPath, _context: HasEffectsContext): boolean { return true; } diff --git a/src/ast/nodes/shared/FunctionBase.ts b/src/ast/nodes/shared/FunctionBase.ts index eb900b9f6b1..65272198cfd 100644 --- a/src/ast/nodes/shared/FunctionBase.ts +++ b/src/ast/nodes/shared/FunctionBase.ts @@ -95,12 +95,8 @@ export default class FunctionBase extends NodeBase { return this.getObjectEntity().hasEffectsWhenAccessedAtPath(path, context); } - hasEffectsWhenAssignedAtPath( - path: ObjectPath, - context: HasEffectsContext, - ignoreAccessors: boolean - ): boolean { - return this.getObjectEntity().hasEffectsWhenAssignedAtPath(path, context, ignoreAccessors); + hasEffectsWhenAssignedAtPath(path: ObjectPath, context: HasEffectsContext): boolean { + return this.getObjectEntity().hasEffectsWhenAssignedAtPath(path, context); } hasEffectsWhenCalledAtPath( diff --git a/src/ast/nodes/shared/ObjectEntity.ts b/src/ast/nodes/shared/ObjectEntity.ts index d49ce08b4d3..a708f2b9db5 100644 --- a/src/ast/nodes/shared/ObjectEntity.ts +++ b/src/ast/nodes/shared/ObjectEntity.ts @@ -317,11 +317,7 @@ export class ObjectEntity extends ExpressionEntity { return false; } - hasEffectsWhenAssignedAtPath( - path: ObjectPath, - context: HasEffectsContext, - ignoreAccessors: boolean - ): boolean { + hasEffectsWhenAssignedAtPath(path: ObjectPath, context: HasEffectsContext): boolean { const [key, ...subPath] = path; if (path.length > 1) { if (typeof key !== 'string') { @@ -329,19 +325,15 @@ export class ObjectEntity extends ExpressionEntity { } const expressionAtPath = this.getMemberExpression(key); if (expressionAtPath) { - return expressionAtPath.hasEffectsWhenAssignedAtPath(subPath, context, ignoreAccessors); + return expressionAtPath.hasEffectsWhenAssignedAtPath(subPath, context); } if (this.prototypeExpression) { - return this.prototypeExpression.hasEffectsWhenAssignedAtPath( - path, - context, - ignoreAccessors - ); + return this.prototypeExpression.hasEffectsWhenAssignedAtPath(path, context); } return true; } - if (ignoreAccessors) return false; + if (key === UnknownNonAccessorKey) return false; if (this.hasLostTrack) return true; if (typeof key === 'string') { if (this.propertiesAndSettersByKey[key]) { diff --git a/src/ast/utils/PathTracker.ts b/src/ast/utils/PathTracker.ts index 68f33736fe7..5b0679a6187 100644 --- a/src/ast/utils/PathTracker.ts +++ b/src/ast/utils/PathTracker.ts @@ -13,6 +13,12 @@ export type ObjectPathKey = export type ObjectPath = ObjectPathKey[]; export const EMPTY_PATH: ObjectPath = []; export const UNKNOWN_PATH: ObjectPath = [UnknownKey]; +// For deoptimizations, this means we are modifying an unknown property but did +// not lose track of the object or are creating a setter/getter; +// For assignment effects it means we do not check for setter/getter effects +// but only if something is mutated that is included, which is relevant for +// Object.defineProperty +export const UNKNOWN_NON_ACCESSOR_PATH: ObjectPath = [UnknownNonAccessorKey]; export const UNKNOWN_INTEGER_PATH: ObjectPath = [UnknownInteger]; const EntitiesKey = Symbol('Entities'); diff --git a/src/ast/variables/GlobalVariable.ts b/src/ast/variables/GlobalVariable.ts index 160d0825fd9..813011e2106 100644 --- a/src/ast/variables/GlobalVariable.ts +++ b/src/ast/variables/GlobalVariable.ts @@ -1,8 +1,8 @@ import { CallOptions } from '../CallOptions'; import { HasEffectsContext } from '../ExecutionContext'; import { getGlobalAtPath } from '../nodes/shared/knownGlobals'; -import { UNKNOWN_PATH } from '../utils/PathTracker'; import type { ObjectPath } from '../utils/PathTracker'; +import { UNKNOWN_NON_ACCESSOR_PATH } from '../utils/PathTracker'; import Variable from './Variable'; export default class GlobalVariable extends Variable { @@ -26,7 +26,7 @@ export default class GlobalVariable extends Variable { !globalAtPath.function || (globalAtPath.mutatesArg1 && (!callOptions.args.length || - callOptions.args[0].hasEffectsWhenAssignedAtPath(UNKNOWN_PATH, context, true))) + callOptions.args[0].hasEffectsWhenAssignedAtPath(UNKNOWN_NON_ACCESSOR_PATH, context))) ); } } diff --git a/src/ast/variables/LocalVariable.ts b/src/ast/variables/LocalVariable.ts index 2ebbd5dd91b..a90613a2825 100644 --- a/src/ast/variables/LocalVariable.ts +++ b/src/ast/variables/LocalVariable.ts @@ -150,17 +150,13 @@ export default class LocalVariable extends Variable { this.init.hasEffectsWhenAccessedAtPath(path, context))!; } - hasEffectsWhenAssignedAtPath( - path: ObjectPath, - context: HasEffectsContext, - ignoreAccessors: boolean - ): boolean { + hasEffectsWhenAssignedAtPath(path: ObjectPath, context: HasEffectsContext): boolean { if (this.included) return true; if (path.length === 0) return false; if (this.isReassigned) return true; return (this.init && !context.assigned.trackEntityAtPathAndGetIfTracked(path, this) && - this.init.hasEffectsWhenAssignedAtPath(path, context, ignoreAccessors))!; + this.init.hasEffectsWhenAssignedAtPath(path, context))!; } hasEffectsWhenCalledAtPath( diff --git a/src/ast/variables/ThisVariable.ts b/src/ast/variables/ThisVariable.ts index c783be89803..7c4cba21045 100644 --- a/src/ast/variables/ThisVariable.ts +++ b/src/ast/variables/ThisVariable.ts @@ -73,14 +73,10 @@ export default class ThisVariable extends LocalVariable { ); } - hasEffectsWhenAssignedAtPath( - path: ObjectPath, - context: HasEffectsContext, - ignoreAccessors: boolean - ): boolean { + hasEffectsWhenAssignedAtPath(path: ObjectPath, context: HasEffectsContext): boolean { return ( this.getInit(context).hasEffectsWhenAssignedAtPath(path, context) || - super.hasEffectsWhenAssignedAtPath(path, context, ignoreAccessors) + super.hasEffectsWhenAssignedAtPath(path, context) ); } From 4bba49d9c7ec25c5be5c74ac9b3bc2886361c970 Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Tue, 10 May 2022 17:55:47 +0200 Subject: [PATCH 7/8] Enable custom call-effect detection per global --- src/ast/nodes/shared/knownGlobals.ts | 31 +++++++++++++++++++++------- src/ast/variables/GlobalVariable.ts | 12 ++++------- 2 files changed, 27 insertions(+), 16 deletions(-) diff --git a/src/ast/nodes/shared/knownGlobals.ts b/src/ast/nodes/shared/knownGlobals.ts index 3b8c05979f0..b42316d6bc3 100644 --- a/src/ast/nodes/shared/knownGlobals.ts +++ b/src/ast/nodes/shared/knownGlobals.ts @@ -1,15 +1,14 @@ /* eslint sort-keys: "off" */ +import { CallOptions } from '../../CallOptions'; +import { HasEffectsContext } from '../../ExecutionContext'; +import { UNKNOWN_NON_ACCESSOR_PATH } from '../../utils/PathTracker'; import type { ObjectPath } from '../../utils/PathTracker'; const ValueProperties = Symbol('Value Properties'); interface ValueDescription { - // Denotes a proper function except for the side effects listed explicitly - function: boolean; - // This assumes that mutation itself cannot have side effects and that this - // does not trigger setters or getters - mutatesArg1: boolean; + hasEffectsWhenCalled(callOptions: CallOptions, context: HasEffectsContext): boolean; } interface GlobalDescription { @@ -18,8 +17,17 @@ interface GlobalDescription { __proto__: null; } -const PURE: ValueDescription = { function: true, mutatesArg1: false }; -const IMPURE: ValueDescription = { function: false, mutatesArg1: false }; +const PURE: ValueDescription = { + hasEffectsWhenCalled() { + return false; + } +}; + +const IMPURE: ValueDescription = { + hasEffectsWhenCalled() { + return true; + } +}; // We use shortened variables to reduce file size here /* OBJECT */ @@ -37,7 +45,14 @@ const PF: GlobalDescription = { /* FUNCTION THAT MUTATES FIRST ARG WITHOUT TRIGGERING ACCESSORS */ const MUTATES_ARG_WITHOUT_ACCESSOR: GlobalDescription = { __proto__: null, - [ValueProperties]: { function: true, mutatesArg1: true } + [ValueProperties]: { + hasEffectsWhenCalled(callOptions, context) { + return ( + !callOptions.args.length || + callOptions.args[0].hasEffectsWhenAssignedAtPath(UNKNOWN_NON_ACCESSOR_PATH, context) + ); + } + } }; /* CONSTRUCTOR */ diff --git a/src/ast/variables/GlobalVariable.ts b/src/ast/variables/GlobalVariable.ts index 813011e2106..7d6928c5f9c 100644 --- a/src/ast/variables/GlobalVariable.ts +++ b/src/ast/variables/GlobalVariable.ts @@ -2,14 +2,16 @@ import { CallOptions } from '../CallOptions'; import { HasEffectsContext } from '../ExecutionContext'; import { getGlobalAtPath } from '../nodes/shared/knownGlobals'; import type { ObjectPath } from '../utils/PathTracker'; -import { UNKNOWN_NON_ACCESSOR_PATH } from '../utils/PathTracker'; import Variable from './Variable'; export default class GlobalVariable extends Variable { + // Ensure we use live-bindings for globals as we do not know if they have + // been reassigned isReassigned = true; hasEffectsWhenAccessedAtPath(path: ObjectPath): boolean { if (path.length === 0) { + // Technically, "undefined" is a global variable of sorts return this.name !== 'undefined' && getGlobalAtPath([this.name]) === null; } return getGlobalAtPath([this.name, ...path].slice(0, -1)) === null; @@ -21,12 +23,6 @@ export default class GlobalVariable extends Variable { context: HasEffectsContext ): boolean { const globalAtPath = getGlobalAtPath([this.name, ...path]); - return ( - globalAtPath === null || - !globalAtPath.function || - (globalAtPath.mutatesArg1 && - (!callOptions.args.length || - callOptions.args[0].hasEffectsWhenAssignedAtPath(UNKNOWN_NON_ACCESSOR_PATH, context))) - ); + return globalAtPath === null || globalAtPath.hasEffectsWhenCalled(callOptions, context); } } From 9ecae34884c9fea27c0798bdc6f0a8f74d8393de Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Wed, 11 May 2022 09:22:38 +0200 Subject: [PATCH 8/8] Improve coverage --- src/ast/nodes/MemberExpression.ts | 10 +++++++--- src/ast/nodes/shared/FunctionBase.ts | 10 ++-------- src/ast/nodes/shared/ObjectEntity.ts | 1 - test/form/samples/known-globals/_expected.js | 3 ++- test/form/samples/known-globals/main.js | 5 ++++- .../unknown-setter-no-side-effect2/_config.js | 4 ++++ .../unknown-setter-no-side-effect2/_expected.js | 9 +++++++++ .../unknown-setter-no-side-effect2/main.js | 15 +++++++++++++++ 8 files changed, 43 insertions(+), 14 deletions(-) create mode 100644 test/form/samples/object-expression/unknown-setter-no-side-effect2/_config.js create mode 100644 test/form/samples/object-expression/unknown-setter-no-side-effect2/_expected.js create mode 100644 test/form/samples/object-expression/unknown-setter-no-side-effect2/main.js diff --git a/src/ast/nodes/MemberExpression.ts b/src/ast/nodes/MemberExpression.ts index dbbfd933575..2d91fa649d5 100644 --- a/src/ast/nodes/MemberExpression.ts +++ b/src/ast/nodes/MemberExpression.ts @@ -129,7 +129,11 @@ export default class MemberExpression extends NodeBase implements DeoptimizableE this.variable.deoptimizePath(path); } else if (!this.replacement) { if (path.length < MAX_PATH_DEPTH) { - this.object.deoptimizePath([this.getPropertyKey(), ...path]); + const propertyKey = this.getPropertyKey(); + this.object.deoptimizePath([ + propertyKey === UnknownKey ? UnknownNonAccessorKey : propertyKey, + ...path + ]); } } } @@ -379,9 +383,9 @@ export default class MemberExpression extends NodeBase implements DeoptimizableE private getPropertyKey(): ObjectPathKey { if (this.propertyKey === null) { - this.propertyKey = UnknownNonAccessorKey; + this.propertyKey = UnknownKey; const value = this.property.getLiteralValueAtPath(EMPTY_PATH, SHARED_RECURSION_TRACKER, this); - return (this.propertyKey = value === UnknownValue ? UnknownNonAccessorKey : String(value)); + return (this.propertyKey = value === UnknownValue ? UnknownKey : String(value)); } return this.propertyKey; } diff --git a/src/ast/nodes/shared/FunctionBase.ts b/src/ast/nodes/shared/FunctionBase.ts index 65272198cfd..38b86e8c92a 100644 --- a/src/ast/nodes/shared/FunctionBase.ts +++ b/src/ast/nodes/shared/FunctionBase.ts @@ -21,10 +21,9 @@ import { NodeBase } from './Node'; import { ObjectEntity } from './ObjectEntity'; -import { OBJECT_PROTOTYPE } from './ObjectPrototype'; import type { PatternNode } from './Pattern'; -export default class FunctionBase extends NodeBase { +export default abstract class FunctionBase extends NodeBase { declare async: boolean; declare body: BlockStatement | ExpressionNode; declare params: readonly PatternNode[]; @@ -164,12 +163,7 @@ export default class FunctionBase extends NodeBase { super.parseNode(esTreeNode); } - protected getObjectEntity(): ObjectEntity { - if (this.objectEntity !== null) { - return this.objectEntity; - } - return (this.objectEntity = new ObjectEntity([], OBJECT_PROTOTYPE)); - } + protected abstract getObjectEntity(): ObjectEntity; } FunctionBase.prototype.preventChildBlockScope = true; diff --git a/src/ast/nodes/shared/ObjectEntity.ts b/src/ast/nodes/shared/ObjectEntity.ts index a708f2b9db5..1aff8dcd598 100644 --- a/src/ast/nodes/shared/ObjectEntity.ts +++ b/src/ast/nodes/shared/ObjectEntity.ts @@ -151,7 +151,6 @@ export class ObjectEntity extends ExpressionEntity { thisParameter: ExpressionEntity, recursionTracker: PathTracker ): void { - if (path.length === 0) return; const [key, ...subPath] = path; if ( diff --git a/test/form/samples/known-globals/_expected.js b/test/form/samples/known-globals/_expected.js index c0b933d7b56..94e37d9d5ec 100644 --- a/test/form/samples/known-globals/_expected.js +++ b/test/form/samples/known-globals/_expected.js @@ -1 +1,2 @@ -console.log('main'); +// retained +Math[globalThis.unknown].foo; diff --git a/test/form/samples/known-globals/main.js b/test/form/samples/known-globals/main.js index 01b36b51d3a..f0a93f55834 100644 --- a/test/form/samples/known-globals/main.js +++ b/test/form/samples/known-globals/main.js @@ -1,4 +1,7 @@ -console.log('main'); +// retained +Math[globalThis.unknown].foo; + +// removed const a = setTimeout; const b = globalThis.setTimeout; const c = Math.max(1, 2); diff --git a/test/form/samples/object-expression/unknown-setter-no-side-effect2/_config.js b/test/form/samples/object-expression/unknown-setter-no-side-effect2/_config.js new file mode 100644 index 00000000000..1d59c54d22e --- /dev/null +++ b/test/form/samples/object-expression/unknown-setter-no-side-effect2/_config.js @@ -0,0 +1,4 @@ +module.exports = { + description: 'removes unknown setter access without side effect', + options: { external: ['external'] } +}; diff --git a/test/form/samples/object-expression/unknown-setter-no-side-effect2/_expected.js b/test/form/samples/object-expression/unknown-setter-no-side-effect2/_expected.js new file mode 100644 index 00000000000..bb9900f6dcc --- /dev/null +++ b/test/form/samples/object-expression/unknown-setter-no-side-effect2/_expected.js @@ -0,0 +1,9 @@ +import { unknown } from 'external'; + +const obj2 = { + set [unknown](value) { + console.log('effect'); + } +}; + +obj2[unknown] = true; diff --git a/test/form/samples/object-expression/unknown-setter-no-side-effect2/main.js b/test/form/samples/object-expression/unknown-setter-no-side-effect2/main.js new file mode 100644 index 00000000000..5a8c4863ad1 --- /dev/null +++ b/test/form/samples/object-expression/unknown-setter-no-side-effect2/main.js @@ -0,0 +1,15 @@ +import { unknown } from 'external'; + +const obj = { + set [unknown](value) {} +}; + +obj[unknown] = true; + +const obj2 = { + set [unknown](value) { + console.log('effect'); + } +}; + +obj2[unknown] = true;