Add and use AstAnalyzer.mayHaveSideEffects()

Right now this just calls the NodeUtil version. I will move the actual implementation once I can clean up all callers to the method that don't pass in the compiler. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=247472160
google · May 9, 2019 · 964833f · 964833f
1 parent 0b44d2b
commit 964833f
Show file tree

Hide file tree

Showing 17 changed files with 541 additions and 55 deletions.
diff --git a/src/com/google/javascript/jscomp/AbstractPeepholeOptimization.java b/src/com/google/javascript/jscomp/AbstractPeepholeOptimization.java
@@ -92,8 +92,7 @@ protected boolean mayEffectMutableState(Node n) {
 
   /** Returns whether the node may have side effects when executed. */
   protected boolean mayHaveSideEffects(Node n) {
-    checkNotNull(compiler);
-    return NodeUtil.mayHaveSideEffects(n, compiler);
+    return astAnalyzer.mayHaveSideEffects(n);
   }
 
   /**

diff --git a/src/com/google/javascript/jscomp/AstAnalyzer.java b/src/com/google/javascript/jscomp/AstAnalyzer.java
@@ -57,4 +57,14 @@ boolean mayEffectMutableState(Node n) {
     // TODO(bradfordcsmith): Move the implementation into this class when possible.
     return NodeUtil.mayEffectMutableState(n, compiler);
   }
+
+  /**
+   * Returns true if the node which may have side effects when executed. This version default to the
+   * "safe" assumptions when the compiler object is not provided (RegExp have side-effects, etc).
+   */
+  public boolean mayHaveSideEffects(Node n) {
+    // TODO(b/131178806): Move implementation here when it is possible to
+    //     remove the mayHaveSideEffects(n) version
+    return NodeUtil.mayHaveSideEffects(n, compiler);
+  }
 }
