From b3fb1b82058f31670964150f2dcb472541663c1c Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Fri, 10 Jun 2022 07:09:26 +0200 Subject: [PATCH] Fix side effect and self-reference handling --- src/ast/nodes/BinaryExpression.ts | 38 +++++++++---------- src/ast/nodes/NewExpression.ts | 3 +- src/ast/nodes/shared/ClassNode.ts | 3 ++ src/ast/nodes/shared/FunctionBase.ts | 11 +++++- src/ast/nodes/shared/ObjectEntity.ts | 4 +- src/ast/utils/PathTracker.ts | 15 ++++---- test/form/samples/instanceof/_expected.js | 12 ++++++ test/form/samples/instanceof/main.js | 26 +++++++------ .../invalid-binary-expressions/_expected.js | 4 ++ .../instanceof/left-hand-effect/_config.js | 3 ++ .../instanceof/left-hand-effect/main.js | 14 +++++++ .../instanceof/right-hand-effect/_config.js | 3 ++ .../instanceof/right-hand-effect/main.js | 14 +++++++ .../samples/instanceof/used/_config.js | 3 ++ test/function/samples/instanceof/used/main.js | 5 +++ 15 files changed, 117 insertions(+), 41 deletions(-) create mode 100644 test/function/samples/instanceof/left-hand-effect/_config.js create mode 100644 test/function/samples/instanceof/left-hand-effect/main.js create mode 100644 test/function/samples/instanceof/right-hand-effect/_config.js create mode 100644 test/function/samples/instanceof/right-hand-effect/main.js create mode 100644 test/function/samples/instanceof/used/_config.js create mode 100644 test/function/samples/instanceof/used/main.js diff --git a/src/ast/nodes/BinaryExpression.ts b/src/ast/nodes/BinaryExpression.ts index 86d6bc9b1b4..4f8c9db133c 100644 --- a/src/ast/nodes/BinaryExpression.ts +++ b/src/ast/nodes/BinaryExpression.ts @@ -3,7 +3,7 @@ import { BLANK } from '../../utils/blank'; import type { NodeRenderOptions, RenderOptions } from '../../utils/renderHelpers'; import type { DeoptimizableEntity } from '../DeoptimizableEntity'; import type { HasEffectsContext, InclusionContext } from '../ExecutionContext'; -import { createHasEffectsContext } from '../ExecutionContext'; +import { createHasEffectsContext, createInclusionContext } from '../ExecutionContext'; import { INTERACTION_ACCESSED, NODE_INTERACTION_UNKNOWN_ASSIGNMENT, @@ -14,7 +14,7 @@ import { type ObjectPath, type PathTracker, SHARED_RECURSION_TRACKER, - TEST_INCLUSION_PATH + TEST_INSTANCEOF_PATH } from '../utils/PathTracker'; import ExpressionStatement from './ExpressionStatement'; import type { LiteralValue } from './Literal'; @@ -59,15 +59,13 @@ const binaryOperators: { '+': (left: any, right: any) => left + right, '-': (left: any, right: any) => left - right, '/': (left: any, right: any) => left / right, - '<': (left, right) => (left as NonNullable) < (right as NonNullable), + '<': (left, right) => left! < right!, '<<': (left: any, right: any) => left << right, - '<=': (left, right) => - (left as NonNullable) <= (right as NonNullable), + '<=': (left, right) => left! <= right!, '==': (left, right) => left == right, '===': (left, right) => left === right, - '>': (left, right) => (left as NonNullable) > (right as NonNullable), - '>=': (left, right) => - (left as NonNullable) >= (right as NonNullable), + '>': (left, right) => left! > right!, + '>=': (left, right) => left! >= right!, '>>': (left: any, right: any) => left >> right, '>>>': (left: any, right: any) => left >>> right, '^': (left: any, right: any) => left ^ right, @@ -93,9 +91,12 @@ export default class BinaryExpression extends NodeBase implements DeoptimizableE ): LiteralValueOrUnknown { if (path.length > 0) return UnknownValue; if (this.operator === 'instanceof') { + // We need to include out-of-order to handle situations where left is the + // only usage of right + this.left.include(createInclusionContext(), false); if ( this.right.hasEffectsOnInteractionAtPath( - TEST_INCLUSION_PATH, + TEST_INSTANCEOF_PATH, NODE_INTERACTION_UNKNOWN_ASSIGNMENT, createHasEffectsContext() ) @@ -134,12 +135,9 @@ export default class BinaryExpression extends NodeBase implements DeoptimizableE return type !== INTERACTION_ACCESSED || path.length > 1; } - // TODO Lukas if the operator is instanceof, only include if necessary include(context: InclusionContext, includeChildrenRecursively: IncludeChildren) { this.deoptimizeInstanceof(); - if (!this.expressionsDependingOnNegativeInstanceof.size) { - super.include(context, includeChildrenRecursively); - } + super.include(context, includeChildrenRecursively); } render( @@ -154,15 +152,17 @@ export default class BinaryExpression extends NodeBase implements DeoptimizableE private deoptimizeInstanceof(context?: HasEffectsContext) { if ( this.operator === 'instanceof' && - this.expressionsDependingOnNegativeInstanceof.size && - this.right.hasEffectsOnInteractionAtPath( - TEST_INCLUSION_PATH, - NODE_INTERACTION_UNKNOWN_ASSIGNMENT, - context || createHasEffectsContext() - ) + (this.included || + (this.expressionsDependingOnNegativeInstanceof.size && + this.right.hasEffectsOnInteractionAtPath( + TEST_INSTANCEOF_PATH, + NODE_INTERACTION_UNKNOWN_ASSIGNMENT, + context || createHasEffectsContext() + ))) ) { for (const expression of this.expressionsDependingOnNegativeInstanceof) { expression.deoptimizeCache(); + this.context.requestTreeshakingPass(); } this.expressionsDependingOnNegativeInstanceof.clear(); } diff --git a/src/ast/nodes/NewExpression.ts b/src/ast/nodes/NewExpression.ts index e12915475e5..974a357bc2c 100644 --- a/src/ast/nodes/NewExpression.ts +++ b/src/ast/nodes/NewExpression.ts @@ -28,8 +28,9 @@ export default class NewExpression extends NodeBase { if ( (this.context.options.treeshake as NormalizedTreeshakingOptions).annotations && this.annotations - ) + ) { return false; + } return ( this.callee.hasEffects(context) || this.callee.hasEffectsOnInteractionAtPath(EMPTY_PATH, this.interaction, context) diff --git a/src/ast/nodes/shared/ClassNode.ts b/src/ast/nodes/shared/ClassNode.ts index 06f54ee3db7..44c44629aab 100644 --- a/src/ast/nodes/shared/ClassNode.ts +++ b/src/ast/nodes/shared/ClassNode.ts @@ -13,6 +13,7 @@ import { type ObjectPath, type PathTracker, SHARED_RECURSION_TRACKER, + TestInstanceof, UNKNOWN_PATH, UnknownKey } from '../../utils/PathTracker'; @@ -95,6 +96,8 @@ export default class ClassNode extends NodeBase implements DeoptimizableEntity { : this.superClass?.hasEffectsOnInteractionAtPath(path, interaction, context)) || false ); + } else if (path[0] === TestInstanceof) { + return false; } else { return this.getObjectEntity().hasEffectsOnInteractionAtPath(path, interaction, context); } diff --git a/src/ast/nodes/shared/FunctionBase.ts b/src/ast/nodes/shared/FunctionBase.ts index a1517cb7e08..1ec9efa35dc 100644 --- a/src/ast/nodes/shared/FunctionBase.ts +++ b/src/ast/nodes/shared/FunctionBase.ts @@ -14,7 +14,13 @@ import { NodeInteractionWithThisArg } from '../../NodeInteractions'; import ReturnValueScope from '../../scopes/ReturnValueScope'; -import { type ObjectPath, PathTracker, UNKNOWN_PATH, UnknownKey } from '../../utils/PathTracker'; +import { + type ObjectPath, + PathTracker, + TestInstanceof, + UNKNOWN_PATH, + UnknownKey +} from '../../utils/PathTracker'; import BlockStatement from '../BlockStatement'; import * as NodeType from '../NodeType'; import RestElement from '../RestElement'; @@ -95,6 +101,9 @@ export default abstract class FunctionBase extends NodeBase { interaction: NodeInteraction, context: HasEffectsContext ): boolean { + if (path[0] === TestInstanceof) { + return false; + } if (path.length > 0 || interaction.type !== INTERACTION_CALLED) { return this.getObjectEntity().hasEffectsOnInteractionAtPath(path, interaction, context); } diff --git a/src/ast/nodes/shared/ObjectEntity.ts b/src/ast/nodes/shared/ObjectEntity.ts index c8ebd64f567..aef1164b87d 100644 --- a/src/ast/nodes/shared/ObjectEntity.ts +++ b/src/ast/nodes/shared/ObjectEntity.ts @@ -11,7 +11,7 @@ import { ObjectPath, ObjectPathKey, PathTracker, - TestInclusionKey, + TestInstanceof, UNKNOWN_INTEGER_PATH, UNKNOWN_PATH, UnknownInteger, @@ -282,7 +282,7 @@ export class ObjectEntity extends ExpressionEntity { context: HasEffectsContext ): boolean { const [key, ...subPath] = path; - if (key === TestInclusionKey) return false; + if (key === TestInstanceof) return true; if (subPath.length || interaction.type === INTERACTION_CALLED) { const expressionAtPath = this.getMemberExpression(key); if (expressionAtPath) { diff --git a/src/ast/utils/PathTracker.ts b/src/ast/utils/PathTracker.ts index 148e429ba50..0c552d2242f 100644 --- a/src/ast/utils/PathTracker.ts +++ b/src/ast/utils/PathTracker.ts @@ -6,21 +6,22 @@ export const UnknownNonAccessorKey = Symbol('Unknown Non-Accessor Key'); export const UnknownInteger = Symbol('Unknown Integer'); /** * A special key that does not actually reference a property but can be used to - * test if a variable is included via - * .hasEffectsOnInteractionAtPath([TestInclusionKey], {type: INTERACTION_ASSIGNED,...}, ...) + * test if the right hand side of "instanceof" is either included or not a class + * or function via + * .hasEffectsOnInteractionAtPath([TestInstanceof], {type: INTERACTION_ASSIGNED,...}, ...) */ -export const TestInclusionKey = Symbol('Test Inclusion Key'); +export const TestInstanceof = Symbol('Test instanceof'); export type ObjectPathKey = | string | typeof UnknownKey | typeof UnknownNonAccessorKey | typeof UnknownInteger - | typeof TestInclusionKey; + | typeof TestInstanceof; export type ObjectPath = ObjectPathKey[]; export const EMPTY_PATH: ObjectPath = []; export const UNKNOWN_PATH: ObjectPath = [UnknownKey]; -export const TEST_INCLUSION_PATH: ObjectPath = [TestInclusionKey]; +export const TEST_INSTANCEOF_PATH: ObjectPath = [TestInstanceof]; // 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 @@ -33,7 +34,7 @@ const EntitiesKey = Symbol('Entities'); interface EntityPaths { [pathSegment: string]: EntityPaths; [EntitiesKey]: Set; - [TestInclusionKey]?: EntityPaths; + [TestInstanceof]?: EntityPaths; [UnknownInteger]?: EntityPaths; [UnknownKey]?: EntityPaths; [UnknownNonAccessorKey]?: EntityPaths; @@ -81,7 +82,7 @@ export const SHARED_RECURSION_TRACKER = new PathTracker(); interface DiscriminatedEntityPaths { [pathSegment: string]: DiscriminatedEntityPaths; [EntitiesKey]: Map>; - [TestInclusionKey]?: DiscriminatedEntityPaths; + [TestInstanceof]?: DiscriminatedEntityPaths; [UnknownInteger]?: DiscriminatedEntityPaths; [UnknownKey]?: DiscriminatedEntityPaths; [UnknownNonAccessorKey]?: DiscriminatedEntityPaths; diff --git a/test/form/samples/instanceof/_expected.js b/test/form/samples/instanceof/_expected.js index 724e99a15d2..291a9f7748c 100644 --- a/test/form/samples/instanceof/_expected.js +++ b/test/form/samples/instanceof/_expected.js @@ -1,3 +1,15 @@ console.log('retained'); console.log('retained'); + +class Used1 {} +const Intermediate = Used1; +const used1 = new Used1(); + +if (used1 instanceof Intermediate) console.log('retained'); +else console.log('does not matter'); + +class Used2 {} + +if (new Used2() instanceof Intermediate) console.log('retained'); +else console.log('does not matter'); diff --git a/test/form/samples/instanceof/main.js b/test/form/samples/instanceof/main.js index 21a926e66ed..718d04ff40a 100644 --- a/test/form/samples/instanceof/main.js +++ b/test/form/samples/instanceof/main.js @@ -1,17 +1,21 @@ class Unused1 {} -class Used1 {} -const Intermediate = Used1; -if (new Intermediate() instanceof Unused1) console.log('removed'); +if ({} instanceof Unused1) console.log('removed'); else console.log('retained'); -class Unused2 {} -class WithEffect { - constructor() { - console.log('effect'); - } -} +function Unused2() {} -if (new WithEffect() instanceof Unused2) - console.log('does not matter, but effect should be retained'); +if ({} instanceof Unused2) console.log('removed'); else console.log('retained'); + +class Used1 {} +const Intermediate = Used1; +const used1 = new Used1(); + +if (used1 instanceof Intermediate) console.log('retained'); +else console.log('does not matter'); + +class Used2 {} + +if (new Used2() instanceof Intermediate) console.log('retained'); +else console.log('does not matter'); diff --git a/test/form/samples/invalid-binary-expressions/_expected.js b/test/form/samples/invalid-binary-expressions/_expected.js index 5e9455acaed..b43a2513a80 100644 --- a/test/form/samples/invalid-binary-expressions/_expected.js +++ b/test/form/samples/invalid-binary-expressions/_expected.js @@ -37,3 +37,7 @@ if (null instanceof true) { if (null instanceof 'y') { console.log('retained'); } + +if (null instanceof {}) { + console.log('retained'); +} diff --git a/test/function/samples/instanceof/left-hand-effect/_config.js b/test/function/samples/instanceof/left-hand-effect/_config.js new file mode 100644 index 00000000000..edadb884deb --- /dev/null +++ b/test/function/samples/instanceof/left-hand-effect/_config.js @@ -0,0 +1,3 @@ +module.exports = { + description: 'retains side effects in the left hand side of instanceof' +}; diff --git a/test/function/samples/instanceof/left-hand-effect/main.js b/test/function/samples/instanceof/left-hand-effect/main.js new file mode 100644 index 00000000000..f29fbed86a7 --- /dev/null +++ b/test/function/samples/instanceof/left-hand-effect/main.js @@ -0,0 +1,14 @@ +let effect = false; + +class Bar {} +class Foo { + constructor() { + effect = true; + } +} + +if (new Foo() instanceof Bar) { + assert.fail('Wrong instance relation'); +} + +assert.ok(effect); diff --git a/test/function/samples/instanceof/right-hand-effect/_config.js b/test/function/samples/instanceof/right-hand-effect/_config.js new file mode 100644 index 00000000000..466922bcd67 --- /dev/null +++ b/test/function/samples/instanceof/right-hand-effect/_config.js @@ -0,0 +1,3 @@ +module.exports = { + description: 'retains side effects in the right hand side of instanceof' +}; diff --git a/test/function/samples/instanceof/right-hand-effect/main.js b/test/function/samples/instanceof/right-hand-effect/main.js new file mode 100644 index 00000000000..01fa8698a53 --- /dev/null +++ b/test/function/samples/instanceof/right-hand-effect/main.js @@ -0,0 +1,14 @@ +let effect = false; + +class Foo {} + +if ( + new Foo() instanceof + class { + [(effect = true)]() {} + } +) { + assert.fail('Wrong instance relation'); +} + +assert.ok(effect); diff --git a/test/function/samples/instanceof/used/_config.js b/test/function/samples/instanceof/used/_config.js new file mode 100644 index 00000000000..fe5faa35411 --- /dev/null +++ b/test/function/samples/instanceof/used/_config.js @@ -0,0 +1,3 @@ +module.exports = { + description: 'retains instanceof if it is true' +}; diff --git a/test/function/samples/instanceof/used/main.js b/test/function/samples/instanceof/used/main.js new file mode 100644 index 00000000000..cbd5feb2583 --- /dev/null +++ b/test/function/samples/instanceof/used/main.js @@ -0,0 +1,5 @@ +class Foo {} + +if (!(new Foo() instanceof Foo)) { + assert.fail('instanceof not resolved correctly'); +}