Repository: jshint/jshint
Pull request #3434 (2019-11-24 06:02:11)

Re-implement comma parsing improvements
@caitp @rwaldron This is the latest in the series of rewrites (the prior patches along these lines were 6759ac9, gh-3419, and gh-3427).

Since the second commit intentionally regresses functionality, we should definitely use a merge commit to land this branch.

Commit messages:

> [[FIX]] Improve parsing of comma operator
>
> Re-implement the improvements reverted by the previous commit. Use
> distinct logic to differentiate this new implementation, including the
> following substantive improvements:
>
> - Parse the expression recursively instead of iteratively. This
>   eliminates the need to duplicate parsing semantics within the grouping
>   operator, and it produces a more predictable syntax tree.
> - Correct the location of the warning emitted for the use of the result
>   of assignment expressions
>
> This patch also removes the `ltBoundary` property assigned to some
> tokens. This was intended to enforce the line terminator restrictions on
> certain productions, but that is actually implemented within the parsing
> logic for the expressions themselves (and not in the general
> `expression` function).
>
> In order to preserve backwards compatibility without significantly
> refactoring the internals, the `isEndOfExpression` logic includes
> sub-optimal special-casing for YieldExpression. JSHint should honor
> binding powers whenever considering infix operators. Doing so would
> further improve the correctness of the parser, allowing it to reject
> similar cases with other productions, for example:
>
>     () => {} + 0;
>
> However, implementing such a change is well beyond the scope of the
> re-implementation introduced in this commit.

> Revert comma parsing improvements
>
> JSHint's parsing of the "comma operator" is derived from code first
> contributed by GitHub user "usrbincc." That user could not be reached to
> sign JSHint's Contributor License Agreement. Revert the user's
> contribution to the application logic (preserving the corresponding
> MIT-licensed test code) so that it may be re-written under the terms of
> the Contributor License Agreement.
>
> Revert the changes merged to `master` in
> 56d10bf0bb2365347a0a183922cea5d8c0def977.

> [[CHORE]] Refactor utility function
>
> The result of the `startLine` utility function was consistently used to
> compare the lines of adjacent tokens. Refactor the function to accept a
> second token and make this comparison internally. This makes call sites
> simpler and easier to understand.


