Repository: eslint/eslint
Pull request #9195 (2017-09-01 19:20:51)

Fix: off-by-one error in eslint-disable comment checking
<!--
    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)**

[x] Bug fix

<!--
    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:
-->
**Tell us about your environment**

* **ESLint Version:** master
* **Node Version:** 8.4.0
* **npm Version:** 5.3.0

**What parser (default, Babel-ESLint, etc.) are you using?**

N/A

**Please show your full configuration:**

N/A

**What did you do? Please include the actual source code causing the issue.**

```js
const eslint = require("eslint");
const linter = new eslint.Linter();

linter.defineRule("checker", context => ({
    Program() {
        context.report({ loc: { line: 1, column: 0 }, message: "foo" });
    }
}));

linter.verify(" /* eslint-disable */", { rules: { checker: "error" } });
```

**What did you expect to happen?**

I expected an error to be reported, because the report location is before the start of the `eslint-disable` comment.

**What actually happened? Please include the actual, raw output from ESLint.**

No error was reported. An empty array was returned.

**What changes did you make? (Give an overview)**

This updates `isDisabledByReportingConfig` in `Linter` to fix an off-by-one error introduced in https://github.com/eslint/eslint/commit/d672aef42b2950c710d598cb970d630f49b2be4b. `isDisabledByReportingConfig` is now accepting locations with 1-based columns rather than 0-based columns, so it needs to offset them appropriately when comparing them against comments with 0-based columns.

**Is there anything you'd like reviewers to focus on?**

Nothing in particular


eslintbot: LGTM

Patch:
diff --git a/docs/rules/prefer-arrow-callback.md b/docs/rules/prefer-arrow-callback.md
index 4942295a5..3335e73a2 100644
--- a/docs/rules/prefer-arrow-callback.md
+++ b/docs/rules/prefer-arrow-callback.md
@@ -1,77 +1,100 @@
-# Suggest using arrow functions as callbacks. (prefer-arrow-callback)
+# Require using arrow functions for callbacks (prefer-arrow-callback)
 
-Arrow functions are suited to callbacks, because:
+Arrow functions can be an attractive alternative to function expressions for callbacks or function arguments.
 
-- `this` keywords in arrow functions bind to the upper scope's.
-- The notation of the arrow function is shorter than function expression's.
+For example, arrow functions are automatically bound to their surrounding scope/context. This provides an alternative to the pre-ES6 standard of explicitly binding function expressions to achieve similar behavior.
+
+Additionally, arrow functions are:
+
+- less verbose, and easier to reason about.
+
+- bound lexically regardless of where or when they are invoked.
 
 ## Rule Details
 
-This rule is aimed to flag usage of function expressions in an argument list.
+This rule locates function expressions used as callbacks or function arguments. An error will be produced for any that could be replaced by an arrow function without changing the result.
 
-The following patterns are considered problems:
+The following examples **will** be flagged:
 
 ```js
-/*eslint prefer-arrow-callback: "error"*/
+/* eslint prefer-arrow-callback: "error" */
 
-foo(function(a) { return a; });
-foo(function() { return this.a; }.bind(this));
+foo(function(a) { return a; }); // ERROR
+// prefer: foo(a => a)
+
+foo(function() { return this.a; }.bind(this)); // ERROR
+// prefer: foo(() => this.a)
 ```
 
-The following patterns are not considered problems:
+Instances where an arrow function would not produce identical results will be ignored.
+
+The following examples **will not** be flagged:
 
 ```js
-/*eslint prefer-arrow-callback: "error"*/
-/*eslint-env es6*/
+/* eslint prefer-arrow-callback: "error" */
+/* eslint-env es6 */
+
+// arrow function callback
+foo(a => a); // OK
 
-foo(a => a);
-foo(function*() { yield; });
+// generator as callback
+foo(function*() { yield; }); // OK
 
-// this is not a callback.
-var foo = function foo(a) { return a; };
+// function expression not used as callback or function argument
+var foo = function foo(a) { return a; }; // OK
 
-// using `this` without `.bind(this)`.
-foo(function() { return this.a; });
+// unbound function expression callback
+foo(function() { return this.a; }); // OK
 
-// recursively.
-foo(function bar(n) { return n && n + bar(n - 1); });
+// recursive named function callback
+foo(function bar(n) { return n && n + bar(n - 1); }); // OK
 ```
 
 ## Options
 
-This rule takes one optional argument, an object which is an options object.
+Access further control over this rule's behavior via an options object.
+
+Default: `{ allowNamedFunctions: false, allowUnboundThis: true }`
 
 ### allowNamedFunctions
 
