Don't inline references to arguments in arrow functions

------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=208513497
google · Aug 14, 2018 · fdf4393 · fdf4393
1 parent 12abeaa
commit fdf4393
Show file tree

Hide file tree

Showing 4 changed files with 51 additions and 9 deletions.
diff --git a/src/com/google/javascript/jscomp/FunctionInjector.java b/src/com/google/javascript/jscomp/FunctionInjector.java
@@ -155,9 +155,9 @@ boolean doesFunctionMeetMinimumRequirements(final String fnName, Node fnNode) {
     final String fnRecursionName = fnNode.getFirstChild().getString();
     checkState(fnRecursionName != null);
 
-    // If the function references "arguments" directly in the function
-    boolean referencesArguments = NodeUtil.isNameReferenced(
-        block, "arguments", NodeUtil.MATCH_NOT_FUNCTION);
+    // If the function references "arguments" directly in the function or in an arrow function
+    boolean referencesArguments =
+        NodeUtil.isNameReferenced(block, "arguments", NodeUtil.MATCH_NOT_VANILLA_FUNCTION);
 
     Predicate<Node> blocksInjection =
         new Predicate<Node>() {

diff --git a/src/com/google/javascript/jscomp/NodeUtil.java b/src/com/google/javascript/jscomp/NodeUtil.java
@@ -2292,7 +2292,7 @@ static Node getFirstComputedPropMatchingKey(Node objlit, Node key) {
    */
   static boolean referencesThis(Node n) {
     Node start = (n.isFunction()) ? n.getLastChild() : n;
-    return containsType(start, Token.THIS, MATCH_NOT_THIS_BINDING);
+    return containsType(start, Token.THIS, MATCH_NOT_VANILLA_FUNCTION);
   }
 
   /**
@@ -4571,7 +4571,8 @@ public boolean apply(Node n) {
 
   static final Predicate<Node> MATCH_NOT_FUNCTION = n -> !n.isFunction();
 
-  static final Predicate<Node> MATCH_NOT_THIS_BINDING = n -> !NodeUtil.isVanillaFunction(n);
+  /** A predicate for matching anything except for a vanilla function (i.e. not arrow functions) */
+  static final Predicate<Node> MATCH_NOT_VANILLA_FUNCTION = n -> !NodeUtil.isVanillaFunction(n);
 
   /**
    * A predicate for matching statements without exiting the current scope.

diff --git a/test/com/google/javascript/jscomp/FunctionInjectorTest.java b/test/com/google/javascript/jscomp/FunctionInjectorTest.java
@@ -1485,6 +1485,17 @@ public void testIssue1101b() {
         INLINE_DIRECT);
   }
 
+  public void testArgumentsReferenceInArrowFunction() {
+    assertFalse(
+        doesFunctionMeetMinimumRequirements("function foo() { return () => arguments; }", "foo"));
+  }
+
+  public void testArgumentsReferenceInNestedVanillaFunction() {
+    assertTrue(
+        doesFunctionMeetMinimumRequirements(
+            "function foo() { return function() { return arguments; }; }", "foo"));
+  }
+
   /**
    * Test case
    *
@@ -1627,6 +1638,26 @@ public boolean call(NodeTraversal t, Node n, Node parent) {
     verifier.checkRecordedChanges("helperInlineReferenceToFunction", mainRoot);
   }
 
+  /**
+   * Calls {@link FunctionInjector#doesFunctionMeetMinimumRequirements(String, Node)}
+   *
+   * <p>This method is called as a prerequisite to checking if a particular reference is inlinable
+   */
+  public boolean doesFunctionMeetMinimumRequirements(final String code, final String fnName) {
+    final Compiler compiler = new Compiler();
+    final FunctionInjector injector =
+        new FunctionInjector(
+            compiler,
+            compiler.getUniqueNameIdSupplier(),
+            allowDecomposition,
+            assumeStrictThis,
+            assumeMinimumCapture);
+    final Node tree = parse(compiler, code);
+
+    final Node fnNode = findFunction(tree, fnName);
+    return injector.doesFunctionMeetMinimumRequirements(fnName, fnNode);
+  }
+
   interface Method {
     boolean call(NodeTraversal t, Node n, Node parent);
   }

diff --git a/test/com/google/javascript/jscomp/InlineFunctionsTest.java b/test/com/google/javascript/jscomp/InlineFunctionsTest.java
@@ -423,10 +423,6 @@ public void testInlineFunctions24() {
     testSame("function foo(x){return this}foo(1)");
   }
 
-  public void testInlineFunctions25() {
-    testSame("function foo(){return arguments[0]}foo()");
-  }
-
   public void testInlineFunctions26() {
     // Don't inline external functions
     testSame("function _foo(x){return x}_foo(1)");
@@ -550,6 +546,20 @@ public void testInlineFunctions35() {
             ""));
   }
 
+  public void testDontInlineFunctionsWithDirectArgumentsReferences() {
+    testSame("function foo() { return arguments[0]; } foo(1);");
+  }
+
+  public void testDontInlineFunctionsWithArgumentsReferencesInArrowFunction() {
+    testSame("function foo() { return () => arguments[0]; } foo(1);");
+  }
+
+  public void testDoInlineFunctionsWithArgumentsReferencesInInnerVanillaFunction() {
+    test(
+        "function foo() { return function() { return arguments[0]; } } foo(1);",
+        "(function() { return arguments[0]; })");
+  }
+
   public void testMixedModeInlining1() {
     // Base line tests, direct inlining
     test("function foo(){return 1}" +