Patch:
diff --git a/src/jshint.js b/src/jshint.js
index a4c99f61..3caab086 100644
--- a/src/jshint.js
+++ b/src/jshint.js
@@ -901,10 +901,16 @@ var JSHINT = (function() {
       return true;
     }
 
-    if (next.infix === curr.infix || curr.ltBoundary === "after" ||
-      next.ltBoundary === "before") {
-      return curr.line !== startLine(next);
+    if (next.infix === curr.infix ||
+      // Infix operators which follow `yield` should only be consumed as part
+      // of the current expression if allowed by the syntactic grammar. In
+      // effect, this prevents automatic semicolon insertion when `yield` is
+      // followed by a newline and a comma operator (without enabling it when
+      // `yield` is followed by a newline and a `[` token).
+      (curr.id === "yield" && curr.rbp < next.rbp)) {
+      return !sameLine(curr, next);
     }
+
     return false;
   }
 
@@ -1011,38 +1017,51 @@ var JSHINT = (function() {
 
   // Functions for conformance of style.
 
-  function startLine(token) {
-    return token.startLine || token.line;
+  function sameLine(first, second) {
+    return first.line === (second.startLine || second.line);
   }
 
   function nobreaknonadjacent(left, right) {
-    if (!state.option.laxbreak && left.line !== startLine(right)) {
+    if (!state.option.laxbreak && !sameLine(left, right)) {
       warning("W014", right, right.value);
     }
   }
 
   function nolinebreak(t) {
     t = t;
-    if (t.line !== startLine(state.tokens.next)) {
+    if (!sameLine(t, state.tokens.next)) {
       warning("E022", t, t.value);
     }
   }
 
-  function parseComma(opts) {
+  /**
+   * Validate the comma token in the "current" position of the token stream.
+   *
+   * @param {object} [opts]
+   * @param {boolean} [opts.property] - flag indicating whether the current
+   *                                    comma token is situated directly within
+   *                                    an object initializer
+   * @param {boolean} [opts.allowTrailing] - flag indicating whether the
+   *                                         current comma token may appear
+   *                                         directly before a delimiter
+   *
+   * @returns {boolean} flag indicating the validity of the current comma
+   *                    token; `false` if the token directly causes a syntax
+   *                    error, `true` otherwise
+   */
+  function checkComma(opts) {
+    var prev = state.tokens.prev;
+    var curr = state.tokens.curr;
     opts = opts || {};
 
-    if (!opts.peek) {
-      if (state.tokens.curr.line !== startLine(state.tokens.next)) {
-        if (!state.option.laxcomma) {
-          if (parseComma.first) {
-            warning("I001");
-            parseComma.first = false;
-          }
-          warning("W014", state.tokens.curr, state.tokens.next.value);
+    if (!sameLine(prev, curr)) {
+      if (!state.option.laxcomma) {
+        if (checkComma.first) {
+          warning("I001", curr);
+          checkComma.first = false;
         }
+        warning("W014", prev, curr.value);
       }
-
-      advance(",");
     }
 
     if (state.tokens.next.identifier && !(opts.property && state.inES5())) {
@@ -1858,18 +1877,18 @@ var JSHINT = (function() {
       // don't complain about unclosed templates / strings
       if (state.tokens.next.isUnclosed) return advance();
 
-      var sameLine = startLine(state.tokens.next) === state.tokens.curr.line &&
-                     state.tokens.next.id !== "(end)";
+      var isSameLine = sameLine(state.tokens.curr, state.tokens.next) &&
+                       state.tokens.next.id !== "(end)";
       var blockEnd = checkPunctuator(state.tokens.next, "}");
 
-      if (sameLine && !blockEnd && !(stmt.id === "do" && state.inES6(true))) {
+      if (isSameLine && !blockEnd && !(stmt.id === "do" && state.inES6(true))) {
         errorAt("E058", state.tokens.curr.line, state.tokens.curr.character);
       } else if (!state.option.asi) {
 
         // If this is the last statement in a block that ends on the same line
         // *and* option lastsemic is on, ignore the warning.  Otherwise, issue
         // a warning about missing semicolon.
-        if (!(blockEnd && sameLine && state.option.lastsemic)) {
+        if (!(blockEnd && isSameLine && state.option.lastsemic)) {
           warningAt("W033", state.tokens.curr.line, state.tokens.curr.character);
         }
       }
@@ -1963,6 +1982,7 @@ var JSHINT = (function() {
       } else if (state.option.nonew && r && r.left && r.id === "(" && r.left.id === "new") {
         warning("W031", t);
       }
+
       parseFinalSemicolon(t);
     }
 
@@ -2383,26 +2403,18 @@ var JSHINT = (function() {
   bitwiseassignop(">>=");
   bitwiseassignop(">>>=");
   infix(",", function(context, left, that) {
-    var expr;
-    that.exprs = [left];
-
     if (state.option.nocomma) {
-      warning("W127");
+      warning("W127", that);
     }
 
-    if (!parseComma({ peek: true })) {
-      return that;
-    }
-    while (true) {
-      if (!(expr = expression(context, 10))) {
-        /* istanbul ignore next */
-        break;
-      }
-      that.exprs.push(expr);
-      if (state.tokens.next.value !== "," || !parseComma()) {
-        break;
-      }
+    that.left = left;
+
+    if (checkComma()) {
+      that.right = expression(context, 10);
+    } else {
+      that.right = null;
     }
+
     return that;
   }, 10, true);
 
@@ -2598,12 +2610,10 @@ var JSHINT = (function() {
   suffix("++");
   prefix("++", "preinc");
   state.syntax["++"].exps = true;
-  state.syntax["++"].ltBoundary = "before";
 
   suffix("--");
   prefix("--", "predec");
   state.syntax["--"].exps = true;
-  state.syntax["--"].ltBoundary = "before";
 
   prefix("delete", function(context) {
     this.arity = "unary";
@@ -3021,7 +3031,7 @@ var JSHINT = (function() {
     }
 
     if (state.option.asi && checkPunctuators(state.tokens.prev, [")", "]"]) &&
-      state.tokens.prev.line !== startLine(state.tokens.curr)) {
+      !sameLine(state.tokens.prev, state.tokens.curr)) {
       warning("W014", state.tokens.curr, state.tokens.curr.id);
     }
 
@@ -3052,7 +3062,8 @@ var JSHINT = (function() {
         if (state.tokens.next.id !== ",") {
           break;
         }
-        parseComma({ allowTrailing: true });
+        advance(",");
+        checkComma({ allowTrailing: true });
 
         if (state.tokens.next.id === ")") {
           if (!state.inES8()) {
@@ -3151,44 +3162,50 @@ var JSHINT = (function() {
       return pn;
     }
 
-    var exprs = [];
-
-    if (state.tokens.next.id !== ")") {
-      for (;;) {
-        exprs.push(expression(context, 10));
+    // The ECMA262 grammar requires an expression between the "opening
+    // parenthesis" and "close parenthesis" tokens of the grouping operator.
+    // However, the "ignore" directive is commonly used to inject values that
+    // are not included in the token stream. For example:
+    //
+    //     return (
+    //       /*jshint ignore:start */
+    //       <div></div>
+    //       /*jshint ignore:end */
+    //     );
+    //
+    // The "empty" grouping operator is permitted in order to tolerate this
+    // pattern.
+    if (state.tokens.next.id === ")") {
+      advance(")");
+      return;
+    }
 
-        if (state.tokens.next.id !== ",") {
-          break;
-        }
+    ret = expression(context, 0);
 
-        if (state.option.nocomma) {
-          warning("W127");
-        }
+    advance(")", this);
 
-        parseComma();
-      }
+    if (!ret) {
+      return;
     }
 
-    advance(")", this);
-    if (state.option.immed && exprs[0] && exprs[0].id === "function") {
+    ret.paren = true;
+
+    if (state.option.immed && ret && ret.id === "function") {
       if (state.tokens.next.id !== "(" &&
         state.tokens.next.id !== "." && state.tokens.next.id !== "[") {
         warning("W068", this);
       }
     }
 
-    if (!exprs.length) {
-      return;
-    }
-
-    if (exprs.length > 1) {
-      ret = Object.create(state.syntax[","]);
-      ret.exprs = exprs;
+    if (ret.id === ",") {
+      first = ret.left;
+      while (first.id === ",") {
+        first = first.left;
+      }
 
-      first = exprs[0];
-      last = exprs[exprs.length - 1];
+      last = ret.right;
     } else {
-      ret = first = last = exprs[0];
+      first = last = ret;
 
       if (!isNecessary) {
         // async functions are identified after parsing due to the complexity
@@ -3226,22 +3243,18 @@ var JSHINT = (function() {
       }
     }
 
-    if (ret) {
-      ret.paren = true;
-
-      // The operator may be necessary to override the default binding power of
-      // neighboring operators (whenever there is an operator in use within the
-      // first expression *or* the current group contains multiple expressions)
-      if (!isNecessary && (isOperator(first) || ret.exprs)) {
-        isNecessary =
-          (rbp > first.lbp) ||
-          (rbp > 0 && rbp === first.lbp) ||
-          (!isEndOfExpr() && last.rbp < state.tokens.next.lbp);
-      }
+    // The operator may be necessary to override the default binding power of
+    // neighboring operators (whenever there is an operator in use within the
+    // first expression *or* the current group contains multiple expressions)
+    if (!isNecessary && (isOperator(first) || first !== last)) {
+      isNecessary =
+        (rbp > first.lbp) ||
+        (rbp > 0 && rbp === first.lbp) ||
+        (!isEndOfExpr() && last.rbp < state.tokens.next.lbp);
+    }
 
-      if (!isNecessary) {
-        warning("W126", opening);
-      }
+    if (!isNecessary) {
+      warning("W126", opening);
     }
 
     return ret;
@@ -3253,7 +3266,7 @@ var JSHINT = (function() {
     var e, s, canUseDot;
 
     if (state.option.asi && checkPunctuators(state.tokens.prev, [")", "]"]) &&
-      state.tokens.prev.line !== startLine(state.tokens.curr)) {
+      !sameLine(state.tokens.prev, state.tokens.curr)) {
       warning("W014", state.tokens.curr, state.tokens.curr.id);
     }
 
@@ -3366,7 +3379,7 @@ var JSHINT = (function() {
         });
       return this;
     }
-    var b = state.tokens.curr.line !== startLine(state.tokens.next);
+    var b = !sameLine(state.tokens.curr, state.tokens.next);
     this.first = [];
     if (b) {
       indent += state.option.indent;
@@ -3401,7 +3414,8 @@ var JSHINT = (function() {
 
       this.first.push(expression(context, 10));
       if (state.tokens.next.id === ",") {
-        parseComma({ allowTrailing: true });
+        advance(",");
+        checkComma({ allowTrailing: true });
         if (state.tokens.next.id === "]" && !state.inES5()) {
           warning("W070", state.tokens.curr);
           break;
@@ -3560,7 +3574,8 @@ var JSHINT = (function() {
         if (pastRest) {
           warning("W131", state.tokens.next);
         }
-        parseComma({ allowTrailing: true });
+        advance(",");
+        checkComma({ allowTrailing: true });
       }
 
       if (state.tokens.next.id === ")") {
@@ -3878,38 +3893,20 @@ var JSHINT = (function() {
     state.funct["(metrics)"].ComplexityCount += 1;
   }
 
-  /**
-   * Detect and warn about assignment that occurs within conditional clauses,
-   * e.g.
-   *
-   *     if (a = 1) { ... }
-   *
-   * This check is disabled in response to parenthesized expressions so that
-   * users may opt out of the warning on a case-by-case bases, e.g.
-   *
-   *     if ((a = 1)) { ... }
-   *
-   * @param {object} expr - the parsed expression within the conditional clause
-   */
-  function checkCondAssignment(expr) {
-    if (!expr || expr.paren || state.option.boss) {
+  // Parse assignments that were found instead of conditionals.
+  // For example: if (a = 1) { ... }
+
+  function checkCondAssignment(token) {
+    if (!token || token.paren) {
       return;
     }
 
-    var id = expr.id;
-
-    // If the expression is composed of multiple sub-expressions via a comma
-    // operator, then check the final sub-expression.
-    if (id === ",") {
-      expr = expr.exprs[expr.exprs.length - 1];
-      id = expr.id;
-
-      if (expr.paren) {
-        return;
-      }
+    if (token.id === ",") {
+      checkCondAssignment(token.right);
+      return;
     }
 
-    switch (id) {
+    switch (token.id) {
     case "=":
     case "+=":
     case "-=":
@@ -3919,7 +3916,9 @@ var JSHINT = (function() {
     case "|=":
     case "^=":
     case "/=":
-      warning("W084");
+      if (!state.option.boss) {
+        warning("W084", token);
+      }
     }
   }
 
@@ -3964,7 +3963,7 @@ var JSHINT = (function() {
       var props = Object.create(null); // All properties, including accessors
       var isAsyncMethod = false;
 
-      b = state.tokens.curr.line !== startLine(state.tokens.next);
+      b = !sameLine(state.tokens.curr, state.tokens.next);
       if (b) {
         indent += state.option.indent;
         if (state.tokens.next.from === indent + state.option.indent) {
@@ -4099,7 +4098,8 @@ var JSHINT = (function() {
         countMember(i);
 
         if (state.tokens.next.id === ",") {
-          parseComma({ allowTrailing: true, property: true });
+          advance(",");
+          checkComma({ allowTrailing: true, property: true });
           if (state.tokens.next.id === ",") {
             /* istanbul ignore next */
             warning("W070", state.tokens.curr);
@@ -4434,7 +4434,8 @@ var JSHINT = (function() {
       }
 
       statement.hasComma = true;
-      parseComma();
+      advance(",");
+      checkComma();
     }
     if (letblock) {
       advance(")");
@@ -4606,7 +4607,8 @@ var JSHINT = (function() {
         break;
       }
       this.hasComma = true;
-      parseComma();
+      advance(",");
+      checkComma();
     }
 
     return this;
@@ -4723,7 +4725,6 @@ var JSHINT = (function() {
   blockstmt("if", function(context) {
     var t = state.tokens.next;
     increaseComplexityCount();
-    state.condition = true;
     advance("(");
     var expr = expression(context, 0);
 
@@ -4747,7 +4748,6 @@ var JSHINT = (function() {
     }
 
     advance(")", t);
-    state.condition = false;
     var s = block(context, true, true);
 
     // When the if is within a for-in loop and the condition has a negative form,
@@ -5246,7 +5246,8 @@ var JSHINT = (function() {
           if (state.tokens.next.id !== ",") {
             break;
           }
-          parseComma();
+          advance(",");
+          checkComma();
         }
       }
       advance(")", t);
@@ -5271,7 +5272,7 @@ var JSHINT = (function() {
       nolinebreak(this);
 
     if (state.tokens.next.identifier &&
-        state.tokens.curr.line === startLine(state.tokens.next)) {
+        sameLine(state.tokens.curr, state.tokens.next)) {
       if (!state.funct["(scope)"].funct.hasLabel(v)) {
         warning("W090", state.tokens.next, v);
       }
@@ -5299,7 +5300,7 @@ var JSHINT = (function() {
       nolinebreak(this);
 
     if (state.tokens.next.identifier) {
-      if (state.tokens.curr.line === startLine(state.tokens.next)) {
+      if (sameLine(state.tokens.curr, state.tokens.next)) {
         if (!state.funct["(scope)"].funct.hasLabel(v)) {
           warning("W090", state.tokens.next, v);
         }
@@ -5315,7 +5316,7 @@ var JSHINT = (function() {
 
 
   stmt("return", function(context) {
-    if (this.line === startLine(state.tokens.next)) {
+    if (sameLine(this, state.tokens.next)) {
       if (state.tokens.next.id !== ";" && !state.tokens.next.reach) {
         this.first = expression(context, 0);
 
@@ -5420,7 +5421,6 @@ var JSHINT = (function() {
   (function(yieldSymbol) {
     yieldSymbol.rbp = yieldSymbol.lbp = 25;
     yieldSymbol.exps = true;
-    yieldSymbol.ltBoundary = "after";
   })(prefix("yield", function(context) {
     if (state.inMoz()) {
       return mozYield.call(this, context);
@@ -5454,7 +5454,7 @@ var JSHINT = (function() {
     }
 
     // Parse operand
-    if (!isEndOfExpr() && state.tokens.next.id !== ",") {
+    if (state.tokens.curr.value === "*" || sameLine(state.tokens.curr, state.tokens.next)) {
       if (state.tokens.next.nud) {
 
         nobreaknonadjacent(state.tokens.curr, state.tokens.next);
@@ -5491,7 +5491,7 @@ var JSHINT = (function() {
       advance("*");
     }
 
-    if (this.line === startLine(state.tokens.next)) {
+    if (sameLine(this, state.tokens.next)) {
       if (delegatingYield ||
           (state.tokens.next.id !== ";" && !state.option.asi &&
            !state.tokens.next.reach && state.tokens.next.nud)) {
@@ -6492,7 +6492,7 @@ var JSHINT = (function() {
       combine(predefined, g || {});
 
       //reset values
-      parseComma.first = true;
+      checkComma.first = true;
 
       advance();
       switch (state.tokens.next.id) {


Test patch:
diff --git a/tests/unit/options.js b/tests/unit/options.js
index d1b8687b..35113b35 100644
--- a/tests/unit/options.js
+++ b/tests/unit/options.js
@@ -1677,16 +1677,16 @@ exports.boss = function (test) {
 
   // By default, warn about suspicious assignments
   TestRun(test)
-    .addError(1, 10, 'Expected a conditional expression and instead saw an assignment.')
-    .addError(4, 24, 'Expected a conditional expression and instead saw an assignment.')
-    .addError(7, 27, 'Expected a conditional expression and instead saw an assignment.')
-    .addError(12, 24, 'Expected a conditional expression and instead saw an assignment.')
+    .addError(1, 7, 'Expected a conditional expression and instead saw an assignment.')
+    .addError(4, 12, 'Expected a conditional expression and instead saw an assignment.')
+    .addError(7, 15, 'Expected a conditional expression and instead saw an assignment.')
+    .addError(12, 12, 'Expected a conditional expression and instead saw an assignment.')
 
     // GH-657
-    .addError(14, 11, 'Expected a conditional expression and instead saw an assignment.')
-    .addError(17, 25, 'Expected a conditional expression and instead saw an assignment.')
-    .addError(20, 28, 'Expected a conditional expression and instead saw an assignment.')
-    .addError(25, 25, 'Expected a conditional expression and instead saw an assignment.')
+    .addError(14, 7, 'Expected a conditional expression and instead saw an assignment.')
+    .addError(17, 12, 'Expected a conditional expression and instead saw an assignment.')
+    .addError(20, 15, 'Expected a conditional expression and instead saw an assignment.')
+    .addError(25, 12, 'Expected a conditional expression and instead saw an assignment.')
 
     // GH-670
     .addError(28, 12, "Did you mean to return a conditional instead of an assignment?")
@@ -2855,7 +2855,7 @@ exports.nocomma = function (test) {
     .test("return 2, 5;", {});
 
   TestRun(test, "nocomma main case")
-    .addError(1, 11, "Unexpected use of a comma operator.")
+    .addError(1, 9, "Unexpected use of a comma operator.")
     .test("return 2, 5;", { nocomma: true });
 
   TestRun(test, "nocomma in an expression")
diff --git a/tests/unit/parser.js b/tests/unit/parser.js
index 96eb8f24..91a03730 100644
--- a/tests/unit/parser.js
+++ b/tests/unit/parser.js
@@ -7479,12 +7479,12 @@ exports["test for GH-1018"] = function (test) {
   run.test(code, {boss: true});
 
   run
-    .addError(1, 11, "Expected a conditional expression and instead saw an assignment.")
-    .addError(2, 16, "Expected a conditional expression and instead saw an assignment.")
-    .addError(3, 14, "Expected a conditional expression and instead saw an assignment.")
-    .addError(4, 20, "Expected a conditional expression and instead saw an assignment.")
-    .addError(5, 20, "Expected a conditional expression and instead saw an assignment.")
-    .addError(6, 15, "Expected a conditional expression and instead saw an assignment.")
+    .addError(1, 7, "Expected a conditional expression and instead saw an assignment.")
+    .addError(2, 12, "Expected a conditional expression and instead saw an assignment.")
+    .addError(3, 10, "Expected a conditional expression and instead saw an assignment.")
+    .addError(4, 16, "Expected a conditional expression and instead saw an assignment.")
+    .addError(5, 16, "Expected a conditional expression and instead saw an assignment.")
+    .addError(6, 11, "Expected a conditional expression and instead saw an assignment.")
     .test(code);
 
   test.done();
@@ -7501,8 +7501,8 @@ exports["test warnings for assignments in conditionals"] = function (test) {
   ];
 
   var run = TestRun(test)
-    .addError(1, 10, "Expected a conditional expression and instead saw an assignment.")
-    .addError(4, 17, "Expected a conditional expression and instead saw an assignment.");
+    .addError(1, 7, "Expected a conditional expression and instead saw an assignment.")
+    .addError(4, 14, "Expected a conditional expression and instead saw an assignment.");
 
   run.test(code); // es5
 
@@ -7584,8 +7584,6 @@ exports["test for crash with invalid condition"] = function (test) {
     .addError(4, 16, "Missing semicolon.")
     .addError(6, 15, "Expected an identifier and instead saw ','.")
     .addError(7, 17, "Unexpected ')'.")
-    .addError(7, 17, "Expected an identifier and instead saw ')'.")
-    .addError(7, 19, "Expected ')' to match '(' from line 7 and instead saw ';'.")
     .addError(8, 15, "Expected an identifier and instead saw ','.")
     .addError(8, 16, "Expected ')' to match '(' from line 8 and instead saw 'b'.")
     .addError(8, 18, "Expected an identifier and instead saw ')'.")