-This is a `boolean` option and it is `false` by default. When set to `true`, the rule doesn't warn on named functions used as callbacks.
+By default `{ "allowNamedFunctions": false }`, this `boolean` option prohibits using named functions as callbacks or function arguments.
+
+Changing this value to `true` will reverse this option's behavior by allowing use of named functions without restriction.
 
-Examples of **correct** code for the `{ "allowNamedFunctions": true }` option:
+`{ "allowNamedFunctions": true }` **will not** flag the following example:
 
 ```js
-/*eslint prefer-arrow-callback: ["error", { "allowNamedFunctions": true }]*/
+/* eslint prefer-arrow-callback: [ "error", { "allowNamedFunctions": true } ] */
 
 foo(function bar() {});
 ```
 
 ### allowUnboundThis
 
-This is a `boolean` option and it is `true` by default. When set to `false`, this option allows the use of `this` without restriction and checks for dynamically assigned `this` values such as when using `Array.prototype.map` with a `context` argument. Normally, the rule will flag the use of `this` whenever a function does not use `bind()` to specify the value of `this` constantly.
+By default `{ "allowUnboundThis": true }`, this `boolean` option allows function expressions containing `this` to be used as callbacks, as long as the function in question has not been explicitly bound.
+
+When set to `false` this option prohibits the use of function expressions as callbacks or function arguments entirely, without exception.
 
-Examples of **incorrect** code for the `{ "allowUnboundThis": false }` option:
+`{ "allowNamedFunctions": false }` **will** flag the following examples:
 
 ```js
-/*eslint prefer-arrow-callback: ["error", { "allowUnboundThis": false }]*/
-/*eslint-env es6*/
+/* eslint prefer-arrow-callback: [ "error", { "allowUnboundThis": false } ] */
+/* eslint-env es6 */
 
 foo(function() { this.a; });
 
 foo(function() { (() => this); });
 
-someArray.map(function (itm) { return this.doSomething(itm); }, someObject);
+someArray.map(function(itm) { return this.doSomething(itm); }, someObject);
 ```
 
 ## When Not To Use It
 
-This rule should not be used in ES3/5 environments.
+- In environments that have not yet adopted ES6 language features (ES3/5).
+
+- In ES6+ environments that allow the use of function expressions when describing callbacks or function arguments.
+
+## Further Reading
 
