From ab63d0d9f1d2cd9ea48ce6ccbaf4ea355f875589 Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Wed, 8 Jun 2022 07:03:26 +0200 Subject: [PATCH 1/3] Treeshake instanceof --- src/ast/nodes/BinaryExpression.ts | 83 +++++++++++++++++-- src/ast/nodes/shared/ObjectEntity.ts | 2 + src/ast/utils/PathTracker.ts | 12 ++- test/form/samples/instanceof/_config.js | 3 + test/form/samples/instanceof/_expected.js | 3 + test/form/samples/instanceof/main.js | 17 ++++ .../invalid-binary-expressions/_expected.js | 4 - 7 files changed, 113 insertions(+), 11 deletions(-) create mode 100644 test/form/samples/instanceof/_config.js create mode 100644 test/form/samples/instanceof/_expected.js create mode 100644 test/form/samples/instanceof/main.js diff --git a/src/ast/nodes/BinaryExpression.ts b/src/ast/nodes/BinaryExpression.ts index d1167831f79..86d6bc9b1b4 100644 --- a/src/ast/nodes/BinaryExpression.ts +++ b/src/ast/nodes/BinaryExpression.ts @@ -2,22 +2,52 @@ import type MagicString from 'magic-string'; import { BLANK } from '../../utils/blank'; import type { NodeRenderOptions, RenderOptions } from '../../utils/renderHelpers'; import type { DeoptimizableEntity } from '../DeoptimizableEntity'; -import type { HasEffectsContext } from '../ExecutionContext'; -import { INTERACTION_ACCESSED, NodeInteraction } from '../NodeInteractions'; +import type { HasEffectsContext, InclusionContext } from '../ExecutionContext'; +import { createHasEffectsContext } from '../ExecutionContext'; +import { + INTERACTION_ACCESSED, + NODE_INTERACTION_UNKNOWN_ASSIGNMENT, + NodeInteraction +} from '../NodeInteractions'; import { EMPTY_PATH, type ObjectPath, type PathTracker, - SHARED_RECURSION_TRACKER + SHARED_RECURSION_TRACKER, + TEST_INCLUSION_PATH } from '../utils/PathTracker'; import ExpressionStatement from './ExpressionStatement'; import type { LiteralValue } from './Literal'; import type * as NodeType from './NodeType'; import { type LiteralValueOrUnknown, UnknownValue } from './shared/Expression'; -import { type ExpressionNode, NodeBase } from './shared/Node'; +import { type ExpressionNode, IncludeChildren, NodeBase } from './shared/Node'; + +type Operator = + | '!=' + | '!==' + | '%' + | '&' + | '*' + | '**' + | '+' + | '-' + | '/' + | '<' + | '<<' + | '<=' + | '==' + | '===' + | '>' + | '>=' + | '>>' + | '>>>' + | '^' + | '|' + | 'in' + | 'instanceof'; const binaryOperators: { - [operator: string]: (left: LiteralValue, right: LiteralValue) => LiteralValueOrUnknown; + [operator in Operator]?: (left: LiteralValue, right: LiteralValue) => LiteralValueOrUnknown; } = { '!=': (left, right) => left != right, '!==': (left, right) => left !== right, @@ -52,6 +82,7 @@ export default class BinaryExpression extends NodeBase implements DeoptimizableE declare operator: keyof typeof binaryOperators; declare right: ExpressionNode; declare type: NodeType.tBinaryExpression; + private expressionsDependingOnNegativeInstanceof = new Set(); deoptimizeCache(): void {} @@ -61,6 +92,19 @@ export default class BinaryExpression extends NodeBase implements DeoptimizableE origin: DeoptimizableEntity ): LiteralValueOrUnknown { if (path.length > 0) return UnknownValue; + if (this.operator === 'instanceof') { + if ( + this.right.hasEffectsOnInteractionAtPath( + TEST_INCLUSION_PATH, + NODE_INTERACTION_UNKNOWN_ASSIGNMENT, + createHasEffectsContext() + ) + ) { + return UnknownValue; + } + this.expressionsDependingOnNegativeInstanceof.add(origin); + return false; + } const leftValue = this.left.getLiteralValueAtPath(EMPTY_PATH, recursionTracker, origin); if (typeof leftValue === 'symbol') return UnknownValue; @@ -79,8 +123,10 @@ export default class BinaryExpression extends NodeBase implements DeoptimizableE this.operator === '+' && this.parent instanceof ExpressionStatement && this.left.getLiteralValueAtPath(EMPTY_PATH, SHARED_RECURSION_TRACKER, this) === '' - ) + ) { return true; + } + this.deoptimizeInstanceof(context); return super.hasEffects(context); } @@ -88,6 +134,14 @@ 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); + } + } + render( code: MagicString, options: RenderOptions, @@ -96,4 +150,21 @@ export default class BinaryExpression extends NodeBase implements DeoptimizableE this.left.render(code, options, { renderedSurroundingElement }); this.right.render(code, options); } + + private deoptimizeInstanceof(context?: HasEffectsContext) { + if ( + this.operator === 'instanceof' && + this.expressionsDependingOnNegativeInstanceof.size && + this.right.hasEffectsOnInteractionAtPath( + TEST_INCLUSION_PATH, + NODE_INTERACTION_UNKNOWN_ASSIGNMENT, + context || createHasEffectsContext() + ) + ) { + for (const expression of this.expressionsDependingOnNegativeInstanceof) { + expression.deoptimizeCache(); + } + this.expressionsDependingOnNegativeInstanceof.clear(); + } + } } diff --git a/src/ast/nodes/shared/ObjectEntity.ts b/src/ast/nodes/shared/ObjectEntity.ts index efbc2026427..c8ebd64f567 100644 --- a/src/ast/nodes/shared/ObjectEntity.ts +++ b/src/ast/nodes/shared/ObjectEntity.ts @@ -11,6 +11,7 @@ import { ObjectPath, ObjectPathKey, PathTracker, + TestInclusionKey, UNKNOWN_INTEGER_PATH, UNKNOWN_PATH, UnknownInteger, @@ -281,6 +282,7 @@ export class ObjectEntity extends ExpressionEntity { context: HasEffectsContext ): boolean { const [key, ...subPath] = path; + if (key === TestInclusionKey) return false; 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 5b0679a6187..148e429ba50 100644 --- a/src/ast/utils/PathTracker.ts +++ b/src/ast/utils/PathTracker.ts @@ -4,15 +4,23 @@ 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'); +/** + * 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,...}, ...) + */ +export const TestInclusionKey = Symbol('Test Inclusion Key'); export type ObjectPathKey = | string | typeof UnknownKey | typeof UnknownNonAccessorKey - | typeof UnknownInteger; + | typeof UnknownInteger + | typeof TestInclusionKey; export type ObjectPath = ObjectPathKey[]; export const EMPTY_PATH: ObjectPath = []; export const UNKNOWN_PATH: ObjectPath = [UnknownKey]; +export const TEST_INCLUSION_PATH: ObjectPath = [TestInclusionKey]; // 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 @@ -25,6 +33,7 @@ const EntitiesKey = Symbol('Entities'); interface EntityPaths { [pathSegment: string]: EntityPaths; [EntitiesKey]: Set; + [TestInclusionKey]?: EntityPaths; [UnknownInteger]?: EntityPaths; [UnknownKey]?: EntityPaths; [UnknownNonAccessorKey]?: EntityPaths; @@ -72,6 +81,7 @@ export const SHARED_RECURSION_TRACKER = new PathTracker(); interface DiscriminatedEntityPaths { [pathSegment: string]: DiscriminatedEntityPaths; [EntitiesKey]: Map>; + [TestInclusionKey]?: DiscriminatedEntityPaths; [UnknownInteger]?: DiscriminatedEntityPaths; [UnknownKey]?: DiscriminatedEntityPaths; [UnknownNonAccessorKey]?: DiscriminatedEntityPaths; diff --git a/test/form/samples/instanceof/_config.js b/test/form/samples/instanceof/_config.js new file mode 100644 index 00000000000..272579e8d05 --- /dev/null +++ b/test/form/samples/instanceof/_config.js @@ -0,0 +1,3 @@ +module.exports = { + description: 'removes instanceof checks if the right side is not included' +}; diff --git a/test/form/samples/instanceof/_expected.js b/test/form/samples/instanceof/_expected.js new file mode 100644 index 00000000000..724e99a15d2 --- /dev/null +++ b/test/form/samples/instanceof/_expected.js @@ -0,0 +1,3 @@ +console.log('retained'); + +console.log('retained'); diff --git a/test/form/samples/instanceof/main.js b/test/form/samples/instanceof/main.js new file mode 100644 index 00000000000..21a926e66ed --- /dev/null +++ b/test/form/samples/instanceof/main.js @@ -0,0 +1,17 @@ +class Unused1 {} +class Used1 {} +const Intermediate = Used1; + +if (new Intermediate() instanceof Unused1) console.log('removed'); +else console.log('retained'); + +class Unused2 {} +class WithEffect { + constructor() { + console.log('effect'); + } +} + +if (new WithEffect() instanceof Unused2) + console.log('does not matter, but effect should be retained'); +else console.log('retained'); diff --git a/test/form/samples/invalid-binary-expressions/_expected.js b/test/form/samples/invalid-binary-expressions/_expected.js index b43a2513a80..5e9455acaed 100644 --- a/test/form/samples/invalid-binary-expressions/_expected.js +++ b/test/form/samples/invalid-binary-expressions/_expected.js @@ -37,7 +37,3 @@ if (null instanceof true) { if (null instanceof 'y') { console.log('retained'); } - -if (null instanceof {}) { - console.log('retained'); -} From 505331cd7c7ab9994fd58ea37c4002a7ca0a6aa5 Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Fri, 10 Jun 2022 07:09:26 +0200 Subject: [PATCH 2/3] 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'); +} From 4547588b1e543419ac227854a2ba9267cbae21f3 Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Fri, 10 Jun 2022 10:45:53 +0200 Subject: [PATCH 3/3] Remove feature and add regression test --- src/ast/nodes/BinaryExpression.ts | 56 ++----------------- src/ast/nodes/shared/ClassNode.ts | 3 - src/ast/nodes/shared/FunctionBase.ts | 11 +--- src/ast/nodes/shared/ObjectEntity.ts | 2 - src/ast/utils/PathTracker.ts | 13 +---- test/form/samples/instanceof/_config.js | 3 - test/form/samples/instanceof/_expected.js | 15 ----- test/form/samples/instanceof/main.js | 21 ------- .../instanceof/used-parameter/_config.js | 3 + .../samples/instanceof/used-parameter/main.js | 12 ++++ 10 files changed, 21 insertions(+), 118 deletions(-) delete mode 100644 test/form/samples/instanceof/_config.js delete mode 100644 test/form/samples/instanceof/_expected.js delete mode 100644 test/form/samples/instanceof/main.js create mode 100644 test/function/samples/instanceof/used-parameter/_config.js create mode 100644 test/function/samples/instanceof/used-parameter/main.js diff --git a/src/ast/nodes/BinaryExpression.ts b/src/ast/nodes/BinaryExpression.ts index 4f8c9db133c..5e73aa8d715 100644 --- a/src/ast/nodes/BinaryExpression.ts +++ b/src/ast/nodes/BinaryExpression.ts @@ -2,25 +2,19 @@ import type MagicString from 'magic-string'; 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, createInclusionContext } from '../ExecutionContext'; -import { - INTERACTION_ACCESSED, - NODE_INTERACTION_UNKNOWN_ASSIGNMENT, - NodeInteraction -} from '../NodeInteractions'; +import type { HasEffectsContext } from '../ExecutionContext'; +import { INTERACTION_ACCESSED, NodeInteraction } from '../NodeInteractions'; import { EMPTY_PATH, type ObjectPath, type PathTracker, - SHARED_RECURSION_TRACKER, - TEST_INSTANCEOF_PATH + SHARED_RECURSION_TRACKER } from '../utils/PathTracker'; import ExpressionStatement from './ExpressionStatement'; import type { LiteralValue } from './Literal'; import type * as NodeType from './NodeType'; import { type LiteralValueOrUnknown, UnknownValue } from './shared/Expression'; -import { type ExpressionNode, IncludeChildren, NodeBase } from './shared/Node'; +import { type ExpressionNode, NodeBase } from './shared/Node'; type Operator = | '!=' @@ -80,7 +74,6 @@ export default class BinaryExpression extends NodeBase implements DeoptimizableE declare operator: keyof typeof binaryOperators; declare right: ExpressionNode; declare type: NodeType.tBinaryExpression; - private expressionsDependingOnNegativeInstanceof = new Set(); deoptimizeCache(): void {} @@ -90,22 +83,6 @@ export default class BinaryExpression extends NodeBase implements DeoptimizableE origin: DeoptimizableEntity ): 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_INSTANCEOF_PATH, - NODE_INTERACTION_UNKNOWN_ASSIGNMENT, - createHasEffectsContext() - ) - ) { - return UnknownValue; - } - this.expressionsDependingOnNegativeInstanceof.add(origin); - return false; - } const leftValue = this.left.getLiteralValueAtPath(EMPTY_PATH, recursionTracker, origin); if (typeof leftValue === 'symbol') return UnknownValue; @@ -127,7 +104,6 @@ export default class BinaryExpression extends NodeBase implements DeoptimizableE ) { return true; } - this.deoptimizeInstanceof(context); return super.hasEffects(context); } @@ -135,11 +111,6 @@ export default class BinaryExpression extends NodeBase implements DeoptimizableE return type !== INTERACTION_ACCESSED || path.length > 1; } - include(context: InclusionContext, includeChildrenRecursively: IncludeChildren) { - this.deoptimizeInstanceof(); - super.include(context, includeChildrenRecursively); - } - render( code: MagicString, options: RenderOptions, @@ -148,23 +119,4 @@ export default class BinaryExpression extends NodeBase implements DeoptimizableE this.left.render(code, options, { renderedSurroundingElement }); this.right.render(code, options); } - - private deoptimizeInstanceof(context?: HasEffectsContext) { - if ( - this.operator === 'instanceof' && - (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/shared/ClassNode.ts b/src/ast/nodes/shared/ClassNode.ts index 44c44629aab..06f54ee3db7 100644 --- a/src/ast/nodes/shared/ClassNode.ts +++ b/src/ast/nodes/shared/ClassNode.ts @@ -13,7 +13,6 @@ import { type ObjectPath, type PathTracker, SHARED_RECURSION_TRACKER, - TestInstanceof, UNKNOWN_PATH, UnknownKey } from '../../utils/PathTracker'; @@ -96,8 +95,6 @@ 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 1ec9efa35dc..a1517cb7e08 100644 --- a/src/ast/nodes/shared/FunctionBase.ts +++ b/src/ast/nodes/shared/FunctionBase.ts @@ -14,13 +14,7 @@ import { NodeInteractionWithThisArg } from '../../NodeInteractions'; import ReturnValueScope from '../../scopes/ReturnValueScope'; -import { - type ObjectPath, - PathTracker, - TestInstanceof, - UNKNOWN_PATH, - UnknownKey -} from '../../utils/PathTracker'; +import { type ObjectPath, PathTracker, UNKNOWN_PATH, UnknownKey } from '../../utils/PathTracker'; import BlockStatement from '../BlockStatement'; import * as NodeType from '../NodeType'; import RestElement from '../RestElement'; @@ -101,9 +95,6 @@ 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 aef1164b87d..efbc2026427 100644 --- a/src/ast/nodes/shared/ObjectEntity.ts +++ b/src/ast/nodes/shared/ObjectEntity.ts @@ -11,7 +11,6 @@ import { ObjectPath, ObjectPathKey, PathTracker, - TestInstanceof, UNKNOWN_INTEGER_PATH, UNKNOWN_PATH, UnknownInteger, @@ -282,7 +281,6 @@ export class ObjectEntity extends ExpressionEntity { context: HasEffectsContext ): boolean { const [key, ...subPath] = path; - 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 0c552d2242f..5b0679a6187 100644 --- a/src/ast/utils/PathTracker.ts +++ b/src/ast/utils/PathTracker.ts @@ -4,24 +4,15 @@ 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'); -/** - * A special key that does not actually reference a property but can be used to - * 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 TestInstanceof = Symbol('Test instanceof'); export type ObjectPathKey = | string | typeof UnknownKey | typeof UnknownNonAccessorKey - | typeof UnknownInteger - | typeof TestInstanceof; + | typeof UnknownInteger; export type ObjectPath = ObjectPathKey[]; export const EMPTY_PATH: ObjectPath = []; export const UNKNOWN_PATH: ObjectPath = [UnknownKey]; -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 @@ -34,7 +25,6 @@ const EntitiesKey = Symbol('Entities'); interface EntityPaths { [pathSegment: string]: EntityPaths; [EntitiesKey]: Set; - [TestInstanceof]?: EntityPaths; [UnknownInteger]?: EntityPaths; [UnknownKey]?: EntityPaths; [UnknownNonAccessorKey]?: EntityPaths; @@ -82,7 +72,6 @@ export const SHARED_RECURSION_TRACKER = new PathTracker(); interface DiscriminatedEntityPaths { [pathSegment: string]: DiscriminatedEntityPaths; [EntitiesKey]: Map>; - [TestInstanceof]?: DiscriminatedEntityPaths; [UnknownInteger]?: DiscriminatedEntityPaths; [UnknownKey]?: DiscriminatedEntityPaths; [UnknownNonAccessorKey]?: DiscriminatedEntityPaths; diff --git a/test/form/samples/instanceof/_config.js b/test/form/samples/instanceof/_config.js deleted file mode 100644 index 272579e8d05..00000000000 --- a/test/form/samples/instanceof/_config.js +++ /dev/null @@ -1,3 +0,0 @@ -module.exports = { - description: 'removes instanceof checks if the right side is not included' -}; diff --git a/test/form/samples/instanceof/_expected.js b/test/form/samples/instanceof/_expected.js deleted file mode 100644 index 291a9f7748c..00000000000 --- a/test/form/samples/instanceof/_expected.js +++ /dev/null @@ -1,15 +0,0 @@ -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 deleted file mode 100644 index 718d04ff40a..00000000000 --- a/test/form/samples/instanceof/main.js +++ /dev/null @@ -1,21 +0,0 @@ -class Unused1 {} - -if ({} instanceof Unused1) console.log('removed'); -else console.log('retained'); - -function Unused2() {} - -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/function/samples/instanceof/used-parameter/_config.js b/test/function/samples/instanceof/used-parameter/_config.js new file mode 100644 index 00000000000..1312a923985 --- /dev/null +++ b/test/function/samples/instanceof/used-parameter/_config.js @@ -0,0 +1,3 @@ +module.exports = { + description: 'retains instanceof for function parameters' +}; diff --git a/test/function/samples/instanceof/used-parameter/main.js b/test/function/samples/instanceof/used-parameter/main.js new file mode 100644 index 00000000000..61c8724c78d --- /dev/null +++ b/test/function/samples/instanceof/used-parameter/main.js @@ -0,0 +1,12 @@ +let effect = false; + +class Foo {} + +function checkInstance(instance) { + if (instance instanceof Foo) { + effect = true; + } +} + +checkInstance(new Foo()); +assert.ok(effect);