Repository: eslint/eslint
Pull request #8808 (2017-06-26 08:22:49)

Fix: no-extra-parens false positives for variables called "let"
<!--
    ESLint adheres to the [JS Foundation Code of Conduct](https://js.foundation/community/code-of-conduct).
-->

**What is the purpose of this pull request? (put an "X" next to item)**

[x] Bug fix

<!--
    If the item you've checked above has a template, please paste the template questions below and answer them. (If this pull request is addressing an issue, you can just paste a link to the issue here instead.)
-->

<!--
    Please ensure your pull request is ready:

    - Read the pull request guide (http://eslint.org/docs/developer-guide/contributing/pull-requests)
    - Include tests for this change
    - Update documentation for this change (if appropriate)
-->

<!--
    The following is required for all pull requests:
-->

**Tell us about your environment**

* **ESLint Version:** master
* **Node Version:** 8.1.2
* **npm Version:** 5.0.3

**What parser (default, Babel-ESLint, etc.) are you using?**

default

**Please show your full configuration:**

```yml
rules:
  no-extra-parens: error
```

**What did you do? Please include the actual source code causing the issue.**

```js
for ((let) in foo); // iterating a variable called "let" over "foo"
for ((let)[foo] in bar); // assigns to the foo property of the "let" variable in a loop
for ((let[foo]) in bar); // assigns to the foo property of the "let" variable in a loop
```

**What did you expect to happen?**

I expected `eslint --fix` to not change the behavior of the code.

**What actually happened? Please include the actual, raw output from ESLint.**

The code was autofixed to:

```js
for (let in foo); // invalid syntax
for (let[foo] in bar); // destructuring variable assignment (behavior change)
for (let[foo] in bar); // destructuring variable assignment (behavior change)
```

**What changes did you make? (Give an overview)**

"let" is unusual because it's sometimes parsed as a variable declaration keyword, and sometimes as an identifier for a variable. This commit fixes some bugs in the `no-extra-parens` rule where parentheses are unnecessary for most variable names, but are necessary when the variable is called "let".

**Is there anything you'd like reviewers to focus on?**

Nothing in particular

eslintbot: LGTM
not-an-aardvark: No, because unlike `let`, `const` is not a valid variable name.

Patch:
diff --git a/docs/developer-guide/nodejs-api.md b/docs/developer-guide/nodejs-api.md
index f48670896..0f006d781 100644
--- a/docs/developer-guide/nodejs-api.md
+++ b/docs/developer-guide/nodejs-api.md
@@ -159,7 +159,7 @@ var messages = linter.verifyAndFix("var foo", {
     rules: {
         semi: 2
     }
-}, { filename: "foo.js" });
+});
 ```
 
 Output object from this method:
diff --git a/docs/developer-guide/working-with-plugins.md b/docs/developer-guide/working-with-plugins.md
index c3bbccc48..0c8bfad22 100644
--- a/docs/developer-guide/working-with-plugins.md
+++ b/docs/developer-guide/working-with-plugins.md
@@ -221,9 +221,9 @@ Add these keywords into your `package.json` file to make it easy for others to f
 
 ### Working with Custom Parsers
 
-If you want to use your own parser and provide additional capabilities for your rules, you can specify your own custom parser. By default, the ESLint parser will use its parse method that takes in the source code as a first parameter and additional optional parameters as a second parameter to create an AST. You can specify a `parse` configuration to use your own custom parser. If a `parseForESLint` method is exposed, this method will be used to parse. Otherwise, the parser will use the parse method. `parseForESLint` behaves like `parse` and takes in the the source code and optional ESLint configurations. When `parseForESLint` is called, the method should return an object that contains the required property `ast` and an optional `services` property. `ast` should contain the AST. The `services` property contains the parser-dependent services. The value of the service property is available to rules as `context.parserServices`
+If you want to use your own parser and provide additional capabilities for your rules, you can specify your own custom parser. If a `parseForESLint` method is exposed on the parser, this method will be used to parse the code. Otherwise, the `parse` method will be used. Both methods should take in the the source code as the first argument, and an optional configuration object as the second argument (provided as `parserOptions` in a config file). The `parse` method should simply return the AST. The `parseForESLint` method should return an object that contains the required property `ast` and an optional `services` property. `ast` should contain the AST. The `services` property can contain any parser-dependent services (such as type checkers for nodes). The value of the `services` property is available to rules as `context.parserServices`.
 
-If no parseForESLint function is found, the parser will use the default parse method with the source code and the parser options. You can find a ESLint parser project [here](https://github.com/eslint/typescript-eslint-parser).
+You can find an ESLint parser project [here](https://github.com/eslint/typescript-eslint-parser).
 
     {
 
diff --git a/lib/linter.js b/lib/linter.js
index d2f1f4657..b8b3a5676 100755
--- a/lib/linter.js
+++ b/lib/linter.js
@@ -1205,6 +1205,7 @@ class Linter extends EventEmitter {
             fixedResult,
             fixed = false,
             passNumber = 0;
+        const debugTextDescription = options && options.filename || `${text.slice(0, 10)}...`;
 
         /**
          * This loop continues until one of the following is true:
@@ -1218,10 +1219,10 @@ class Linter extends EventEmitter {
         do {
             passNumber++;
 
-            debug(`Linting code for ${options.filename} (pass ${passNumber})`);
+            debug(`Linting code for ${debugTextDescription} (pass ${passNumber})`);
             messages = this.verify(text, config, options);
 
-            debug(`Generating fixed text for ${options.filename} (pass ${passNumber})`);
+            debug(`Generating fixed text for ${debugTextDescription} (pass ${passNumber})`);
             fixedResult = SourceCodeFixer.applyFixes(this.getSourceCode(), messages);
 
             // stop if there are any syntax errors.
diff --git a/lib/rules/dot-notation.js b/lib/rules/dot-notation.js
index 55225b8cc..142f0f24a 100644
--- a/lib/rules/dot-notation.js
+++ b/lib/rules/dot-notation.js
@@ -116,6 +116,15 @@ module.exports = {
                                 return null;
                             }
 
+                            if (node.object.type === "Identifier" && node.object.name === "let") {
+
+                                /*
+                                 * A statement that starts with `let[` is parsed as a destructuring variable declaration, not
+                                 * a MemberExpression.
+                                 */
+                                return null;
+                            }
+
                             return fixer.replaceTextRange(
                                 [dot.range[0], node.property.range[1]],
                                 `[${textAfterDot}"${node.property.name}"]`
diff --git a/lib/rules/no-extra-parens.js b/lib/rules/no-extra-parens.js
index 9f48f1d81..fd6fd0b78 100644
--- a/lib/rules/no-extra-parens.js
+++ b/lib/rules/no-extra-parens.js
@@ -426,7 +426,7 @@ module.exports = {
                     secondToken.type === "Keyword" && (
                         secondToken.value === "function" ||
                         secondToken.value === "class" ||
-                        secondToken.value === "let" && astUtils.isOpeningBracketToken(sourceCode.getTokenAfter(secondToken))
+                        secondToken.value === "let" && astUtils.isOpeningBracketToken(sourceCode.getTokenAfter(secondToken, astUtils.isNotClosingParenToken))
                     )
                 )
             ) {
@@ -512,16 +512,27 @@ module.exports = {
             ExportDefaultDeclaration: node => checkExpressionOrExportStatement(node.declaration),
             ExpressionStatement: node => checkExpressionOrExportStatement(node.expression),
 
-            ForInStatement(node) {
-                if (hasExcessParens(node.right)) {
-                    report(node.right);
-                }
-                if (hasExcessParens(node.left)) {
-                    report(node.left);
-                }
-            },
+            "ForInStatement, ForOfStatement"(node) {
+                if (node.left.type !== "VariableDeclarator") {
+                    const firstLeftToken = sourceCode.getFirstToken(node.left, astUtils.isNotOpeningParenToken);
+
+                    if (
+                        firstLeftToken.value === "let" && (
+
+                            // If `let` is the only thing on the left side of the loop, it's the loop variable: `for ((let) of foo);`
+                            // Removing it will cause a syntax error, because it will be parsed as the start of a VariableDeclarator.
+                            firstLeftToken.range[1] === node.left.range[1] ||
 
-            ForOfStatement(node) {
+                            // If `let` is followed by a `[` token, it's a property access on the `let` value: `for ((let[foo]) of bar);`
+                            // Removing it will cause the property access to be parsed as a destructuring declaration of `foo` instead.
+                            astUtils.isOpeningBracketToken(
+                                sourceCode.getTokenAfter(firstLeftToken, astUtils.isNotClosingParenToken)
+                            )
+                        )
+                    ) {
+                        tokensToIgnore.add(firstLeftToken);
+                    }
+                }
                 if (hasExcessParens(node.right)) {
                     report(node.right);
                 }


Test patch:
diff --git a/tests/lib/linter.js b/tests/lib/linter.js
index 4acc46b3f..88ce63cb1 100644
--- a/tests/lib/linter.js
+++ b/tests/lib/linter.js
@@ -3776,6 +3776,20 @@ describe("eslint", () => {
             assert.equal(messages.output, "var a;", "Fixes were applied correctly");
             assert.isTrue(messages.fixed);
         });
+
+        it("does not require a third argument", () => {
+            const fixResult = linter.verifyAndFix("var a", {
+                rules: {
+                    semi: 2
+                }
+            });
+
+            assert.deepEqual(fixResult, {
+                fixed: true,
+                messages: [],
+                output: "var a;"
+            });
+        });
     });
 
     describe("Edge cases", () => {
diff --git a/tests/lib/rules/dot-notation.js b/tests/lib/rules/dot-notation.js
index f20334f66..08013540a 100644
--- a/tests/lib/rules/dot-notation.js
+++ b/tests/lib/rules/dot-notation.js
@@ -178,6 +178,12 @@ ruleTester.run("dot-notation", rule, {
             code: "foo['bar']instanceof baz",
             output: "foo.bar instanceof baz",
             errors: [{ message: "[\"bar\"] is better written in dot notation." }]
+        },
+        {
+            code: "let.if()",
+            output: null, // `let["if"]()` is a syntax error because `let[` indicates a destructuring variable declaration
+            options: [{ allowKeywords: false }],
+            errors: [{ message: ".if is a syntax error." }]
         }
     ]
 });
diff --git a/tests/lib/rules/no-extra-parens.js b/tests/lib/rules/no-extra-parens.js
index 54990b96e..be32041c8 100644
--- a/tests/lib/rules/no-extra-parens.js
+++ b/tests/lib/rules/no-extra-parens.js
@@ -450,7 +450,12 @@ ruleTester.run("no-extra-parens", rule, {
         {
             code: "((function(){}).foo)();",
             options: ["functions"]
-        }
+        },
+        "(let)[foo]",
+        "for ((let) in foo);",
+        "for ((let[foo]) in bar);",
+        "for ((let)[foo] in bar);",
+        "for ((let[foo].bar) in baz);"
     ],
 
     invalid: [
@@ -1052,6 +1057,24 @@ ruleTester.run("no-extra-parens", rule, {
             "MemberExpression",
             1,
             { parserOptions: { ecmaVersion: 2015 } }
+        ),
+        invalid(
+            "(let).foo",
+            "let.foo",
+            "Identifier",
+            1
+        ),
+        invalid(
+            "for ((let.foo) in bar);",
+            "for (let.foo in bar);",
+            "MemberExpression",
+            1
+        ),
+        invalid(
+            "for ((let).foo.bar in baz);",
+            "for (let.foo.bar in baz);",
+            "Identifier",
+            1
         )
     ]
 });