Repository: eslint/eslint
Pull request #8330 (2017-03-26 07:08:12)

Fix: dot-notation autofix produces errors on parenthesized computed keys
**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:**
* **Node Version:**
* **npm Version:**

**What parser (default, Babel-ESLint, etc.) are you using?**

**Please show your full configuration:**

```yml
rules:
  dot-notation: error
```

**What did you do? Please include the actual source code causing the issue.**

```js
foo[('bar')];
```

**What did you expect to happen?**

I expected eslint to autofix the code to

```js
foo.bar;
```

**What actually happened? Please include the actual, raw output from ESLint.**

eslint autofixed the code to invalid syntax:

```js
foo[.bar]
```

**What changes did you make? (Give an overview)**

This commit updates the dot-notation autofixer logic to ensure that parenthesized computed keys are handled correctly. Previously, the rule didn't account for the possibility that a computed key might be parenthesized, so it ended up using the parens instead of the square brackets for the fix range.

**Is there anything you'd like reviewers to focus on?**

Nothing in particular


mention-bot: @not-an-aardvark, thanks for your PR! By analyzing the history of the files in this pull request, we identified @vitorbal, @gyandeeps and @goatslacker to be potential reviewers.
eslintbot: LGTM
eslintbot: LGTM

Patch:
diff --git a/lib/rules/dot-notation.js b/lib/rules/dot-notation.js
index d55b098b2..3f5eae0bb 100644
--- a/lib/rules/dot-notation.js
+++ b/lib/rules/dot-notation.js
@@ -4,6 +4,12 @@
  */
 "use strict";
 
+//------------------------------------------------------------------------------
+// Requirements
+//------------------------------------------------------------------------------
+
+const astUtils = require("../ast-utils");
+
 //------------------------------------------------------------------------------
 // Rule Definition
 //------------------------------------------------------------------------------
