From c3b03b7a071336ceed05660b619a311f7abdcd63 Mon Sep 17 00:00:00 2001 From: Marijn Haverbeke Date: Thu, 25 Mar 2021 19:38:37 +0100 Subject: [PATCH] Track static class fields and improve handling of class getters/setters This aims to improve tree-shaking of code that uses static class properties (#3989) and to improve detection of side effects through class getters/setters (#4016). The first part works by keeping a map of positively known static properties (methods and simple getters) in `ClassNode.staticPropertyMap`, along with a flag (`ClassNode.deoptimizedStatic`) that indicates that something happened that removed our confidence that we know anything about the class object. Access and calls to these known static properties are handled by routing the calls to `getLiteralValueAtPath`, `getReturnExpressionWhenCalledAtPath`, and `hasEffectsWhenCalledAtPath` to the known values in the properties. In contrast to `ObjectExpression`, this class does not try to keep track of multiple expressions associated with a property, since that doesn't come up a lot on classes. The handling of side effect detection through getters and setters is done by, _if_ the entire class object (or its prototype in case of access to the prototype) hasn't been deoptimized, scanning through the directly defined getters and setters to see if one exists (calling through to superclasses as appropriate). I believe that this is solid because any code that would be able to change the set of getters and setters on a class would cause the entire object to be deoptimized. --- src/ast/nodes/shared/ClassNode.ts | 212 ++++++++++++++++-- .../literals-from-class-statics/_config.js | 3 + .../literals-from-class-statics/_expected.js | 13 ++ .../literals-from-class-statics/main.js | 21 ++ .../_config.js | 3 + .../_expected.js | 29 +++ .../main.js | 64 ++++++ .../side-effects-static-methods/_config.js | 3 + .../side-effects-static-methods/_expected.js | 28 +++ .../side-effects-static-methods/main.js | 33 +++ 10 files changed, 396 insertions(+), 13 deletions(-) create mode 100644 test/form/samples/literals-from-class-statics/_config.js create mode 100644 test/form/samples/literals-from-class-statics/_expected.js create mode 100644 test/form/samples/literals-from-class-statics/main.js create mode 100644 test/form/samples/side-effects-class-getters-setters/_config.js create mode 100644 test/form/samples/side-effects-class-getters-setters/_expected.js create mode 100644 test/form/samples/side-effects-class-getters-setters/main.js create mode 100644 test/form/samples/side-effects-static-methods/_config.js create mode 100644 test/form/samples/side-effects-static-methods/_expected.js create mode 100644 test/form/samples/side-effects-static-methods/main.js diff --git a/src/ast/nodes/shared/ClassNode.ts b/src/ast/nodes/shared/ClassNode.ts index c6fe89cc24d..3a11d5f77d4 100644 --- a/src/ast/nodes/shared/ClassNode.ts +++ b/src/ast/nodes/shared/ClassNode.ts @@ -1,11 +1,15 @@ +import { getOrCreate } from '../../../utils/getOrCreate'; import { CallOptions } from '../../CallOptions'; +import { DeoptimizableEntity } from '../../DeoptimizableEntity'; import { HasEffectsContext } from '../../ExecutionContext'; import ChildScope from '../../scopes/ChildScope'; import Scope from '../../scopes/Scope'; -import { EMPTY_PATH, ObjectPath } from '../../utils/PathTracker'; +import { EMPTY_PATH, ObjectPath, PathTracker } from '../../utils/PathTracker'; +import { LiteralValueOrUnknown, UnknownValue, UNKNOWN_EXPRESSION } from '../../values'; import ClassBody from '../ClassBody'; import Identifier from '../Identifier'; import MethodDefinition from '../MethodDefinition'; +import { ExpressionEntity } from './Expression'; import { ExpressionNode, NodeBase } from './Node'; export default class ClassNode extends NodeBase { @@ -13,19 +17,137 @@ export default class ClassNode extends NodeBase { id!: Identifier | null; superClass!: ExpressionNode | null; private classConstructor!: MethodDefinition | null; + private deoptimizedPrototype = false; + private deoptimizedStatic = false; + // Collect deoptimization information if we can resolve a property access, by property name + private expressionsToBeDeoptimized = new Map(); + // Known, simple, non-deoptimized static properties are kept in here. They are removed when deoptimized. + private staticPropertyMap: {[name: string]: ExpressionNode} | null = null; + + bind() { + super.bind(); + } createScope(parentScope: Scope) { this.scope = new ChildScope(parentScope); } - hasEffectsWhenAccessedAtPath(path: ObjectPath) { - if (path.length <= 1) return false; - return path.length > 2 || path[0] !== 'prototype'; + deoptimizeAllStatics() { + for (const name in this.staticPropertyMap) { + this.deoptimizeStatic(name); + } + this.deoptimizedStatic = this.deoptimizedPrototype = true; + } + + deoptimizeCache() { + this.deoptimizeAllStatics(); + } + + deoptimizePath(path: ObjectPath) { + const propertyMap = this.getStaticPropertyMap(); + const key = path[0]; + const definition = typeof key === 'string' && propertyMap[key]; + if (path.length === 1) { + if (definition) { + this.deoptimizeStatic(key as string); + } else if (typeof key !== 'string') { + this.deoptimizeAllStatics(); + } + } else if (key === 'prototype' && typeof path[1] !== 'string') { + this.deoptimizedPrototype = true; + } else if (path.length > 1 && definition) { + definition.deoptimizePath(path.slice(1)); + } + } + + deoptimizeStatic(name: string) { + delete this.staticPropertyMap![name]; + const deoptEntities = this.expressionsToBeDeoptimized.get(name); + if (deoptEntities) { + for (const entity of deoptEntities) { + entity.deoptimizeCache(); + } + } + } + + getLiteralValueAtPath( + path: ObjectPath, + recursionTracker: PathTracker, + origin: DeoptimizableEntity + ): LiteralValueOrUnknown { + const key = path[0]; + const definition = typeof key === 'string' && this.getStaticPropertyMap()[key]; + if (path.length === 0 || !definition || + (key === 'prototype' ? this.deoptimizedPrototype : this.deoptimizedStatic)) { + return UnknownValue; + } + + getOrCreate(this.expressionsToBeDeoptimized, key, () => []).push(origin); + return definition.getLiteralValueAtPath( + path.slice(1), + recursionTracker, + origin + ); } - hasEffectsWhenAssignedAtPath(path: ObjectPath) { - if (path.length <= 1) return false; - return path.length > 2 || path[0] !== 'prototype'; + getReturnExpressionWhenCalledAtPath( + path: ObjectPath, + recursionTracker: PathTracker, + origin: DeoptimizableEntity + ): ExpressionEntity { + const key = path[0]; + const definition = typeof key === 'string' && this.getStaticPropertyMap()[key]; + + if (path.length === 0 || !definition || + (key === 'prototype' ? this.deoptimizedPrototype : this.deoptimizedStatic)) { + return UNKNOWN_EXPRESSION; + } + + getOrCreate(this.expressionsToBeDeoptimized, key, () => []).push(origin); + return definition.getReturnExpressionWhenCalledAtPath( + path.slice(1), + recursionTracker, + origin + ); + } + + hasEffectsWhenAccessedAtPath(path: ObjectPath, context: HasEffectsContext) { + if (path.length === 0) return false; + if (this.deoptimizedStatic) return true; + if (this.superClass && this.superClass.hasEffectsWhenAccessedAtPath(path, context)) return true; + const key = path[0]; + if (key === 'prototype') { + if (path.length === 1) return false; + if (this.deoptimizedPrototype) return true; + const key2 = path[1]; + if (path.length === 2 && typeof key2 === 'string') { + return mayHaveGetterSetterEffect(this.body, 'get', false, key2, context); + } + return true; + } else if (typeof key === 'string' && path.length === 1) { + return mayHaveGetterSetterEffect(this.body, 'get', true, key, context); + } else { + return true; + } + } + + hasEffectsWhenAssignedAtPath(path: ObjectPath, context: HasEffectsContext) { + if (this.deoptimizedStatic) return true; + if (this.superClass && this.superClass.hasEffectsWhenAssignedAtPath(path, context)) return true; + const key = path[0]; + if (key === 'prototype') { + if (path.length === 1) return false; + if (this.deoptimizedPrototype) return true; + const key2 = path[1]; + if (path.length === 2 && typeof key2 === 'string') { + return mayHaveGetterSetterEffect(this.body, 'set', false, key2, context); + } + return true; + } else if (typeof key === 'string' && path.length === 1) { + return mayHaveGetterSetterEffect(this.body, 'set', true, key, context); + } else { + return true; + } } hasEffectsWhenCalledAtPath( @@ -33,12 +155,19 @@ export default class ClassNode extends NodeBase { callOptions: CallOptions, context: HasEffectsContext ) { - return !callOptions.withNew || - path.length > 0 || - (this.classConstructor !== null && - this.classConstructor.hasEffectsWhenCalledAtPath(EMPTY_PATH, callOptions, context)) || - (this.superClass !== null && - this.superClass.hasEffectsWhenCalledAtPath(path, callOptions, context)); + if (callOptions.withNew) { + return path.length > 0 || + (this.classConstructor !== null && + this.classConstructor.hasEffectsWhenCalledAtPath(EMPTY_PATH, callOptions, context)) || + (this.superClass !== null && + this.superClass.hasEffectsWhenCalledAtPath(path, callOptions, context)); + } else { + if (path.length !== 1 || this.deoptimizedStatic) return true; + const key = path[0]; + const definition = typeof key === 'string' && this.getStaticPropertyMap()[key]; + if (!definition) return true; + return definition.hasEffectsWhenCalledAtPath([], callOptions, context); + } } initialise() { @@ -53,4 +182,61 @@ export default class ClassNode extends NodeBase { } this.classConstructor = null; } + + mayModifyThisWhenCalledAtPath( + path: ObjectPath + ) { + const key = path[0]; + const definition = typeof key === 'string' && this.getStaticPropertyMap()[key]; + if (!definition || this.deoptimizedStatic) return true; + return definition.mayModifyThisWhenCalledAtPath(path.slice(1)); + } + + private getStaticPropertyMap(): {[name: string]: ExpressionNode} { + if (this.staticPropertyMap) return this.staticPropertyMap; + + const propertyMap = this.staticPropertyMap = Object.create(null); + const seen: {[name: string]: boolean} = Object.create(null); + for (const definition of this.body.body) { + if (!definition.static) continue; + // If there are non-identifier-named statics, give up. + if (definition.computed || !(definition.key instanceof Identifier)) { + return this.staticPropertyMap = Object.create(null); + } + const key = definition.key.name; + // Not handling duplicate definitions. + if (seen[key]) { + delete propertyMap[key]; + continue; + } + seen[key] = true; + if (definition instanceof MethodDefinition) { + if (definition.kind === "method") { + propertyMap[key] = definition.value; + } + } else if (definition.value) { + propertyMap[key] = definition.value; + } + } + return this.staticPropertyMap = propertyMap; + } +} + +function mayHaveGetterSetterEffect( + body: ClassBody, + kind: 'get' | 'set', isStatic: boolean, name: string, + context: HasEffectsContext +) { + for (const definition of body.body) { + if (definition instanceof MethodDefinition && definition.static === isStatic && definition.kind === kind) { + if (definition.computed || !(definition.key instanceof Identifier)) { + return true; + } + if (definition.key.name === name && + definition.value.hasEffectsWhenCalledAtPath([], {args: [], withNew: false}, context)) { + return true; + } + } + } + return false; } diff --git a/test/form/samples/literals-from-class-statics/_config.js b/test/form/samples/literals-from-class-statics/_config.js new file mode 100644 index 00000000000..0847b183aac --- /dev/null +++ b/test/form/samples/literals-from-class-statics/_config.js @@ -0,0 +1,3 @@ +module.exports = { + description: 'tracks literal values in class static fields' +}; diff --git a/test/form/samples/literals-from-class-statics/_expected.js b/test/form/samples/literals-from-class-statics/_expected.js new file mode 100644 index 00000000000..849a60add18 --- /dev/null +++ b/test/form/samples/literals-from-class-statics/_expected.js @@ -0,0 +1,13 @@ +log("t"); +log("x"); + +class Undef { + static y; +} +if (Undef.y) log("y"); + +class Deopt { + static z = false; +} +unknown(Deopt); +if (Deopt.z) log("z"); diff --git a/test/form/samples/literals-from-class-statics/main.js b/test/form/samples/literals-from-class-statics/main.js new file mode 100644 index 00000000000..13c231108ff --- /dev/null +++ b/test/form/samples/literals-from-class-statics/main.js @@ -0,0 +1,21 @@ +class Static { + static t() { return true; } + static f() { return false; } + static x = 10; +} + +if (Static.t()) log("t"); +if (Static.f()) log("f"); +if (!Static.t()) log("!t"); +if (Static.x) log("x"); + +class Undef { + static y; +} +if (Undef.y) log("y"); + +class Deopt { + static z = false; +} +unknown(Deopt); +if (Deopt.z) log("z"); diff --git a/test/form/samples/side-effects-class-getters-setters/_config.js b/test/form/samples/side-effects-class-getters-setters/_config.js new file mode 100644 index 00000000000..201e3e082f9 --- /dev/null +++ b/test/form/samples/side-effects-class-getters-setters/_config.js @@ -0,0 +1,3 @@ +module.exports = { + description: 'treat getters and setters on classes as function calls' +}; diff --git a/test/form/samples/side-effects-class-getters-setters/_expected.js b/test/form/samples/side-effects-class-getters-setters/_expected.js new file mode 100644 index 00000000000..c7f6dc30b38 --- /dev/null +++ b/test/form/samples/side-effects-class-getters-setters/_expected.js @@ -0,0 +1,29 @@ +class RetainedByGetter { + get a() { log(); } +} +RetainedByGetter.prototype.a; + +class RetainedBySetter { + set a(v) { log(); } +} +RetainedBySetter.prototype.a = 10; + +class RetainedByStaticGetter { + static get a() { log(); } +} +RetainedByStaticGetter.a; + +class RetainedByStaticSetter { + static set a(v) { log(); } +} +RetainedByStaticSetter.a = 10; + +class RetainedSuper { + static get a() { log(); } +} +class RetainedSub extends RetainedSuper {} +RetainedSub.a; + +class DeoptProto {} +unknown(DeoptProto.prototype); +DeoptProto.prototype.a; diff --git a/test/form/samples/side-effects-class-getters-setters/main.js b/test/form/samples/side-effects-class-getters-setters/main.js new file mode 100644 index 00000000000..b1f88f385c6 --- /dev/null +++ b/test/form/samples/side-effects-class-getters-setters/main.js @@ -0,0 +1,64 @@ +class Removed { + get a() { log(); } + set a(v) { log(); } + static get a() { log(); } + static set a(v) { log(); } +} + +class RemovedNoEffect { + get a() {} + set a(v) {} + static get a() {} + static set a(v) {} +} +RemovedNoEffect.prototype.a; +RemovedNoEffect.prototype.a = 1; +RemovedNoEffect.a; +RemovedNoEffect.a = 1; + +class RetainedByGetter { + get a() { log(); } +} +RetainedByGetter.prototype.a; + +class RetainedBySetter { + set a(v) { log(); } +} +RetainedBySetter.prototype.a = 10; + +class RetainedByStaticGetter { + static get a() { log(); } +} +RetainedByStaticGetter.a; + +class RetainedByStaticSetter { + static set a(v) { log(); } +} +RetainedByStaticSetter.a = 10; + +class RemovedSetters { + set a(v) { log(); } + static set a(v) { log(); } +} +RemovedSetters.prototype.a; +RemovedSetters.a; + +class RemovedWrongProp { + get a() { log(); } + static get a() { log(); } +} +RemovedWrongProp.prototype.b +RemovedWrongProp.b + +class RetainedSuper { + static get a() { log(); } +} +class RetainedSub extends RetainedSuper {} +RetainedSub.a; + +class RemovedSub extends RetainedSuper {} +RemovedSub.b; + +class DeoptProto {} +unknown(DeoptProto.prototype); +DeoptProto.prototype.a; diff --git a/test/form/samples/side-effects-static-methods/_config.js b/test/form/samples/side-effects-static-methods/_config.js new file mode 100644 index 00000000000..129d0e1d6de --- /dev/null +++ b/test/form/samples/side-effects-static-methods/_config.js @@ -0,0 +1,3 @@ +module.exports = { + description: 'allow calls to pure static methods to be tree-shaken' +}; diff --git a/test/form/samples/side-effects-static-methods/_expected.js b/test/form/samples/side-effects-static-methods/_expected.js new file mode 100644 index 00000000000..5f8c42d4aeb --- /dev/null +++ b/test/form/samples/side-effects-static-methods/_expected.js @@ -0,0 +1,28 @@ +class Effect { + static a() { log(); } +} +Effect.a(); + +class DeoptComputed { + static a() {} + static [foo]() { log(); } +} +DeoptComputed.a(); + +class DeoptGetter { + static a() {} + static get a() {} +} +DeoptGetter.a(); + +class DeoptAssign { + static a() {} +} +DeoptAssign.a = log; +DeoptAssign.a(); + +class DeoptFully { + static a() {} +} +unknown(DeoptFully); +DeoptFully.a(); diff --git a/test/form/samples/side-effects-static-methods/main.js b/test/form/samples/side-effects-static-methods/main.js new file mode 100644 index 00000000000..0d3d32153f9 --- /dev/null +++ b/test/form/samples/side-effects-static-methods/main.js @@ -0,0 +1,33 @@ +class NoEffect { + static a() {} +} +NoEffect.a(); + +class Effect { + static a() { log(); } +} +Effect.a(); + +class DeoptComputed { + static a() {} + static [foo]() { log(); } +} +DeoptComputed.a(); + +class DeoptGetter { + static a() {} + static get a() {} +} +DeoptGetter.a(); + +class DeoptAssign { + static a() {} +} +DeoptAssign.a = log; +DeoptAssign.a(); + +class DeoptFully { + static a() {} +} +unknown(DeoptFully); +DeoptFully.a();