Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle recursive this mutation detection #4025

Merged
merged 2 commits into from Mar 28, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 12 additions & 3 deletions src/ast/nodes/CallExpression.ts
Expand Up @@ -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';

Expand Down Expand Up @@ -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) {
Expand Down
8 changes: 4 additions & 4 deletions src/ast/nodes/Identifier.ts
Expand Up @@ -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(
Expand Down
11 changes: 6 additions & 5 deletions src/ast/nodes/MemberExpression.ts
Expand Up @@ -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(
Expand Down
10 changes: 5 additions & 5 deletions src/ast/nodes/ObjectExpression.ts
Expand Up @@ -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(
Expand Down
4 changes: 1 addition & 3 deletions src/ast/nodes/shared/Expression.ts
Expand Up @@ -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;
}
4 changes: 2 additions & 2 deletions src/ast/nodes/shared/MultiExpression.ts
Expand Up @@ -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));
}
}
11 changes: 6 additions & 5 deletions src/ast/nodes/shared/Node.ts
Expand Up @@ -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[];
Expand Down Expand Up @@ -229,10 +232,8 @@ export class NodeBase implements ExpressionNode {
}
}

mayModifyThisWhenCalledAtPath(
_path: ObjectPath
) {
return true
mayModifyThisWhenCalledAtPath(_path: ObjectPath, _recursionTracker: PathTracker) {
return true;
}

parseNode(esTreeNode: GenericEsTreeNode) {
Expand Down
13 changes: 9 additions & 4 deletions src/ast/variables/LocalVariable.ts
Expand Up @@ -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;
}
}
6 changes: 2 additions & 4 deletions src/ast/variables/Variable.ts
Expand Up @@ -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) {
Expand Down
3 changes: 3 additions & 0 deletions test/form/samples/recursive-property-call/_config.js
@@ -0,0 +1,3 @@
module.exports = {
description: 'handles calls to properties of recursively defined variables'
};
9 changes: 9 additions & 0 deletions 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();
9 changes: 9 additions & 0 deletions 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()