Repository: eslint/eslint (js)

Issue:
camelcase duplicate warning bug
**Tell us about your environment**

* **ESLint Version:** 5.4.0
* **Node Version:** 8.11.3
* **npm Version:** 5.6.0

**What parser (default, Babel-ESLint, etc.) are you using?**
Coffeescript (ran into this while working on [eslint-plugin-coffee](https://github.com/helixbass/eslint-plugin-coffee))

**Please show your full configuration:**

The relevant configuration would be `camelcase: error`. I encountered this while testing the Coffeescript custom parser against the existing ESLint `lib/rules/camelcase.js` rule

**What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.**

<!-- Paste the source code below: -->
I was running against a copied version of `tests/lib/rules/camelcase.js` where the tested pieces of code were converted to Coffeescript, but these are the corresponding tests in `tests/lib/rules/camelcase.js` that would have been failing:
- `invalid` test `var { category_id } = query;` (where `ignoreDestructuring` is implicitly `false`)
- `invalid` test `function foo({ no_camelcased }) {};` (where `ignoreDestructuring` is implicitly `false`)

**What did you expect to happen?**
For these tests to pass

**What actually happened? Please include the actual, raw output from ESLint.**
They failed due to reporting 2 errors instead of the expected one. I found the bug and will open a fixing PR momentarily



Pull request:
Fix: camelcase duplicate warning bug (fixes #10801)
<!--
    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.)
-->
See #10801 for description of behavior

<!--
    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)**
Fixed the logic in `lib/rules/camelcase.js`, and added a test that would've been broken before the fix

Basically, there appears to have been faulty logic introduced by #10373:

This guard (pre-#10373) was preventing reporting the `key` of a `Property`:
```
node.parent.key === node && node.parent.value !== node
```
Then #10373 replaced that guard with this:
```
!assignmentKeyEqualsValue && node.parent.key === node
```
The hole introduced in the logic is if `assignmentKeyEqualsValue` but `node.parent.key !== node.parent.value`, the `value` gets reported twice (once while visiting the `key` and once while visiting the `value`)

I believe that the only reason existing `invalid` tests like `var { category_id } = query;` weren't failing on duplicate warnings is that the default parser (`espree`?) must reuse the same `Identifier` node for the `key` and `value` of a shorthand `Property` (as opposed to the Coffeescript custom parser I'm working on, which doesn't reuse the same node, thus how I found this bug)

But then by adding an `invalid` test for `var { category_id: category_id } = query;`, you get a duplicate warning (it exposes the hole in the logic, since `assignmentKeyEqualsValue` but the `Property`'s `key` and `value` are two different `Identifier` objects)

So the fix is basically to restore the pre-#10373 guard logic so that you effectively never visit the `Property` `key`

**Is there anything you'd like reviewers to focus on?**




Patch:
diff --git a/docs/rules/no-useless-concat.md b/docs/rules/no-useless-concat.md
index b1e54cc82..5dc5f1fc0 100644
--- a/docs/rules/no-useless-concat.md
+++ b/docs/rules/no-useless-concat.md
@@ -22,8 +22,9 @@ Examples of **incorrect** code for this rule:
 /*eslint no-useless-concat: "error"*/
 /*eslint-env es6*/
 
-// these are the same as "10"
 var a = `some` + `string`;
+
+// these are the same as "10"
 var a = '1' + '0';
 var a = '1' + `0`;
 var a = `1` + '0';
diff --git a/docs/user-guide/integrations.md b/docs/user-guide/integrations.md
index 17d037860..582f77e3e 100644
--- a/docs/user-guide/integrations.md
+++ b/docs/user-guide/integrations.md
@@ -10,11 +10,12 @@
     * [Syntastic](https://github.com/vim-syntastic/syntastic/tree/master/syntax_checkers/javascript)
 * Emacs: [Flycheck](http://www.flycheck.org/) supports ESLint with the [javascript-eslint](http://www.flycheck.org/en/latest/languages.html#javascript) checker.
 * Eclipse Orion: ESLint is the [default linter](https://dev.eclipse.org/mhonarc/lists/orion-dev/msg02718.html)
-* Eclipse IDE with [Tern ESLint linter](https://github.com/angelozerr/tern.java/wiki/Tern-Linter-ESLint)
-* [TextMate 2](https://github.com/natesilva/javascript-eslint.tmbundle)
+* Eclipse IDE: [Tern ESLint linter](https://github.com/angelozerr/tern.java/wiki/Tern-Linter-ESLint)
+* TextMate 2: [ESLint Bundle](https://github.com/natesilva/javascript-eslint.tmbundle)
 * Atom: [linter-eslint](https://atom.io/packages/linter-eslint)
-* [IntelliJ IDEA, RubyMine, WebStorm, PhpStorm, PyCharm, AppCode, Android Studio, 0xDBE](https://plugins.jetbrains.com/plugin/7494-eslint)
-* [Visual Studio Code](https://code.visualstudio.com) with the [ESLint Extension](https://marketplace.visualstudio.com/items?itemName=dbaeumer.vscode-eslint)
+* IntelliJ IDEA, RubyMine, WebStorm, PhpStorm, PyCharm, AppCode, Android Studio, 0xDBE: [ESLint Plugin](https://plugins.jetbrains.com/plugin/7494-eslint)
+* Visual Studio Code: [ESLint Extension](https://marketplace.visualstudio.com/items?itemName=dbaeumer.vscode-eslint)
+* Brackets: Included and [Brackets ESLint](https://github.com/brackets-userland/brackets-eslint)
 
 ## Build tools
 
diff --git a/lib/formatters/codeframe.js b/lib/formatters/codeframe.js
index 0b97a0d81..a42872c5a 100644
--- a/lib/formatters/codeframe.js
+++ b/lib/formatters/codeframe.js
@@ -5,7 +5,7 @@
 "use strict";
 
 const chalk = require("chalk");
-const codeFrame = require("babel-code-frame");
+const { codeFrameColumns } = require("@babel/code-frame");
 const path = require("path");
 
 //------------------------------------------------------------------------------
@@ -63,7 +63,7 @@ function formatMessage(message, parentResult) {
 
     if (sourceCode) {
         result.push(
-            codeFrame(sourceCode, message.line, message.column, { highlightCode: false })
+            codeFrameColumns(sourceCode, { start: { line: message.line, column: message.column } }, { highlightCode: false })
         );
     }
 
diff --git a/lib/rules/camelcase.js b/lib/rules/camelcase.js
index ec6116621..3005963ab 100644
--- a/lib/rules/camelcase.js
+++ b/lib/rules/camelcase.js
@@ -145,7 +145,7 @@ module.exports = {
                         const assignmentKeyEqualsValue = node.parent.key.name === node.parent.value.name;
 
                         // prevent checking righthand side of destructured object
-                        if (!assignmentKeyEqualsValue && node.parent.key === node) {
+                        if (node.parent.key === node && node.parent.value !== node) {
                             return;
                         }
 
diff --git a/lib/rules/max-params.js b/lib/rules/max-params.js
index 902391b74..4089e8ae8 100644
--- a/lib/rules/max-params.js
+++ b/lib/rules/max-params.js
@@ -53,7 +53,7 @@ module.exports = {
     },
 
     create(context) {
-
+        const sourceCode = context.getSourceCode();
         const option = context.options[0];
         let numParams = 3;
 
@@ -76,6 +76,7 @@ module.exports = {
         function checkFunction(node) {
             if (node.params.length > numParams) {
                 context.report({
+                    loc: astUtils.getFunctionHeadLoc(node, sourceCode),
                     node,
                     message: "{{name}} has too many parameters ({{count}}). Maximum allowed is {{max}}.",
                     data: {
diff --git a/package.json b/package.json
index 65681ae36..7c49f5fa6 100644
--- a/package.json
+++ b/package.json
@@ -35,8 +35,8 @@
   "homepage": "https://eslint.org",
   "bugs": "https://github.com/eslint/eslint/issues/",
   "dependencies": {
+    "@babel/code-frame": "^7.0.0",
     "ajv": "^6.5.3",
-    "babel-code-frame": "^6.26.0",
     "chalk": "^2.1.0",
     "cross-spawn": "^6.0.5",
     "debug": "^3.1.0",


Test patch:
diff --git a/tests/lib/rules/camelcase.js b/tests/lib/rules/camelcase.js
index f21459085..481842f97 100644
--- a/tests/lib/rules/camelcase.js
+++ b/tests/lib/rules/camelcase.js
@@ -352,6 +352,17 @@ ruleTester.run("camelcase", rule, {
                 }
             ]
         },
+        {
+            code: "var { category_id: category_id } = query;",
+            parserOptions: { ecmaVersion: 6 },
+            errors: [
+                {
+                    messageId: "notCamelCase",
+                    data: { name: "category_id" },
+                    type: "Identifier"
+                }
+            ]
+        },
         {
             code: "var { category_id = 1 } = query;",
             parserOptions: { ecmaVersion: 6 },
diff --git a/tests/lib/rules/max-params.js b/tests/lib/rules/max-params.js
index 3cb47044f..959f5d7b8 100644
--- a/tests/lib/rules/max-params.js
+++ b/tests/lib/rules/max-params.js
@@ -86,6 +86,20 @@ ruleTester.run("max-params", rule, {
                 message: "Function 'test' has too many parameters (3). Maximum allowed is 2.",
                 type: "FunctionDeclaration"
             }]
+        },
+
+        // Error location should not cover the entire function; just the name.
+        {
+            code: `function test(a, b, c) {
+              // Just to make it longer
+            }`,
+            options: [{ max: 2 }],
+            errors: [{
+                line: 1,
+                column: 1,
+                endLine: 1,
+                endColumn: 14
+            }]
         }
     ]
 });