Repository: eslint/eslint (js)

Issue:
no-extra-bind fix could be unsafe
<!--
    ESLint adheres to the [JS Foundation Code of Conduct](https://js.foundation/community/code-of-conduct).

    This template is for bug reports. If you are here for another reason, please see below:

    1. To propose a new rule: https://eslint.org/docs/developer-guide/contributing/new-rules
    2. To request a rule change: https://eslint.org/docs/developer-guide/contributing/rule-changes
    3. To request a change that is not a bug fix, rule change, or new rule: https://eslint.org/docs/developer-guide/contributing/changes
    4. 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**
Came across this while testing `no-extra-bind` for Coffeescript custom parser
* **ESLint Version:**
5.5.0
* **Node Version:**
8.11.4
* **npm Version:**
6.2.0

**What parser (default, Babel-ESLint, etc.) are you using?**
Coffeescript custom parser (but the issue is the same with standard parser)

**Please show your full configuration:**
The relevant configuration is `no-extra-bind: error` with fixing enabled

**What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.**

I noticed that `no-extra-bind` when fixing blindly removes the `.bind(...)` without inspecting the argument to `.bind()`. This seems like it could change meaning if the argument is not side-effect-free

For example:
<!-- Paste the source code below: -->
```
function () {}.bind(a())
```
or:
```
function () {}.bind(b++)
```

**What did you expect to happen?**
It should presumably still report the unnecessary `.bind()` but not fix if it's unsafe to remove the `.bind(...)`

**What actually happened? Please include the actual, raw output from ESLint.**
The fix removed the `.bind(...)`

I could take a crack at fixing this but it'd be helpful to know if there are already established patterns for checking if a node is "side-effect-free"?

Pull request description:
Fix: no-extra-bind No autofix if arg may have side effect (fixes #10846)
<!--
    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)**

Ensure autofix is only performed if the argument to `.bind()` is definitely side-effect free. For most practical situations, this means ensuring the argument is a Literal, Identifier, or FunctionExpression.

I intentionally kept the test cases to clear side effects, in case we want to loosen the checks later.

**Is there anything you'd like reviewers to focus on?**

Not really.


Patch:
diff --git a/docs/developer-guide/README.md b/docs/developer-guide/README.md
index 2070b2da1..ea1943099 100644
--- a/docs/developer-guide/README.md
+++ b/docs/developer-guide/README.md
@@ -34,7 +34,7 @@ You're finally ready to start working with rules. You may want to fix an existin
 
 You've developed library-specific rules for ESLint and you want to share it with the community. You can publish an ESLint plugin on npm.
 
-## Section 6: [Working with Parsers](working-with-parsers.md)
+## Section 6: [Working with Custom Parsers](working-with-custom-parsers.md)
 
 If you aren't going to use the default parser of ESLint, this section explains about using custom parsers.
 
diff --git a/docs/rules/no-unsafe-finally.md b/docs/rules/no-unsafe-finally.md
index 1783e501f..fb3bb1ea5 100644
--- a/docs/rules/no-unsafe-finally.md
+++ b/docs/rules/no-unsafe-finally.md
@@ -49,7 +49,7 @@ JavaScript suspends the control flow statements of `try` and `catch` blocks unti
 // We expect this function to return 0 from try block.
 (() => {
   label: try {
-    return 0; // 1 is returned but suspended until finally block ends
+    return 0; // 0 is returned but suspended until finally block ends
   } finally {
     break label; // It breaks out the try-finally block, before 0 is returned.
   }
diff --git a/docs/rules/space-infix-ops.md b/docs/rules/space-infix-ops.md
index f0381877d..406e2030c 100644
--- a/docs/rules/space-infix-ops.md
+++ b/docs/rules/space-infix-ops.md
@@ -73,3 +73,7 @@ var {a = 0} = bar;
 
 function foo(a = 0) { }
 ```
+
+## When Not To Use It
+
+You can turn this rule off if you are not concerned with the consistency of spacing around infix operators.
diff --git a/docs/user-guide/command-line-interface.md b/docs/user-guide/command-line-interface.md
index 7de5a50a1..62ecd3c20 100644
--- a/docs/user-guide/command-line-interface.md
+++ b/docs/user-guide/command-line-interface.md
@@ -401,7 +401,7 @@ Store the info about processed files in order to only operate on the changed one
 
 **Note:** If you run ESLint with `--cache` and then run ESLint without `--cache`, the `.eslintcache` file will be deleted. This is necessary because the results of the lint might change and make `.eslintcache` invalid. If you want to control when the cache file is deleted, then use `--cache-location` to specify an alternate location for the cache file.
 
-**Note:**: As of now, only the results for successfully linted files are stored in the cache. Files which fail linting are not stored in the cache, so they will be linted every time.
+**Note:**: Autofixed files are not placed in the cache. Subsequent linting that does not trigger an autofix will place it in the cache.
 
 #### `--cache-file`
 
diff --git a/lib/rules/no-extra-bind.js b/lib/rules/no-extra-bind.js
index b8376adfc..6d6cad13e 100644
--- a/lib/rules/no-extra-bind.js
+++ b/lib/rules/no-extra-bind.js
@@ -10,6 +10,12 @@
 
 const astUtils = require("../util/ast-utils");
 
+//------------------------------------------------------------------------------
+// Helpers
+//------------------------------------------------------------------------------
+
+const SIDE_EFFECT_FREE_NODE_TYPES = new Set(["Literal", "Identifier", "ThisExpression", "FunctionExpression"]);
+
 //------------------------------------------------------------------------------
 // Rule Definition
 //------------------------------------------------------------------------------
@@ -35,6 +41,18 @@ module.exports = {
     create(context) {
         let scopeInfo = null;
 
+        /**
+         * Checks if a node is free of side effects.
+         *
+         * This check is stricter than it needs to be, in order to keep the implementation simple.
+         *
+         * @param {ASTNode} node A node to check.
+         * @returns {boolean} True if the node is known to be side-effect free, false otherwise.
+         */
+        function isSideEffectFree(node) {
+            return SIDE_EFFECT_FREE_NODE_TYPES.has(node.type);
+        }
+
         /**
          * Reports a given function node.
          *
@@ -48,6 +66,10 @@ module.exports = {
                 messageId: "unexpected",
                 loc: node.parent.property.loc.start,
                 fix(fixer) {
+                    if (node.parent.parent.arguments.length && !isSideEffectFree(node.parent.parent.arguments[0])) {
+                        return null;
+                    }
+
                     const firstTokenToRemove = context.getSourceCode()
                         .getFirstTokenBetween(node.parent.object, node.parent.property, astUtils.isNotClosingParenToken);
 
diff --git a/lib/rules/no-unused-vars.js b/lib/rules/no-unused-vars.js
index fff3e33d9..6f36813ac 100644
--- a/lib/rules/no-unused-vars.js
+++ b/lib/rules/no-unused-vars.js
@@ -469,7 +469,7 @@ module.exports = {
             const posteriorParams = params.slice(params.indexOf(variable) + 1);
 
             // If any used parameters occur after this parameter, do not report.
-            return !posteriorParams.some(v => v.references.length > 0);
+            return !posteriorParams.some(v => v.references.length > 0 || v.eslintUsed);
         }
 
         /**
diff --git a/lib/rules/one-var.js b/lib/rules/one-var.js
index 15b664d77..3efd0f27f 100644
--- a/lib/rules/one-var.js
+++ b/lib/rules/one-var.js
@@ -314,6 +314,11 @@ module.exports = {
         function splitDeclarations(declaration) {
             return fixer => declaration.declarations.map(declarator => {
                 const tokenAfterDeclarator = sourceCode.getTokenAfter(declarator);
+
+                if (tokenAfterDeclarator === null) {
+                    return null;
+                }
+
                 const afterComma = sourceCode.getTokenAfter(tokenAfterDeclarator, { includeComments: true });
 
                 if (tokenAfterDeclarator.value !== ",") {
diff --git a/lib/rules/space-infix-ops.js b/lib/rules/space-infix-ops.js
index 49b64658d..17b49cc11 100644
--- a/lib/rules/space-infix-ops.js
+++ b/lib/rules/space-infix-ops.js
@@ -34,37 +34,25 @@ module.exports = {
 
     create(context) {
         const int32Hint = context.options[0] ? context.options[0].int32Hint === true : false;
-
-        const OPERATORS = [
-            "*", "/", "%", "+", "-", "<<", ">>", ">>>", "<", "<=", ">", ">=", "in",
-            "instanceof", "==", "!=", "===", "!==", "&", "^", "|", "&&", "||", "=",
-            "+=", "-=", "*=", "/=", "%=", "<<=", ">>=", ">>>=", "&=", "^=", "|=",
-            "?", ":", ",", "**"
-        ];
-
         const sourceCode = context.getSourceCode();
 
         /**
          * Returns the first token which violates the rule
          * @param {ASTNode} left - The left node of the main node
          * @param {ASTNode} right - The right node of the main node
+         * @param {string} op - The operator of the main node
          * @returns {Object} The violator token or null
          * @private
          */
-        function getFirstNonSpacedToken(left, right) {
-            const tokens = sourceCode.getTokensBetween(left, right, 1);
-
-            for (let i = 1, l = tokens.length - 1; i < l; ++i) {
-                const op = tokens[i];
-
-                if (
-                    (op.type === "Punctuator" || op.type === "Keyword") &&
-                    OPERATORS.indexOf(op.value) >= 0 &&
-                    (tokens[i - 1].range[1] >= op.range[0] || op.range[1] >= tokens[i + 1].range[0])
-                ) {
-                    return op;
-                }
+        function getFirstNonSpacedToken(left, right, op) {
+            const operator = sourceCode.getFirstTokenBetween(left, right, token => token.value === op);
+            const prev = sourceCode.getTokenBefore(operator);
+            const next = sourceCode.getTokenAfter(operator);
+
+            if (!sourceCode.isSpaceBetweenTokens(prev, operator) || !sourceCode.isSpaceBetweenTokens(operator, next)) {
+                return operator;
             }
+
             return null;
         }
 
@@ -110,7 +98,10 @@ module.exports = {
             const leftNode = (node.left.typeAnnotation) ? node.left.typeAnnotation : node.left;
             const rightNode = node.right;
 
-            const nonSpacedNode = getFirstNonSpacedToken(leftNode, rightNode);
+            // search for = in AssignmentPattern nodes
+            const operator = node.operator || "=";
+
+            const nonSpacedNode = getFirstNonSpacedToken(leftNode, rightNode, operator);
 
             if (nonSpacedNode) {
                 if (!(int32Hint && sourceCode.getText(node).endsWith("|0"))) {
@@ -126,8 +117,8 @@ module.exports = {
          * @private
          */
         function checkConditional(node) {
-            const nonSpacedConsequesntNode = getFirstNonSpacedToken(node.test, node.consequent);
-            const nonSpacedAlternateNode = getFirstNonSpacedToken(node.consequent, node.alternate);
+            const nonSpacedConsequesntNode = getFirstNonSpacedToken(node.test, node.consequent, "?");
+            const nonSpacedAlternateNode = getFirstNonSpacedToken(node.consequent, node.alternate, ":");
 
             if (nonSpacedConsequesntNode) {
                 report(node, nonSpacedConsequesntNode);
@@ -147,7 +138,7 @@ module.exports = {
             const rightNode = node.init;
 
             if (rightNode) {
-                const nonSpacedNode = getFirstNonSpacedToken(leftNode, rightNode);
+                const nonSpacedNode = getFirstNonSpacedToken(leftNode, rightNode, "=");
 
                 if (nonSpacedNode) {
                     report(node, nonSpacedNode);
diff --git a/package.json b/package.json
index e970cf87c..cfc6b1b01 100644
--- a/package.json
+++ b/package.json
@@ -66,7 +66,7 @@
     "path-is-inside": "^1.0.2",
     "pluralize": "^7.0.0",
     "progress": "^2.0.0",
-    "regexpp": "^2.0.0",
+    "regexpp": "^2.0.1",
     "require-uncached": "^1.0.3",
     "semver": "^5.5.1",
     "strip-ansi": "^4.0.0",


Test patch:
diff --git a/tests/lib/rules/no-extra-bind.js b/tests/lib/rules/no-extra-bind.js
index 4dfec87ac..4e5a08f36 100644
--- a/tests/lib/rules/no-extra-bind.js
+++ b/tests/lib/rules/no-extra-bind.js
@@ -70,10 +70,32 @@ ruleTester.run("no-extra-bind", rule, {
             output: "var a = function() { function c(){ this.d } }",
             errors
         },
+        {
+            code: "var a = function() { return 1; }.bind(this)",
+            output: "var a = function() { return 1; }",
+            errors
+        },
         {
             code: "var a = function() { (function(){ (function(){ this.d }.bind(c)) }) }.bind(b)",
             output: "var a = function() { (function(){ (function(){ this.d }.bind(c)) }) }",
             errors: [{ messageId: "unexpected", type: "CallExpression", column: 71 }]
+        },
+
+        // Should not autofix if bind expression args have side effects
+        {
+            code: "var a = function() {}.bind(b++)",
+            output: null,
+            errors
+        },
+        {
+            code: "var a = function() {}.bind(b())",
+            output: null,
+            errors
+        },
+        {
+            code: "var a = function() {}.bind(b.c)",
+            output: null,
+            errors
         }
     ]
 });
diff --git a/tests/lib/rules/no-invalid-regexp.js b/tests/lib/rules/no-invalid-regexp.js
index 305f4e0d3..0062a5bbc 100644
--- a/tests/lib/rules/no-invalid-regexp.js
+++ b/tests/lib/rules/no-invalid-regexp.js
@@ -46,6 +46,12 @@ ruleTester.run("no-invalid-regexp", rule, {
     invalid: [
         { code: "RegExp('[');", errors: [{ message: "Invalid regular expression: /[/: Unterminated character class.", type: "CallExpression" }] },
         { code: "RegExp('.', 'z');", errors: [{ message: "Invalid flags supplied to RegExp constructor 'z'.", type: "CallExpression" }] },
-        { code: "new RegExp(')');", errors: [{ message: "Invalid regular expression: /)/: Unmatched ')'.", type: "NewExpression" }] }
+        { code: "new RegExp(')');", errors: [{ message: "Invalid regular expression: /)/: Unmatched ')'.", type: "NewExpression" }] },
+
+        // https://github.com/eslint/eslint/issues/10861
+        {
+            code: String.raw`new RegExp('\\');`,
+            errors: [{ message: "Invalid regular expression: /\\/: \\ at end of pattern.", type: "NewExpression" }]
+        }
     ]
 });
diff --git a/tests/lib/rules/no-unused-vars.js b/tests/lib/rules/no-unused-vars.js
index f188f8ce3..316df18e0 100644
--- a/tests/lib/rules/no-unused-vars.js
+++ b/tests/lib/rules/no-unused-vars.js
@@ -271,7 +271,11 @@ ruleTester.run("no-unused-vars", rule, {
             code: "(({a, ...rest}) => rest)",
             options: [{ args: "all", ignoreRestSiblings: true }],
             parserOptions: { ecmaVersion: 2018 }
-        }
+        },
+
+        // https://github.com/eslint/eslint/issues/10952
+        "/*eslint use-every-a:1*/ !function(b, a) { return 1 }"
+
     ],
     invalid: [
         { code: "function foox() { return foox(); }", errors: [definedError("foox")] },
diff --git a/tests/lib/rules/one-var.js b/tests/lib/rules/one-var.js
index b628f512a..e5470d2e9 100644
--- a/tests/lib/rules/one-var.js
+++ b/tests/lib/rules/one-var.js
@@ -949,6 +949,17 @@ ruleTester.run("one-var", rule, {
                 column: 1
             }]
         },
+        {
+            code: "var a = 1, b = 2",
+            output: "var a = 1; var b = 2",
+            options: ["never"],
+            errors: [{
+                message: "Split 'var' declarations into multiple statements.",
+                type: "VariableDeclaration",
+                line: 1,
+                column: 1
+            }]
+        },
         {
             code: "var foo = require('foo'), bar;",
             output: null,
diff --git a/tests/lib/rules/space-infix-ops.js b/tests/lib/rules/space-infix-ops.js
index 3f98c88d4..57e28addc 100644
--- a/tests/lib/rules/space-infix-ops.js
+++ b/tests/lib/rules/space-infix-ops.js
@@ -18,8 +18,14 @@ const ruleTester = new RuleTester();
 ruleTester.run("space-infix-ops", rule, {
     valid: [
         "a + b",
+        "a + ++b",
+        "a++ + b",
+        "a++ + ++b",
         "a     + b",
         "(a) + (b)",
+        "((a)) + ((b))",
+        "(((a))) + (((b)))",
+        "a + +b",
         "a + (b)",
         "a + +(b)",
         "a + (+(b))",