From f9fa184ad1c2f47a641acbfa88d40b65c8769f70 Mon Sep 17 00:00:00 2001 From: blickly Date: Wed, 7 Dec 2016 12:35:04 -0800 Subject: [PATCH] Various simplifications to remove Node.isFor from our codebase. Also added a Node.isAnyFor method for the cases where one wants to check if a node represents some for, be it a for-in, for-of, or for(;;) ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=141342706 --- ...BranchCoverageInstrumentationCallback.java | 2 +- .../jscomp/CoalesceVariableNames.java | 2 +- .../jscomp/ConvertToTypedInterface.java | 2 +- .../jscomp/DeadAssignmentsElimination.java | 6 +-- .../javascript/jscomp/DefinitionsRemover.java | 2 +- .../google/javascript/jscomp/Denormalize.java | 3 +- .../jscomp/Es6RewriteGenerators.java | 2 +- .../jscomp/Es6SyntacticScopeCreator.java | 15 +++++-- .../javascript/jscomp/InlineVariables.java | 2 +- .../javascript/jscomp/NameAnalyzer.java | 42 +++++++++---------- .../google/javascript/jscomp/NodeUtil.java | 7 +++- .../google/javascript/jscomp/Normalize.java | 2 +- .../jscomp/PeepholeRemoveDeadCode.java | 7 +--- .../javascript/jscomp/RemoveUnusedVars.java | 4 +- .../jscomp/RescopeGlobalSymbols.java | 5 +-- src/com/google/javascript/rhino/Node.java | 2 + 16 files changed, 53 insertions(+), 52 deletions(-) diff --git a/src/com/google/javascript/jscomp/BranchCoverageInstrumentationCallback.java b/src/com/google/javascript/jscomp/BranchCoverageInstrumentationCallback.java index cfc5ebc4ef5..cce614b2e31 100644 --- a/src/com/google/javascript/jscomp/BranchCoverageInstrumentationCallback.java +++ b/src/com/google/javascript/jscomp/BranchCoverageInstrumentationCallback.java @@ -80,7 +80,7 @@ public void visit(NodeTraversal traversal, Node node, Node parent) { fileName, new FileInstrumentationData(fileName, createArrayName(traversal))); } processBranchInfo(node, instrumentationData.get(fileName), getChildrenBlocks(node)); - } else if (node.isFor() || node.isWhile() || node.isDo()) { + } else if (NodeUtil.isLoopStructure(node)) { List blocks = getChildrenBlocks(node); ControlFlowGraph cfg = traversal.getControlFlowGraph(); for (DiGraph.DiGraphEdge outEdge : cfg.getOutEdges(node)) { diff --git a/src/com/google/javascript/jscomp/CoalesceVariableNames.java b/src/com/google/javascript/jscomp/CoalesceVariableNames.java index 2927f22227f..27fecf9c93c 100644 --- a/src/com/google/javascript/jscomp/CoalesceVariableNames.java +++ b/src/com/google/javascript/jscomp/CoalesceVariableNames.java @@ -347,7 +347,7 @@ private static void removeVarDeclaration(Node name) { Node assign = IR.assign(name, value).srcref(name); // We don't need to wrapped it with EXPR node if it is within a FOR. - if (!parent.isFor()) { + if (!parent.isVanillaFor()) { assign = NodeUtil.newExpr(assign); } parent.replaceChild(var, assign); diff --git a/src/com/google/javascript/jscomp/ConvertToTypedInterface.java b/src/com/google/javascript/jscomp/ConvertToTypedInterface.java index 9b81816158c..1de35bad3be 100644 --- a/src/com/google/javascript/jscomp/ConvertToTypedInterface.java +++ b/src/com/google/javascript/jscomp/ConvertToTypedInterface.java @@ -302,7 +302,7 @@ public void visit(NodeTraversal t, Node n, Node parent) { Node body = NodeUtil.getLoopCodeBlock(n); parent.addChildAfter(body.detach(), n); NodeUtil.removeChild(parent, n); - Node initializer = n.isFor() ? n.getFirstChild() : IR.empty(); + Node initializer = NodeUtil.isAnyFor(n) ? n.getFirstChild() : IR.empty(); if (initializer.isVar() && initializer.hasOneChild()) { parent.addChildBefore(initializer.detach(), body); processName(initializer.getFirstChild(), initializer); diff --git a/src/com/google/javascript/jscomp/DeadAssignmentsElimination.java b/src/com/google/javascript/jscomp/DeadAssignmentsElimination.java index 3315dec781d..4370cba5fc3 100644 --- a/src/com/google/javascript/jscomp/DeadAssignmentsElimination.java +++ b/src/com/google/javascript/jscomp/DeadAssignmentsElimination.java @@ -226,7 +226,7 @@ private void tryRemoveAssignment(NodeTraversal t, Node n, Node exprRoot, // Ignore declarations that don't initialize a value. Dead code removal will kill those nodes. // Also ignore the var declaration if it's in a for-loop instantiation since there's not a // safe place to move the side-effects. - if (isDeclarationNode && (rhs == null || parent.getParent().isFor())) { + if (isDeclarationNode && (rhs == null || NodeUtil.isAnyFor(parent.getParent()))) { return; } @@ -291,9 +291,7 @@ private void tryRemoveAssignment(NodeTraversal t, Node n, Node exprRoot, IR.voidNode(IR.number(0).srcref(n))); } else if (n.isComma() && n != parent.getLastChild()) { parent.removeChild(n); - } else if (parent.isFor() - && !parent.isForIn() - && NodeUtil.getConditionExpression(parent) != n) { + } else if (parent.isVanillaFor() && NodeUtil.getConditionExpression(parent) != n) { parent.replaceChild(n, IR.empty()); } else { // Cannot replace x = a++ with x = a because that's not valid diff --git a/src/com/google/javascript/jscomp/DefinitionsRemover.java b/src/com/google/javascript/jscomp/DefinitionsRemover.java index 35163160767..5fcef6588f8 100644 --- a/src/com/google/javascript/jscomp/DefinitionsRemover.java +++ b/src/com/google/javascript/jscomp/DefinitionsRemover.java @@ -422,7 +422,7 @@ public void performRemove() { "AST should be normalized first"); Node parent = var.getParent(); Node rValue = name.removeFirstChild(); - Preconditions.checkState(!parent.isFor()); + Preconditions.checkState(!NodeUtil.isLoopStructure(parent)); parent.replaceChild(var, NodeUtil.newExpr(rValue)); } diff --git a/src/com/google/javascript/jscomp/Denormalize.java b/src/com/google/javascript/jscomp/Denormalize.java index 1dbf2dd1168..b6f26582045 100644 --- a/src/com/google/javascript/jscomp/Denormalize.java +++ b/src/com/google/javascript/jscomp/Denormalize.java @@ -96,8 +96,7 @@ private void maybeCollapseIntoForStatements(Node n, Node parent) { compiler.reportCodeChange(); } } - } else if (nextSibling.isFor() - && nextSibling.getFirstChild().isEmpty()) { + } else if (nextSibling.isVanillaFor() && nextSibling.getFirstChild().isEmpty()) { // Does the current node contain an in operator? If so, embedding // the expression in a for loop can cause some JavaScript parsers (such diff --git a/src/com/google/javascript/jscomp/Es6RewriteGenerators.java b/src/com/google/javascript/jscomp/Es6RewriteGenerators.java index a750be17e1b..3cd25625cdf 100644 --- a/src/com/google/javascript/jscomp/Es6RewriteGenerators.java +++ b/src/com/google/javascript/jscomp/Es6RewriteGenerators.java @@ -767,7 +767,7 @@ private void visitLoop(String label) { body = currentStatement.removeFirstChild(); initializer = IR.empty(); incr = IR.empty(); - } else if (currentStatement.isFor()) { + } else if (currentStatement.isVanillaFor()) { initializer = currentStatement.removeFirstChild(); if (initializer.isAssign()) { initializer = IR.exprResult(initializer); diff --git a/src/com/google/javascript/jscomp/Es6SyntacticScopeCreator.java b/src/com/google/javascript/jscomp/Es6SyntacticScopeCreator.java index 257bb17791b..80982d72cd9 100644 --- a/src/com/google/javascript/jscomp/Es6SyntacticScopeCreator.java +++ b/src/com/google/javascript/jscomp/Es6SyntacticScopeCreator.java @@ -106,7 +106,10 @@ private void scanRoot(Node n) { declareVar(classNameNode); } } - } else if (n.isBlock() || n.isFor() || n.isForOf() || n.isSwitch() || n.isModuleBody()) { + } else if (n.isBlock() + || NodeUtil.isAnyFor(n) + || n.isSwitch() + || n.isModuleBody()) { if (scope.getParent() != null) { inputId = NodeUtil.getInputId(n); } @@ -262,8 +265,14 @@ private static boolean isShadowingDisallowed(Scope s, String name) { private boolean isNodeAtCurrentLexicalScope(Node n) { Node parent = n.getParent(); Node grandparent = parent.getParent(); - Preconditions.checkState(parent.isBlock() || parent.isFor() || parent.isForOf() - || parent.isScript() || parent.isModuleBody() || parent.isLabel(), parent); + Preconditions.checkState( + parent.isBlock() + || (parent.isVanillaFor() || parent.isForIn()) + || parent.isForOf() + || parent.isScript() + || parent.isModuleBody() + || parent.isLabel(), + parent); if (parent.isSyntheticBlock() && grandparent != null && (grandparent.isCase() || grandparent.isDefaultCase())) { diff --git a/src/com/google/javascript/jscomp/InlineVariables.java b/src/com/google/javascript/jscomp/InlineVariables.java index 90211b41576..850ff143509 100644 --- a/src/com/google/javascript/jscomp/InlineVariables.java +++ b/src/com/google/javascript/jscomp/InlineVariables.java @@ -638,7 +638,7 @@ private boolean canMoveModerately( */ private boolean isValidDeclaration(Reference declaration) { return (declaration.getParent().isVar() - && !declaration.getGrandparent().isFor()) + && !NodeUtil.isLoopStructure(declaration.getGrandparent())) || NodeUtil.isFunctionDeclaration(declaration.getParent()); } diff --git a/src/com/google/javascript/jscomp/NameAnalyzer.java b/src/com/google/javascript/jscomp/NameAnalyzer.java index 9bbc23d532b..cb3242f3f19 100644 --- a/src/com/google/javascript/jscomp/NameAnalyzer.java +++ b/src/com/google/javascript/jscomp/NameAnalyzer.java @@ -566,7 +566,7 @@ private void recordAssignment(NodeTraversal t, Node n, Node recordNode) { Node parent = n.getParent(); NameInformation ns = createNameInformation(t, nameNode); if (ns != null) { - if (parent.isFor() && !parent.isForIn()) { + if (parent.isVanillaFor()) { // Patch for assignments that appear in the init, // condition or iteration part of a FOR loop. Without // this change, all 3 of those parts try to claim the for @@ -790,10 +790,10 @@ private void addSimplifiedExpression(Node n, Node parent) { if (value != null) { addSimplifiedChildren(value); } - } else if (n.isAssign() && - (parent.isExprResult() || - parent.isFor() || - parent.isReturn())) { + } else if (n.isAssign() + && (parent.isExprResult() + || parent.isVanillaFor() + || parent.isReturn())) { for (Node child : n.children()) { addSimplifiedChildren(child); } @@ -816,20 +816,18 @@ public boolean shouldTraverse(NodeTraversal t, Node n, Node parent) { // arguments to function calls with side effects or are used in // control structure predicates. These names are always // referenced when the enclosing function is called. - if (n.isFor()) { - if (!n.isForIn()) { - Node decl = n.getFirstChild(); - Node pred = decl.getNext(); - Node step = pred.getNext(); - addSimplifiedExpression(decl, n); - addSimplifiedExpression(pred, n); - addSimplifiedExpression(step, n); - } else { // n.getChildCount() == 3 - Node decl = n.getFirstChild(); - Node iter = decl.getNext(); - addAllChildren(decl); - addAllChildren(iter); - } + if (n.isVanillaFor()) { + Node decl = n.getFirstChild(); + Node pred = decl.getNext(); + Node step = pred.getNext(); + addSimplifiedExpression(decl, n); + addSimplifiedExpression(pred, n); + addSimplifiedExpression(step, n); + } else if (n.isForIn()) { + Node decl = n.getFirstChild(); + Node iter = decl.getNext(); + addAllChildren(decl); + addAllChildren(iter); } if (parent.isVar() || @@ -1817,7 +1815,7 @@ private void replaceWithRhs(Node parent, Node n) { newReplacements.add(valueExpr); changeProxy.replaceWith( parent, n, collapseReplacements(newReplacements)); - } else if (n.isAssign() && !parent.isFor()) { + } else if (n.isAssign() && !(parent.isVanillaFor() || parent.isForIn())) { // assignment appears in a RHS expression. we have already // considered names in the assignment's RHS as being referenced; // replace the assignment with its RHS. @@ -1857,7 +1855,7 @@ private void replaceTopLevelExpressionWithRhs(Node parent, Node n) { break; case ASSIGN: Preconditions.checkArgument( - parent.isFor(), + parent.isVanillaFor() || parent.isForIn(), "Unsupported assignment in replaceWithRhs. parent: %s", parent.getToken()); break; @@ -1872,7 +1870,7 @@ private void replaceTopLevelExpressionWithRhs(Node parent, Node n) { replacements.addAll(getSideEffectNodes(rhs)); } - if (parent.isFor()) { + if (parent.isVanillaFor() || parent.isForIn()) { // tweak replacements array s.t. it is a single expression node. if (replacements.isEmpty()) { replacements.add(IR.empty()); diff --git a/src/com/google/javascript/jscomp/NodeUtil.java b/src/com/google/javascript/jscomp/NodeUtil.java index 336fa85775a..fc71a97e779 100644 --- a/src/com/google/javascript/jscomp/NodeUtil.java +++ b/src/com/google/javascript/jscomp/NodeUtil.java @@ -2373,6 +2373,10 @@ static boolean isEnhancedFor(Node n) { return n.isForOf() || n.isForIn(); } + static boolean isAnyFor(Node n) { + return n.isVanillaFor() || n.isForIn() || n.isForOf(); + } + /** Use Node.isForIn instead. */ @Deprecated public static boolean isForIn(Node n) { @@ -2685,8 +2689,7 @@ public static void removeChild(Node parent, Node node) { parent.removeChild(node); // A LABEL without children can not be referred to, remove it. removeChild(parent.getParent(), parent); - } else if (parent.isFor() - && parent.getChildCount() == 4) { + } else if (parent.isVanillaFor()) { // Only Token.FOR can have an Token.EMPTY other control structure // need something for the condition. Others need to be replaced // or the structure removed. diff --git a/src/com/google/javascript/jscomp/Normalize.java b/src/com/google/javascript/jscomp/Normalize.java index ee34ac1e258..e333d63cd00 100644 --- a/src/com/google/javascript/jscomp/Normalize.java +++ b/src/com/google/javascript/jscomp/Normalize.java @@ -823,7 +823,7 @@ private void replaceVarWithAssignment(Node n, Node parent, Node grandparent) { // It is an empty reference remove it. if (NodeUtil.isStatementBlock(grandparent)) { grandparent.removeChild(parent); - } else if (grandparent.isFor()) { + } else if (grandparent.isForIn()) { // This is the "for (var a in b)..." case. We don't need to worry // about initializers in "for (var a;;)..." as those are moved out // as part of the other normalizations. diff --git a/src/com/google/javascript/jscomp/PeepholeRemoveDeadCode.java b/src/com/google/javascript/jscomp/PeepholeRemoveDeadCode.java index dd9a0a5f00c..99c230ff0e8 100644 --- a/src/com/google/javascript/jscomp/PeepholeRemoveDeadCode.java +++ b/src/com/google/javascript/jscomp/PeepholeRemoveDeadCode.java @@ -59,7 +59,6 @@ Node optimizeSubtree(Node subtree) { case WHILE: return tryFoldWhile(subtree); case FOR: - case FOR_IN: { Node condition = NodeUtil.getConditionExpression(subtree); if (condition != null) { @@ -876,11 +875,7 @@ Node tryFoldWhile(Node n) { * Removes FORs that always evaluate to false. */ Node tryFoldFor(Node n) { - Preconditions.checkArgument(n.isFor()); - // If this is a FOR-IN loop skip it. - if (n.isForIn()) { - return n; - } + Preconditions.checkArgument(n.isVanillaFor()); Node init = n.getFirstChild(); Node cond = init.getNext(); diff --git a/src/com/google/javascript/jscomp/RemoveUnusedVars.java b/src/com/google/javascript/jscomp/RemoveUnusedVars.java index 98b6e786908..4f7f02207db 100644 --- a/src/com/google/javascript/jscomp/RemoveUnusedVars.java +++ b/src/com/google/javascript/jscomp/RemoveUnusedVars.java @@ -844,9 +844,7 @@ private void removeUnreferencedVars() { toRemove.getFirstChild().setString(""); } // Don't remove bleeding functions. - } else if (parent != null && - parent.isFor() && - parent.getChildCount() < 4) { + } else if (parent != null && parent.isForIn()) { // foreach iterations have 3 children. Leave them alone. } else if (toRemove.isVar() && nameNode.hasChildren() && diff --git a/src/com/google/javascript/jscomp/RescopeGlobalSymbols.java b/src/com/google/javascript/jscomp/RescopeGlobalSymbols.java index ff2a964905f..00dab1fbda5 100644 --- a/src/com/google/javascript/jscomp/RescopeGlobalSymbols.java +++ b/src/com/google/javascript/jscomp/RescopeGlobalSymbols.java @@ -21,7 +21,6 @@ import com.google.javascript.jscomp.NodeTraversal.Callback; import com.google.javascript.rhino.IR; import com.google.javascript.rhino.Node; - import java.util.ArrayList; import java.util.HashSet; import java.util.List; @@ -461,7 +460,7 @@ public void visit(NodeTraversal t, Node n, Node parent) { if (!c.isName()) { allName = false; } - if (c.isAssign() || parent.isFor()) { + if (c.isAssign() || NodeUtil.isAnyFor(parent)) { interestingChildren.add(c); } } @@ -472,7 +471,7 @@ public void visit(NodeTraversal t, Node n, Node parent) { return; } for (Node c : interestingChildren) { - if (parent.isFor() && parent.getFirstChild() == n) { + if (NodeUtil.isAnyFor(parent) && parent.getFirstChild() == n) { commas.add(c.cloneTree()); } else { // Var statement outside of for-loop. diff --git a/src/com/google/javascript/rhino/Node.java b/src/com/google/javascript/rhino/Node.java index 331a4e574b2..887720daf70 100644 --- a/src/com/google/javascript/rhino/Node.java +++ b/src/com/google/javascript/rhino/Node.java @@ -2955,6 +2955,8 @@ public boolean isFalse() { return this.token == Token.FALSE; } + /** Use isVanillaFor, isForIn, or NodeUtil.isAnyFor instead */ + @Deprecated public boolean isFor() { return this.isVanillaFor() || this.isForIn(); }