Handle recursive this mutation detection (#4025)

* Handle recursive this mutation detection * Improve coverage
rollup · Mar 28, 2021 · fb26847 · fb26847
1 parent dbf369f
commit fb26847
Show file tree

Hide file tree

Showing 12 changed files with 68 additions and 35 deletions.
diff --git a/src/ast/nodes/CallExpression.ts b/src/ast/nodes/CallExpression.ts
@@ -21,7 +21,13 @@ import Identifier from './Identifier';
 import MemberExpression from './MemberExpression';
 import * as NodeType from './NodeType';
 import { ExpressionEntity } from './shared/Expression';
-import { Annotation, ExpressionNode, IncludeChildren, INCLUDE_PARAMETERS, NodeBase } from './shared/Node';
+import {
+	Annotation,
+	ExpressionNode,
+	IncludeChildren,
+	INCLUDE_PARAMETERS,
+	NodeBase
+} from './shared/Node';
 import SpreadElement from './SpreadElement';
 import Super from './Super';
 
@@ -65,8 +71,11 @@ export default class CallExpression extends NodeBase implements DeoptimizableEnt
 		// ensure the returnExpression is set for the tree-shaking passes
 		this.getReturnExpression(SHARED_RECURSION_TRACKER);
 		// This deoptimizes "this" for non-namespace calls until we have a better solution
-		if (this.callee instanceof MemberExpression && !this.callee.variable &&
-		    this.callee.mayModifyThisWhenCalledAtPath([])) {
+		if (
+			this.callee instanceof MemberExpression &&
+			!this.callee.variable &&
+			this.callee.mayModifyThisWhenCalledAtPath([], SHARED_RECURSION_TRACKER)
+		) {
 			this.callee.object.deoptimizePath(UNKNOWN_PATH);
 		}
 		for (const argument of this.arguments) {

diff --git a/src/ast/nodes/Identifier.ts b/src/ast/nodes/Identifier.ts
@@ -141,10 +141,10 @@ export default class Identifier extends NodeBase implements PatternNode {
 		this.variable!.includeCallArguments(context, args);
 	}
 
-	mayModifyThisWhenCalledAtPath(
-		path: ObjectPath
-	) {
-		return this.variable ? this.variable.mayModifyThisWhenCalledAtPath(path) : true
+	mayModifyThisWhenCalledAtPath(path: ObjectPath, recursionTracker: PathTracker) {
+		return this.variable
+			? this.variable.mayModifyThisWhenCalledAtPath(path, recursionTracker)
+			: true;
 	}
 
 	render(

diff --git a/src/ast/nodes/MemberExpression.ts b/src/ast/nodes/MemberExpression.ts
@@ -233,13 +233,14 @@ export default class MemberExpression extends NodeBase implements DeoptimizableE
 		this.propertyKey = getResolvablePropertyKey(this);
 	}
 
-	mayModifyThisWhenCalledAtPath(
-		path: ObjectPath
-	) {
+	mayModifyThisWhenCalledAtPath(path: ObjectPath, recursionTracker: PathTracker) {
 		if (this.variable) {
-			return this.variable.mayModifyThisWhenCalledAtPath(path);
+			return this.variable.mayModifyThisWhenCalledAtPath(path, recursionTracker);
 		}
-		return this.object.mayModifyThisWhenCalledAtPath([this.propertyKey as ObjectPathKey].concat(path));
+		return this.object.mayModifyThisWhenCalledAtPath(
+			[this.propertyKey as ObjectPathKey].concat(path),
+			recursionTracker
+		);
 	}
 
 	render(

diff --git a/src/ast/nodes/ObjectExpression.ts b/src/ast/nodes/ObjectExpression.ts
@@ -256,14 +256,14 @@ export default class ObjectExpression extends NodeBase implements DeoptimizableE
 		return false;
 	}
 
-	mayModifyThisWhenCalledAtPath(
-		path: ObjectPath
-	) {
-		if (!path.length || typeof path[0] !== "string") {
+	mayModifyThisWhenCalledAtPath(path: ObjectPath, recursionTracker: PathTracker) {
+		if (!path.length || typeof path[0] !== 'string') {
 			return true;
 		}
 		const property = this.getPropertyMap()[path[0]]?.exactMatchRead;
-		return property ? property.value.mayModifyThisWhenCalledAtPath(path.slice(1)) : true;
+		return property
+			? property.value.mayModifyThisWhenCalledAtPath(path.slice(1), recursionTracker)
+			: true;
 	}
 
 	render(

diff --git a/src/ast/nodes/shared/Expression.ts b/src/ast/nodes/shared/Expression.ts
@@ -33,7 +33,5 @@ export interface ExpressionEntity extends WritableEntity {
 	): boolean;
 	include(context: InclusionContext, includeChildrenRecursively: IncludeChildren): void;
 	includeCallArguments(context: InclusionContext, args: (ExpressionNode | SpreadElement)[]): void;
-	mayModifyThisWhenCalledAtPath(
-		path: ObjectPath
-	): boolean;
+	mayModifyThisWhenCalledAtPath(path: ObjectPath, recursionTracker: PathTracker): boolean;
 }
diff --git a/src/ast/nodes/shared/MultiExpression.ts b/src/ast/nodes/shared/MultiExpression.ts
@@ -74,7 +74,7 @@ export class MultiExpression implements ExpressionEntity {
 
 	includeCallArguments(): void {}
 
-	mayModifyThisWhenCalledAtPath(path: ObjectPath) {
-		return this.expressions.some(e => e.mayModifyThisWhenCalledAtPath(path));
+	mayModifyThisWhenCalledAtPath(path: ObjectPath, recursionTracker: PathTracker) {
+		return this.expressions.some(e => e.mayModifyThisWhenCalledAtPath(path, recursionTracker));
 	}
 }
diff --git a/src/ast/nodes/shared/Node.ts b/src/ast/nodes/shared/Node.ts
@@ -26,7 +26,10 @@ export interface GenericEsTreeNode extends acorn.Node {
 
 export const INCLUDE_PARAMETERS: 'variables' = 'variables';
 export type IncludeChildren = boolean | typeof INCLUDE_PARAMETERS;
-export interface Annotation {comment?: acorn.Comment, pure?: boolean}
+export interface Annotation {
+	comment?: acorn.Comment;
+	pure?: boolean;
+}
 
 export interface Node extends Entity {
 	annotations?: Annotation[];
@@ -229,10 +232,8 @@ export class NodeBase implements ExpressionNode {
 		}
 	}
 
-	mayModifyThisWhenCalledAtPath(
-		_path: ObjectPath
-	) {
-		return true
+	mayModifyThisWhenCalledAtPath(_path: ObjectPath, _recursionTracker: PathTracker) {
+		return true;
 	}
 
 	parseNode(esTreeNode: GenericEsTreeNode) {

diff --git a/src/ast/variables/LocalVariable.ts b/src/ast/variables/LocalVariable.ts
@@ -189,12 +189,17 @@ export default class LocalVariable extends Variable {
 		this.calledFromTryStatement = true;
 	}
 
-	mayModifyThisWhenCalledAtPath(
-		path: ObjectPath
-	) {
+	mayModifyThisWhenCalledAtPath(path: ObjectPath, recursionTracker: PathTracker) {
 		if (this.isReassigned || !this.init || path.length > MAX_PATH_DEPTH) {
 			return true;
 		}
-		return this.init.mayModifyThisWhenCalledAtPath(path);
+		const trackedEntities = recursionTracker.getEntities(path);
+		if (trackedEntities.has(this.init)) {
+			return true;
+		}
+		trackedEntities.add(this.init);
+		const result = this.init.mayModifyThisWhenCalledAtPath(path, recursionTracker);
+		trackedEntities.delete(this.init);
+		return result;
 	}
 }
diff --git a/src/ast/variables/Variable.ts b/src/ast/variables/Variable.ts
@@ -96,10 +96,8 @@ export default class Variable implements ExpressionEntity {
 
 	markCalledFromTryStatement() {}
 
-	mayModifyThisWhenCalledAtPath(
-		_path: ObjectPath
-	) {
-		return true
+	mayModifyThisWhenCalledAtPath(_path: ObjectPath, _recursionTracker: PathTracker) {
+		return true;
 	}
 
 	setRenderNames(baseName: string | null, name: string | null) {

diff --git a/test/form/samples/recursive-property-call/_config.js b/test/form/samples/recursive-property-call/_config.js
@@ -0,0 +1,3 @@
+module.exports = {
+	description: 'handles calls to properties of recursively defined variables'
+};
diff --git a/test/form/samples/recursive-property-call/_expected.js b/test/form/samples/recursive-property-call/_expected.js
@@ -0,0 +1,9 @@
+var obj1 = obj1;
+console.log(obj1.foo());
+
+var obj2 = {foo: () => {}};
+var obj2 = {foo: log};
+obj2.foo();
+
+var obj3 = {obj: obj3};
+obj3.obj.obj.obj.obj.obj.obj.obj.obj.obj.foo();
diff --git a/test/form/samples/recursive-property-call/main.js b/test/form/samples/recursive-property-call/main.js
@@ -0,0 +1,9 @@
+var obj1 = obj1;
+console.log(obj1.foo());
+
+var obj2 = {foo: () => {}};
+var obj2 = {foo: log};
+obj2.foo();
+
+var obj3 = {obj: obj3}
+obj3.obj.obj.obj.obj.obj.obj.obj.obj.obj.foo()