diff --git a/src/com/google/javascript/jscomp/CollapseProperties.java b/src/com/google/javascript/jscomp/CollapseProperties.java index 99d006884e4..dec3ec4af1f 100644 --- a/src/com/google/javascript/jscomp/CollapseProperties.java +++ b/src/com/google/javascript/jscomp/CollapseProperties.java @@ -856,8 +856,7 @@ private void declareVariablesForObjLitValues( // for the same global name.) if (isJsIdentifier && p != null) { if (!discardKeys) { - Ref newAlias = p.getDeclaration().cloneAndReclassify(Ref.Type.ALIASING_GET, refNode); - p.addRef(newAlias); + p.addAliasingGetClonedFromDeclaration(refNode); } p.updateRefNode(p.getDeclaration(), nameNode); diff --git a/src/com/google/javascript/jscomp/GlobalNamespace.java b/src/com/google/javascript/jscomp/GlobalNamespace.java index c2659fd8a5d..5632bce804f 100644 --- a/src/com/google/javascript/jscomp/GlobalNamespace.java +++ b/src/com/google/javascript/jscomp/GlobalNamespace.java @@ -687,19 +687,16 @@ void handleSetFromGlobal( nameObj.isModuleProp = true; } - Ref set = new Ref(module, scope, n, nameObj, Ref.Type.SET_FROM_GLOBAL, - currentPreOrderIndex++); - nameObj.addRef(set); - if (isNestedAssign(parent)) { // This assignment is both a set and a get that creates an alias. - Ref get = new Ref(module, scope, n, nameObj, Ref.Type.ALIASING_GET, - currentPreOrderIndex++); - nameObj.addRef(get); - Ref.markTwins(set, get); - } else if (isTypeDeclaration(n)) { - // Names with a @constructor or @enum annotation are always collapsed - nameObj.setDeclaredType(); + nameObj.addTwinRefs(module, scope, n, Ref.Type.SET_FROM_GLOBAL, currentPreOrderIndex); + currentPreOrderIndex += 2; // addTwinRefs uses 2 index values + } else { + nameObj.addSingleRef(module, scope, n, Ref.Type.SET_FROM_GLOBAL, currentPreOrderIndex++); + if (isTypeDeclaration(n)) { + // Names with a @constructor or @enum annotation are always collapsed + nameObj.setDeclaredType(); + } } } @@ -744,19 +741,16 @@ void handleSetFromLocal(JSModule module, Scope scope, Node n, Node parent, Strin } Name nameObj = getOrCreateName(name); - Ref set = new Ref(module, scope, n, nameObj, - Ref.Type.SET_FROM_LOCAL, currentPreOrderIndex++); - nameObj.addRef(set); if (n.getBooleanProp(Node.MODULE_EXPORT)) { nameObj.isModuleProp = true; } if (isNestedAssign(parent)) { // This assignment is both a set and a get that creates an alias. - Ref get = new Ref(module, scope, n, nameObj, - Ref.Type.ALIASING_GET, currentPreOrderIndex++); - nameObj.addRef(get); - Ref.markTwins(set, get); + nameObj.addTwinRefs(module, scope, n, Ref.Type.SET_FROM_LOCAL, currentPreOrderIndex); + currentPreOrderIndex += 2; // addTwinRefs uses 2 index values + } else { + nameObj.addSingleRef(module, scope, n, Ref.Type.SET_FROM_LOCAL, currentPreOrderIndex++); } } @@ -859,7 +853,7 @@ void handleGet(JSModule module, Scope scope, Node n, Node parent, String name, R Name nameObj = getOrCreateName(name); // No need to look up additional ancestors, since they won't be used. - nameObj.addRef(new Ref(module, scope, n, nameObj, type, currentPreOrderIndex++)); + nameObj.addSingleRef(module, scope, n, type, currentPreOrderIndex++); } private boolean isClassDefiningCall(Node callNode) { @@ -1212,7 +1206,72 @@ public StaticTypedScope getScope() { throw new UnsupportedOperationException(); } - void addRef(Ref ref) { + /** + * Add a pair of Refs for the same Node. + * + *

This covers cases like `var a = b = 0`. The 'b' node needs a ALIASING_GET reference and a + * SET_FROM_GLOBAL or SET_FROM_LOCAL reference. + * + * @param module + * @param scope + * @param node + * @param setType either SET_FROM_LOCAL or SET_FROM_GLOBAL + * @param setRefPreOrderIndex used for setter Ref, getter ref will be this value + 1 + */ + private void addTwinRefs( + JSModule module, Scope scope, Node node, Ref.Type setType, int setRefPreOrderIndex) { + checkArgument( + setType == Ref.Type.SET_FROM_GLOBAL || setType == Ref.Type.SET_FROM_LOCAL, setType); + Ref setRef = createNewRef(module, scope, node, setType, setRefPreOrderIndex); + Ref getRef = + createNewRef(module, scope, node, Ref.Type.ALIASING_GET, setRefPreOrderIndex + 1); + setRef.twin = getRef; + getRef.twin = setRef; + addRef(setRef); + addRef(getRef); + } + + private void addSingleRef( + JSModule module, Scope scope, Node node, Ref.Type type, int preOrderIndex) { + addRef(createNewRef(module, scope, node, type, preOrderIndex)); + } + + private Ref createNewRef( + JSModule module, Scope scope, Node node, Ref.Type type, int preOrderIndex) { + return new Ref( + module, // null if the compilation isn't using JSModules + checkNotNull(scope), + checkNotNull(node), // may be null later, but not on creation + this, + type, + preOrderIndex); + } + + Ref addSingleRefForTesting(Ref.Type type, int preOrderIndex) { + Ref ref = + new Ref( + /* module= */ null, /* scope= */ null, /* node = */ null, this, type, preOrderIndex); + addRef(ref); + return ref; + } + + /** + * Add an ALIASING_GET Ref for the given Node using the same Ref properties as the declaration + * Ref, which must exist. + * + *

Only for use by CollapseProperties. + * + * @param newRefNode newly added AST node that refers to this Name and appears in the same + * module and scope as the Ref that declares this Name + */ + void addAliasingGetClonedFromDeclaration(Node newRefNode) { + // TODO(bradfordcsmith): It would be good to add checks that the scope and module are correct. + Ref declRef = checkNotNull(declaration); + addSingleRef( + declRef.module, declRef.scope, newRefNode, Ref.Type.ALIASING_GET, declRef.preOrderIndex); + } + + private void addRef(Ref ref) { addRefInternal(ref); switch (ref.type) { case SET_FROM_GLOBAL: @@ -1872,9 +1931,13 @@ enum Type { private Ref twin = null; /** - * Creates a reference at the current node. + * Creates a Ref + * + *

No parameter checking is done here, because we allow nulls for several fields in Refs + * created just for testing. However, all Refs for real use must be created by methods on the + * Name class, which does do argument checking. */ - Ref(JSModule module, Scope scope, Node node, Name name, Type type, int index) { + private Ref(JSModule module, Scope scope, Node node, Name name, Type type, int index) { this.node = node; this.name = name; this.module = module; @@ -1883,23 +1946,6 @@ enum Type { this.preOrderIndex = index; } - private Ref(Ref original, Type type, int index, Node refNode) { - this.node = refNode; - this.name = original.name; - this.module = original.module; - this.type = type; - this.scope = original.scope; - this.preOrderIndex = index; - } - - private Ref(Type type, int index) { - this.type = type; - this.module = null; - this.scope = null; - this.name = null; - this.preOrderIndex = index; - } - @Override public Node getNode() { return node; @@ -1928,30 +1974,6 @@ boolean isSet() { return type == Type.SET_FROM_GLOBAL || type == Type.SET_FROM_LOCAL; } - static void markTwins(Ref a, Ref b) { - checkArgument( - (a.type == Type.ALIASING_GET || b.type == Type.ALIASING_GET) - && (a.type == Type.SET_FROM_GLOBAL - || a.type == Type.SET_FROM_LOCAL - || b.type == Type.SET_FROM_GLOBAL - || b.type == Type.SET_FROM_LOCAL)); - a.twin = b; - b.twin = a; - } - - /** - * Create a new ref that is the same as this one, but of a different class and for a different - * AST node. - */ - Ref cloneAndReclassify(Type type, Node refNode) { - Ref clone = new Ref(this, type, this.preOrderIndex, refNode); - return clone; - } - - static Ref createRefForTesting(Type type) { - return new Ref(type, -1); - } - @Override public String toString() { return node.toString(); diff --git a/test/com/google/javascript/jscomp/GlobalNamespaceTest.java b/test/com/google/javascript/jscomp/GlobalNamespaceTest.java index 7e7e4c8aa28..0da8bb49717 100644 --- a/test/com/google/javascript/jscomp/GlobalNamespaceTest.java +++ b/test/com/google/javascript/jscomp/GlobalNamespaceTest.java @@ -33,6 +33,7 @@ import com.google.javascript.rhino.JSTypeExpression; import com.google.javascript.rhino.Node; import com.google.javascript.rhino.jstype.JSType; +import java.util.List; import javax.annotation.Nullable; import org.junit.Test; import org.junit.runner.RunWith; @@ -49,41 +50,41 @@ public final class GlobalNamespaceTest { @Nullable private Compiler lastCompiler = null; @Test - public void testRemoveDeclaration1() { + public void firstGlobalAssignmentIsConsideredDeclaration() { Name n = Name.createForTesting("a"); - Ref set1 = createNodelessRef(Ref.Type.SET_FROM_GLOBAL); - Ref set2 = createNodelessRef(Ref.Type.SET_FROM_GLOBAL); + Ref set1 = n.addSingleRefForTesting(Ref.Type.SET_FROM_GLOBAL, 0); + Ref set2 = n.addSingleRefForTesting(Ref.Type.SET_FROM_GLOBAL, 1); - n.addRef(set1); - n.addRef(set2); + List refs = n.getRefs(); + assertThat(refs).containsExactly(set1, set2); assertThat(n.getDeclaration()).isEqualTo(set1); assertThat(n.getGlobalSets()).isEqualTo(2); - assertThat(n.getRefs()).hasSize(2); n.removeRef(set1); + // declaration moves to next global assignment when first is removed assertThat(n.getDeclaration()).isEqualTo(set2); assertThat(n.getGlobalSets()).isEqualTo(1); - assertThat(n.getRefs()).hasSize(1); + assertThat(n.getRefs()).containsExactly(set2); } @Test - public void testRemoveDeclaration2() { + public void localAssignmentWillNotBeConsideredADeclaration() { Name n = Name.createForTesting("a"); - Ref set1 = createNodelessRef(Ref.Type.SET_FROM_GLOBAL); - Ref set2 = createNodelessRef(Ref.Type.SET_FROM_LOCAL); + Ref set1 = n.addSingleRefForTesting(Ref.Type.SET_FROM_GLOBAL, 0); + Ref localSet = n.addSingleRefForTesting(Ref.Type.SET_FROM_LOCAL, 1); - n.addRef(set1); - n.addRef(set2); + List refs = n.getRefs(); + assertThat(refs).containsExactly(set1, localSet); assertThat(n.getDeclaration()).isEqualTo(set1); assertThat(n.getGlobalSets()).isEqualTo(1); assertThat(n.getLocalSets()).isEqualTo(1); - assertThat(n.getRefs()).hasSize(2); n.removeRef(set1); + // local set will not be used as the declaration assertThat(n.getDeclaration()).isNull(); assertThat(n.getGlobalSets()).isEqualTo(0); } @@ -421,8 +422,4 @@ private GlobalNamespace parse(String js) { return new GlobalNamespace(compiler, compiler.getRoot()); } - - private Ref createNodelessRef(Ref.Type type) { - return Ref.createRefForTesting(type); - } }