@@ -64,12 +70,10 @@ module.exports = {
                                 propertyValue: JSON.stringify(node.property.value)
                             },
                             fix(fixer) {
-                                const leftBracket = sourceCode.getTokenBefore(node.property);
-                                const rightBracket = sourceCode.getTokenAfter(node.property);
-                                const textBeforeProperty = sourceCode.text.slice(leftBracket.range[1], node.property.range[0]);
-                                const textAfterProperty = sourceCode.text.slice(node.property.range[1], rightBracket.range[0]);
+                                const leftBracket = sourceCode.getTokenAfter(node.object, astUtils.isOpeningBracketToken);
+                                const rightBracket = sourceCode.getLastToken(node);
 
-                                if (textBeforeProperty.trim() || textAfterProperty.trim()) {
+                                if (sourceCode.getFirstTokenBetween(leftBracket, rightBracket, { includeComments: true, filter: astUtils.isCommentToken })) {
 
                                     // Don't perform any fixes if there are comments inside the brackets.
                                     return null;
diff --git a/lib/rules/no-else-return.js b/lib/rules/no-else-return.js
index 6ba0b2426..68ab4c760 100644
--- a/lib/rules/no-else-return.js
+++ b/lib/rules/no-else-return.js
@@ -9,6 +9,7 @@
 // Requirements
 //------------------------------------------------------------------------------
 
+const astUtils = require("../ast-utils");
 const FixTracker = require("../util/fix-tracker");
 
 //------------------------------------------------------------------------------
@@ -199,9 +200,11 @@ module.exports = {
             let consequents,
                 alternate;
 
-            // Only "top-level" if statements are checked, meaning the first `if`
-            // in a `if-else-if-...` chain.
-            if (parent.type === "IfStatement" && parent.alternate === node) {
+            /*
+             * Fixing this would require splitting one statement into two, so no error should
+             * be reported if this node is in a position where only one statement is allowed.
+             */
+            if (!astUtils.STATEMENT_LIST_PARENTS.has(parent.type)) {
                 return;
             }
 
diff --git a/lib/rules/object-shorthand.js b/lib/rules/object-shorthand.js
index 1d32d1722..1d3d9dae1 100644
--- a/lib/rules/object-shorthand.js
+++ b/lib/rules/object-shorthand.js
@@ -230,7 +230,10 @@ module.exports = {
                 const functionToken = sourceCode.getTokens(node.value).find(token => token.type === "Keyword" && token.value === "function");
                 const tokenBeforeParams = node.value.generator ? sourceCode.getTokenAfter(functionToken) : functionToken;
 
-                return fixer.replaceTextRange([firstKeyToken.range[0], tokenBeforeParams.range[1]], keyPrefix + keyText);
+                return fixer.replaceTextRange(
+                    [firstKeyToken.range[0], node.range[1]],
+                    keyPrefix + keyText + sourceCode.text.slice(tokenBeforeParams.range[1], node.value.range[1])
+                );
             }
             const arrowToken = sourceCode.getTokens(node.value).find(token => token.value === "=>");
             const tokenBeforeArrow = sourceCode.getTokenBefore(arrowToken);
@@ -238,7 +241,10 @@ module.exports = {
             const oldParamText = sourceCode.text.slice(sourceCode.getFirstToken(node.value, node.value.async ? 1 : 0).range[0], tokenBeforeArrow.range[1]);
             const newParamText = hasParensAroundParameters ? oldParamText : `(${oldParamText})`;
 
-            return fixer.replaceTextRange([firstKeyToken.range[0], arrowToken.range[1]], keyPrefix + keyText + newParamText);
+            return fixer.replaceTextRange(
+                [firstKeyToken.range[0], node.range[1]],
+                keyPrefix + keyText + newParamText + sourceCode.text.slice(arrowToken.range[1], node.value.range[1])
+            );
 
         }
 
diff --git a/lib/rules/quotes.js b/lib/rules/quotes.js
index 1085ea33b..b1117b85f 100644
--- a/lib/rules/quotes.js
+++ b/lib/rules/quotes.js
@@ -209,6 +209,7 @@ module.exports = {
 
                 // LiteralPropertyName.
                 case "Property":
+                case "MethodDefinition":
                     return parent.key === node && !parent.computed;
 
                 // ModuleSpecifier.


Test patch:
diff --git a/tests/lib/rules/dot-notation.js b/tests/lib/rules/dot-notation.js
index 83b212efd..e228899e1 100644
--- a/tests/lib/rules/dot-notation.js
+++ b/tests/lib/rules/dot-notation.js
@@ -153,6 +153,21 @@ ruleTester.run("dot-notation", rule, {
             options: [{ allowKeywords: false }],
             errors: [{ message: ".while is a syntax error." }],
             output: null // Not fixed due to comment
+        },
+        {
+            code: "foo[('bar')]",
+            output: "foo.bar",
+            errors: [{ message: "[\"bar\"] is better written in dot notation." }]
+        },
+        {
+            code: "foo[(null)]",
+            output: "foo.null",
+            errors: [{ message: "[null] is better written in dot notation." }]
+        },
+        {
+            code: "(foo)['bar']",
+            output: "(foo).bar",
+            errors: [{ message: "[\"bar\"] is better written in dot notation." }]
         }
     ]
 });
diff --git a/tests/lib/rules/no-else-return.js b/tests/lib/rules/no-else-return.js
index e99b83d6c..6aea5c867 100644
--- a/tests/lib/rules/no-else-return.js
+++ b/tests/lib/rules/no-else-return.js
@@ -27,7 +27,22 @@ ruleTester.run("no-else-return", rule, {
         "function foo() { if (true) notAReturn(); else return y; }",
         "function foo() {if (x) { notAReturn(); } else if (y) { return true; } else { notAReturn(); } }",
         "function foo() {if (x) { return true; } else if (y) { notAReturn() } else { notAReturn(); } }",
-        "if (0) { if (0) {} else {} } else {}"
+        "if (0) { if (0) {} else {} } else {}",
+        `
+            function foo() {
+                if (foo)
+                    if (bar) return;
+                    else baz;
+                else qux;
+            }
+        `,
+        `
+            function foo() {
+                while (foo)
+                    if (bar) return;
+                    else baz;
+            }
+        `
     ],
     invalid: [
         {
diff --git a/tests/lib/rules/object-shorthand.js b/tests/lib/rules/object-shorthand.js
index 8ef707d52..d961849ae 100644
--- a/tests/lib/rules/object-shorthand.js
+++ b/tests/lib/rules/object-shorthand.js
@@ -973,6 +973,17 @@ ruleTester.run("object-shorthand", rule, {
             `,
             options: ["always", { avoidExplicitReturnArrows: true }],
             errors: [METHOD_ERROR]
+        },
+        {
+            code: "({ a: (function(){ return foo; }) })",
+            output: "({ a(){ return foo; } })",
+            errors: [METHOD_ERROR]
+        },
+        {
+            code: "({ a: (() => { return foo; }) })",
+            output: "({ a() { return foo; } })",
+            options: ["always", { avoidExplicitReturnArrows: true }],
+            errors: [METHOD_ERROR]
         }
     ]
 });
diff --git a/tests/lib/rules/quotes.js b/tests/lib/rules/quotes.js
index 98ccd043e..3201e594c 100644
--- a/tests/lib/rules/quotes.js
+++ b/tests/lib/rules/quotes.js
@@ -68,8 +68,10 @@ ruleTester.run("quotes", rule, {
         { code: "import a from \"a\"; import b from 'b';", options: ["backtick"], parserOptions: { sourceType: "module" } },
         { code: "export * from \"a\"; export * from 'b';", options: ["backtick"], parserOptions: { sourceType: "module" } },
 
-        // `backtick` should not warn property names (not computed).
-        { code: "var obj = {\"key0\": 0, 'key1': 1};", options: ["backtick"], parserOptions: { ecmaVersion: 6 } }
+        // `backtick` should not warn property/method names (not computed).
+        { code: "var obj = {\"key0\": 0, 'key1': 1};", options: ["backtick"], parserOptions: { ecmaVersion: 6 } },
+        { code: "class Foo { 'bar'(){} }", options: ["backtick"], parserOptions: { ecmaVersion: 6 } },
+        { code: "class Foo { static ''(){} }", options: ["backtick"], parserOptions: { ecmaVersion: 6 } }
     ],
     invalid: [
         {
@@ -205,7 +207,7 @@ ruleTester.run("quotes", rule, {
             errors: [{ message: "Strings must use backtick.", type: "Literal" }]
         },
 
-        // `backtick` should not warn computed property names.
+        // `backtick` should warn computed property names.
         {
             code: "var obj = {[\"key0\"]: 0, ['key1']: 1};",
             output: "var obj = {[`key0`]: 0, [`key1`]: 1};",
@@ -216,6 +218,16 @@ ruleTester.run("quotes", rule, {
                 { message: "Strings must use backtick.", type: "Literal" }
             ]
         },
+        {
+            code: "class Foo { ['a'](){} static ['b'](){} }",
+            output: "class Foo { [`a`](){} static [`b`](){} }",
+            options: ["backtick"],
+            parserOptions: { ecmaVersion: 6 },
+            errors: [
+                { message: "Strings must use backtick.", type: "Literal" },
+                { message: "Strings must use backtick.", type: "Literal" }
+            ]
+        },
 
         // https://github.com/eslint/eslint/issues/7084
         {