Remove MarkNoSideEffects in favor of PureFunctionIdentifier. MarkNoSi…

…deEffects was only used for unit tests but encouraged confusing tests. This leaves PureFunctionIdentifier as the last remaining use of NameBasedDefinitionProvider. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=240792582
google · Mar 28, 2019 · 0797e0f · 0797e0f
1 parent 433e128
commit 0797e0f
Show file tree

Hide file tree

Showing 13 changed files with 56 additions and 618 deletions.
diff --git a/src/com/google/javascript/jscomp/CompilerOptions.java b/src/com/google/javascript/jscomp/CompilerOptions.java
@@ -808,9 +808,6 @@ public void setReplaceMessagesWithChromeI18n(
   protected transient
       Multimap<CustomPassExecutionTime, CompilerPass> customPasses;
 
-  /** Mark no side effect calls */
-  public boolean markNoSideEffectCalls;
-
   /** Replacements for @defines. Will be Boolean, Numbers, or Strings */
   private Map<String, Object> defineReplacements;
 
@@ -1296,7 +1293,6 @@ public CompilerOptions() {
     stripNamePrefixes = ImmutableSet.of();
     stripTypePrefixes = ImmutableSet.of();
     customPasses = null;
-    markNoSideEffectCalls = false;
     defineReplacements = new HashMap<>();
     tweakProcessing = TweakProcessing.OFF;
     tweakReplacements = new HashMap<>();
@@ -2475,10 +2471,6 @@ public void addCustomPass(CustomPassExecutionTime time, CompilerPass customPass)
     customPasses.put(time, customPass);
   }
 
-  public void setMarkNoSideEffectCalls(boolean markNoSideEffectCalls) {
-    this.markNoSideEffectCalls = markNoSideEffectCalls;
-  }
-
   public void setDefineReplacements(Map<String, Object> defineReplacements) {
     this.defineReplacements = defineReplacements;
   }
@@ -2939,7 +2931,6 @@ public String toString() {
             .add("lineLengthThreshold", lineLengthThreshold)
             .add("locale", locale)
             .add("markAsCompiled", markAsCompiled)
-            .add("markNoSideEffectCalls", markNoSideEffectCalls)
             .add("maxFunctionSizeAfterInlining", maxFunctionSizeAfterInlining)
             .add("messageBundle", messageBundle)
             .add("moduleRoots", moduleRoots)

diff --git a/src/com/google/javascript/jscomp/DefaultPassConfig.java b/src/com/google/javascript/jscomp/DefaultPassConfig.java
@@ -651,17 +651,6 @@ protected List<PassFactory> getOptimizations() {
 
     if (options.computeFunctionSideEffects) {
       passes.add(markPureFunctions);
-    } else if (options.markNoSideEffectCalls) {
-      // TODO(user) The properties that this pass adds to CALL and NEW
-      // AST nodes increase the AST's in-memory size.  Given that we are
-      // already running close to our memory limits, we could run into
-      // trouble if we end up using the @nosideeffects annotation a lot
-      // or compute @nosideeffects annotations by looking at function
-      // bodies.  It should be easy to propagate @nosideeffects
-      // annotations as part of passes that depend on this property and
-      // store the result outside the AST (which would allow garbage
-      // collection once the pass is done).
-      passes.add(markNoSideEffectCalls);
     }
 
     if (options.smartNameRemoval) {
@@ -2555,20 +2544,6 @@ protected FeatureSet featureSet() {
         }
       };
 
-  /** Look for function calls that have no side effects, and annotate them that way. */
-  private final PassFactory markNoSideEffectCalls =
-      new PassFactory(PassNames.MARK_NO_SIDE_EFFECT_CALLS, true) {
-        @Override
-        protected CompilerPass create(AbstractCompiler compiler) {
-          return new MarkNoSideEffectCalls(compiler);
-        }
-
-        @Override
-        protected FeatureSet featureSet() {
-          return ES5;
-        }
-      };
-
   /** Inlines variables heuristically. */
   private final PassFactory inlineVariables =
       new PassFactory(PassNames.INLINE_VARIABLES, false) {

diff --git a/src/com/google/javascript/jscomp/MarkNoSideEffectCalls.java b/src/com/google/javascript/jscomp/MarkNoSideEffectCalls.java
diff --git a/src/com/google/javascript/jscomp/PassNames.java b/src/com/google/javascript/jscomp/PassNames.java
@@ -70,7 +70,6 @@ public final class PassNames {
   public static final String INLINE_TYPE_ALIASES = "inlineTypeAliases";
   public static final String INLINE_VARIABLES = "inlineVariables";
   public static final String LINT_CHECKS = "lintChecks";
-  public static final String MARK_NO_SIDE_EFFECT_CALLS = "markNoSideEffectCalls";
   public static final String MOVE_FUNCTION_DECLARATIONS = "moveFunctionDeclarations";
   public static final String NAME_ANONYMOUS_FUNCTIONS = "nameAnonymousFunctions";
   public static final String NORMALIZE = "normalize";

diff --git a/src/com/google/javascript/jscomp/debugger/common/CompilationParam.java b/src/com/google/javascript/jscomp/debugger/common/CompilationParam.java
@@ -453,18 +453,6 @@ public boolean isApplied(CompilerOptions options) {
     }
   },
 
-  MARK_NO_SIDE_EFFECT_CALLS(ParamGroup.OPTIMIZATION){
-    @Override
-    public void apply(CompilerOptions options, boolean value) {
-      options.setMarkNoSideEffectCalls(value);
-    }
-
-    @Override
-    public boolean isApplied(CompilerOptions options) {
-      return options.markNoSideEffectCalls;
-    }
-  },
-
   /** Converts quoted property accesses to dot syntax (a['b'] -> a.b) */
   CONVERT_TO_DOTTED_PROPERTIES(ParamGroup.OPTIMIZATION){
     @Override

diff --git a/test/com/google/javascript/jscomp/CompilerTestCase.java b/test/com/google/javascript/jscomp/CompilerTestCase.java
@@ -164,11 +164,6 @@ public abstract class CompilerTestCase {
    */
   private DiagnosticType expectedSymbolTableError;
 
-  /**
-   * Whether the MarkNoSideEffectsCalls pass runs before the pass being tested
-   */
-  private boolean markNoSideEffects;
-
   /**
    * Whether the PureFunctionIdentifier pass runs before the pass being tested
    */
@@ -631,7 +626,6 @@ public void setUp() throws Exception {
     this.verifyNoNewGettersOrSetters = false;
     this.inferConsts = false;
     this.languageOut = LanguageMode.ECMASCRIPT5;
-    this.markNoSideEffects = false;
     this.multistageCompilation = true;
     this.normalizeEnabled = false;
     this.parseTypeInfo = false;
@@ -938,20 +932,10 @@ protected final void disableNormalize() {
     normalizeEnabled = false;
   }
 
-  /**
-   * Run the MarkSideEffectCalls pass before running the test pass.
-   *
-   * @see MarkNoSideEffectCalls
-   */
-  protected final void enableMarkNoSideEffects() {
-    checkState(this.setUpRan, "Attempted to configure before running setUp().");
-    markNoSideEffects = true;
-  }
-
   /**
    * Run the PureFunctionIdentifier pass before running the test pass.
    *
-   * @see MarkNoSideEffectCalls
+   * @see PureFunctionIdentifier
    */
   protected final void enableComputeSideEffects() {
     checkState(this.setUpRan, "Attempted to configure before running setUp().");
@@ -1603,11 +1587,6 @@ private void testInternal(
           hasCodeChanged = hasCodeChanged || recentChange.hasCodeChanged();
         }
 
-        if (markNoSideEffects && i == 0) {
-          MarkNoSideEffectCalls mark = new MarkNoSideEffectCalls(compiler);
-          mark.process(externsRoot, mainRoot);
-        }
-
         if (gatherExternPropertiesEnabled && i == 0) {
           (new GatherExternProperties(compiler)).process(externsRoot, mainRoot);
         }

diff --git a/test/com/google/javascript/jscomp/FlowSensitiveInlineVariablesTest.java b/test/com/google/javascript/jscomp/FlowSensitiveInlineVariablesTest.java
@@ -50,12 +50,11 @@ protected int getNumRepetitions() {
 
   @Override
   protected CompilerPass getProcessor(final Compiler compiler) {
-    //return new FlowSensitiveInlineVariables(compiler);
     return new CompilerPass() {
       @Override
       public void process(Node externs, Node root) {
-        (new MarkNoSideEffectCalls(compiler)).process(externs, root);
-        (new FlowSensitiveInlineVariables(compiler)).process(externs, root);
+        new PureFunctionIdentifier.Driver(compiler).process(externs, root);
+        new FlowSensitiveInlineVariables(compiler).process(externs, root);
       }
     };
   }

diff --git a/test/com/google/javascript/jscomp/FunctionInjectorTest.java b/test/com/google/javascript/jscomp/FunctionInjectorTest.java
@@ -1863,7 +1863,7 @@ public void helperInlineReferenceToFunction(
     final Node expectedRoot = parseExpected(new Compiler(), expectedResult);
 
     Node mainRoot = tree;
-    MarkNoSideEffectCalls mark = new MarkNoSideEffectCalls(compiler);
+    PureFunctionIdentifier.Driver mark = new PureFunctionIdentifier.Driver(compiler);
     mark.process(externsRoot, mainRoot);
 
     Normalize normalize = new Normalize(compiler, false);

diff --git a/test/com/google/javascript/jscomp/FunctionToBlockMutatorTest.java b/test/com/google/javascript/jscomp/FunctionToBlockMutatorTest.java
@@ -288,7 +288,7 @@ public void helperMutate(String code, String expectedResult, String fnName, Stri
     compiler.externsRoot = new Node(Token.ROOT);
     compiler.jsRoot = IR.root(script);
     compiler.externAndJsRoot = IR.root(compiler.externsRoot, compiler.jsRoot);
-    MarkNoSideEffectCalls mark = new MarkNoSideEffectCalls(compiler);
+    PureFunctionIdentifier.Driver mark = new PureFunctionIdentifier.Driver(compiler);
     mark.process(compiler.externsRoot, compiler.jsRoot);
 
     final Node fnNode = findFunction(script, fnName);

diff --git a/test/com/google/javascript/jscomp/InlineFunctionsTest.java b/test/com/google/javascript/jscomp/InlineFunctionsTest.java
@@ -2538,7 +2538,7 @@ public void testIssue5159924b() {
   @Test
   public void testInlineObject() {
     disableCompareAsTree();
-    enableMarkNoSideEffects();
+    enableComputeSideEffects();
 
     this.inliningReach = Reach.LOCAL_ONLY;
     assumeStrictThis = true;