Don't inline variables across scopes

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=170759251
google · Oct 3, 2017 · 4036fdf · 4036fdf
1 parent e81fdef
commit 4036fdf
Show file tree

Hide file tree

Showing 3 changed files with 53 additions and 11 deletions.
diff --git a/src/com/google/javascript/jscomp/InlineVariables.java b/src/com/google/javascript/jscomp/InlineVariables.java
@@ -274,8 +274,7 @@ private void inlineNonConstants(
       } else if (refCount == firstRefAfterInit) {
         // The variable likely only read once, try some more
         // complex inlining heuristics.
-        Reference reference = referenceInfo.references.get(
-            firstRefAfterInit - 1);
+        Reference reference = referenceInfo.references.get(firstRefAfterInit - 1);
         if (canInline(declaration, init, reference)) {
           inline(v, declaration, init, reference);
           staleVars.add(v);
@@ -588,8 +587,12 @@ private boolean canInline(
         }
       }
 
-      return canMoveAggressively(value) ||
-          canMoveModerately(initialization, reference);
+      if (initialization.getScope() != declaration.getScope()
+          || !initialization.getScope().contains(reference.getScope())) {
+        return false;
+      }
+
+      return canMoveAggressively(value) || canMoveModerately(initialization, reference);
     }
 
     /**

diff --git a/src/com/google/javascript/jscomp/Scope.java b/src/com/google/javascript/jscomp/Scope.java
@@ -84,6 +84,20 @@ public int getDepth() {
     return depth;
   }
 
+  /**
+   * @return True if this scope contains {@code other}, or is the same scope as {@code other}.
+   */
+  boolean contains(Scope other) {
+    Scope s = checkNotNull(other);
+    while (s != null) {
+      if (s == this) {
+        return true;
+      }
+      s = s.getParent();
+    }
+    return false;
+  }
+
   /**
    * Gets the container node of the scope. This is typically the FUNCTION
    * node or the global BLOCK/SCRIPT node.

diff --git a/test/com/google/javascript/jscomp/InlineVariablesTest.java b/test/com/google/javascript/jscomp/InlineVariablesTest.java
@@ -54,9 +54,8 @@ public void tearDown() {
     inlineLocalsOnly = false;
   }
 
-  // TODO(tbreisacher): Fix this and re-enable InlineVariables in ES6-out mode.
-  public void testPassProducesInvalidCode() {
-    test(
+  public void testPassDoesntProduceInvalidCode1() {
+    testSame(
         LINE_JOINER.join(
             "function f(x = void 0) {",
             "  var z;",
@@ -66,15 +65,42 @@ public void testPassProducesInvalidCode() {
             "    z = y;",
             "  }",
             "  return z;",
-            "}"),
+            "}"));
+  }
+
+  public void testPassDoesntProduceInvalidCode2() {
+    testSame(
         LINE_JOINER.join(
             "function f(x = void 0) {",
             "  {",
+            "    var z;",
             "    const y = {};",
             "    x && (y['x'] = x);",
+            "    z = y;",
+            "  }",
+            "  return z;",
+            "}"));
+  }
+
+  public void testPassDoesntProduceInvalidCode3() {
+   test(
+        LINE_JOINER.join(
+            "function f(x = void 0) {",
+            "  var z;",
+            "  const y = {};",
+            "  x && (y['x'] = x);",
+            "  z = y;",
+            "  {",
+            "    return z;",
+            "  }",
+            "}"),
+        LINE_JOINER.join(
+            "function f(x = void 0) {",
+            "  const y = {};",
+            "  x && (y['x'] = x);",
+            "  {",
+            "    return y;",
             "  }",
-            "  // NOTE! Invalid code: y is no longer in scope here.",
-            "  return y;",
             "}"));
   }
 
@@ -473,7 +499,6 @@ public void testNoInlineIntoNamedFunction() {
   }
 
   public void testInlineIntoNestedNonHoistedNamedFunctions() {
-    setAcceptedLanguage(CompilerOptions.LanguageMode.ECMASCRIPT_2015);
     test("f(); var x = false; if (false) function f() { alert(x); };",
          "f(); if (false) function f() { alert(false); };");
   }