Repository: eslint/eslint (js)

Issue:
Proposal: removing `parserOptions.experimentalObjectRestSpread` option
## Context

Since ESLint `4.18.0`, we have supported Rest/Spread Properties in formal with `parserOptions.ecmaVersion: 2018`. So the `parserOptions.experimentalObjectRestSpread` option has finished its role.

The `parserOptions.experimentalObjectRestSpread` option has some confusion because we added it early time.

- There are three node types for the same syntax; `ExperimentalRestProperty`, `RestProperty`, and `RestElement`. And spread properties are so. This is unfortunate for plugin ecosystem.

Also, the `parserOptions.experimentalObjectRestSpread` option sometimes has hidden bug fixes in acorn. For example:

- https://github.com/eslint/espree/pull/353
- https://github.com/eslint/espree/pull/341

The cause is that we make some overridden methods on espree to implement it. In fact, the implementation of `parserOptions.experimentalObjectRestSpread` has a bug about the duplication check of ES modules currently.

```js
export const b = 1
export const {a, ...b} = obj // "SyntaxError: Identifier 'b' has already been declared" in `ecmaVersion:2018`, but not in `experimentalObjectRestSpread:true`.
```

## Proposal

Remove the `parserOptions.experimentalObjectRestSpread` option in the next major version, 5.0.0.
To replace it by `parserOptions.ecmaVersion: 2018` to migrate.

