Repository: eslint/eslint (js)

Issue:
`no-self-compare` does not handle complex expressions correctly
<!--
This template is for bug reports. If you are reporting a bug, please continue on. If you are here for another reason, please see below:

1. To propose a new rule: http://eslint.org/docs/developer-guide/contributing/new-rules
2. To request a change: http://eslint.org/docs/developer-guide/contributing/changes
3. If you have any questions, please stop by our chatroom: https://gitter.im/eslint/eslint

Note that leaving sections blank will make it difficult for us to troubleshoot and we may have to close the issue.
-->

**Tell us about your environment**

* **ESLint Version:** master
* **Node Version:** 7.2.0
* **npm Version:** 3.10.9

**What parser (default, Babel-ESLint, etc.) are you using?**

**Please show your full configuration:**

(none)

**What did you do? Please include the actual source code causing the issue.**

<!-- Paste the source code below: -->
```js
/* eslint no-self-compare: error */

foo.bar === foo.bar;
```

**What did you expect to happen?**

I expected ESLint to report an error for the self-comparison.

**What actually happened? Please include the actual, raw output from ESLint.**

No errors were reported.

I'm marking this as "evaluating" since I'm not sure whether it's intended behavior.

Pull request description:
Update: fix no-self-compare false negative on non-literals (fixes #7677)
**What is the purpose of this pull request? (put an "X" next to item)**

[x] Bug fix (https://github.com/eslint/eslint/issues/7677)

<!--
    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:
-->

**What changes did you make? (Give an overview)**

Previously, the `no-self-compare` rule would only check comparisons between identifiers and did not check comparisons between more complex expressions. (I assume this is because the APIs that would allow comparing complex expressions didn't exist yet at the time the rule was implemented.) The rule is still applicable to non-literals, so this commit updates the rule to check comparisons that aren't between literals.

**Is there anything you'd like reviewers to focus on?**

Nothing in particular

Patch:
diff --git a/docs/rules/no-undefined.md b/docs/rules/no-undefined.md
index 19458c6ee..ff8bea720 100644
--- a/docs/rules/no-undefined.md
+++ b/docs/rules/no-undefined.md
@@ -1,6 +1,6 @@
 # Disallow Use of `undefined` Variable (no-undefined)
 
-The `undefined` variable is unique in JavaScript because it is actually a property of the global object. As such, in ECMAScript 3 it was possible to overwrite the value of `undefined`. While ECMAScript 5 disallows overwriting `undefined`, it's still possible to shadow `undefined`, such as:
+The `undefined` variable in JavaScript is actually a property of the global object. As such, in ECMAScript 3 it was possible to overwrite the value of `undefined`. While ECMAScript 5 disallows overwriting `undefined`, it's still possible to shadow `undefined`, such as:
 
 ```js
 function doSomething(data) {
@@ -14,24 +14,15 @@ function doSomething(data) {
 }
 ```
 
-This represents a problem for `undefined` that doesn't exist for `null`, which is a keyword and primitive value that can neither be overwritten nor shadowed.
+Because `undefined` can be overwritten or shadowed, reading `undefined` can give an unexpected value. (This is not the case for `null`, which is a keyword that always produces the same value.) To guard against this, you can avoid all uses of `undefined`, which is what some style guides recommend and what this rule enforces. Those style guides then also recommend:
 
-All uninitialized variables automatically get the value of `undefined`:
-
-```js
-var foo;
-
-console.log(foo === undefined);     // true (assuming no shadowing)
-```
-
-For this reason, it's not necessary to explicitly initialize a variable to `undefined`.
-
-Taking all of this into account, some style guides forbid the use of `undefined`, recommending instead:
-
-* Variables that should be `undefined` are simply left uninitialized.
+* Variables that should be `undefined` are simply left uninitialized. (All uninitialized variables automatically get the value of `undefined` in JavaScript.)
 * Checking if a value is `undefined` should be done with `typeof`.
 * Using the `void` operator to generate the value of `undefined` if necessary.
 
+As an alternative, you can use the [no-global-assign](no-global-assign.md) and [no-shadow-restricted-names](no-shadow-restricted-names.md) rules to prevent `undefined` from being shadowed or assigned a different value. This ensures that `undefined` will always hold its original, expected value.
+
+
 ## Rule Details
 
 This rule aims to eliminate the use of `undefined`, and as such, generates a warning whenever it is used.
@@ -84,3 +75,5 @@ If you want to allow the use of `undefined` in your code, then you can safely tu
 
 * [no-undef-init](no-undef-init.md)
 * [no-void](no-void.md)
+* [no-shadow-restricted-names](no-shadow-restricted-names.md)
+* [no-global-assign](no-global-assign.md)
diff --git a/lib/rules/brace-style.js b/lib/rules/brace-style.js
index bb4433cc4..44bd6d2c8 100644
--- a/lib/rules/brace-style.js
+++ b/lib/rules/brace-style.js
@@ -62,10 +62,12 @@ module.exports = {
         function removeNewlineBetween(firstToken, secondToken) {
             const textRange = [firstToken.range[1], secondToken.range[0]];
             const textBetween = sourceCode.text.slice(textRange[0], textRange[1]);
-            const NEWLINE_REGEX = astUtils.createGlobalLinebreakMatcher();
 
             // Don't do a fix if there is a comment between the tokens
-            return fixer => fixer.replaceTextRange(textRange, textBetween.trim() ? null : textBetween.replace(NEWLINE_REGEX, ""));
+            if (textBetween.trim()) {
+                return null;
+            }
+            return fixer => fixer.replaceTextRange(textRange, " ");
         }
 
         /**
diff --git a/lib/rules/no-self-compare.js b/lib/rules/no-self-compare.js
index 54f907f59..5beaa181b 100644
--- a/lib/rules/no-self-compare.js
+++ b/lib/rules/no-self-compare.js
@@ -22,15 +22,28 @@ module.exports = {
     },
 
     create(context) {
+        const sourceCode = context.getSourceCode();
+
+        /**
+         * Determines whether two nodes are composed of the same tokens.
+         * @param {ASTNode} nodeA The first node
+         * @param {ASTNode} nodeB The second node
+         * @returns {boolean} true if the nodes have identical token representations
+         */
+        function hasSameTokens(nodeA, nodeB) {
+            const tokensA = sourceCode.getTokens(nodeA);
+            const tokensB = sourceCode.getTokens(nodeB);
+
+            return tokensA.length === tokensB.length &&
+                tokensA.every((token, index) => token.type === tokensB[index].type && token.value === tokensB[index].value);
+        }
 
         return {
 
             BinaryExpression(node) {
-                const operators = ["===", "==", "!==", "!=", ">", "<", ">=", "<="];
+                const operators = new Set(["===", "==", "!==", "!=", ">", "<", ">=", "<="]);
 
-                if (operators.indexOf(node.operator) > -1 &&
-                    (node.left.type === "Identifier" && node.right.type === "Identifier" && node.left.name === node.right.name ||
-                    node.left.type === "Literal" && node.right.type === "Literal" && node.left.value === node.right.value)) {
+                if (operators.has(node.operator) && hasSameTokens(node.left, node.right)) {
                     context.report({ node, message: "Comparing to itself is potentially pointless." });
                 }
             }


Test patch:
diff --git a/tests/lib/rules/brace-style.js b/tests/lib/rules/brace-style.js
index 75c86571e..1dc11ac38 100644
--- a/tests/lib/rules/brace-style.js
+++ b/tests/lib/rules/brace-style.js
@@ -200,7 +200,7 @@ ruleTester.run("brace-style", rule, {
     invalid: [
         {
             code: "if (f) {\nbar;\n}\nelse\nbaz;",
-            output: "if (f) {\nbar;\n}else\nbaz;",
+            output: "if (f) {\nbar;\n} else\nbaz;",
             errors: [{ message: CLOSE_MESSAGE, type: "Punctuator" }]
         },
         {
@@ -216,83 +216,83 @@ ruleTester.run("brace-style", rule, {
         },
         {
             code: "function foo() \n { \n return; }",
-            output: "function foo()  { \n return; \n}",
+            output: "function foo() { \n return; \n}",
             errors: [{ message: OPEN_MESSAGE, type: "Punctuator" }, { message: CLOSE_MESSAGE_SINGLE, type: "Punctuator" }]
         },
         {
             code: "!function foo() \n { \n return; }",
-            output: "!function foo()  { \n return; \n}",
+            output: "!function foo() { \n return; \n}",
             errors: [{ message: OPEN_MESSAGE, type: "Punctuator" }, { message: CLOSE_MESSAGE_SINGLE, type: "Punctuator" }]
         },
         {
             code: "if (foo) \n { \n bar(); }",
-            output: "if (foo)  { \n bar(); \n}",
+            output: "if (foo) { \n bar(); \n}",
             errors: [{ message: OPEN_MESSAGE, type: "Punctuator" }, { message: CLOSE_MESSAGE_SINGLE, type: "Punctuator" }]
         },
         {
             code: "if (foo) \n { \n bar(); }",
-            output: "if (foo)  { \n bar(); \n}",
+            output: "if (foo) { \n bar(); \n}",
             errors: [{ message: OPEN_MESSAGE, type: "Punctuator" }, { message: CLOSE_MESSAGE_SINGLE, type: "Punctuator" }]
         },
         {
             code: "if (a) { \nb();\n } else \n { c(); }",
-            output: "if (a) { \nb();\n } else  {\n c(); \n}",
+            output: "if (a) { \nb();\n } else {\n c(); \n}",
             errors: [{ message: OPEN_MESSAGE, type: "Punctuator" }, { message: BODY_MESSAGE, type: "Punctuator" }, { message: CLOSE_MESSAGE_SINGLE, type: "Punctuator" }]
         },
         {
             code: "while (foo) \n { \n bar(); }",
-            output: "while (foo)  { \n bar(); \n}",
+            output: "while (foo) { \n bar(); \n}",
             errors: [{ message: OPEN_MESSAGE, type: "Punctuator" }, { message: CLOSE_MESSAGE_SINGLE, type: "Punctuator" }]
         },
         {
             code: "for (;;) \n { \n bar(); }",
-            output: "for (;;)  { \n bar(); \n}",
+            output: "for (;;) { \n bar(); \n}",
             errors: [{ message: OPEN_MESSAGE, type: "Punctuator" }, { message: CLOSE_MESSAGE_SINGLE, type: "Punctuator" }]
         },
         {
             code: "with (foo) \n { \n bar(); }",
-            output: "with (foo)  { \n bar(); \n}",
+            output: "with (foo) { \n bar(); \n}",
             errors: [{ message: OPEN_MESSAGE, type: "Punctuator" }, { message: CLOSE_MESSAGE_SINGLE, type: "Punctuator" }]
         },
         {
             code: "switch (foo) \n { \n case \"bar\": break; }",
-            output: "switch (foo)  { \n case \"bar\": break; \n}",
+            output: "switch (foo) { \n case \"bar\": break; \n}",
             errors: [{ message: OPEN_MESSAGE, type: "Punctuator" }, { message: CLOSE_MESSAGE_SINGLE, type: "Punctuator" }]
         },
         {
             code: "switch (foo) \n { }",
-            output: "switch (foo)  { }",
+            output: "switch (foo) { }",
             errors: [{ message: OPEN_MESSAGE, type: "Punctuator" }]
         },
         {
             code: "try \n { \n bar(); \n } catch (e) {}",
-            output: "try  { \n bar(); \n } catch (e) {}",
+            output: "try { \n bar(); \n } catch (e) {}",
             errors: [{ message: OPEN_MESSAGE, type: "Punctuator" }]
         },
         {
             code: "try { \n bar(); \n } catch (e) \n {}",
-            output: "try { \n bar(); \n } catch (e)  {}",
+            output: "try { \n bar(); \n } catch (e) {}",
             errors: [{ message: OPEN_MESSAGE, type: "Punctuator" }]
         },
         {
             code: "do \n { \n bar(); \n} while (true)",
-            output: "do  { \n bar(); \n} while (true)",
+            output: "do { \n bar(); \n} while (true)",
             errors: [{ message: OPEN_MESSAGE, type: "Punctuator" }]
         },
         {
             code: "for (foo in bar) \n { \n baz(); \n }",
-            output: "for (foo in bar)  { \n baz(); \n }",
+            output: "for (foo in bar) { \n baz(); \n }",
             errors: [{ message: OPEN_MESSAGE, type: "Punctuator" }]
         },
         {
             code: "for (foo of bar) \n { \n baz(); \n }",
-            output: "for (foo of bar)  { \n baz(); \n }",
+            output: "for (foo of bar) { \n baz(); \n }",
             parserOptions: { ecmaVersion: 6 },
             errors: [{ message: OPEN_MESSAGE, type: "Punctuator" }]
         },
         {
             code: "try { \n bar(); \n }\ncatch (e) {\n}",
-            output: "try { \n bar(); \n }catch (e) {\n}",
+            output: "try { \n bar(); \n } catch (e) {\n}",
             errors: [{ message: CLOSE_MESSAGE, type: "Punctuator" }]
         },
         {
@@ -302,7 +302,7 @@ ruleTester.run("brace-style", rule, {
         },
         {
             code: "if (a) { \nb();\n } \n else { \nc();\n }",
-            output: "if (a) { \nb();\n }  else { \nc();\n }",
+            output: "if (a) { \nb();\n } else { \nc();\n }",
             errors: [{ message: CLOSE_MESSAGE, type: "Punctuator" }]
         },
         {
@@ -443,7 +443,7 @@ ruleTester.run("brace-style", rule, {
         },
         {
             code: "if (a) { b(); }\nelse { c(); }",
-            output: "if (a) { b(); }else { c(); }",
+            output: "if (a) { b(); } else { c(); }",
             options: ["1tbs", { allowSingleLine: true }],
             errors: [{ message: CLOSE_MESSAGE, type: "Punctuator" }]
         },
@@ -467,49 +467,49 @@ ruleTester.run("brace-style", rule, {
         },
         {
             code: "switch (foo) \n { \n case \"bar\": break; }",
-            output: "switch (foo)  { \n case \"bar\": break; \n}",
+            output: "switch (foo) { \n case \"bar\": break; \n}",
             options: ["1tbs", { allowSingleLine: true }],
             errors: [{ message: OPEN_MESSAGE, type: "Punctuator" }, { message: CLOSE_MESSAGE_SINGLE, type: "Punctuator" }]
         },
         {
             code: "switch (foo) \n { }",
-            output: "switch (foo)  { }",
+            output: "switch (foo) { }",
             options: ["1tbs", { allowSingleLine: true }],
             errors: [{ message: OPEN_MESSAGE, type: "Punctuator" }]
         },
         {
             code: "try {  bar(); }\ncatch (e) { baz();  }",
-            output: "try {  bar(); }catch (e) { baz();  }",
+            output: "try {  bar(); } catch (e) { baz();  }",
             options: ["1tbs", { allowSingleLine: true }],
             errors: [{ message: CLOSE_MESSAGE, type: "Punctuator" }]
         },
         {
             code: "try \n { \n bar(); \n } catch (e) {}",
-            output: "try  { \n bar(); \n } catch (e) {}",
+            output: "try { \n bar(); \n } catch (e) {}",
             options: ["1tbs", { allowSingleLine: true }],
             errors: [{ message: OPEN_MESSAGE, type: "Punctuator" }]
         },
         {
             code: "try { \n bar(); \n } catch (e) \n {}",
-            output: "try { \n bar(); \n } catch (e)  {}",
+            output: "try { \n bar(); \n } catch (e) {}",
             options: ["1tbs", { allowSingleLine: true }],
             errors: [{ message: OPEN_MESSAGE, type: "Punctuator" }]
         },
         {
             code: "do \n { \n bar(); \n} while (true)",
-            output: "do  { \n bar(); \n} while (true)",
+            output: "do { \n bar(); \n} while (true)",
             options: ["1tbs", { allowSingleLine: true }],
             errors: [{ message: OPEN_MESSAGE, type: "Punctuator" }]
         },
         {
             code: "for (foo in bar) \n { \n baz(); \n }",
-            output: "for (foo in bar)  { \n baz(); \n }",
+            output: "for (foo in bar) { \n baz(); \n }",
             options: ["1tbs", { allowSingleLine: true }],
             errors: [{ message: OPEN_MESSAGE, type: "Punctuator" }]
         },
         {
             code: "try { \n bar(); \n }\ncatch (e) {\n}",
-            output: "try { \n bar(); \n }catch (e) {\n}",
+            output: "try { \n bar(); \n } catch (e) {\n}",
             options: ["1tbs", { allowSingleLine: true }],
             errors: [{ message: CLOSE_MESSAGE, type: "Punctuator" }]
         },
@@ -521,7 +521,7 @@ ruleTester.run("brace-style", rule, {
         },
         {
             code: "if (a) { \nb();\n } \n else { \nc();\n }",
-            output: "if (a) { \nb();\n }  else { \nc();\n }",
+            output: "if (a) { \nb();\n } else { \nc();\n }",
             options: ["1tbs", { allowSingleLine: true }],
             errors: [{ message: CLOSE_MESSAGE, type: "Punctuator" }]
         },
@@ -599,12 +599,12 @@ ruleTester.run("brace-style", rule, {
         },
         {
             code: "class Foo\n{\n}",
-            output: "class Foo{\n}",
+            output: "class Foo {\n}",
             errors: [{ message: OPEN_MESSAGE, type: "Punctuator" }]
         },
         {
             code: "(class\n{\n})",
-            output: "(class{\n})",
+            output: "(class {\n})",
             errors: [{ message: OPEN_MESSAGE, type: "Punctuator" }]
         },
         {
@@ -634,6 +634,30 @@ ruleTester.run("brace-style", rule, {
             output: "class\nFoo\n{}",
             options: ["allman"],
             errors: [{ message: OPEN_MESSAGE_ALLMAN, type: "Punctuator" }]
+        },
+
+        // https://github.com/eslint/eslint/issues/7621
+        {
+            code: `
+                if (foo)
+                {
+                    bar
+                }
+                else {
+                    baz
+                }
+            `,
+            output: `
+                if (foo) {
+                    bar
+                } else {
+                    baz
+                }
+            `,
+            errors: [
+                { message: OPEN_MESSAGE, type: "Punctuator" },
+                { message: CLOSE_MESSAGE, type: "Punctuator" }
+            ]
         }
     ]
 });
diff --git a/tests/lib/rules/no-self-compare.js b/tests/lib/rules/no-self-compare.js
index 6b19bdbfb..b937a9214 100644
--- a/tests/lib/rules/no-self-compare.js
+++ b/tests/lib/rules/no-self-compare.js
@@ -21,10 +21,9 @@ const ruleTester = new RuleTester();
 ruleTester.run("no-self-compare", rule, {
     valid: [
         "if (x === y) { }",
-        "if (f() === f()) { }",
-        "if (a[1] === a[1]) { }",
         "if (1 === 2) { }",
-        "y=x*x"
+        "y=x*x",
+        "foo.bar.baz === foo.bar.qux"
     ],
     invalid: [
         { code: "if (x === x) { }", errors: [{ message: "Comparing to itself is potentially pointless.", type: "BinaryExpression" }] },
@@ -39,6 +38,7 @@ ruleTester.run("no-self-compare", rule, {
         { code: "x > x", errors: [{ message: "Comparing to itself is potentially pointless.", type: "BinaryExpression" }] },
         { code: "x < x", errors: [{ message: "Comparing to itself is potentially pointless.", type: "BinaryExpression" }] },
         { code: "x >= x", errors: [{ message: "Comparing to itself is potentially pointless.", type: "BinaryExpression" }] },
-        { code: "x <= x", errors: [{ message: "Comparing to itself is potentially pointless.", type: "BinaryExpression" }] }
+        { code: "x <= x", errors: [{ message: "Comparing to itself is potentially pointless.", type: "BinaryExpression" }] },
+        { code: "foo.bar().baz.qux >= foo.bar ().baz .qux", errors: [{ message: "Comparing to itself is potentially pointless.", type: "BinaryExpression" }] }
     ]
 });