Repository: eslint/eslint (js)

Issue:
no-extra-parens does not warn around spread arguments
**Tell us about your environment**

* **ESLint Version:** 3.16.1
* **Node Version:** 7.6.0
* **npm Version:** 4.2.0

**What parser (default, Babel-ESLint, etc.) are you using?**

- default

**Please show your full configuration:**

- nothing

**What did you do? Please include the actual source code causing the issue.**

I checked the following code:

```js
/*eslint no-extra-parens: [error, all] */

let a = [...(b)]
let c = {...(d)}
```

```bash
$ eslint test.js --parser-options "ecmaVersion:2015,ecmaFeatures:{experimentalObjectRestSpread:true}" --no-eslintrc --no-ignore
```

**What did you expect to happen?**

ESLint warns the parentheses.

**What actually happened? Please include the actual, raw output from ESLint.**

No errors.


Pull request:
Update: no-extra-parens overlooked spread and superClass (fixes #8175)
## What is the purpose of this pull request? (put an "X" next to item)

[X] Bug fix ([template](https://raw.githubusercontent.com/eslint/eslint/master/templates/bug-report.md))

Fixes #8175.

## What changes did you make? (Give an overview)

This PR fixes the overlooking of `no-extra-parens` around spread operators and super classes.

```js
//✘ BAD
let a = [ ...(b) ]
let b = { ...(c) }
class A extends (B) { }

//✔ GOOD
class C extends (D,E) { } // Allow this parentheses. 
                          // `SequenceExpression` is not a `LeftHandSideExpression`, 
                          // so if the parentheses are removed then will get a syntax error.

class F extends (await G()) { } // Arrow this parentheses.
```

- The argument of spread operators is an `AssignmentExpression`. So parentheses around the argument of spread operators should be always extra.
- The super class of `ClassDeclaration`/`ClassExpression` is an `LeftHandSideExpression`. If a super class is a `LeftHandSideExpression`, the parentheses of the super class should be extra.

## Is there anything you'd like reviewers to focus on?

I'm not familiar with `no-extra-parens` rule.
Is this fix correct?


Patch:
diff --git a/lib/config/config-file.js b/lib/config/config-file.js
index 2c4dde64c..4e886b8af 100644
--- a/lib/config/config-file.js
+++ b/lib/config/config-file.js
@@ -183,6 +183,22 @@ function loadPackageJSONConfigFile(filePath) {
     }
 }
 
+/**
+ * Creates an error to notify about a missing config to extend from.
+ * @param {string} configName The name of the missing config.
+ * @returns {Error} The error object to throw
+ * @private
+ */
+function configMissingError(configName) {
+    const error = new Error(`Failed to load config "${configName}" to extend from.`);
+
+    error.messageTemplate = "extend-config-missing";
+    error.messageData = {
+        configName
+    };
+    return error;
+}
+
 /**
  * Loads a configuration file regardless of the source. Inspects the file path
  * to determine the correctly way to load the config file.
@@ -199,6 +215,9 @@ function loadConfigFile(file) {
             config = loadJSConfigFile(filePath);
             if (file.configName) {
                 config = config.configs[file.configName];
+                if (!config) {
+                    throw configMissingError(file.configFullName);
+                }
             }
             break;
 
@@ -340,6 +359,33 @@ function getLookupPath(configFilePath) {
     return path.join(basedir, "node_modules");
 }
 
+/**
+ * Resolves a eslint core config path
+ * @param {string} name The eslint config name.
+ * @returns {string} The resolved path of the config.
+ * @private
+ */
+function getEslintCoreConfigPath(name) {
+    if (name === "eslint:recommended") {
+
+       /*
+        * Add an explicit substitution for eslint:recommended to
+        * conf/eslint-recommended.js.
+        */
+        return path.resolve(__dirname, "../../conf/eslint-recommended.js");
+    }
+
+    if (name === "eslint:all") {
+
+       /*
+        * Add an explicit substitution for eslint:all to conf/eslint-all.js
+        */
+        return path.resolve(__dirname, "../../conf/eslint-all.js");
+    }
+
+    throw configMissingError(name);
+}
+
 /**
  * Applies values from the "extends" field in a configuration file.
  * @param {Object} config The configuration information.
@@ -360,43 +406,23 @@ function applyExtends(config, filePath, relativeTo) {
 
     // Make the last element in an array take the highest precedence
     config = configExtends.reduceRight((previousValue, parentPath) => {
-
-        if (parentPath === "eslint:recommended") {
-
-            /*
-             * Add an explicit substitution for eslint:recommended to
-             * conf/eslint-recommended.js.
-             */
-            parentPath = path.resolve(__dirname, "../../conf/eslint-recommended.js");
-        } else if (parentPath === "eslint:all") {
-
-            /*
-             * Add an explicit substitution for eslint:all to conf/eslint-all.js
-             */
-            parentPath = path.resolve(__dirname, "../../conf/eslint-all.js");
-        } else if (isFilePath(parentPath)) {
-
-            /*
-             * If the `extends` path is relative, use the directory of the current configuration
-             * file as the reference point. Otherwise, use as-is.
-             */
-            parentPath = (!path.isAbsolute(parentPath)
-                ? path.join(relativeTo || path.dirname(filePath), parentPath)
-                : parentPath
-            );
-        }
-
         try {
+            if (parentPath.startsWith("eslint:")) {
+                parentPath = getEslintCoreConfigPath(parentPath);
+            } else if (isFilePath(parentPath)) {
+
+                /*
+                 * If the `extends` path is relative, use the directory of the current configuration
+                 * file as the reference point. Otherwise, use as-is.
+                 */
+                parentPath = (path.isAbsolute(parentPath)
+                    ? parentPath
+                    : path.join(relativeTo || path.dirname(filePath), parentPath)
+                );
+            }
             debug(`Loading ${parentPath}`);
             return ConfigOps.merge(load(parentPath, false, relativeTo), previousValue);
         } catch (e) {
-            if (parentPath.indexOf("plugin:") === 0 || parentPath.indexOf("eslint:") === 0) {
-                e.message = `Failed to load config "${parentPath}" to extend from.`;
-                e.messageTemplate = "extend-config-missing";
-                e.messageData = {
-                    configName: parentPath
-                };
-            }
 
             /*
              * If the file referenced by `extends` failed to load, add the path
@@ -462,7 +488,10 @@ function normalizePackageName(name, prefix) {
  * or package name.
  * @param {string} filePath The filepath to resolve.
  * @param {string} [relativeTo] The path to resolve relative to.
- * @returns {Object} A path that can be used directly to load the configuration.
+ * @returns {Object} An object containing 3 properties:
+ * - 'filePath' (required) the resolved path that can be used directly to load the configuration.
+ * - 'configName' the name of the configuration inside the plugin.
+ * - 'configFullName' the name of the configuration as used in the eslint config (e.g. 'plugin:node/recommended').
  * @private
  */
 function resolve(filePath, relativeTo) {
@@ -471,14 +500,15 @@ function resolve(filePath, relativeTo) {
     }
     let normalizedPackageName;
 
-    if (filePath.indexOf("plugin:") === 0) {
-        const packagePath = filePath.substr(7, filePath.lastIndexOf("/") - 7);
+    if (filePath.startsWith("plugin:")) {
+        const configFullName = filePath;
+        const pluginName = filePath.substr(7, filePath.lastIndexOf("/") - 7);
         const configName = filePath.substr(filePath.lastIndexOf("/") + 1, filePath.length - filePath.lastIndexOf("/") - 1);
 
-        normalizedPackageName = normalizePackageName(packagePath, "eslint-plugin");
+        normalizedPackageName = normalizePackageName(pluginName, "eslint-plugin");
         debug(`Attempting to resolve ${normalizedPackageName}`);
         filePath = resolver.resolve(normalizedPackageName, getLookupPath(relativeTo));
-        return { filePath, configName };
+        return { filePath, configName, configFullName };
     }
     normalizedPackageName = normalizePackageName(filePath, "eslint-config");
     debug(`Attempting to resolve ${normalizedPackageName}`);
diff --git a/lib/rules/no-extra-parens.js b/lib/rules/no-extra-parens.js
index f175d1ab2..b9348b61c 100644
--- a/lib/rules/no-extra-parens.js
+++ b/lib/rules/no-extra-parens.js
@@ -67,6 +67,7 @@ module.exports = {
         const NESTED_BINARY = ALL_NODES && context.options[1] && context.options[1].nestedBinaryExpressions === false;
         const EXCEPT_RETURN_ASSIGN = ALL_NODES && context.options[1] && context.options[1].returnAssign === false;
         const IGNORE_JSX = ALL_NODES && context.options[1] && context.options[1].ignoreJSX;
+        const PRECEDENCE_OF_UPDATE_EXPR = precedence({ type: "UpdateExpression" });
 
         /**
          * Determines if this rule should be enforced for a node given the current configuration.
@@ -428,6 +429,38 @@ module.exports = {
             }
         }
 
+        /**
+         * Check the parentheses around the super class of the given class definition.
+         * @param {ASTNode} node The node of class declarations to check.
+         * @returns {void}
+         */
+        function dryClass(node) {
+            if (!node.superClass) {
+                return;
+            }
+
+            // If `node.superClass` is a LeftHandSideExpression, parentheses are extra.
+            // Otherwise, parentheses are needed.
+            const hasExtraParens = precedence(node.superClass) > PRECEDENCE_OF_UPDATE_EXPR
+                ? hasExcessParens(node.superClass)
+                : hasDoubleExcessParens(node.superClass);
+
+            if (hasExtraParens) {
+                report(node.superClass);
+            }
+        }
+
+        /**
+         * Check the parentheses around the argument of the given spread operator.
+         * @param {ASTNode} node The node of spread elements/properties to check.
+         * @returns {void}
+         */
+        function drySpreadOperator(node) {
+            if (node.argument && hasExcessParens(node.argument)) {
+                report(node.argument);
+            }
+        }
+
         return {
             ArrayExpression(node) {
                 [].forEach.call(node.elements, e => {
@@ -668,7 +701,14 @@ module.exports = {
                         report(node.argument);
                     }
                 }
-            }
+            },
+
+            ClassDeclaration: dryClass,
+            ClassExpression: dryClass,
+
+            SpreadElement: drySpreadOperator,
+            SpreadProperty: drySpreadOperator,
+            ExperimentalSpreadProperty: drySpreadOperator
         };
 
     }
diff --git a/lib/rules/no-global-assign.js b/lib/rules/no-global-assign.js
index caf250023..5a1cc64ae 100644
--- a/lib/rules/no-global-assign.js
+++ b/lib/rules/no-global-assign.js
@@ -14,7 +14,7 @@ module.exports = {
         docs: {
             description: "disallow assignments to native objects or read-only global variables",
             category: "Best Practices",
-            recommended: false
+            recommended: true
         },
 
         schema: [


Test patch:
diff --git a/tests/lib/config/config-file.js b/tests/lib/config/config-file.js
index ed5842779..5b6521909 100644
--- a/tests/lib/config/config-file.js
+++ b/tests/lib/config/config-file.js
@@ -233,20 +233,63 @@ describe("ConfigFile", () => {
 
         });
 
+        it("should throw an error when a parser in a plugin config is not found", () => {
+            const resolvedPath = path.resolve(PROJECT_PATH, "./node_modules/eslint-plugin-test/index.js");
+
+            const configDeps = {
+                "../util/module-resolver": createStubModuleResolver({
+                    "eslint-plugin-test": resolvedPath
+                }),
+                "require-uncached"(filename) {
+                    return configDeps[filename];
+                }
+            };
+
+            configDeps[resolvedPath] = {
+                configs: {
+                    bar: {
+                        parser: "babel-eslint"
+                    }
+                }
+            };
+
+            const StubbedConfigFile = proxyquire("../../../lib/config/config-file", configDeps);
+
+            assert.throws(() => {
+                StubbedConfigFile.applyExtends({
+                    extends: "plugin:test/bar",
+                    rules: { eqeqeq: 2 }
+                }, "/whatever");
+            }, /Cannot find module 'babel-eslint'/);
+
+        });
+
         it("should throw an error when a plugin config is not found", () => {
+            const resolvedPath = path.resolve(PROJECT_PATH, "./node_modules/eslint-plugin-test/index.js");
 
             const configDeps = {
-                "../util/module-resolver": createStubModuleResolver({})
+                "../util/module-resolver": createStubModuleResolver({
+                    "eslint-plugin-test": resolvedPath
+                }),
+                "require-uncached"(filename) {
+                    return configDeps[filename];
+                }
+            };
+
+            configDeps[resolvedPath] = {
+                configs: {
+                    baz: {}
+                }
             };
 
             const StubbedConfigFile = proxyquire("../../../lib/config/config-file", configDeps);
 
             assert.throws(() => {
                 StubbedConfigFile.applyExtends({
-                    extends: "plugin:foo",
+                    extends: "plugin:test/bar",
                     rules: { eqeqeq: 2 }
                 }, "/whatever");
-            }, /Failed to load config "plugin:foo" to extend from./);
+            }, /Failed to load config "plugin:test\/bar" to extend from./);
 
         });
 
diff --git a/tests/lib/rules/no-extra-parens.js b/tests/lib/rules/no-extra-parens.js
index 85f7ba12f..b1e945e5e 100644
--- a/tests/lib/rules/no-extra-parens.js
+++ b/tests/lib/rules/no-extra-parens.js
@@ -367,7 +367,35 @@ ruleTester.run("no-extra-parens", rule, {
             "const Component = (<div",
             "  prop={true}",
             "/>)"
-        ].join("\n"), options: ["all", { ignoreJSX: "multi-line" }] }
+        ].join("\n"), options: ["all", { ignoreJSX: "multi-line" }] },
+
+        {
+            code: "let a = [ ...b ]",
+            parserOptions: { ecmaVersion: 2015 }
+        },
+        {
+            code: "let a = { ...b }",
+            parserOptions: {
+                ecmaVersion: 2015,
+                ecmaFeatures: { experimentalObjectRestSpread: true }
+            }
+        },
+        {
+            code: "class A extends B {}",
+            parserOptions: { ecmaVersion: 2015 }
+        },
+        {
+            code: "const A = class extends B {}",
+            parserOptions: { ecmaVersion: 2015 }
+        },
+        {
+            code: "class A extends (B=C) {}",
+            parserOptions: { ecmaVersion: 2015 }
+        },
+        {
+            code: "const A = class extends (B=C) {}",
+            parserOptions: { ecmaVersion: 2015 }
+        }
     ],
 
     invalid: [
@@ -841,6 +869,55 @@ ruleTester.run("no-extra-parens", rule, {
             "</div>)"
         ].join("\n"), "const Component = <div>\n<p />\n</div>", "JSXElement", 1, {
             options: ["all", { ignoreJSX: "none" }]
-        })
+        }),
+
+        // https://github.com/eslint/eslint/issues/8175
+        invalid(
+            "let a = [...(b)]",
+            "let a = [...b]",
+            "Identifier",
+            1,
+            { parserOptions: { ecmaVersion: 2015 } }
+        ),
+        invalid(
+            "let a = {...(b)}",
+            "let a = {...b}",
+            "Identifier",
+            1,
+            {
+                parserOptions: {
+                    ecmaVersion: 2015,
+                    ecmaFeatures: { experimentalObjectRestSpread: true }
+                }
+            }
+        ),
+        invalid(
+            "class A extends (B) {}",
+            "class A extends B {}",
+            "Identifier",
+            1,
+            { parserOptions: { ecmaVersion: 2015 } }
+        ),
+        invalid(
+            "const A = class extends (B) {}",
+            "const A = class extends B {}",
+            "Identifier",
+            1,
+            { parserOptions: { ecmaVersion: 2015 } }
+        ),
+        invalid(
+            "class A extends ((B=C)) {}",
+            "class A extends (B=C) {}",
+            "AssignmentExpression",
+            1,
+            { parserOptions: { ecmaVersion: 2015 } }
+        ),
+        invalid(
+            "const A = class extends ((B=C)) {}",
+            "const A = class extends (B=C) {}",
+            "AssignmentExpression",
+            1,
+            { parserOptions: { ecmaVersion: 2015 } }
+        )
     ]
 });