fix: add this option to context.ignore (#4842)

TrickyPi · lukastaegert · web-flow · commit 5d81532f6883 · 2023-02-10T06:10:04.000+01:00
* feat: add this option to context.ignore

* Track this deoptimizations

* test: tweak test description

---------

Co-authored-by: Lukas Taegert-Atkinson &lt;lukas.taegert-atkinson@tngtech.com&gt;
diff --git a/src/ast/ExecutionContext.ts b/src/ast/ExecutionContext.ts
@@ -8,6 +8,7 @@ interface ExecutionContextIgnore {
 	continues: boolean;
 	labels: Set<string>;
 	returnYield: boolean;
+	this: boolean;
 }
 
 export const BROKEN_FLOW_NONE = 0;
@@ -51,7 +52,8 @@ export function createHasEffectsContext(): HasEffectsContext {
 			breaks: false,
 			continues: false,
 			labels: new Set(),
-			returnYield: false
+			returnYield: false,
+			this: false
 		},
 		includedLabels: new Set(),
 		instantiated: new DiscriminatedPathTracker(),
diff --git a/src/ast/nodes/ArrowFunctionExpression.ts b/src/ast/nodes/ArrowFunctionExpression.ts
@@ -43,7 +43,8 @@ export default class ArrowFunctionExpression extends FunctionBase {
 				breaks: false,
 				continues: false,
 				labels: new Set(),
-				returnYield: true
+				returnYield: true,
+				this: false
 			};
 			if (this.body.hasEffects(context)) return true;
 			context.ignore = ignore;
diff --git a/src/ast/nodes/shared/FunctionNode.ts b/src/ast/nodes/shared/FunctionNode.ts
@@ -20,9 +20,14 @@ export default class FunctionNode extends FunctionBase {
 	declare preventChildBlockScope: true;
 	declare scope: FunctionScope;
 	protected objectEntity: ObjectEntity | null = null;
+	private declare constructedEntity: ObjectEntity;
 
 	createScope(parentScope: FunctionScope): void {
 		this.scope = new FunctionScope(parentScope, this.context);
+		this.constructedEntity = new ObjectEntity(Object.create(null), OBJECT_PROTOTYPE);
+		// This makes sure that all deoptimizations of "this" are applied to the
+		// constructed entity.
+		this.scope.thisVariable.addEntityToBeDeoptimized(this.constructedEntity);
 	}
 
 	deoptimizeThisOnInteractionAtPath(
@@ -51,16 +56,15 @@ export default class FunctionNode extends FunctionBase {
 			const thisInit = context.replacedVariableInits.get(this.scope.thisVariable);
 			context.replacedVariableInits.set(
 				this.scope.thisVariable,
-				interaction.withNew
-					? new ObjectEntity(Object.create(null), OBJECT_PROTOTYPE)
-					: UNKNOWN_EXPRESSION
+				interaction.withNew ? this.constructedEntity : UNKNOWN_EXPRESSION
 			);
 			const { brokenFlow, ignore, replacedVariableInits } = context;
 			context.ignore = {
 				breaks: false,
 				continues: false,
 				labels: new Set(),
-				returnYield: true
+				returnYield: true,
+				this: interaction.withNew
 			};
 			if (this.body.hasEffects(context)) return true;
 			context.brokenFlow = brokenFlow;
diff --git a/src/ast/variables/ThisVariable.ts b/src/ast/variables/ThisVariable.ts
@@ -74,13 +74,15 @@ export default class ThisVariable extends LocalVariable {
 		interaction: NodeInteraction,
 		context: HasEffectsContext
 	): boolean {
-		return (
-			this.getInit(context).hasEffectsOnInteractionAtPath(path, interaction, context) ||
-			super.hasEffectsOnInteractionAtPath(path, interaction, context)
-		);
-	}
-
-	private getInit(context: HasEffectsContext): ExpressionEntity {
-		return context.replacedVariableInits.get(this) || UNKNOWN_EXPRESSION;
+		const replacedVariableInit = context.replacedVariableInits.get(this);
+		if (replacedVariableInit) {
+			return (
+				replacedVariableInit.hasEffectsOnInteractionAtPath(path, interaction, context) ||
+				// If the surrounding function is included, all mutations of "this" must
+				// be counted as side effects, which is what this second line does.
+				(!context.ignore.this && super.hasEffectsOnInteractionAtPath(path, interaction, context))
+			);
+		}
+		return UNKNOWN_EXPRESSION.hasEffectsOnInteractionAtPath(path, interaction, context);
 	}
 }
diff --git a/test/form/samples/side-effects-this/_config.js b/test/form/samples/side-effects-this/_config.js
@@ -0,0 +1,4 @@
+module.exports = {
+	description:
+		'When a constructor is called with the new keyword, changing its this has no side effects'
+};
diff --git a/test/form/samples/side-effects-this/_expected.js b/test/form/samples/side-effects-this/_expected.js
@@ -0,0 +1,13 @@
+class Main {
+	constructor() {
+		this.a = 1;
+	}
+}
+const b = new Main();
+assert.ok(b);
+
+function Foo() {
+	this.a = 1;
+}
+const d = new Foo();
+assert.ok(d);
diff --git a/test/form/samples/side-effects-this/main.js b/test/form/samples/side-effects-this/main.js
@@ -0,0 +1,16 @@
+class Main {
+	constructor() {
+		this.a = 1;
+	}
+}
+const a = new Main(); // removed
+const b = new Main();
+assert.ok(b);
+
+function Foo() {
+	this.a = 1;
+}
+
+const c = new Foo(); // removed
+const d = new Foo();
+assert.ok(d);
diff --git a/test/function/samples/this-side-effect/_config.js b/test/function/samples/this-side-effect/_config.js
@@ -0,0 +1,3 @@
+module.exports = {
+	description: 'detects side effects when mutating "this" in a constructor'
+};
diff --git a/test/function/samples/this-side-effect/_expected.js b/test/function/samples/this-side-effect/_expected.js
@@ -0,0 +1,7 @@
+class Main {
+	constructor() {
+		this.a = 1;
+	}
+}
+const b = new Main();
+assert.ok(b);
diff --git a/test/function/samples/this-side-effect/main.js b/test/function/samples/this-side-effect/main.js
@@ -0,0 +1,15 @@
+let effect = false;
+
+class Foo {
+	constructor() {
+		Object.defineProperty(this, 'x', {
+			set() {
+				effect = true;
+			}
+		});
+		this.x = 2;
+	}
+}
+
+new Foo();
+assert.ok(effect);

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+module.exports = {`
	`2`	`+ description: 'detects side effects when mutating "this" in a constructor'`
	`3`	`+};`