This is similar to [Migrating to 2.0.0](https://eslint.org/docs/user-guide/migrating-to-2.0.0#language-options) we have done in past.

### Pros

- Reducing confusion about AST node types. This removal removes `ExperimentalRestProperty` and `ExperimentalSpreadProperty`.
- Reducing bugs. This removal removes the double management of the implementation of Rest/Spread Properties. This will prevent bugs such as https://github.com/eslint/espree/pull/341 and https://github.com/eslint/espree/pull/353. We can get bug fixes in acorn quickly in the future.
- Making consistent. We specify ECMA262 Standard version by `parserOptions.ecmaVersion` option since 2.0.0. The `parserOptions.experimentalObjectRestSpread` option is the last exception.

### Cons

- Many users are using the `parserOptions.experimentalObjectRestSpread` option. This removal can cause confusion in migrating to 5.0.0.

Pull request:
Update: re-enable experimentalObjectRestSpread (fixes #9990)
**What is the purpose of this pull request? (put an "X" next to item)**

[X] Other, please explain: fixes #9990

**What changes did you make? (Give an overview)**

1. If `parserOptions.ecmaFeatures.experimentalObjectRestSpread` is given and `parserOptions.ecmaVersion` is less than `9`, `linter` enables `parserOptions.ecmaVersion: 9` for backward compatibility.
2. If `parserOptions.ecmaFeatures.experimentalObjectRestSpread` is given, `linter` generates a deprecation warning to show the deprecation.

**Is there anything you'd like reviewers to focus on?**

I make the deprecation warning is as a message with `severity: 1`. Should it be with another way?


Patch:
diff --git a/docs/rules/no-mixed-spaces-and-tabs.md b/docs/rules/no-mixed-spaces-and-tabs.md
index 4f76b2617..ba34dbe49 100644
--- a/docs/rules/no-mixed-spaces-and-tabs.md
+++ b/docs/rules/no-mixed-spaces-and-tabs.md
@@ -41,7 +41,7 @@ function add(x, y) {
 
 This rule has a string option.
 
-* `"smart-tabs"` allows mixed spaces and tabs when the latter are used for alignment.
+* `"smart-tabs"` allows mixed tabs and spaces when the spaces are used for alignment.
 
 ### smart-tabs
 
diff --git a/lib/config/config-validator.js b/lib/config/config-validator.js
index 1a5b3ef13..ed9a34cb0 100644
--- a/lib/config/config-validator.js
+++ b/lib/config/config-validator.js
@@ -9,7 +9,8 @@
 // Requirements
 //------------------------------------------------------------------------------
 
-const ajv = require("../util/ajv"),
+const path = require("path"),
+    ajv = require("../util/ajv"),
     lodash = require("lodash"),
     configSchema = require("../../conf/config-schema.js"),
     util = require("util");
@@ -21,6 +22,12 @@ const ruleValidators = new WeakMap();
 //------------------------------------------------------------------------------
 let validateSchema;
 
+// Defitions for deprecation warnings.
+const deprecationWarningMessages = Object.freeze({
+    ESLINT_LEGACY_ECMAFEATURES: "The 'ecmaFeatures' config file property is deprecated, and has no effect.",
+    ESLINT_LEGACY_OBJECT_REST_SPREAD: "The 'parserOptions.ecmaFeatures.experimentalObjectRestSpread' option is deprecated. Use 'parserOptions.ecmaVersion' instead."
+});
+
 /**
  * Gets a complete options schema for a rule.
  * @param {{create: Function, schema: (Array|null)}} rule A new-style rule object
@@ -194,18 +201,18 @@ function formatErrors(errors) {
  * for each unique file path, but repeated invocations with the same file path have no effect.
  * No warnings are emitted if the `--no-deprecation` or `--no-warnings` Node runtime flags are active.
  * @param {string} source The name of the configuration source to report the warning for.
+ * @param {string} errorCode The warning message to show.
  * @returns {void}
  */
-const emitEcmaFeaturesWarning = lodash.memoize(source => {
-
-    /*
-     * util.deprecate seems to be the only way to emit a warning in Node 4.x while respecting the --no-warnings flag.
-     * (In Node 6+, process.emitWarning could be used instead.)
-     */
-    util.deprecate(
-        () => {},
-        `[eslint] The 'ecmaFeatures' config file property is deprecated, and has no effect. (found in ${source})`
-    )();
+const emitDeprecationWarning = lodash.memoize((source, errorCode) => {
+    const rel = path.relative(process.cwd(), source);
+    const message = deprecationWarningMessages[errorCode];
+
+    process.emitWarning(
+        `${message} (found in "${rel}")`,
+        "DeprecationWarning",
+        errorCode
+    );
 });
 
 /**
@@ -221,8 +228,17 @@ function validateConfigSchema(config, source) {
         throw new Error(`ESLint configuration in ${source} is invalid:\n${formatErrors(validateSchema.errors)}`);
     }
 
-    if (Object.prototype.hasOwnProperty.call(config, "ecmaFeatures")) {
-        emitEcmaFeaturesWarning(source);
+    if (Object.hasOwnProperty.call(config, "ecmaFeatures")) {
+        emitDeprecationWarning(source, "ESLINT_LEGACY_ECMAFEATURES");
+    }
+
+    if (
+        (config.parser || "espree") === "espree" &&
+        config.parserOptions &&
+        config.parserOptions.ecmaFeatures &&
+        config.parserOptions.ecmaFeatures.experimentalObjectRestSpread
+    ) {
+        emitDeprecationWarning(source, "ESLINT_LEGACY_OBJECT_REST_SPREAD");
     }
 }
 
diff --git a/lib/linter.js b/lib/linter.js
index 7ca835c15..ec4b4f6d9 100644
--- a/lib/linter.js
+++ b/lib/linter.js
@@ -410,11 +410,12 @@ function normalizeVerifyOptions(providedOptions) {
 
 /**
  * Combines the provided parserOptions with the options from environments
+ * @param {string} parserName The parser name which uses this options.
  * @param {Object} providedOptions The provided 'parserOptions' key in a config
  * @param {Environment[]} enabledEnvironments The environments enabled in configuration and with inline comments
  * @returns {Object} Resulting parser options after merge
  */
-function resolveParserOptions(providedOptions, enabledEnvironments) {
+function resolveParserOptions(parserName, providedOptions, enabledEnvironments) {
     const parserOptionsFromEnv = enabledEnvironments
         .filter(env => env.parserOptions)
         .reduce((parserOptions, env) => ConfigOps.merge(parserOptions, env.parserOptions), {});
@@ -431,6 +432,16 @@ function resolveParserOptions(providedOptions, enabledEnvironments) {
 
     mergedParserOptions.ecmaVersion = normalizeEcmaVersion(mergedParserOptions.ecmaVersion, isModule);
 
+    // TODO: For backward compatibility. Will remove on v6.0.0.
+    if (
+        parserName === DEFAULT_PARSER_NAME &&
+        mergedParserOptions.ecmaFeatures &&
+        mergedParserOptions.ecmaFeatures.experimentalObjectRestSpread &&
+        (!mergedParserOptions.ecmaVersion || mergedParserOptions.ecmaVersion < 9)
+    ) {
+        mergedParserOptions.ecmaVersion = 9;
+    }
+
     return mergedParserOptions;
 }
 
@@ -923,9 +934,9 @@ module.exports = class Linter {
             .map(envName => this.environments.get(envName))
             .filter(env => env);
 
-        const parserOptions = resolveParserOptions(config.parserOptions || {}, enabledEnvs);
-        const configuredGlobals = resolveGlobals(config.globals || {}, enabledEnvs);
         const parserName = config.parser || DEFAULT_PARSER_NAME;
+        const parserOptions = resolveParserOptions(parserName, config.parserOptions || {}, enabledEnvs);
+        const configuredGlobals = resolveGlobals(config.globals || {}, enabledEnvs);
         const settings = config.settings || {};
 
         if (!lastSourceCodes.get(this)) {
diff --git a/lib/rules/object-shorthand.js b/lib/rules/object-shorthand.js
index c5239a71d..71a320a0e 100644
--- a/lib/rules/object-shorthand.js
+++ b/lib/rules/object-shorthand.js
@@ -233,6 +233,10 @@ module.exports = {
             const keyText = sourceCode.text.slice(firstKeyToken.range[0], lastKeyToken.range[1]);
             let keyPrefix = "";
 
+            if (sourceCode.commentsExistBetween(lastKeyToken, node.value)) {
+                return null;
+            }
+
             if (node.value.async) {
                 keyPrefix += "async ";
             }
diff --git a/lib/util/module-resolver.js b/lib/util/module-resolver.js
index 470a54f7c..1e9b66330 100644
--- a/lib/util/module-resolver.js
+++ b/lib/util/module-resolver.js
@@ -59,9 +59,7 @@ class ModuleResolver {
          * subsequent calls to this function. Then, move the extraLookupPath to the
          * top of the lookup paths list so it will be searched first.
          */
-        const lookupPaths = this.options.lookupPaths.concat();
-
-        lookupPaths.unshift(extraLookupPath);
+        const lookupPaths = [extraLookupPath, ...this.options.lookupPaths];
 
         /**
          * Module._findPath is an internal method to Node.js, then one they use to
diff --git a/package.json b/package.json
index e1199bb32..7007d48ba 100644
--- a/package.json
+++ b/package.json
@@ -90,7 +90,7 @@
     "eslint-plugin-rulesdir": "^0.1.0",
     "eslint-release": "^0.11.1",
     "eslint-rule-composer": "^0.1.1",
-    "eslump": "^1.6.1",
+    "eslump": "^1.6.2",
     "esprima": "^4.0.0",
     "istanbul": "^0.4.5",
     "jsdoc": "^3.5.5",
diff --git a/tools/eslint-fuzzer.js b/tools/eslint-fuzzer.js
index ea84e994b..342dee3e4 100644
--- a/tools/eslint-fuzzer.js
+++ b/tools/eslint-fuzzer.js
@@ -110,7 +110,7 @@ function fuzz(options) {
         const text = codeGenerator({ sourceType });
         const config = {
             rules: lodash.mapValues(ruleConfigs, lodash.sample),
-            parserOptions: { sourceType, ecmaVersion: 2017 }
+            parserOptions: { sourceType, ecmaVersion: 2018 }
         };
 
         let autofixResult;


Test patch:
diff --git a/tests/fixtures/config-file/experimental-object-rest-spread/another-parser/.eslintrc.yml b/tests/fixtures/config-file/experimental-object-rest-spread/another-parser/.eslintrc.yml
new file mode 100644
index 000000000..c64b2c309
--- /dev/null
+++ b/tests/fixtures/config-file/experimental-object-rest-spread/another-parser/.eslintrc.yml
@@ -0,0 +1,6 @@
+root: true
+
+parser: ./parser.js
+parserOptions:
+  ecmaFeatures:
+    experimentalObjectRestSpread: true
diff --git a/tests/fixtures/config-file/experimental-object-rest-spread/another-parser/parser.js b/tests/fixtures/config-file/experimental-object-rest-spread/another-parser/parser.js
new file mode 100644
index 000000000..f193589ef
--- /dev/null
+++ b/tests/fixtures/config-file/experimental-object-rest-spread/another-parser/parser.js
@@ -0,0 +1,1 @@
+module.exports.parse = () => ({ type: "Program", body: [] })
diff --git a/tests/fixtures/config-file/experimental-object-rest-spread/basic/.eslintrc.yml b/tests/fixtures/config-file/experimental-object-rest-spread/basic/.eslintrc.yml
new file mode 100644
index 000000000..a5c9e4ebf
--- /dev/null
+++ b/tests/fixtures/config-file/experimental-object-rest-spread/basic/.eslintrc.yml
@@ -0,0 +1,5 @@
+root: true
+
+parserOptions:
+  ecmaFeatures:
+    experimentalObjectRestSpread: true
diff --git a/tests/fixtures/config-file/experimental-object-rest-spread/extends/.eslintrc.yml b/tests/fixtures/config-file/experimental-object-rest-spread/extends/.eslintrc.yml
new file mode 100644
index 000000000..2daef0dea
--- /dev/null
+++ b/tests/fixtures/config-file/experimental-object-rest-spread/extends/.eslintrc.yml
@@ -0,0 +1,4 @@
+root: true
+
+extends:
+  - ./common.yml
diff --git a/tests/fixtures/config-file/experimental-object-rest-spread/extends/common.yml b/tests/fixtures/config-file/experimental-object-rest-spread/extends/common.yml
new file mode 100644
index 000000000..99975a7bd
--- /dev/null
+++ b/tests/fixtures/config-file/experimental-object-rest-spread/extends/common.yml
@@ -0,0 +1,3 @@
+parserOptions:
+  ecmaFeatures:
+    experimentalObjectRestSpread: true
diff --git a/tests/fixtures/config-file/experimental-object-rest-spread/subdir/.eslintrc.yml b/tests/fixtures/config-file/experimental-object-rest-spread/subdir/.eslintrc.yml
new file mode 100644
index 000000000..a5c9e4ebf
--- /dev/null
+++ b/tests/fixtures/config-file/experimental-object-rest-spread/subdir/.eslintrc.yml
@@ -0,0 +1,5 @@
+root: true
+
+parserOptions:
+  ecmaFeatures:
+    experimentalObjectRestSpread: true
diff --git a/tests/fixtures/config-file/experimental-object-rest-spread/subdir/lib/.eslintrc.yml b/tests/fixtures/config-file/experimental-object-rest-spread/subdir/lib/.eslintrc.yml
new file mode 100644
index 000000000..78dc43786
--- /dev/null
+++ b/tests/fixtures/config-file/experimental-object-rest-spread/subdir/lib/.eslintrc.yml
@@ -0,0 +1,2 @@
+rules:
+  no-undef: off
diff --git a/tests/lib/config.js b/tests/lib/config.js
index 1f9690339..6baae0e05 100644
--- a/tests/lib/config.js
+++ b/tests/lib/config.js
@@ -83,6 +83,14 @@ function assertConfigsEqual(actual, expected) {
     }
 }
 
+/**
+ * Wait for the next tick.
+ * @returns {Promise<void>} -
+ */
+function nextTick() {
+    return new Promise(resolve => process.nextTick(resolve));
+}
+
 //------------------------------------------------------------------------------
 // Tests
 //------------------------------------------------------------------------------
@@ -1086,7 +1094,6 @@ describe("Config", () => {
             });
         });
 
-
         describe("with overrides", () => {
 
             /**
@@ -1359,6 +1366,112 @@ describe("Config", () => {
                 assertConfigsEqual(actual, expected);
             });
         });
+
+        describe("deprecation warnings", () => {
+            let warning = null;
+
+            function onWarning(w) { // eslint-disable-line require-jsdoc
+
+                // Node.js 6.x does not have 'w.code' property.
+                if (!w.hasOwnProperty("code") || typeof w.code === "string" && w.code.startsWith("ESLINT_")) {
+                    warning = w;
+                }
+            }
+
+            beforeEach(() => {
+                warning = null;
+                process.on("warning", onWarning);
+            });
+            afterEach(() => {
+                process.removeListener("warning", onWarning);
+            });
+
+            it("should emit a deprecation warning if 'ecmaFeatures' is given.", () => Promise.resolve()
+                .then(() => {
+                    const cwd = path.resolve(__dirname, "../fixtures/config-file/ecma-features/");
+                    const config = new Config({ cwd }, linter);
+
+                    config.getConfig("test.js");
+
+                    // Wait for "warning" event.
+                    return nextTick();
+                })
+                .then(() => {
+                    assert.notStrictEqual(warning, null);
+                    assert.strictEqual(
+                        warning.message,
+                        `The 'ecmaFeatures' config file property is deprecated, and has no effect. (found in "tests${path.sep}fixtures${path.sep}config-file${path.sep}ecma-features${path.sep}.eslintrc.yml")`
+                    );
+                }));
+
+            it("should emit a deprecation warning if 'parserOptions.ecmaFeatures.experimentalObjectRestSpread' is given.", () => Promise.resolve()
+                .then(() => {
+                    const cwd = path.resolve(__dirname, "../fixtures/config-file/experimental-object-rest-spread/basic/");
+                    const config = new Config({ cwd }, linter);
+
+                    config.getConfig("test.js");
+
+                    // Wait for "warning" event.
+                    return nextTick();
+                })
+                .then(() => {
+                    assert.notStrictEqual(warning, null);
+                    assert.strictEqual(
+                        warning.message,
+                        `The 'parserOptions.ecmaFeatures.experimentalObjectRestSpread' option is deprecated. Use 'parserOptions.ecmaVersion' instead. (found in "tests${path.sep}fixtures${path.sep}config-file${path.sep}experimental-object-rest-spread${path.sep}basic${path.sep}.eslintrc.yml")`
+                    );
+                }));
+
+            it("should emit a deprecation warning if 'parserOptions.ecmaFeatures.experimentalObjectRestSpread' is given in a parent config.", () => Promise.resolve()
+                .then(() => {
+                    const cwd = path.resolve(__dirname, "../fixtures/config-file/experimental-object-rest-spread/subdir/");
+                    const config = new Config({ cwd }, linter);
+
+                    config.getConfig("lib/test.js");
+
+                    // Wait for "warning" event.
+                    return nextTick();
+                })
+                .then(() => {
+                    assert.notStrictEqual(warning, null);
+                    assert.strictEqual(
+                        warning.message,
+                        `The 'parserOptions.ecmaFeatures.experimentalObjectRestSpread' option is deprecated. Use 'parserOptions.ecmaVersion' instead. (found in "tests${path.sep}fixtures${path.sep}config-file${path.sep}experimental-object-rest-spread${path.sep}subdir${path.sep}.eslintrc.yml")`
+                    );
+                }));
+
+            it("should emit a deprecation warning if 'parserOptions.ecmaFeatures.experimentalObjectRestSpread' is given in a shareable config.", () => Promise.resolve()
+                .then(() => {
+                    const cwd = path.resolve(__dirname, "../fixtures/config-file/experimental-object-rest-spread/extends/");
+                    const config = new Config({ cwd }, linter);
+
+                    config.getConfig("test.js");
+
+                    // Wait for "warning" event.
+                    return nextTick();
+                })
+                .then(() => {
+                    assert.notStrictEqual(warning, null);
+                    assert.strictEqual(
+                        warning.message,
+                        `The 'parserOptions.ecmaFeatures.experimentalObjectRestSpread' option is deprecated. Use 'parserOptions.ecmaVersion' instead. (found in "tests${path.sep}fixtures${path.sep}config-file${path.sep}experimental-object-rest-spread${path.sep}extends${path.sep}common.yml")`
+                    );
+                }));
+
+            it("should NOT emit a deprecation warning even if 'parserOptions.ecmaFeatures.experimentalObjectRestSpread' is given, if parser is not espree.", () => Promise.resolve()
+                .then(() => {
+                    const cwd = path.resolve(__dirname, "../fixtures/config-file/experimental-object-rest-spread/another-parser/");
+                    const config = new Config({ cwd }, linter);
+
+                    config.getConfig("test.js");
+
+                    // Wait for "warning" event.
+                    return nextTick();
+                })
+                .then(() => {
+                    assert.strictEqual(warning, null);
+                }));
+        });
     });
 
     describe("Plugin Environments", () => {
diff --git a/tests/lib/linter.js b/tests/lib/linter.js
index 53290c1ed..667e75e5b 100644
--- a/tests/lib/linter.js
+++ b/tests/lib/linter.js
@@ -3492,6 +3492,15 @@ describe("Linter", () => {
             linter.verify("x", { rules: { "foo-bar-baz": "error" } });
             assert(spy.calledOnce);
         });
+
+        it("should parse ES2018 code for backward compatibility if 'parserOptions.ecmaFeatures.experimentalObjectRestSpread' is given.", () => {
+            const messages = linter.verify(
+                "async function* f() { let {a, ...rest} = { a, ...obj }; }",
+                { parserOptions: { ecmaFeatures: { experimentalObjectRestSpread: true } } }
+            );
+
+            assert(messages.length === 0);
+        });
     });
 
     describe("Variables and references", () => {
diff --git a/tests/lib/rules/object-shorthand.js b/tests/lib/rules/object-shorthand.js
index 31abe8faa..c52f43af0 100644
--- a/tests/lib/rules/object-shorthand.js
+++ b/tests/lib/rules/object-shorthand.js
@@ -427,6 +427,16 @@ ruleTester.run("object-shorthand", rule, {
             output: "var x = {\n  *f() {\n    /* comment */\n    a(b);\n    }\n  }",
             errors: [METHOD_ERROR]
         },
+        {
+            code: "var x = {\n  f: /* comment */ function() {\n  }\n  }",
+            output: null,
+            errors: [METHOD_ERROR]
+        },
+        {
+            code: "var x = {\n f /* comment */: function() {\n  }\n  }",
+            output: null,
+            errors: [METHOD_ERROR]
+        },
         {
             code: "var x = {y: function() {}}",
             output: "var x = {y() {}}",