From adc0b3be92e9f8a1b510926feba6bb357c64deb2 Mon Sep 17 00:00:00 2001 From: bradfordcsmith Date: Mon, 11 Feb 2019 11:45:01 -0800 Subject: [PATCH] BuildGlobalNamespace will not create duplicate Refs Previously it was up to the caller of scanNewNodes() to avoid passing BuildGlobalNamespace nodes that might already have references. This has led to exceptions being thrown because multiple references were created for the same Node. This change makes BuildGlobalNamespace and the Name class it depends on smart enough to avoid creation of additional Refs for the same Node even when scanNewNodes() is passed AST nodes that already have Refs. Effectively this means that code modifying the AST can safely pass any Nodes they think may need reconsideration to the scanNewNodes() method and rely on BuildGlobalNamespace to do the right thing. On the other hand, I've added several checks to restrict changing a Ref's Node. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=233448314 --- .../jscomp/AggressiveInlineAliases.java | 4 +- .../javascript/jscomp/GlobalNamespace.java | 439 ++++++++++++------ .../jscomp/GlobalNamespaceTest.java | 243 ++++++++++ 3 files changed, 545 insertions(+), 141 deletions(-) diff --git a/src/com/google/javascript/jscomp/AggressiveInlineAliases.java b/src/com/google/javascript/jscomp/AggressiveInlineAliases.java index d3346cec2de..2678f03481f 100644 --- a/src/com/google/javascript/jscomp/AggressiveInlineAliases.java +++ b/src/com/google/javascript/jscomp/AggressiveInlineAliases.java @@ -565,8 +565,8 @@ private void inlineGlobalAliasIfPossible(Name name, Ref alias, GlobalNamespace n checkState(aliasParent.isAssign(), aliasParent); Node aliasGrandparent = aliasParent.getParent(); aliasParent.replaceWith(alias.getNode().detach()); - aliasingName.removeRef(aliasDeclaration); - aliasingName.removeRef(aliasDeclaration.getTwin()); + // remove both of the refs + aliasingName.removeTwinRefs(aliasDeclaration); newNodes.add(new AstChange(alias.module, alias.scope, alias.getNode())); compiler.reportChangeToEnclosingScope(aliasGrandparent); } else { diff --git a/src/com/google/javascript/jscomp/GlobalNamespace.java b/src/com/google/javascript/jscomp/GlobalNamespace.java index 5d496c82bdc..4ca92ad4678 100644 --- a/src/com/google/javascript/jscomp/GlobalNamespace.java +++ b/src/com/google/javascript/jscomp/GlobalNamespace.java @@ -20,6 +20,7 @@ import static com.google.common.base.Preconditions.checkState; import static com.google.javascript.rhino.jstype.JSTypeNative.GLOBAL_THIS; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Joiner; import com.google.common.base.MoreObjects; import com.google.common.collect.ImmutableList; @@ -67,20 +68,19 @@ enum SourceKind { CODE; static SourceKind fromScriptNode(Node n) { - if (!n.isFromExterns()) { - return CODE; - } else if (NodeUtil.isFromTypeSummary(n)) { - return TYPE_SUMMARY; - } else { - return EXTERN; - } + if (!n.isFromExterns()) { + return CODE; + } else if (NodeUtil.isFromTypeSummary(n)) { + return TYPE_SUMMARY; + } else { + return EXTERN; + } } } /** - * Each reference has an index in post-order. - * Notice that some nodes are represented by 2 Ref objects, so - * this index is not necessarily unique. + * Each reference has an index in post-order. Notice that some nodes are represented by 2 Ref + * objects, so this index is not necessarily unique. */ private int currentPreOrderIndex = 0; @@ -104,11 +104,10 @@ static SourceKind fromScriptNode(Node n) { * Creates an instance that may emit warnings when building the namespace. * * @param compiler The AbstractCompiler, for reporting code changes - * @param externsRoot The root of the externs to build a namespace for. If - * this is null, externs and properties defined on extern types will not - * be included in the global namespace. If non-null, it allows - * user-defined function on extern types to be included in the global - * namespace. E.g. String.foo. + * @param externsRoot The root of the externs to build a namespace for. If this is null, externs + * and properties defined on extern types will not be included in the global namespace. If + * non-null, it allows user-defined function on extern types to be included in the global + * namespace. E.g. String.foo. * @param root The root of the rest of the code to build a namespace for. */ GlobalNamespace(AbstractCompiler compiler, Node externsRoot, Node root) { @@ -171,8 +170,8 @@ private void ensureGenerated() { } /** - * Gets a list of the roots of the forest of the global names, where the - * roots are the top-level names. + * Gets a list of the roots of the forest of the global names, where the roots are the top-level + * names. */ List getNameForest() { ensureGenerated(); @@ -180,8 +179,8 @@ List getNameForest() { } /** - * Gets an index of all the global names, indexed by full qualified name - * (as in "a", "a.b.c", etc.). + * Gets an index of all the global names, indexed by full qualified name (as in "a", "a.b.c", + * etc.). */ Map getNameIndex() { ensureGenerated(); @@ -189,8 +188,8 @@ Map getNameIndex() { } /** - * A simple data class that contains the information necessary to inspect - * a node for changes to the global namespace. + * A simple data class that contains the information necessary to inspect a node for changes to + * the global namespace. */ static class AstChange { final JSModule module; @@ -221,8 +220,9 @@ public int hashCode() { } /** - * If the client adds new nodes to the AST, scan these new nodes - * to see if they've added any references to the global namespace. + * If the client adds new nodes to the AST, scan these new nodes to see if they've added any + * references to the global namespace. + * * @param newNodes New nodes to check. */ void scanNewNodes(Set newNodes) { @@ -257,9 +257,7 @@ private void scanFromNode(BuildGlobalNamespace builder, JSModule module, Scope s builder.collect(module, scope, n); } - /** - * Builds the namespace lazily. - */ + /** Builds the namespace lazily. */ private void process() { if (hasExternsRoot()) { sourceKind = SourceKind.EXTERN; @@ -273,8 +271,7 @@ private void process() { } /** - * Determines whether a name reference in a particular scope is a global name - * reference. + * Determines whether a name reference in a particular scope is a global name reference. * * @param name A variable or property name (e.g. "a" or "a.b.c.d") * @param s The scope in which the name is referenced @@ -297,8 +294,8 @@ private static String getTopVarName(String name) { } /** - * Determines whether a variable name reference in a particular scope is a - * global variable reference. + * Determines whether a variable name reference in a particular scope is a global variable + * reference. * * @param name A variable name (e.g. "a") * @param s The scope in which the name is referenced @@ -485,7 +482,6 @@ public void collect(JSModule module, Scope scope, Node n) { return; } - if (isSet) { // Use the closest hoist scope to select handleSetFromGlobal or handleSetFromLocal // because they use the term 'global' in an ES5, pre-block-scoping sense. @@ -501,20 +497,17 @@ public void collect(JSModule module, Scope scope, Node n) { } /** - * Gets the fully qualified name corresponding to an object literal key, - * as long as it and its prefix property names are valid JavaScript - * identifiers. The object literal may be nested inside of other object - * literals. + * Gets the fully qualified name corresponding to an object literal key, as long as it and its + * prefix property names are valid JavaScript identifiers. The object literal may be nested + * inside of other object literals. * - * For example, if called with node {@code n} representing "z" in any of - * the following expressions, the result would be "w.x.y.z": - * var w = {x: {y: {z: 0}}}; - * w.x = {y: {z: 0}}; - * w.x.y = {'a': 0, 'z': 0}; + *

For example, if called with node {@code n} representing "z" in any of the following + * expressions, the result would be "w.x.y.z": var w = {x: {y: {z: 0}}}; + * w.x = {y: {z: 0}}; w.x.y = {'a': 0, 'z': 0}; * * @param n A child of an OBJLIT node - * @return The global name, or null if {@code n} doesn't correspond to the - * key of an object literal that can be named + * @return The global name, or null if {@code n} doesn't correspond to the key of an object + * literal that can be named */ String getNameForObjLitKey(Node n) { Node parent = n.getParent(); @@ -570,19 +563,16 @@ String getNameForObjLitKey(Node n) { } /** - * Gets the fully qualified name corresponding to an class member function, - * as long as it and its prefix property names are valid JavaScript - * identifiers. + * Gets the fully qualified name corresponding to an class member function, as long as it and + * its prefix property names are valid JavaScript identifiers. * - * For example, if called with node {@code n} representing "y" in any of - * the following expressions, the result would be "x.y": - * class x{y(){}}; - * var x = class{y(){}}; - * var x; x = class{y(){}}; + *

For example, if called with node {@code n} representing "y" in any of the following + * expressions, the result would be "x.y": class x{y(){}}; + * var x = class{y(){}}; var x; x = class{y(){}}; * * @param n A child of an CLASS_MEMBERS node - * @return The global name, or null if {@code n} doesn't correspond to - * a class member function that can be named + * @return The global name, or null if {@code n} doesn't correspond to a class member function + * that can be named */ String getNameForClassMembers(Node n) { Node parent = n.getParent(); @@ -693,10 +683,10 @@ void handleSetFromGlobal( if (isNestedAssign(parent)) { // This assignment is both a set and a get that creates an alias. - nameObj.addTwinRefs(module, scope, n, Ref.Type.SET_FROM_GLOBAL, currentPreOrderIndex); - currentPreOrderIndex += 2; // addTwinRefs uses 2 index values + Ref.Type refType = Ref.Type.SET_FROM_GLOBAL; + addOrConfirmTwinRefs(nameObj, n, refType, module, scope); } else { - nameObj.addSingleRef(module, scope, n, Ref.Type.SET_FROM_GLOBAL, currentPreOrderIndex++); + addOrConfirmRef(nameObj, n, Ref.Type.SET_FROM_GLOBAL, module, scope); if (isTypeDeclaration(n)) { // Names with a @constructor or @enum annotation are always collapsed nameObj.setDeclaredType(); @@ -704,6 +694,31 @@ void handleSetFromGlobal( } } + /** + * If Refs already exist for the given Node confirm they match what we would create. Otherwise, + * create them. + * + * @param nameObj + * @param node + * @param setRefType + * @param module + * @param scope + */ + private void addOrConfirmTwinRefs( + Name nameObj, Node node, Ref.Type setRefType, JSModule module, Scope scope) { + ImmutableList existingRefs = nameObj.getRefsForNode(node); + if (existingRefs.isEmpty()) { + nameObj.addTwinRefs(module, scope, node, setRefType, currentPreOrderIndex); + currentPreOrderIndex += 2; // addTwinRefs uses 2 index values + } else { + checkState(existingRefs.size() == 2, "unexpected existing refs: %s", existingRefs); + Ref setRef = existingRefs.get(0); + // module and scope are dependent on Node, so not much point in checking them + // the type of the getRef is set within the Name class, so no need to check that either. + checkState(setRef.type == setRefType, "unexpected existing set Ref type: %s", setRef.type); + } + } + /** * Determines whether a set operation is a constructor or enumeration or interface declaration. * The set operation may either be an assignment to a name, a variable declaration, or an object @@ -751,10 +766,9 @@ void handleSetFromLocal(JSModule module, Scope scope, Node n, Node parent, Strin if (isNestedAssign(parent)) { // This assignment is both a set and a get that creates an alias. - nameObj.addTwinRefs(module, scope, n, Ref.Type.SET_FROM_LOCAL, currentPreOrderIndex); - currentPreOrderIndex += 2; // addTwinRefs uses 2 index values + addOrConfirmTwinRefs(nameObj, n, Ref.Type.SET_FROM_LOCAL, module, scope); } else { - nameObj.addSingleRef(module, scope, n, Ref.Type.SET_FROM_LOCAL, currentPreOrderIndex++); + addOrConfirmRef(nameObj, n, Ref.Type.SET_FROM_LOCAL, module, scope); } } @@ -857,14 +871,34 @@ 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.addSingleRef(module, scope, n, type, currentPreOrderIndex++); + addOrConfirmRef(nameObj, n, type, module, scope); + } + + /** + * If there is already a Ref for the given name & node, confirm it matches what we would create. + * Otherwise add a new one. + */ + private void addOrConfirmRef( + Name nameObj, Node node, Ref.Type refType, JSModule module, Scope scope) { + ImmutableList existingRefs = nameObj.getRefsForNode(node); + if (existingRefs.isEmpty()) { + nameObj.addSingleRef(module, scope, node, refType, currentPreOrderIndex++); + } else { + checkState(existingRefs.size() == 1, "unexpected twin refs: %s", existingRefs); + // module and scope are dependent on Node, so not much point in checking them + Ref.Type existingRefType = existingRefs.get(0).type; + checkState( + existingRefType == refType, + "existing ref type: %s expected: %s", + existingRefType, + refType); + } } private boolean isClassDefiningCall(Node callNode) { CodingConvention convention = compiler.getCodingConvention(); // Look for goog.inherits, goog.mixin - SubclassRelationship classes = - convention.getClassesDefinedByCall(callNode); + SubclassRelationship classes = convention.getClassesDefinedByCall(callNode); if (classes != null) { return true; } @@ -892,17 +926,17 @@ private boolean isObjectHasOwnPropertyCall(Node callNode) { } /** - * Determines whether the result of a hook (x?y:z) or boolean expression - * (x||y) or (x&&y) is assigned to a specific global name. + * Determines whether the result of a hook (x?y:z) or boolean expression (x||y) or (x&&y) is + * assigned to a specific global name. * * @param module The current module * @param scope The current scope - * @param parent The parent of the current node in the traversal. This node - * should already be known to be a HOOK, AND, or OR node. - * @param name A name that is already known to be global in the current - * scope (e.g. "a" or "a.b.c.d") - * @return The expression's get type, either {@link Ref.Type#DIRECT_GET} or - * {@link Ref.Type#ALIASING_GET} + * @param parent The parent of the current node in the traversal. This node should already be + * known to be a HOOK, AND, or OR node. + * @param name A name that is already known to be global in the current scope (e.g. "a" or + * "a.b.c.d") + * @return The expression's get type, either {@link Ref.Type#DIRECT_GET} or {@link + * Ref.Type#ALIASING_GET} */ Ref.Type determineGetTypeForHookOrBooleanExpr( JSModule module, Scope scope, Node parent, String name) { @@ -935,7 +969,7 @@ Ref.Type determineGetTypeForHookOrBooleanExpr( return Ref.Type.ALIASING_GET; } break; - case NAME: // a variable declaration + case NAME: // a variable declaration if (!name.equals(anc.getString())) { return Ref.Type.ALIASING_GET; } @@ -956,9 +990,9 @@ Ref.Type determineGetTypeForHookOrBooleanExpr( } /** - * Updates our representation of the global namespace to reflect a read - * of a global name's longest prefix before the "prototype" property if the - * name includes the "prototype" property. Does nothing otherwise. + * Updates our representation of the global namespace to reflect a read of a global name's + * longest prefix before the "prototype" property if the name includes the "prototype" property. + * Does nothing otherwise. * * @param module The current module * @param scope The current scope @@ -967,8 +1001,8 @@ Ref.Type determineGetTypeForHookOrBooleanExpr( * @param name The global name (e.g. "a" or "a.b.c.d") * @return Whether the name was handled */ - boolean maybeHandlePrototypePrefix(JSModule module, Scope scope, - Node n, Node parent, String name) { + boolean maybeHandlePrototypePrefix( + JSModule module, Scope scope, Node n, Node parent, String name) { // We use a string-based approach instead of inspecting the parse tree // to avoid complexities with object literals, possibly nested, beneath // assignments. @@ -1008,12 +1042,10 @@ boolean maybeHandlePrototypePrefix(JSModule module, Scope scope, } /** - * Determines whether an assignment is nested (i.e. whether its return - * value is used). + * Determines whether an assignment is nested (i.e. whether its return value is used). * * @param parent The parent of the current traversal node (not null) - * @return Whether it appears that the return value of the assignment is - * used + * @return Whether it appears that the return value of the assignment is used */ boolean isNestedAssign(Node parent) { return parent.isAssign() && !parent.getParent().isExprResult(); @@ -1070,7 +1102,10 @@ private enum Type { private Ref declaration; /** All references to a name. This must contain {@code declaration}. */ - private LinkedHashSet refs; + private final LinkedHashSet refs = new LinkedHashSet<>(); + + /** Keep track of which Nodes are Refs for this Name */ + private final Map> refsForNodeMap = new HashMap<>(); /** All Es6 subclasses of a name that is an Es6 class. Must be null if not an ES6 class. */ @Nullable List subclasses; @@ -1231,13 +1266,25 @@ private void addTwinRefs( createNewRef(module, scope, node, Ref.Type.ALIASING_GET, setRefPreOrderIndex + 1); setRef.twin = getRef; getRef.twin = setRef; - addRef(setRef); - addRef(getRef); + refsForNodeMap.put(node, ImmutableList.of(setRef, getRef)); + refs.add(setRef); + updateStateForAddedRef(setRef); + refs.add(getRef); + updateStateForAddedRef(getRef); } private void addSingleRef( JSModule module, Scope scope, Node node, Ref.Type type, int preOrderIndex) { - addRef(createNewRef(module, scope, node, type, preOrderIndex)); + checkNoExistingRefsForNode(node); + Ref ref = createNewRef(module, scope, node, type, preOrderIndex); + refs.add(ref); + refsForNodeMap.put(node, ImmutableList.of(ref)); + updateStateForAddedRef(ref); + } + + private void checkNoExistingRefsForNode(Node node) { + ImmutableList refsForNode = refsForNodeMap.get(node); + checkState(refsForNode == null, "Refs already exist for node: %s", refsForNode); } private Ref createNewRef( @@ -1255,7 +1302,9 @@ Ref addSingleRefForTesting(Ref.Type type, int preOrderIndex) { Ref ref = new Ref( /* module= */ null, /* scope= */ null, /* node = */ null, this, type, preOrderIndex); - addRef(ref); + refs.add(ref); + // node is Null for testing in this case, so nothing to add to refsForNodeMap + updateStateForAddedRef(ref); return ref; } @@ -1275,8 +1324,14 @@ void addAliasingGetClonedFromDeclaration(Node newRefNode) { declRef.module, declRef.scope, newRefNode, Ref.Type.ALIASING_GET, declRef.preOrderIndex); } - private void addRef(Ref ref) { - addRefInternal(ref); + /** + * Updates counters and JSDocInfo recorded for the name to include a newly added Ref. + * + *

Must be called exactly once when a new Ref is added. + * + * @param ref a Ref that has just been added for this Name + */ + private void updateStateForAddedRef(Ref ref) { switch (ref.type) { case SET_FROM_GLOBAL: if (declaration == null) { @@ -1330,18 +1385,87 @@ && isQnameDeclarationWithoutAssignment(node)) { /** * This is the only safe way to update the Node belonging to a Ref once it is added to a Name. + * + *

This is a specialized method that exists only for use by CollapseProperties. + * + * @param ref reference to update - it must belong to this name + * @param newNode new value for the ref's node + */ + void updateRefNode(Ref ref, @Nullable Node newNode) { + checkArgument(ref.node != newNode, "redundant update to Ref node: %s", ref); + + // Once a Ref's node is set to null, it shouldn't ever be set to anything else. + // TODO(bradfordcsmith): Document here what it means when we set the node to null. + // Seems to be a way to keep name.getDeclaration() returning the original declaration + // Ref even though its node is no longer in the AST. + Node oldNode = ref.getNode(); + checkState(oldNode != null, "Ref's node is already null: %s", ref); + ref.node = newNode; + + // If this ref was a twin, it isn't anymore, and its previous twin is now the only ref to the + // original node. + Ref twinRef = ref.getTwin(); + if (twinRef != null) { + ref.twin = null; + twinRef.twin = null; + refsForNodeMap.put(oldNode, ImmutableList.of(twinRef)); + } else { + refsForNodeMap.remove(oldNode); // this ref was the only reference on the node + } + + if (newNode != null) { + ImmutableList existingRefsForNewNode = refsForNodeMap.get(newNode); + checkArgument( + existingRefsForNewNode == null, "refs already exist: %s", existingRefsForNewNode); + refsForNodeMap.put(newNode, ImmutableList.of(ref)); + } + } + + /** + * Remove a Ref and its twin at the same time. + * + *

If you intend to remove both, it is more efficient and less error prone to use this method + * instead of removing them one at a time. + * + * @param ref A Ref that has a twin. */ - void updateRefNode(Ref ref, @Nullable Node node) { - // TODO(bradfordcsmith): Add this checkState when it can be done more efficiently than - // searching through all of the refs. - // checkState(refs != null && refs.contains(ref), "unknown ref: %s", ref); - ref.node = node; + void removeTwinRefs(Ref ref) { + checkArgument( + ref.name == this, "removeTwinRefs(%s): node does not belong to this name: %s", ref, this); + checkState(refs.contains(ref), "removeRef(%s): unknown ref", ref); + Ref twinRef = ref.getTwin(); + checkArgument(twinRef != null, ref); + + removeTwinRefsFromNodeMap(ref); + removeRefAndUpdateState(ref); + removeRefAndUpdateState(twinRef); } + /** + * Removes the given Ref, which must belong to this Name. + * + *

NOTE: if ref has a twin, they will no longer be twins after this method finishes. Use + * removeTwinRefs() to remove a pair of twins at the same time. + * + * @param ref + */ void removeRef(Ref ref) { checkState( ref.name == this, "removeRef(%s): node does not belong to this name: %s", ref, this); - checkState(refs != null && refs.contains(ref), "removeRef(%s): unknown ref", ref); + checkState(refs.contains(ref), "removeRef(%s): unknown ref", ref); + Node refNode = ref.getNode(); + if (refNode != null) { + removeSingleRefFromNodeMap(ref); + } + removeRefAndUpdateState(ref); + } + + /** + * Update counts, declaration, and JSDoc to reflect removal of the given Ref. + * + * @param ref + */ + private void removeRefAndUpdateState(Ref ref) { refs.remove(ref); if (ref == declaration) { declaration = null; @@ -1388,22 +1512,71 @@ void removeRef(Ref ref) { } } + private void removeSingleRefFromNodeMap(Ref ref) { + Node refNode = checkNotNull(ref.getNode(), ref); + if (ref.getTwin() != null) { + removeTwinRefsFromNodeMap(ref); + Ref twinRef = ref.getTwin(); + // break the twin relationship + ref.twin = null; + twinRef.twin = null; + // put twin back alone, since we're not really removing it + refsForNodeMap.put(refNode, ImmutableList.of(twinRef)); + } else { + ImmutableList refsForNode = refsForNodeMap.get(refNode); + checkState( + refsForNode.size() == 1 && refsForNode.get(0) == ref, + "Unexpected Refs for Node: %s: when removing Ref: %s", + refsForNode, + ref); + refsForNodeMap.remove(refNode); + } + } + + private void removeTwinRefsFromNodeMap(Ref ref) { + Ref twinRef = checkNotNull(ref.getTwin(), ref); + Node refNode = checkNotNull(ref.getNode(), ref); + ImmutableList refsForNode = refsForNodeMap.get(refNode); + + checkState( + refsForNode.size() == 2, + "unexpected Refs for Node: %s, when removing: %s", + refsForNode, + ref); + checkState( + refsForNode.contains(ref), + "Refs for Node: %s does not contain Ref to remove: %s", + refsForNode, + ref); + checkState( + refsForNode.contains(twinRef), + "Refs for Node: %s does not contain expected twin: %s", + refsForNode, + twinRef); + refsForNodeMap.remove(refNode); + } + Collection getRefs() { return refs == null ? ImmutableList.of() : Collections.unmodifiableCollection(refs); } + /** + * Get the Refs for this name that belong to the given node. + * + *

Returns an empty list if there are no Refs, or a list with only one Ref, or a list with + * exactly 2 refs that are twins of each other. + */ + @VisibleForTesting + ImmutableList getRefsForNode(Node node) { + ImmutableList refsForNode = refsForNodeMap.get(checkNotNull(node)); + return (refsForNode == null) ? ImmutableList.of() : refsForNode; + } + Ref getFirstRef() { checkState(!refs.isEmpty(), "no first Ref to get"); return Iterables.get(refs, 0); } - private void addRefInternal(Ref ref) { - if (refs == null) { - refs = new LinkedHashSet<>(); - } - refs.add(ref); - } - boolean canEliminate() { if (!canCollapseUnannotatedChildNames() || totalGets > 0) { return false; @@ -1674,8 +1847,11 @@ boolean canCollapseUnannotatedChildNames() { * goog.provide namespace chains. */ private Inlinability canCollapseOrInlineChildNames() { - if (type == Type.OTHER || isGetOrSetDefinition() - || globalSets != 1 || localSets != 0 || deleteProps != 0) { + if (type == Type.OTHER + || isGetOrSetDefinition() + || globalSets != 1 + || localSets != 0 + || deleteProps != 0) { // condition (a) and (b) return Inlinability.DO_NOT_INLINE; } @@ -1750,8 +1926,7 @@ boolean needsToBeStubbed() { void setDeclaredType() { declaredType = true; - for (Name ancestor = parent; ancestor != null; - ancestor = ancestor.parent) { + for (Name ancestor = parent; ancestor != null; ancestor = ancestor.parent) { ancestor.isDeclared = true; } } @@ -1764,31 +1939,31 @@ boolean isConstructor() { Node declNode = declaration.node; Node rvalueNode = NodeUtil.getRValueOfLValue(declNode); JSDocInfo jsdoc = NodeUtil.getBestJSDocInfo(declNode); - return rvalueNode != null && rvalueNode.isFunction() - && jsdoc != null && jsdoc.isConstructor(); + return rvalueNode != null + && rvalueNode.isFunction() + && jsdoc != null + && jsdoc.isConstructor(); } /** - * Determines whether this name is a prefix of at least one class or enum - * name. Because classes and enums are always collapsed, the namespace will - * have different properties in compiled code than in uncompiled code. + * Determines whether this name is a prefix of at least one class or enum name. Because classes + * and enums are always collapsed, the namespace will have different properties in compiled code + * than in uncompiled code. * - * For example, if foo.bar.DomHelper is a class, then foo and foo.bar are - * considered namespaces. + *

For example, if foo.bar.DomHelper is a class, then foo and foo.bar are considered + * namespaces. */ boolean isNamespaceObjectLit() { return isDeclared && type == Type.OBJECTLIT; } - /** - * Determines whether this is a simple name (as opposed to a qualified - * name). - */ + /** Determines whether this is a simple name (as opposed to a qualified name). */ boolean isSimpleName() { return parent == null; } - @Override public String toString() { + @Override + public String toString() { return getFullName() + " (" + type @@ -1814,9 +1989,7 @@ public JSDocInfo getJSDocInfo() { : firstQnameDeclarationWithoutAssignmentJsDocInfo; } - /** - * Tries to get the doc info for a given declaration ref. - */ + /** Tries to get the doc info for a given declaration ref. */ private static JSDocInfo getDocInfoForDeclaration(Ref ref) { if (ref.node != null) { Node refParent = ref.node.getParent(); @@ -1884,33 +2057,21 @@ enum Type { PROTOTYPE_GET, /** - * Includes all uses that prevent a name's properties from being collapsed: - * var x = a.b.c - * f(a.b.c) - * new Foo(a.b.c) + * Includes all uses that prevent a name's properties from being collapsed: var x = a.b.c + * f(a.b.c) new Foo(a.b.c) */ ALIASING_GET, /** * Includes all uses that prevent a name from being completely eliminated: - * goog.inherits(anotherName, a.b.c) - * new a.b.c() - * x instanceof a.b.c - * void a.b.c - * if (a.b.c) {} + * goog.inherits(anotherName, a.b.c) new a.b.c() x instanceof a.b.c void a.b.c if (a.b.c) {} */ DIRECT_GET, - /** - * Calling a name: a.b.c(); - * Prevents a name from being collapsed if never set. - */ + /** Calling a name: a.b.c(); Prevents a name from being collapsed if never set. */ CALL_GET, - /** - * Deletion of a property: delete a.b.c; - * Prevents a name from being collapsed at all. - */ + /** Deletion of a property: delete a.b.c; Prevents a name from being collapsed at all. */ DELETE_PROP, /** ES6 subclassing ref: class extends A {} */ @@ -1927,14 +2088,14 @@ enum Type { * this scope may not be the correct hoist scope of the aliasing VAR. */ final Scope scope; + final int preOrderIndex; /** - * Certain types of references are actually double-refs. For example, - * var a = b = 0; - * counts as both a "set" of b and an "alias" of b. + * Certain types of references are actually double-refs. For example, var a = b = 0; counts as + * both a "set" of b and an "alias" of b. * - * We create two Refs for this node, and mark them as twins of each other. + *

We create two Refs for this node, and mark them as twins of each other. */ private Ref twin = null; diff --git a/test/com/google/javascript/jscomp/GlobalNamespaceTest.java b/test/com/google/javascript/jscomp/GlobalNamespaceTest.java index 951ad30ab66..200cabab6c6 100644 --- a/test/com/google/javascript/jscomp/GlobalNamespaceTest.java +++ b/test/com/google/javascript/jscomp/GlobalNamespaceTest.java @@ -18,10 +18,12 @@ import static com.google.common.base.Preconditions.checkState; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth.assertWithMessage; import static com.google.javascript.jscomp.CompilerTypeTestCase.lines; import static com.google.javascript.rhino.testing.NodeSubject.assertNode; import static com.google.javascript.rhino.testing.TypeSubject.assertType; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.javascript.jscomp.GlobalNamespace.AstChange; @@ -33,6 +35,7 @@ import com.google.javascript.rhino.JSTypeExpression; import com.google.javascript.rhino.Node; import com.google.javascript.rhino.jstype.JSType; +import java.util.Collection; import javax.annotation.Nullable; import org.junit.Test; import org.junit.runner.RunWith; @@ -48,6 +51,16 @@ public final class GlobalNamespaceTest { @Nullable private Compiler lastCompiler = null; + // TODO(b/124228379): We should be using assertThrows from JUnit4. + private static void assertThrows(Object exceptionClass, Runnable runnable) { + try { + runnable.run(); + assertWithMessage("Did not get expected exception: %s", exceptionClass).fail(); + } catch (Exception expectedException) { + // expected + } + } + @Test public void firstGlobalAssignmentIsConsideredDeclaration() { Name n = Name.createForTesting("a"); @@ -179,6 +192,236 @@ public void testStaticInheritedReferencesDontReferToSuperclass() { assertThat(subclassStaticMethod.getDeclaration()).isNull(); } + @Test + public void updateRefNodeRejectsRedundantUpdate() { + GlobalNamespace namespace = parse("const A = 3;"); + + Name nameA = namespace.getOwnSlot("A"); + Ref refA = nameA.getFirstRef(); + + assertThrows( + IllegalArgumentException.class, + () -> { + nameA.updateRefNode(refA, refA.getNode()); + }); + } + + @Test + public void updateRefNodeMovesRefFromOldNodeToNewNode() { + GlobalNamespace namespace = parse("const A = 3;"); + + Name nameA = namespace.getOwnSlot("A"); + Ref refA = nameA.getFirstRef(); + + Node oldNode = refA.getNode(); + Node newNode = IR.name("A"); + + assertThat(nameA.getRefsForNode(oldNode)).containsExactly(refA); + + nameA.updateRefNode(refA, newNode); + + assertThat(refA.getNode()).isEqualTo(newNode); + assertThat(nameA.getRefsForNode(oldNode)).isEmpty(); + assertThat(nameA.getRefsForNode(newNode)).containsExactly(refA); + } + + @Test + public void updateRefNodeCanSetNodeToNullButPreventsFurtherUpdates() { + GlobalNamespace namespace = parse("const A = 3;"); + + Name nameA = namespace.getOwnSlot("A"); + Ref refA = nameA.getFirstRef(); + + Node oldNode = refA.getNode(); + + assertThat(nameA.getRefsForNode(oldNode)).containsExactly(refA); + + nameA.updateRefNode(refA, null); + + assertThat(refA.getNode()).isNull(); + assertThat(nameA.getRefsForNode(oldNode)).isEmpty(); + // cannot get refs for null + assertThrows( + NullPointerException.class, + () -> { + nameA.getRefsForNode(null); + }); + // cannot update the node again once it's been set to null + assertThrows( + IllegalArgumentException.class, + () -> { + nameA.updateRefNode(refA, null); + }); + assertThrows( + IllegalStateException.class, + () -> { + nameA.updateRefNode(refA, oldNode); + }); + } + + @Test + public void updateRefNodeRejectsNodeWithExistingRefs() { + GlobalNamespace namespace = + parse( + lines( + "const A = 3;", // declaration ref + "A;")); // use ref + + Name nameA = namespace.getOwnSlot("A"); + Ref declarationRef = nameA.getDeclaration(); + Ref useRef = Iterables.get(nameA.getRefs(), 1); // use ref is 2nd + + Node useNode = useRef.getNode(); + + assertThrows( + IllegalArgumentException.class, + () -> { + nameA.updateRefNode(declarationRef, useNode); + }); + } + + @Test + public void confirmTwinsAreCreated() { + GlobalNamespace namespace = + parse( + lines( + "let A;", // + "const B = A = 3;")); // A will have twin refs here + + Name nameA = namespace.getOwnSlot("A"); + // first ref is declaration of A + Ref setTwinRef = Iterables.get(nameA.getRefs(), 1); // second is the SET twin + Ref getTwinRef = Iterables.get(nameA.getRefs(), 2); // third is the GET twin + + // confirm that they start as twins + assertThat(setTwinRef.getTwin()).isEqualTo(getTwinRef); + assertThat(getTwinRef.getTwin()).isEqualTo(setTwinRef); + + Node oldNode = getTwinRef.getNode(); + assertThat(setTwinRef.getNode()).isEqualTo(oldNode); + + // confirm that they are both associated with oldNode + assertThat(nameA.getRefsForNode(oldNode)).containsExactly(setTwinRef, getTwinRef); + } + + @Test + public void updateRefNodeRemovesTwinRelationship() { + GlobalNamespace namespace = + parse( + lines( + "let A;", // + "const B = A = 3;")); // A will have twin refs here + + Name nameA = namespace.getOwnSlot("A"); + // first ref is declaration of A + Ref setTwinRef = Iterables.get(nameA.getRefs(), 1); // second is the SET twin + Ref getTwinRef = Iterables.get(nameA.getRefs(), 2); // third is the GET twin + + Node oldNode = getTwinRef.getNode(); + + // move the getTwinRef + Node newNode = IR.name("A"); + nameA.updateRefNode(getTwinRef, newNode); + + // see confirmTwinsAreCreated() for verification of the original twin relationship + + // confirm that getTwinRef has been updated + assertThat(getTwinRef.getNode()).isEqualTo(newNode); + assertThat(nameA.getRefsForNode(newNode)).containsExactly(getTwinRef); + + // confirm that the getTwinRef and setTwinRef are no longer twins + assertThat(getTwinRef.getTwin()).isNull(); + assertThat(setTwinRef.getTwin()).isNull(); + + // confirm that setTwinRef remains otherwise unchanged + assertThat(setTwinRef.getNode()).isEqualTo(oldNode); + assertThat(nameA.getRefsForNode(oldNode)).containsExactly(setTwinRef); + } + + @Test + public void removeTwinRefsTogether() { + GlobalNamespace namespace = + parse( + lines( + "let A;", // + "const B = A = 3;")); // A will have twin refs here + + Name nameA = namespace.getOwnSlot("A"); + // first ref is declaration of A + Ref setTwinRef = Iterables.get(nameA.getRefs(), 1); // second is the SET twin + Ref getTwinRef = Iterables.get(nameA.getRefs(), 2); // third is the GET twin + + // see confirmTwinsAreCreated() for verification of the original twin relationship + + Node oldNode = getTwinRef.getNode(); + + nameA.removeRef(setTwinRef); + + assertThat(nameA.getRefs()).doesNotContain(setTwinRef); + assertThat(nameA.getRefs()).contains(getTwinRef); // twin is still there + assertThat(getTwinRef.getTwin()).isNull(); // and not a twin anymore + assertThat(nameA.getRefsForNode(oldNode)).containsExactly(getTwinRef); + } + + @Test + public void removeOneRefOfAPairOfTwins() { + GlobalNamespace namespace = + parse( + lines( + "let A;", // + "const B = A = 3;")); // A will have twin refs here + + Name nameA = namespace.getOwnSlot("A"); + // first ref is declaration of A + Ref setTwinRef = Iterables.get(nameA.getRefs(), 1); // second is the SET twin + Ref getTwinRef = Iterables.get(nameA.getRefs(), 2); // third is the GET twin + + // see confirmTwinsAreCreated() for verification of the original twin relationship + + Node oldNode = getTwinRef.getNode(); + + // confirm that they are both associated with oldNode + assertThat(nameA.getRefsForNode(oldNode)).containsExactly(setTwinRef, getTwinRef); + + nameA.removeTwinRefs(setTwinRef); + + assertThat(nameA.getRefs()).doesNotContain(setTwinRef); + assertThat(nameA.getRefs()).doesNotContain(getTwinRef); + assertThat(nameA.getRefsForNode(oldNode)).isEmpty(); + } + + @Test + public void rescanningExistingNodesDoesNotCreateDuplicateRefs() { + GlobalNamespace namespace = parse("class Foo {} const Bar = Foo; const Baz = Bar;"); + + Name foo = namespace.getOwnSlot("Foo"); + Name bar = namespace.getOwnSlot("Bar"); + Name baz = namespace.getOwnSlot("Baz"); + Collection originalFooRefs = ImmutableList.copyOf(foo.getRefs()); + Collection originalBarRefs = ImmutableList.copyOf(bar.getRefs()); + Collection originalBazRefs = ImmutableList.copyOf(baz.getRefs()); + + // Rescan all of the nodes for which we got refs as if they were newly added + Node root = lastCompiler.getJsRoot(); + ImmutableSet.Builder astChangeSetBuilder = ImmutableSet.builder(); + for (Name name : ImmutableList.of(foo, bar, baz)) { + for (Ref ref : name.getRefs()) { + astChangeSetBuilder.add(createGlobalAstChangeForNode(root, ref.getNode())); + } + } + namespace.scanNewNodes(astChangeSetBuilder.build()); + + // We should get the same Name objects + assertThat(namespace.getOwnSlot("Foo")).isEqualTo(foo); + assertThat(namespace.getOwnSlot("Bar")).isEqualTo(bar); + assertThat(namespace.getOwnSlot("Baz")).isEqualTo(baz); + + // ...and they should contain the same refs with no duplicates added + assertThat(foo.getRefs()).containsExactlyElementsIn(originalFooRefs).inOrder(); + assertThat(bar.getRefs()).containsExactlyElementsIn(originalBarRefs).inOrder(); + assertThat(baz.getRefs()).containsExactlyElementsIn(originalBazRefs).inOrder(); + } + @Test public void testScanFromNodeDoesntDuplicateVarDeclarationSets() { GlobalNamespace namespace = parse("class Foo {} const Bar = Foo; const Baz = Bar;");