Repository: eslint/eslint
Pull request #11596 (2019-04-06 07:52:24)

Fix: object-shorthand ignoreConstructors option wrongly identifies '_' as constructor name
<!--
    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)**

[ ] Documentation update
[ X ] Bug fix ([template](https://raw.githubusercontent.com/eslint/eslint/master/templates/bug-report.md))
[ ] New rule ([template](https://raw.githubusercontent.com/eslint/eslint/master/templates/rule-proposal.md))
[ ] Changes an existing rule ([template](https://raw.githubusercontent.com/eslint/eslint/master/templates/rule-change-proposal.md))
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

<!--
    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 (https://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:
-->

**What changes did you make? (Give an overview)**

Fix for https://github.com/eslint/eslint/issues/11595.

Also added test for normal operation of `object-shorthand` `ignoreConstructors` option.

**Is there anything you'd like reviewers to focus on?**

Please see comments in https://github.com/eslint/eslint/issues/11595 about my queries about correct behavior. Specifically:

* Should `_Foo` or `$Foo` be considered constructor names? (this PR assumes not)
* Are there any other unicode chars which should be similarly be considered not valid as first char of constructor names? (this PR assumes not)

However, this PR does address the most common cases of method names starting with `'_'` or `'$'` and should not break anything else.


jsf-clabot: [![CLA assistant check](https://cla.js.foundation/pull/badge/not_signed)](https://cla.js.foundation/eslint/eslint?pullRequest=11596) <br/>Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our [Contributor License Agreement](https://cla.js.foundation/eslint/eslint?pullRequest=11596) before we can accept your contribution.<br/>
overlookmotel: Thanks @btmills!
overlookmotel: Amended PR according to discussion on #11595.
platinumazure: @btmills @mysticatea Can we use Unicode property escapes? My understanding is those are only available with `--harmony` in the 8.x line (per node.green). Am I missing something?
mysticatea: Ouch, you are right, @platinumazure. My memory was wrong.
platinumazure: That said @mysticatea, we could use the regenerate library to possibly cover this case better, similar to how we handle `capitalized-comments`. But I don't think we should require that in this PR; it could be done as a separate enhancement.

What do you think?

Patch:
diff --git a/lib/rules/object-shorthand.js b/lib/rules/object-shorthand.js
index 31010f0e7..6fa1b1834 100644
--- a/lib/rules/object-shorthand.js
+++ b/lib/rules/object-shorthand.js
@@ -113,6 +113,8 @@ module.exports = {
         // Helpers
         //--------------------------------------------------------------------------
 
+        const CTOR_PREFIX_REGEX = /[^_$0-9]/u;
+
         /**
          * Determines if the first character of the name is a capital letter.
          * @param {string} name The name of the node to evaluate.
@@ -120,7 +122,14 @@ module.exports = {
          * @private
          */
         function isConstructor(name) {
-            const firstChar = name.charAt(0);
+            const match = CTOR_PREFIX_REGEX.exec(name);
+
+            // Not a constructor if name has no characters apart from '_', '$' and digits e.g. '_', '$$', '_8'
+            if (!match) {
+                return false;
+            }
+
+            const firstChar = name.charAt(match.index);
 
             return firstChar === firstChar.toUpperCase();
         }
diff --git a/package.json b/package.json
index d15aa4d1f..d478057ab 100644
--- a/package.json
+++ b/package.json
@@ -55,7 +55,7 @@
     "import-fresh": "^3.0.0",
     "imurmurhash": "^0.1.4",
     "inquirer": "^6.2.2",
-    "js-yaml": "^3.13.0",
+    "js-yaml": "^3.13.1",
     "json-stable-stringify-without-jsonify": "^1.0.1",
     "levn": "^0.3.0",
     "lodash": "^4.17.11",
@@ -72,16 +72,16 @@
     "text-table": "^0.2.0"
   },
   "devDependencies": {
-    "@babel/core": "^7.2.2",
-    "@babel/polyfill": "^7.2.5",
-    "@babel/preset-env": "^7.3.1",
+    "@babel/core": "^7.4.3",
+    "@babel/polyfill": "^7.4.3",
+    "@babel/preset-env": "^7.4.3",
     "babel-loader": "^8.0.5",
     "beefy": "^2.1.8",
-    "brfs": "^2.0.0",
+    "brfs": "^2.0.2",
     "chai": "^4.0.1",
     "cheerio": "^0.22.0",
     "common-tags": "^1.8.0",
-    "coveralls": "^3.0.1",
+    "coveralls": "^3.0.3",
     "dateformat": "^3.0.3",
     "ejs": "^2.6.1",
     "eslint-config-eslint": "file:packages/eslint-config-eslint",
@@ -99,19 +99,19 @@
     "karma-webpack": "^4.0.0-rc.6",
     "leche": "^2.2.3",
     "load-perf": "^0.2.0",
-    "markdownlint": "^0.12.0",
-    "mocha": "^5.0.5",
+    "markdownlint": "^0.13.0",
+    "mocha": "^6.1.2",
     "mock-fs": "^4.8.0",
     "npm-license": "^0.3.3",
     "nyc": "^13.3.0",
     "proxyquire": "^2.0.1",
-    "puppeteer": "^1.12.2",
+    "puppeteer": "^1.14.0",
     "shelljs": "^0.8.2",
     "sinon": "^3.3.0",
     "temp": "^0.9.0",
     "through": "^2.3.8",
-    "webpack": "^4.29.3",
-    "webpack-cli": "^3.2.3"
+    "webpack": "^4.29.6",
+    "webpack-cli": "^3.3.0"
   },
   "keywords": [
     "ast",


Test patch:
diff --git a/tests/lib/rules/object-shorthand.js b/tests/lib/rules/object-shorthand.js
index 19d235290..72bd7ba6e 100644
--- a/tests/lib/rules/object-shorthand.js
+++ b/tests/lib/rules/object-shorthand.js
@@ -131,6 +131,22 @@ ruleTester.run("object-shorthand", rule, {
             code: "var x = {ConstructorFunction: function(){}, a: b}",
             options: ["always", { ignoreConstructors: true }]
         },
+        {
+            code: "var x = {_ConstructorFunction: function(){}, a: b}",
+            options: ["always", { ignoreConstructors: true }]
+        },
+        {
+            code: "var x = {$ConstructorFunction: function(){}, a: b}",
+            options: ["always", { ignoreConstructors: true }]
+        },
+        {
+            code: "var x = {__ConstructorFunction: function(){}, a: b}",
+            options: ["always", { ignoreConstructors: true }]
+        },
+        {
+            code: "var x = {_0ConstructorFunction: function(){}, a: b}",
+            options: ["always", { ignoreConstructors: true }]
+        },
         {
             code: "var x = {notConstructorFunction(){}, b: c}",
             options: ["always", { ignoreConstructors: true }]
@@ -139,6 +155,22 @@ ruleTester.run("object-shorthand", rule, {
             code: "var x = {ConstructorFunction: function(){}, a: b}",
             options: ["methods", { ignoreConstructors: true }]
         },
+        {
+            code: "var x = {_ConstructorFunction: function(){}, a: b}",
+            options: ["methods", { ignoreConstructors: true }]
+        },
+        {
+            code: "var x = {$ConstructorFunction: function(){}, a: b}",
+            options: ["methods", { ignoreConstructors: true }]
+        },
+        {
+            code: "var x = {__ConstructorFunction: function(){}, a: b}",
+            options: ["methods", { ignoreConstructors: true }]
+        },
+        {
+            code: "var x = {_0ConstructorFunction: function(){}, a: b}",
+            options: ["methods", { ignoreConstructors: true }]
+        },
         {
             code: "var x = {notConstructorFunction(){}, b: c}",
             options: ["methods", { ignoreConstructors: true }]
@@ -708,6 +740,46 @@ ruleTester.run("object-shorthand", rule, {
             errors: [LONGFORM_PROPERTY_ERROR]
         },
 
+        // ignoreConstructors
+        {
+            code: "var x = {y: function() {}}",
+            output: "var x = {y() {}}",
+            options: ["methods", { ignoreConstructors: true }],
+            errors: [METHOD_ERROR]
+        },
+        {
+
+            // https://github.com/eslint/eslint/issues/11595
+            code: "var x = {_y: function() {}}",
+            output: "var x = {_y() {}}",
+            options: ["methods", { ignoreConstructors: true }],
+            errors: [METHOD_ERROR]
+        },
+        {
+
+            // https://github.com/eslint/eslint/issues/11595
+            code: "var x = {$y: function() {}}",
+            output: "var x = {$y() {}}",
+            options: ["methods", { ignoreConstructors: true }],
+            errors: [METHOD_ERROR]
+        },
+        {
+
+            // https://github.com/eslint/eslint/issues/11595
+            code: "var x = {__y: function() {}}",
+            output: "var x = {__y() {}}",
+            options: ["methods", { ignoreConstructors: true }],
+            errors: [METHOD_ERROR]
+        },
+        {
+
+            // https://github.com/eslint/eslint/issues/11595
+            code: "var x = {_0y: function() {}}",
+            output: "var x = {_0y() {}}",
+            options: ["methods", { ignoreConstructors: true }],
+            errors: [METHOD_ERROR]
+        },
+
         // avoidQuotes
         {
             code: "var x = {a: a}",