Recurse into functions to check for out-of-scope variables in definit…

…ions

When considering inlining a definition like "x = y", FlowSensitiveInlineVariables previously traversed the right hand side "y" to make sure it was safe to inline, but did not traverse into functions on the right hand side.

This CL adds a second traversal of the rhs. It traverses into functions and finds variable references that are not out-of-scope at the point we're inlining into.

Also removes unnecessary logic special-casing "catch(e)".

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=187396431

src/com/google/javascript/jscomp/FlowSensitiveInlineVariables.java

@@ -441,55 +441,7 @@ private boolean canInline(final Scope scope) {
         return false;
       }
 
-      // We give up inlining stuff with R-Value that has:
-      // 1) GETPROP, GETELEM,
-      // 2) anything that creates a new object.
-      // 3) a direct reference to a catch expression.
-      // 4) a reference to a variable not defined in the use's scope
-      // Example:
-      // var x = a.b.c; j.c = 1; print(x);
-      // Inlining print(a.b.c) is not safe - consider if j were an alias to a.b.
-      // TODO(user): We could get more accuracy by looking more in-detail
-      // what j is and what x is trying to into to.
-      // TODO(johnlenz): rework catch expression handling when we
-      // have lexical scope support so catch expressions don't
-      // need to be special cased.
-      if (NodeUtil.has(
-          def.getLastChild(),
-          new Predicate<Node>() {
-            @Override
-            public boolean apply(Node input) {
-              switch (input.getToken()) {
-                case GETELEM:
-                case GETPROP:
-                case ARRAYLIT:
-                case OBJECTLIT:
-                case REGEXP:
-                case NEW:
-                  return true;
-                case NAME:
-                  Var var = scope.getSlot(input.getString());
-                  if (var != null && var.getParentNode().isCatch()) {
-                    return true;
-                  } else if (var == null) {
-                    return true;
-                  }
-                  // fall through
-                default:
-                  break;
-              }
-              return false;
-            }
-          },
-          new Predicate<Node>() {
-            @Override
-            public boolean apply(Node input) {
-              // Recurse if the node is not a function.
-              // TODO(b/73559627): we need to check inside functions for references to block-scoped
-              // variables.
-              return !input.isFunction();
-            }
-          })) {
+      if (!isRhsSafeToInline(scope)) {
         return false;
       }
 
@@ -622,6 +574,77 @@ private boolean isAssignChain(Node child, Node ancestor) {
 
       NodeTraversal.traverseEs6(compiler, cfgNode, gatherCb);
     }
+
+    /**
+     * Check if the definition we're considering inline has anything that makes inlining unsafe
+     * (that hasn't already been caught).
+     *
+     * @param usageScope The scope we will inline the variable into.
+     */
+    private boolean isRhsSafeToInline(Scope usageScope) {
+      // Don't inline definitions with an R-Value that has:
+      // 1) GETPROP, GETELEM,
+      // 2) anything that creates a new object.
+      // Example:
+      // var x = a.b.c; j.c = 1; print(x);
+      // Inlining print(a.b.c) is not safe - consider if j were an alias to a.b.
+      if (NodeUtil.has(
+          def.getLastChild(),
+          new Predicate<Node>() {
+            @Override
+            public boolean apply(Node input) {
+              switch (input.getToken()) {
+                case GETELEM:
+                case GETPROP:
+                case ARRAYLIT:
+                case OBJECTLIT:
+                case REGEXP:
+                case NEW:
+                  return true; // unsafe to inline.
+                default:
+                  break;
+              }
+              return false;
+            }
+          },
+          new Predicate<Node>() {
+            @Override
+            public boolean apply(Node input) {
+              // Recurse if the node is not a function.
+              return !input.isFunction();
+            }
+          })) {
+        return false;
+      }
+
+      // Don't inline definitions with an rvalue referencing names that are not declared in the
+      // usage's scope. (Unlike the above check, this includes names referenced inside function
+      // expressions in the rvalue).
+      // e.g. the name "a" below in the definition of "b":
+      //   {
+      //     let a = 3;
+      //     var b = a;
+      //   }
+      // return b;   // "a" is not declared in this scope so we can't inline this to "return a;"
+      if (NodeUtil.has(
+          def.getLastChild(),
+          new Predicate<Node>() {
+            @Override
+            public boolean apply(Node input) {
+              if (input.isName()) {
+                String name = input.getString();
+                if (!name.isEmpty() && !usageScope.isDeclared(name, true)) {
+                  return true; // unsafe to inline.
+                }
+              }
+              return false;
+            }
+          },
+          Predicates.<Node>alwaysTrue())) {
+        return false;
+      }
+      return true;
+    }
   }
 
   /**

test/com/google/javascript/jscomp/FlowSensitiveInlineVariablesTest.java

@@ -319,6 +319,10 @@ public void testInlineExpressions13() {
            "var k = z;");
   }
 
+  public void testInlineExpressions14() {
+    inline("var a = function() {}; var b = a;", "var a; var b = function() {}");
+  }
+
   public void testNoInlineIfDefinitionMayNotReach() {
     noInline("var x; if (x=1) {} x;");
   }
@@ -345,8 +349,8 @@ public void testCatch() {
     noInline("try { } catch (x) { print(x) }");
   }
 
-  public void testNoInlineGetProp() {
-    // We don't know if j alias a.b
+  public void testNoInlineGetProp1() {
+    // We don't know if j aliases a.b
     noInline("var x = a.b.c; j.c = 1; print(x);");
   }
 
@@ -356,11 +360,12 @@ public void testNoInlineGetProp2() {
 
   public void testNoInlineGetProp3() {
     // Anything inside a function is fine.
-    inline("var x = function(){1 * a.b.c}; print(x);",
-           "var x; print(function(){1 * a.b.c});");
+    inline(
+        "var a = {b: {}}; var x = function(){1 * a.b.c}; print(x);",
+        "var a = {b: {}}; var x; print(function(){1 * a.b.c});");
   }
 
-  public void testNoInlineGetEle() {
+  public void testNoInlineGetElem() {
     // Again we don't know if i = j
     noInline("var x = a[i]; a[j] = 2; print(x); ");
   }
@@ -758,11 +763,7 @@ public void testBlockScoping_shouldntInline() {
         "}",
         "alert(JSCompiler_inline_result);"));
 
-    // TODO(b/73559627): don't inline "g" because "value" is not defined out of the block.
-    // This causes a VarCheck error in a full compile.
-    inline(
-        lines("{ let value = 1; var g = () => value; } return g;"),
-        lines("{ let value = 1; var g;               } return () => value;"));
+    noInline("{ let value = 1; var g = () => value; } return g;");
   }
 
   public void testInlineInGenerators() {