diff --git a/src/com/google/javascript/jscomp/CheckSideEffects.java b/src/com/google/javascript/jscomp/CheckSideEffects.java
@@ -118,7 +118,7 @@ public void visit(NodeTraversal t, Node n, Node parent) {
     boolean isResultUsed = NodeUtil.isExpressionResultUsed(n);
     boolean isSimpleOp = NodeUtil.isSimpleOperator(n);
     if (!isResultUsed) {
-      if (isSimpleOp || !NodeUtil.mayHaveSideEffects(n, t.getCompiler())) {
+      if (isSimpleOp || !t.getCompiler().getAstAnalyzer().mayHaveSideEffects(n)) {
         if (report) {
           String msg = "This code lacks side-effects. Is there a bug?";
           if (n.isString() || n.isTemplateLit()) {

diff --git a/src/com/google/javascript/jscomp/Es6RewriteClassExtendsExpressions.java b/src/com/google/javascript/jscomp/Es6RewriteClassExtendsExpressions.java
@@ -124,7 +124,7 @@ private boolean canDecomposeSimply(Node classNode) {
         Node lhsNode = classNodeParent.getFirstChild();
         // We can extract a temporary variable for some_expression as long as lhs expression
         // has no side effects.
-        return !NodeUtil.mayHaveSideEffects(lhsNode, compiler);
+        return !compiler.getAstAnalyzer().mayHaveSideEffects(lhsNode);
       } else {
         return false;
       }

diff --git a/src/com/google/javascript/jscomp/Es6RewriteRestAndSpread.java b/src/com/google/javascript/jscomp/Es6RewriteRestAndSpread.java
@@ -352,7 +352,7 @@ private void visitCallContainingSpread(Node spreadParent) {
     Node callee = spreadParent.getFirstChild();
     // Check if the callee has side effects before removing it from the AST (since some NodeUtil
     // methods assume the node they are passed has a non-null parent).
-    boolean calleeMayHaveSideEffects = NodeUtil.mayHaveSideEffects(callee, compiler);
+    boolean calleeMayHaveSideEffects = compiler.getAstAnalyzer().mayHaveSideEffects(callee);
     // Must remove callee before extracting argument groups.
     spreadParent.removeChild(callee);
 

diff --git a/src/com/google/javascript/jscomp/ExpressionDecomposer.java b/src/com/google/javascript/jscomp/ExpressionDecomposer.java
@@ -62,6 +62,7 @@ enum DecompositionType {
   }
 
   private final AbstractCompiler compiler;
+  private final AstAnalyzer astAnalyzer;
   private final AstFactory astFactory;
   private final Supplier<String> safeNameIdSupplier;
   private final Set<String> knownConstants;
@@ -86,6 +87,7 @@ enum DecompositionType {
     checkNotNull(safeNameIdSupplier);
     checkNotNull(constNames);
     this.compiler = compiler;
+    this.astAnalyzer = compiler.getAstAnalyzer();
     this.astFactory = compiler.createAstFactory();
     this.safeNameIdSupplier = safeNameIdSupplier;
     this.knownConstants = constNames;
@@ -164,7 +166,7 @@ void exposeExpression(Node expression) {
   private void exposeExpression(Node expressionRoot, Node subExpression) {
     Node nonconditionalExpr = findNonconditionalParent(subExpression, expressionRoot);
     // Before extraction, record whether there are side-effect
-    boolean hasFollowingSideEffects = NodeUtil.mayHaveSideEffects(nonconditionalExpr, compiler);
+    boolean hasFollowingSideEffects = astAnalyzer.mayHaveSideEffects(nonconditionalExpr);
 
     Node exprInjectionPoint = findInjectionPoint(nonconditionalExpr);
     DecompositionState state = new DecompositionState();
@@ -860,7 +862,7 @@ DecompositionType canExposeExpression(Node subExpression) {
   /** @see {@link #canDecomposeExpression} */
   private DecompositionType isSubexpressionMovable(Node expressionRoot, Node subExpression) {
     boolean requiresDecomposition = false;
-    boolean seenSideEffects = NodeUtil.mayHaveSideEffects(subExpression, compiler);
+    boolean seenSideEffects = astAnalyzer.mayHaveSideEffects(subExpression);
 
     Node child = subExpression;
     for (Node parent : child.getAncestors()) {
@@ -1088,7 +1090,7 @@ && isTempConstantValueName(tree.getFirstChild())) {
     } else {
       // The function called doesn't have side-effects but check to see if there
       // are side-effects that that may affect it.
-      return NodeUtil.mayHaveSideEffects(tree, compiler);
+      return astAnalyzer.mayHaveSideEffects(tree);
     }
   }
 }
diff --git a/src/com/google/javascript/jscomp/FlowSensitiveInlineVariables.java b/src/com/google/javascript/jscomp/FlowSensitiveInlineVariables.java
@@ -417,7 +417,7 @@ private boolean canInline(final Scope scope) {
       // TODO(user): Side-effect is OK sometimes. As long as there are no
       // side-effect function down all paths to the use. Once we have all the
       // side-effect analysis tool.
-      if (NodeUtil.mayHaveSideEffects(def.getLastChild(), compiler)) {
+      if (compiler.getAstAnalyzer().mayHaveSideEffects(def.getLastChild())) {
         return false;
       }
 

diff --git a/src/com/google/javascript/jscomp/FunctionArgumentInjector.java b/src/com/google/javascript/jscomp/FunctionArgumentInjector.java
@@ -96,7 +96,7 @@ private static Node inject(
         // Remove the value.  This isn't required but it ensures that we won't
         // inject side-effects multiple times as it will trigger the null
         // check above if we do.
-        if (NodeUtil.mayHaveSideEffects(replacementTemplate, compiler)) {
+        if (compiler.getAstAnalyzer().mayHaveSideEffects(replacementTemplate)) {
           replacements.remove(THIS_MARKER);
         }
 
@@ -304,7 +304,7 @@ static void maybeAddTempsForCallArguments(
       boolean safe = true;
       int references = NodeUtil.getNameReferenceCount(block, argName);
 
-      boolean argSideEffects = NodeUtil.mayHaveSideEffects(cArg, compiler);
+      boolean argSideEffects = compiler.getAstAnalyzer().mayHaveSideEffects(cArg);
       if (!argSideEffects && references == 0) {
         safe = true;
       } else if (isTrivialBody && hasMinimalParameters

diff --git a/src/com/google/javascript/jscomp/FunctionToBlockMutator.java b/src/com/google/javascript/jscomp/FunctionToBlockMutator.java
@@ -340,17 +340,15 @@ private Node aliasAndInlineArguments(
 
             Node value = entry.getValue();
             if (!value.isThis()
-                && (referencesThis
-                    || NodeUtil.mayHaveSideEffects(value, compiler))) {
+                && (referencesThis || compiler.getAstAnalyzer().mayHaveSideEffects(value))) {
               String newName = getUniqueThisName();
               Node newValue = entry.getValue().cloneTree();
-              Node newNode = NodeUtil.newVarNode(newName, newValue)
-                  .useSourceInfoIfMissingFromForTree(newValue);
+              Node newNode =
+                  NodeUtil.newVarNode(newName, newValue)
+                      .useSourceInfoIfMissingFromForTree(newValue);
               newVars.add(0, newNode);
               // Remove the parameter from the list to replace.
-              newArgMap.put(THIS_MARKER,
-                  IR.name(newName)
-                      .srcrefTree(newValue));
+              newArgMap.put(THIS_MARKER, IR.name(newName).srcrefTree(newValue));
             }
           } else {
             Node newValue = entry.getValue().cloneTree();

diff --git a/src/com/google/javascript/jscomp/InlineProperties.java b/src/com/google/javascript/jscomp/InlineProperties.java
@@ -240,7 +240,7 @@ public void visit(NodeTraversal t, Node n, Node parent) {
             && info != INVALIDATED
             && isMatchingType(target, info.type)) {
           Node replacement = info.value.cloneTree();
-          if (NodeUtil.mayHaveSideEffects(n.getFirstChild(), compiler)) {
+          if (compiler.getAstAnalyzer().mayHaveSideEffects(n.getFirstChild())) {
             replacement = IR.comma(n.removeFirstChild(), replacement).srcref(n);
           }
           parent.replaceChild(n, replacement);

diff --git a/src/com/google/javascript/jscomp/InlineSimpleMethods.java b/src/com/google/javascript/jscomp/InlineSimpleMethods.java
@@ -59,8 +59,11 @@ class InlineSimpleMethods extends MethodCompilerPass {
   private static final Logger logger =
       Logger.getLogger(InlineSimpleMethods.class.getName());
 
+  private final AstAnalyzer astAnalyzer;
+
   InlineSimpleMethods(AbstractCompiler compiler) {
     super(compiler);
+    astAnalyzer = compiler.getAstAnalyzer();
   }
 
   @Override
@@ -103,14 +106,14 @@ void visit(NodeTraversal t, Node callNode, Node parent, String callName) {
               }
               inlinePropertyReturn(parent, callNode, returned);
             } else if (NodeUtil.isLiteralValue(returned, false)
-                && !NodeUtil.mayHaveSideEffects(callNode.getFirstChild(), compiler)) {
+                && !astAnalyzer.mayHaveSideEffects(callNode.getFirstChild())) {
               if (logger.isLoggable(Level.FINE)) {
                 logger.fine("Inlining constant accessor: " + callName);
               }
               inlineConstReturn(parent, callNode, returned);
             }
           } else if (isEmptyMethod(firstDefinition)
-              && !NodeUtil.mayHaveSideEffects(callNode.getFirstChild(), compiler)) {
+              && !astAnalyzer.mayHaveSideEffects(callNode.getFirstChild())) {
             if (logger.isLoggable(Level.FINE)) {
               logger.fine("Inlining empty method: " + callName);
             }
@@ -256,7 +259,7 @@ private boolean argsMayHaveSideEffects(Node call) {
     for (Node currentChild = call.getSecondChild();
          currentChild != null;
          currentChild = currentChild.getNext()) {
-      if (NodeUtil.mayHaveSideEffects(currentChild, compiler)) {
+      if (astAnalyzer.mayHaveSideEffects(currentChild)) {
         return true;
       }
     }

diff --git a/src/com/google/javascript/jscomp/NodeIterators.java b/src/com/google/javascript/jscomp/NodeIterators.java
@@ -198,7 +198,7 @@ private LocalVarMotion(
       this.compiler = checkNotNull(compiler);
       this.varName = nameNode.getString();
       this.valueHasSideEffects =
-          valueNode != null && NodeUtil.mayHaveSideEffects(valueNode, compiler);
+          valueNode != null && compiler.getAstAnalyzer().mayHaveSideEffects(valueNode);
       this.iterator = iterator;
       advanceLookAhead(true);
     }

diff --git a/src/com/google/javascript/jscomp/OptimizeParameters.java b/src/com/google/javascript/jscomp/OptimizeParameters.java
@@ -16,6 +16,7 @@
 
 package com.google.javascript.jscomp;
 
+import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
 
 import com.google.common.annotations.VisibleForTesting;
@@ -46,10 +47,12 @@
  */
 class OptimizeParameters implements CompilerPass, OptimizeCalls.CallGraphCompilerPass {
   private final AbstractCompiler compiler;
+  private final AstAnalyzer astAnalyzer;
   private Scope globalScope;
 
   OptimizeParameters(AbstractCompiler compiler) {
-    this.compiler = compiler;
+    this.compiler = checkNotNull(compiler);
+    this.astAnalyzer = compiler.getAstAnalyzer();
   }
 
   @Override
@@ -238,7 +241,7 @@ void tryEliminateUnusedArgs(ArrayList<Node> refs) {
               break;
             }
 
-            if (!unremovable.get(paramIndex) && NodeUtil.mayHaveSideEffects(param, compiler)) {
+            if (!unremovable.get(paramIndex) && astAnalyzer.mayHaveSideEffects(param)) {
               unremovable.set(paramIndex);
             }
 
@@ -301,7 +304,7 @@ void recordRemovalCallArguments(
           arg.getNext(), index + 1);
 
       if (index < firstRestIndex && unused.get(index)) {
-        if (!NodeUtil.mayHaveSideEffects(arg, compiler)) {
+        if (!astAnalyzer.mayHaveSideEffects(arg)) {
           if (unremovable.get(index)) {
             if (!arg.isNumber() || arg.getDouble() != 0) {
               toReplaceWithZero.add(arg);
@@ -316,7 +319,7 @@ void recordRemovalCallArguments(
     void removeArgAndFollowing(Node arg) {
       if (arg != null) {
         removeArgAndFollowing(arg.getNext());
-        if (!NodeUtil.mayHaveSideEffects(arg, compiler)) {
+        if (!astAnalyzer.mayHaveSideEffects(arg)) {
           toRemove.add(arg);
         }
       }
@@ -731,7 +734,7 @@ private boolean buildInitialParameterList(List<Parameter> parameters, Node cur)
   }
 
   private void setParameterSideEffectInfo(Parameter p, Node value) {
-    p.setHasSideEffects(NodeUtil.mayHaveSideEffects(value, compiler));
+    p.setHasSideEffects(astAnalyzer.mayHaveSideEffects(value));
     p.setCanBeSideEffected(NodeUtil.canBeSideEffected(value));
     p.setMayBeUndefined(NodeUtil.mayBeUndefined(value));
   }