Repository: jshint/jshint
Pull request #1989 (2014-11-18 22:14:05)

Fat arrow args 5
In gh-1984, @caitp proposed a quick solution to gh-1983. This is an attempt to resolve the same issue in a more fundamental way. It includes an additional test that demonstrates how this approach addresses a more fundamental issue.

I reused @caitp's commit to preserve the unit test, refactored the parenthesis-balancing logic, and included the fix itself in a separate commit. Here's the message for that last commit:

> Re-use code to parse "fat arrow" function params
>
> Prior to this commit, JSHint parsed "fat arrow" function parameters as though they were expressions in a list. This had unintended side-effects relating to:
>
> 1. the scope of the declared variables. This problem was only partially addressed through manual modification of the data structure for global variables.
> 2. the syntax accepted within the arguments list. Most significantly, "default parameters" expressions were misinterpreted.
>
> By re-using the `doFunction` function, the same logic can be used when parsing parameters of both "traditional" functions and "fat arrow functions".
>
> This change requires supplying additional context to `doFunction` because the parser may have already consumed certain tokens depending on the form of the expression.
>
> Fixes gh-1983


Patch:
diff --git a/src/jshint.js b/src/jshint.js
index 0d0f3440..6fd9eecf 100644
--- a/src/jshint.js
+++ b/src/jshint.js
@@ -1169,14 +1169,10 @@ var JSHINT = (function () {
     var x = symbol(s, 42);
 
     x.led = function (left) {
-      if (!state.option.esnext) {
-        warning("W119", state.tokens.curr, "arrow function syntax (=>)");
-      }
-
       nobreaknonadjacent(state.tokens.prev, state.tokens.curr);
 
       this.left = left;
-      this.right = doFunction(undefined, undefined, false, left);
+      this.right = doFunction(undefined, undefined, false, { loneArg: left });
       return this;
     };
     return x;
@@ -1455,6 +1451,14 @@ var JSHINT = (function () {
       }
     } else {
       error("E030", state.tokens.next, state.tokens.next.value);
+
+      // The token should be consumed after a warning is issued so the parser
+      // can continue as though an identifier were found. The semicolon token
+      // should not be consumed in this way so that the parser interprets it as
+      // a statement delimeter;
+      if (state.tokens.next.id !== ";") {
+        advance();
+      }
     }
   }
 
@@ -1877,6 +1881,18 @@ var JSHINT = (function () {
       var f;
       var block;
 
+      // If this identifier is the lone parameter to a shorthand "fat arrow"
+      // function definition, i.e.
+      //
+      //     x => x;
+      //
+      // ...it should not be considered as a variable in the current scope. It
+      // will be added to the scope of the new function when the next token is
+      // parsed, so it can be safely ignored for now.
+      if (state.tokens.next.id === "=>") {
+        return this;
+      }
+
       if (typeof s === "function") {
         // Protection against accidental inheritance.
         s = undefined;
@@ -2427,8 +2443,8 @@ var JSHINT = (function () {
       }
       if (!left.identifier && left.id !== "." && left.id !== "[" &&
           left.id !== "(" && left.id !== "&&" && left.id !== "||" &&
-          left.id !== "?" && !(state.option.esnext && left.id === "=>")) {
-        warning("W067", left);
+          left.id !== "?" && !(state.option.esnext && left["(name)"])) {
+        warning("W067", that);
       }
     }
 
@@ -2438,13 +2454,11 @@ var JSHINT = (function () {
 
   prefix("(", function () {
     var bracket, brackets = [];
-    var pn, pn1, i = 0;
+    var pn = state.tokens.next, pn1, i = -1;
     var ret, triggerFnExpr;
     var parens = 1;
 
     do {
-      pn = peek(i);
-
       if (pn.value === "(") {
         parens += 1;
       } else if (pn.value === ")") {
@@ -2452,19 +2466,27 @@ var JSHINT = (function () {
       }
 
       i += 1;
-      pn1 = peek(i);
-    } while (!(parens === 0 && pn.value === ")") &&
-             pn1.value !== "=>" && pn1.value !== ";" && pn1.type !== "(end)");
+      pn1 = pn;
+      pn = peek(i);
+    } while (!(parens === 0 && pn1.value === ")") &&
+             pn.value !== ";" && pn.type !== "(end)");
 
     if (state.tokens.next.id === "function") {
       triggerFnExpr = state.tokens.next.immed = true;
     }
 
+    // If the balanced grouping operator is followed by a "fat arrow", the
+    // current token marks the beginning of a "fat arrow" function and parsing
+    // should proceed accordingly.
+    if (pn.value === "=>") {
+      return doFunction(null, null, null, { parsedParen: true });
+    }
+
     var exprs = [];
 
     if (state.tokens.next.id !== ")") {
       for (;;) {
-        if (pn1.value === "=>" && _.contains(["{", "["], state.tokens.next.value)) {
+        if (pn.value === "=>" && _.contains(["{", "["], state.tokens.next.value)) {
           bracket = state.tokens.next;
           bracket.left = destructuringExpression();
           brackets.push(bracket);
@@ -2682,40 +2704,25 @@ var JSHINT = (function () {
     return id;
   }
 
-  function functionparams(parsed) {
-    var curr, next;
+  function functionparams(fatarrow) {
+    var next;
     var params = [];
     var ident;
     var tokens = [];
     var t;
     var pastDefault = false;
+    var loneArg = fatarrow && fatarrow.loneArg;
 
-    if (parsed) {
-      if (Array.isArray(parsed)) {
-        for (var i in parsed) {
-          curr = parsed[i];
-          if (curr.value === "...") {
-            if (!state.option.esnext) {
-              warning("W119", curr, "spread/rest operator");
-            }
-            continue;
-          } else if (curr.value !== ",") {
-            params.push(curr.value);
-            addlabel(curr.value, { type: "unused", token: curr, param: true });
-          }
-        }
-        return params;
-      } else {
-        if (parsed.identifier === true) {
-          addlabel(parsed.value, { type: "unused", token: parsed, param: true });
-          return [parsed];
-        }
-      }
+    if (loneArg && loneArg.identifier === true) {
+      addlabel(loneArg.value, { type: "unused", token: loneArg });
+      return [loneArg];
     }
 
     next = state.tokens.next;
 
-    advance("(");
+    if (!fatarrow || !fatarrow.parsedParen) {
+      advance("(");
+    }
 
     if (state.tokens.next.id === ")") {
       advance(")");
@@ -2841,7 +2848,18 @@ var JSHINT = (function () {
     };
   }
 
-  function doFunction(name, statement, generator, fatarrowparams) {
+  /**
+   * @param {Object} [fatarrow] In the case that the function being parsed
+   *                            takes the "fat arrow" form, this object will
+   *                            contain details about the in-progress parsing
+   *                            operation.
+   * @param {Token} [fatarrow.loneArg] The argument to the function in cases
+   *                                   where it was defined using the single-
+   *                                   argument shorthand.
+   * @param {bool} [fatarrow.parsedParen] Whether the opening parenthesis has
+   *                                      already been parsed.
+   */
+  function doFunction(name, statement, generator, fatarrow) {
     var f;
     var oldOption = state.option;
     var oldIgnored = state.ignored;
@@ -2866,19 +2884,20 @@ var JSHINT = (function () {
       addlabel(name, { type: "function" });
     }
 
-    funct["(params)"] = functionparams(fatarrowparams);
+    funct["(params)"] = functionparams(fatarrow);
     funct["(metrics)"].verifyMaxParametersPerFunction(funct["(params)"]);
 
-    // So we parse fat-arrow functions after we encounter =>. So basically
-    // doFunction is called with the left side of => as its last argument.
-    // This means that the parser, at that point, had already added its
-    // arguments to the undefs array and here we undo that.
+    if (fatarrow) {
+      if (!state.option.esnext) {
+        warning("W119", state.tokens.curr, "arrow function syntax (=>)");
+      }
 
-    JSHINT.undefs = _.filter(JSHINT.undefs, function (item) {
-      return !_.contains(_.union(fatarrowparams), item[2]);
-    });
+      if (!fatarrow.loneArg) {
+        advance("=>");
+      }
+    }
 
-    block(false, true, true, fatarrowparams ? true : false);
+    block(false, true, true, !!fatarrow);
 
     if (!state.option.noyield && generator &&
         funct["(generator)"] !== "yielded") {


Test patch:
diff --git a/tests/unit/parser.js b/tests/unit/parser.js
index 66ead85b..57698a2e 100644
--- a/tests/unit/parser.js
+++ b/tests/unit/parser.js
@@ -821,8 +821,6 @@ exports["destructuring var in function scope"] = function (test) {
     .addError(9,  "'a' is already defined.")
     .addError(9,  "'bar' is already defined.")
     .addError(10,  "Expected an identifier and instead saw '1'.")
-    .addError(10,  "Expected ',' and instead saw '1'.")
-    .addError(10,  "Expected an identifier and instead saw ']'.")
     .addError(11, "Expected ',' and instead saw ';'.")
     .addError(11, "'a' is already defined.")
     .addError(11, "'b' is already defined.")
@@ -997,8 +995,6 @@ exports["destructuring var errors"] = function (test) {
 
   TestRun(test)
     .addError(9,  "Expected an identifier and instead saw '1'.")
-    .addError(9,  "Expected ',' and instead saw '1'.")
-    .addError(9,  "Expected an identifier and instead saw ']'.")
     .addError(10, "Expected ',' and instead saw ';'.")
     .addError(11, "Expected ']' to match '[' from line 11 and instead saw ';'.")
     .addError(11, "Missing semicolon.")
@@ -1218,8 +1214,6 @@ exports["destructuring const errors"] = function (test) {
     .addError(2, "const 'b' has already been declared.")
     .addError(2, "const 'c' has already been declared.")
     .addError(3, "Expected an identifier and instead saw '1'.")
-    .addError(3, "Expected ',' and instead saw '1'.")
-    .addError(3, "Expected an identifier and instead saw ']'.")
     .addError(4, "Expected ',' and instead saw ';'.")
     .addError(5, "Expected ']' to match '[' from line 5 and instead saw ';'.")
     .addError(5, "Missing semicolon.")
@@ -3700,6 +3694,53 @@ exports["fat arrows support"] = function (test) {
   test.done();
 };
 
+exports["fat arrow nested function scoping"] = function (test) {
+  var code = [
+    "(() => {",
+    "  for (var i = 0; i < 10; i++) {",
+    "    var x;",
+    "  }",
+    "  var arrow = (x) => {",
+    "    return x;",
+    "  };",
+    "  var arrow2 = (x) => x;",
+    "  arrow();",
+    "  arrow2();",
+    "})();",
+    "(function() {",
+    "  for (var i = 0; i < 10; i++) {",
+    "    var x;",
+    "  }",
+    "  var arrow = (x) => {",
+    "    return x;",
+    "  };",
+    "  var arrow2 = (x) => x;",
+    "  arrow();",
+    "  arrow2();",
+    "})();"
+  ];
+
+  TestRun(test)
+    .test(code, {esnext: true});
+
+  test.done();
+};
+
+exports["default arguments in fat arrow functions"] = function (test) {
+  TestRun(test)
+    .test("(x = 0) => { return x; };", { expr: true, unused: true, esnext: true });
+
+  test.done();
+};
+
+exports["expressions in place of arrow function parameters"] = function (test) {
+  TestRun(test)
+    .addError(1, "Expected an identifier and instead saw '1'.")
+    .test("(1) => {};", { expr: true, esnext: true });
+
+  test.done();
+};
+
 var conciseMethods = exports.conciseMethods = {};
 
 conciseMethods.basicSupport = function (test) {