Skip to content

Commit

Permalink
* Resolve #1710
Browse files Browse the repository at this point in the history
* When simplifying object shapes due to unknown assignments, make sure
  the same assignment is not added repeatedly
* Reintroduce UNDEFINED_ASSIGNMENT which behaves like UNKNOWN_ASSIGNMENT
  but does not simplify object shapes. This solves a problem with
  assignments to exports. This might change again in the future.
* Improve object shape efficiency by assigning associated variables
  instead of AST nodes to object shapes if possible and prevent
  assignment of variables to themselves.
  • Loading branch information
lukastaegert committed Nov 10, 2017
1 parent 6d0e492 commit bc928fc
Show file tree
Hide file tree
Showing 11 changed files with 54 additions and 13 deletions.
4 changes: 2 additions & 2 deletions src/ast/nodes/BlockStatement.js
@@ -1,12 +1,12 @@
import Statement from './shared/Statement.js';
import BlockScope from '../scopes/BlockScope';
import { UNKNOWN_ASSIGNMENT } from '../values';
import { UNDEFINED_ASSIGNMENT } from '../values';

export default class BlockStatement extends Statement {
bindImplicitReturnExpressionToScope () {
const lastStatement = this.body[ this.body.length - 1 ];
if ( !lastStatement || lastStatement.type !== 'ReturnStatement' ) {
this.scope.addReturnExpression( UNKNOWN_ASSIGNMENT );
this.scope.addReturnExpression( UNDEFINED_ASSIGNMENT );
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/ast/nodes/ReturnStatement.js
@@ -1,5 +1,5 @@
import Statement from './shared/Statement.js';
import {UNKNOWN_ASSIGNMENT} from '../values';
import { UNDEFINED_ASSIGNMENT } from '../values';

export default class ReturnStatement extends Statement {
hasEffects ( options ) {
Expand All @@ -8,6 +8,6 @@ export default class ReturnStatement extends Statement {
}

initialiseNode () {
this.scope.addReturnExpression( this.argument || UNKNOWN_ASSIGNMENT );
this.scope.addReturnExpression( this.argument || UNDEFINED_ASSIGNMENT );
}
}
4 changes: 2 additions & 2 deletions src/ast/nodes/UnaryExpression.js
@@ -1,5 +1,5 @@
import Node from '../Node.js';
import { UNKNOWN_ASSIGNMENT, UNKNOWN_VALUE } from '../values';
import { UNDEFINED_ASSIGNMENT, UNKNOWN_VALUE } from '../values';
import ExecutionPathOptions from '../ExecutionPathOptions';

const operators = {
Expand All @@ -15,7 +15,7 @@ const operators = {
export default class UnaryExpression extends Node {
bindNode () {
if ( this.operator === 'delete' ) {
this.argument.bindAssignmentAtPath( [], UNKNOWN_ASSIGNMENT, ExecutionPathOptions.create() );
this.argument.bindAssignmentAtPath( [], UNDEFINED_ASSIGNMENT, ExecutionPathOptions.create() );
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/ast/scopes/ModuleScope.js
Expand Up @@ -2,7 +2,7 @@ import { forOwn } from '../../utils/object.js';
import relativeId from '../../utils/relativeId.js';
import Scope from './Scope.js';
import LocalVariable from '../variables/LocalVariable';
import { UNKNOWN_ASSIGNMENT } from '../values';
import { UNDEFINED_ASSIGNMENT } from '../values';

export default class ModuleScope extends Scope {
constructor ( module ) {
Expand All @@ -12,7 +12,7 @@ export default class ModuleScope extends Scope {
} );

this.module = module;
this.variables.this = new LocalVariable( 'this', null, UNKNOWN_ASSIGNMENT );
this.variables.this = new LocalVariable( 'this', null, UNDEFINED_ASSIGNMENT );
}

deshadow ( names ) {
Expand Down
4 changes: 2 additions & 2 deletions src/ast/scopes/Scope.js
@@ -1,7 +1,7 @@
import { blank, keys } from '../../utils/object.js';
import LocalVariable from '../variables/LocalVariable';
import ExportDefaultVariable from '../variables/ExportDefaultVariable';
import { UNKNOWN_ASSIGNMENT } from '../values';
import { UNDEFINED_ASSIGNMENT } from '../values';

export default class Scope {
constructor ( options = {} ) {
Expand All @@ -28,7 +28,7 @@ export default class Scope {
variable.addDeclaration( identifier );
options.init && variable.bindAssignmentAtPath( [], options.init );
} else {
this.variables[ name ] = new LocalVariable( identifier.name, identifier, options.init || UNKNOWN_ASSIGNMENT );
this.variables[ name ] = new LocalVariable( identifier.name, identifier, options.init || UNDEFINED_ASSIGNMENT );
}
return this.variables[ name ];
}
Expand Down
11 changes: 11 additions & 0 deletions src/ast/values.js
Expand Up @@ -10,3 +10,14 @@ export const UNKNOWN_ASSIGNMENT = {
someReturnExpressionWhenCalledAtPath: () => true,
toString: () => '[[UNKNOWN]]'
};

export const UNDEFINED_ASSIGNMENT = {
type: 'UNDEFINED',
bindAssignmentAtPath: () => {},
forEachReturnExpressionWhenCalledAtPath: () => {},
hasEffectsWhenAccessedAtPath: path => path.length > 0,
hasEffectsWhenAssignedAtPath: path => path.length > 0,
hasEffectsWhenCalledAtPath: () => true,
someReturnExpressionWhenCalledAtPath: () => true,
toString: () => '[[UNDEFINED]]'
};
4 changes: 2 additions & 2 deletions src/ast/variables/ArgumentsVariable.js
@@ -1,9 +1,9 @@
import LocalVariable from './LocalVariable';
import { UNKNOWN_ASSIGNMENT } from '../values';
import { UNDEFINED_ASSIGNMENT, UNKNOWN_ASSIGNMENT } from '../values';

const getParameterVariable = ( path, options ) =>
(path[ 0 ] < options.getArgumentsVariables().length && options.getArgumentsVariables()[ path[ 0 ] ] )
|| UNKNOWN_ASSIGNMENT;
|| UNDEFINED_ASSIGNMENT;

export default class ArgumentsVariable extends LocalVariable {
constructor ( parameters ) {
Expand Down
5 changes: 4 additions & 1 deletion src/ast/variables/LocalVariable.js
Expand Up @@ -19,7 +19,10 @@ export default class LocalVariable extends Variable {
}

bindAssignmentAtPath ( path, expression, options ) {
if ( path.length > MAX_PATH_LENGTH || this.boundExpressions.hasAtPath( path, expression ) ) return;
if ( expression.variable ) {
expression = expression.variable;
}
if ( path.length > MAX_PATH_LENGTH || expression === this || this.boundExpressions.hasAtPath( path, expression ) ) return;
this.boundExpressions.addAtPath( path, expression );
this.boundExpressions.forEachAssignedToPath( path, ( subPath, node ) => {
subPath.length > 0
Expand Down
8 changes: 8 additions & 0 deletions src/ast/variables/VariableShapeTracker.js
Expand Up @@ -67,6 +67,7 @@ export default class VariableShapeTracker {
}

hasAtPath ( path, assignment ) {
if ( this._assignments === UNKNOWN_ASSIGNMENTS ) return true;
if ( path.length === 0 ) {
return this._assignments.get( SET_KEY ).has( assignment );
} else {
Expand Down Expand Up @@ -100,4 +101,11 @@ export default class VariableShapeTracker {
)
);
}

// For debugging purposes
toString ( pathString = '' ) {
return Array.from( this._assignments ).map( ( [ subPath, subAssignment ] ) => subPath === SET_KEY
? Array.from( subAssignment ).map( assignment => pathString + assignment.toString() ).join( '\n' )
: subAssignment.toString( pathString + subPath + ': ' ) ).join( '\n' );
}
}
@@ -0,0 +1,3 @@
module.exports = {
description: 'Avoid maximum call stack error with recursive parameter assignments (#1710).'
};
16 changes: 16 additions & 0 deletions test/function/samples/recursive-parameter-assignments/main.js
@@ -0,0 +1,16 @@
class Test {
constructor ( name, opts ) {
opts = opts || {};

if ( name && typeof name === 'object' ) {
opts = name;
name = opts.name;
delete opts.name;
}

this.name = name;
this.opts = opts;
}
}

new Test( 'a', {} );

0 comments on commit bc928fc

Please sign in to comment.