-In ES2015 (ES6) or later, if you don't want to be notified about function expressions in an argument list, you can safely disable this rule.
+- [More on ES6 arrow functions]('https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions')
diff --git a/lib/linter.js b/lib/linter.js
index 7b3117d92..0b9c6e0d8 100755
--- a/lib/linter.js
+++ b/lib/linter.js
@@ -404,7 +404,7 @@ function modifyConfigsFromComments(filename, ast, config, linterContext) {
  * Check if message of rule with ruleId should be ignored in location
  * @param  {Object[]} reportingConfig  Collection of ignore records
  * @param  {string} ruleId   Id of rule
- * @param  {Object} location Location of message
+ * @param  {Object} location 1-indexed location of message
  * @returns {boolean}          True if message should be ignored, false otherwise
  */
 function isDisabledByReportingConfig(reportingConfig, ruleId, location) {
@@ -414,8 +414,8 @@ function isDisabledByReportingConfig(reportingConfig, ruleId, location) {
         const ignore = reportingConfig[i];
 
         if ((!ignore.rule || ignore.rule === ruleId) &&
-            (location.line > ignore.start.line || (location.line === ignore.start.line && location.column >= ignore.start.column)) &&
-            (!ignore.end || (location.line < ignore.end.line || (location.line === ignore.end.line && location.column <= ignore.end.column)))) {
+            (location.line > ignore.start.line || (location.line === ignore.start.line && location.column > ignore.start.column)) &&
+            (!ignore.end || (location.line < ignore.end.line || (location.line === ignore.end.line && location.column - 1 <= ignore.end.column)))) {
             return true;
         }
     }
diff --git a/lib/report-translator.js b/lib/report-translator.js
index fe59d9a75..9c4ab8b9a 100644
--- a/lib/report-translator.js
+++ b/lib/report-translator.js
@@ -41,7 +41,7 @@ function normalizeMultiArgReportCall() {
 
     // If there is one argument, it is considered to be a new-style call already.
     if (arguments.length === 1) {
-        return Object.assign({}, arguments[0]);
+        return arguments[0];
     }
 
     // If the second argument is a string, the arguments are interpreted as [node, message, data, fix].
@@ -67,7 +67,7 @@ function normalizeMultiArgReportCall() {
 /**
  * Asserts that either a loc or a node was provided, and the node is valid if it was provided.
  * @param {MessageDescriptor} descriptor A descriptor to validate
- * @returns {MessageDescriptor} The same descriptor
+ * @returns {void}
  * @throws AssertionError if neither a node nor a loc was provided, or if the node is not an object
  */
 function assertValidNodeInfo(descriptor) {
@@ -76,46 +76,39 @@ function assertValidNodeInfo(descriptor) {
     } else {
         assert(descriptor.loc, "Node must be provided when reporting error if location is not provided");
     }
-
-    return descriptor;
 }
 
 /**
  * Normalizes a MessageDescriptor to always have a `loc` with `start` and `end` properties
- * @param {MessageDescriptor} descriptor A descriptor for the report from a rule. This descriptor may be mutated
- * by this function.
- * @returns {MessageDescriptor} The updated MessageDescriptor that infers the `start` and `end` properties from
- * the `node` of the original descriptor, or infers the `start` from the `loc` of the original descriptor.
+ * @param {MessageDescriptor} descriptor A descriptor for the report from a rule.
+ * @returns {{start: Location, end: (Location|null)}} An updated location that infers the `start` and `end` properties
+ * from the `node` of the original descriptor, or infers the `start` from the `loc` of the original descriptor.
  */
 function normalizeReportLoc(descriptor) {
     if (descriptor.loc) {
         if (descriptor.loc.start) {
-            return descriptor;
+            return descriptor.loc;
         }
-        return Object.assign(descriptor, { loc: { start: descriptor.loc, end: null } });
+        return { start: descriptor.loc, end: null };
     }
-
-    return Object.assign(descriptor, { loc: descriptor.node.loc });
+    return descriptor.node.loc;
 }
 
 /**
  * Interpolates data placeholders in report messages
- * @param {MessageDescriptor} descriptor The report message descriptor. This descriptor may be mutated
- * by this function.
- * @returns {MessageDescriptor} An new descriptor with a message containing the interpolated data
+ * @param {MessageDescriptor} descriptor The report message descriptor.
+ * @returns {string} The interpolated message for the descriptor
  */
 function normalizeMessagePlaceholders(descriptor) {
     if (!descriptor.data) {
-        return descriptor;
+        return descriptor.message;
     }
-    return Object.assign(descriptor, {
-        message: descriptor.message.replace(/\{\{\s*([^{}]+?)\s*\}\}/g, (fullMatch, term) => {
-            if (term in descriptor.data) {
-                return descriptor.data[term];
-            }
-
-            return fullMatch;
-        })
+    return descriptor.message.replace(/\{\{\s*([^{}]+?)\s*\}\}/g, (fullMatch, term) => {
+        if (term in descriptor.data) {
+            return descriptor.data[term];
+        }
+
+        return fullMatch;
     });
 }
 
@@ -134,7 +127,7 @@ function compareFixesByRange(a, b) {
  * Merges the given fixes array into one.
  * @param {Fix[]} fixes The fixes to merge.
  * @param {SourceCode} sourceCode The source code object to get the text between fixes.
- * @returns {void}
+ * @returns {{text: string, range: [number, number]}} The merged fixes
  */
 function mergeFixes(fixes, sourceCode) {
     if (fixes.length === 0) {
@@ -169,14 +162,13 @@ function mergeFixes(fixes, sourceCode) {
 /**
  * Gets one fix object from the given descriptor.
  * If the descriptor retrieves multiple fixes, this merges those to one.
- * @param {MessageDescriptor} descriptor The report descriptor. This descriptor may be mutated
- * by this function.
+ * @param {MessageDescriptor} descriptor The report descriptor.
  * @param {SourceCode} sourceCode The source code object to get text between fixes.
- * @returns {MessageDescriptor} The updated descriptor.
+ * @returns {({text: string, range: [number, number]}|null)} The fix for the descriptor
  */
 function normalizeFixes(descriptor, sourceCode) {
     if (typeof descriptor.fix !== "function") {
-        return Object.assign(descriptor, { fix: null });
+        return null;
     }
 
     // @type {null | Fix | Fix[] | IterableIterator<Fix>}
@@ -184,16 +176,22 @@ function normalizeFixes(descriptor, sourceCode) {
 
     // Merge to one.
     if (fix && Symbol.iterator in fix) {
-        return Object.assign(descriptor, { fix: mergeFixes(Array.from(fix), sourceCode) });
+        return mergeFixes(Array.from(fix), sourceCode);
     }
-    return Object.assign(descriptor, { fix });
+    return fix;
 }
 
 /**
  * Creates information about the report from a descriptor
- * @param {MessageDescriptor} descriptor The message descriptor
- * @param {string} ruleId The rule ID of the problem
- * @param {(0|1|2)} severity The severity of the problem
+ * @param {{
+ *     ruleId: string,
+ *     severity: (0|1|2),
+ *     node: (ASTNode|null),
+ *     message: string,
+ *     loc: {start: SourceLocation, end: (SourceLocation|null)},
+ *     fix: ({text: string, range: [number, number]}|null),
+ *     sourceLines: string[]
+ * }} options Information about the problem
  * @returns {function(...args): {
  *      ruleId: string,
  *      severity: (0|1|2),
@@ -207,23 +205,24 @@ function normalizeFixes(descriptor, sourceCode) {
  *      fix: ({text: string, range: [number, number]}|null)
  * }} Information about the report
  */
-function createProblemFromDescriptor(descriptor, ruleId, severity) {
+function createProblem(options) {
     const problem = {
-        ruleId,
-        severity,
-        message: descriptor.message,
-        line: descriptor.loc.start.line,
-        column: descriptor.loc.start.column + 1,
-        nodeType: descriptor.node && descriptor.node.type || null
+        ruleId: options.ruleId,
+        severity: options.severity,
+        message: options.message,
+        line: options.loc.start.line,
+        column: options.loc.start.column + 1,
+        nodeType: options.node && options.node.type || null,
+        source: options.sourceLines[options.loc.start.line - 1] || ""
     };
 
-    if (descriptor.loc.end) {
-        problem.endLine = descriptor.loc.end.line;
-        problem.endColumn = descriptor.loc.end.column + 1;
+    if (options.loc.end) {
+        problem.endLine = options.loc.end.line;
+        problem.endColumn = options.loc.end.column + 1;
     }
 
-    if (descriptor.fix) {
-        problem.fix = descriptor.fix;
+    if (options.fix) {
+        problem.fix = options.fix;
     }
 
     return problem;
@@ -261,14 +260,15 @@ module.exports = function createReportTranslator(metadata) {
         const descriptor = normalizeMultiArgReportCall.apply(null, arguments);
 
         assertValidNodeInfo(descriptor);
-        normalizeReportLoc(descriptor);
-        normalizeMessagePlaceholders(descriptor);
-        normalizeFixes(descriptor, metadata.sourceCode);
-
-        const problem = createProblemFromDescriptor(descriptor, metadata.ruleId, metadata.severity);
-
-        problem.source = metadata.sourceCode.lines[problem.line - 1] || "";
 
-        return problem;
+        return createProblem({
+            ruleId: metadata.ruleId,
+            severity: metadata.severity,
+            node: descriptor.node,
+            message: normalizeMessagePlaceholders(descriptor),
+            loc: normalizeReportLoc(descriptor),
+            fix: normalizeFixes(descriptor, metadata.sourceCode),
+            sourceLines: metadata.sourceCode.lines
+        });
     };
 };
diff --git a/lib/rules/prefer-arrow-callback.js b/lib/rules/prefer-arrow-callback.js
index de7c33940..7565e445f 100644
--- a/lib/rules/prefer-arrow-callback.js
+++ b/lib/rules/prefer-arrow-callback.js
@@ -132,7 +132,7 @@ function hasDuplicateParams(paramsList) {
 module.exports = {
     meta: {
         docs: {
-            description: "require arrow functions as callbacks",
+            description: "require using arrow functions for callbacks",
             category: "ECMAScript 6",
             recommended: false
         },


Test patch:
diff --git a/tests/lib/linter.js b/tests/lib/linter.js
index c175d3310..ac0c45c89 100644
--- a/tests/lib/linter.js
+++ b/tests/lib/linter.js
@@ -1548,6 +1548,33 @@ describe("Linter", () => {
             assert.equal(messages.length, 0);
         });
 
+        it("should report a violation when the report is right before the comment", () => {
+            const code = " /* eslint-disable */ ";
+
+            linter.defineRule("checker", context => ({
+                Program() {
+                    context.report({ loc: { line: 1, column: 0 }, message: "foo" });
+                }
+            }));
+            const problems = linter.verify(code, { rules: { checker: "error" } });
+
+            assert.strictEqual(problems.length, 1);
+            assert.strictEqual(problems[0].message, "foo");
+        });
+
+        it("should not report a violation when the report is right at the start of the comment", () => {
+            const code = " /* eslint-disable */ ";
+
+            linter.defineRule("checker", context => ({
+                Program() {
+                    context.report({ loc: { line: 1, column: 1 }, message: "foo" });
+                }
+            }));
+            const problems = linter.verify(code, { rules: { checker: "error" } });
+
+            assert.strictEqual(problems.length, 0);
+        });
+
         it("rules should not change initial config", () => {
             const config = { rules: { "test-plugin/test-rule": 2 } };
             const codeA = "/*eslint test-plugin/test-rule: 0*/ var a = \"trigger violation\";";