Update: remove suggestion if it didn't provide a fix (fixes #13723) (#…

…13772)
eslint · Oct 23, 2020 · bfddced · bfddced
1 parent 5183b14
commit bfddced
Show file tree

Hide file tree

Showing 3 changed files with 233 additions and 9 deletions.
diff --git a/docs/developer-guide/working-with-rules.md b/docs/developer-guide/working-with-rules.md
@@ -382,6 +382,8 @@ Best practices for suggestions:
 1. Don't try to do too much and suggest large refactors that could introduce a lot of breaking changes.
 1. As noted above, don't try to conform to user-defined styles.
 
+Suggestions are intended to provide fixes. ESLint will automatically remove the whole suggestion from the linting output if the suggestion's `fix` function returned `null` or an empty array/sequence.
+
 #### Suggestion `messageId`s
 
 Instead of using a `desc` key for suggestions a `messageId` can be used instead. This works the same way as `messageId`s for the overall error (see [messageIds](#messageIds)). Here is an example of how to use it in a rule:

diff --git a/lib/linter/report-translator.js b/lib/linter/report-translator.js
@@ -196,15 +196,19 @@ function mapSuggestions(descriptor, sourceCode, messages) {
         return [];
     }
 
-    return descriptor.suggest.map(suggestInfo => {
-        const computedDesc = suggestInfo.desc || messages[suggestInfo.messageId];
-
-        return {
-            ...suggestInfo,
-            desc: interpolate(computedDesc, suggestInfo.data),
-            fix: normalizeFixes(suggestInfo, sourceCode)
-        };
-    });
+    return descriptor.suggest
+        .map(suggestInfo => {
+            const computedDesc = suggestInfo.desc || messages[suggestInfo.messageId];
+
+            return {
+                ...suggestInfo,
+                desc: interpolate(computedDesc, suggestInfo.data),
+                fix: normalizeFixes(suggestInfo, sourceCode)
+            };
+        })
+
+        // Remove suggestions that didn't provide a fix
+        .filter(({ fix }) => fix);
 }
 
 /**

diff --git a/tests/lib/linter/report-translator.js b/tests/lib/linter/report-translator.js
@@ -553,6 +553,224 @@ describe("createReportTranslator", () => {
                 }
             );
         });
+
+        it("should remove the whole suggestion if 'fix' function returned `null`.", () => {
+            const reportDescriptor = {
+                node,
+                loc: location,
+                message,
+                suggest: [{
+                    desc: "A suggestion for the issue",
+                    fix: () => null
+                }]
+            };
+
+            assert.deepStrictEqual(
+                translateReport(reportDescriptor),
+                {
+                    ruleId: "foo-rule",
+                    severity: 2,
+                    message: "foo",
+                    line: 2,
+                    column: 1,
+                    nodeType: "ExpressionStatement"
+                }
+            );
+        });
+
+        it("should remove the whole suggestion if 'fix' function returned an empty array.", () => {
+            const reportDescriptor = {
+                node,
+                loc: location,
+                message,
+                suggest: [{
+                    desc: "A suggestion for the issue",
+                    fix: () => []
+                }]
+            };
+
+            assert.deepStrictEqual(
+                translateReport(reportDescriptor),
+                {
+                    ruleId: "foo-rule",
+                    severity: 2,
+                    message: "foo",
+                    line: 2,
+                    column: 1,
+                    nodeType: "ExpressionStatement"
+                }
+            );
+        });
+
+        it("should remove the whole suggestion if 'fix' function returned an empty sequence.", () => {
+            const reportDescriptor = {
+                node,
+                loc: location,
+                message,
+                suggest: [{
+                    desc: "A suggestion for the issue",
+                    *fix() {}
+                }]
+            };
+
+            assert.deepStrictEqual(
+                translateReport(reportDescriptor),
+                {
+                    ruleId: "foo-rule",
+                    severity: 2,
+                    message: "foo",
+                    line: 2,
+                    column: 1,
+                    nodeType: "ExpressionStatement"
+                }
+            );
+        });
+
+        // This isn't offically supported, but autofix works the same way
+        it("should remove the whole suggestion if 'fix' function didn't return anything.", () => {
+            const reportDescriptor = {
+                node,
+                loc: location,
+                message,
+                suggest: [{
+                    desc: "A suggestion for the issue",
+                    fix() {}
+                }]
+            };
+
+            assert.deepStrictEqual(
+                translateReport(reportDescriptor),
+                {
+                    ruleId: "foo-rule",
+                    severity: 2,
+                    message: "foo",
+                    line: 2,
+                    column: 1,
+                    nodeType: "ExpressionStatement"
+                }
+            );
+        });
+
+        it("should keep suggestion before a removed suggestion.", () => {
+            const reportDescriptor = {
+                node,
+                loc: location,
+                message,
+                suggest: [{
+                    desc: "Suggestion with a fix",
+                    fix: () => ({ range: [1, 2], text: "foo" })
+                }, {
+                    desc: "Suggestion without a fix",
+                    fix: () => null
+                }]
+            };
+
+            assert.deepStrictEqual(
+                translateReport(reportDescriptor),
+                {
+                    ruleId: "foo-rule",
+                    severity: 2,
+                    message: "foo",
+                    line: 2,
+                    column: 1,
+                    nodeType: "ExpressionStatement",
+                    suggestions: [{
+                        desc: "Suggestion with a fix",
+                        fix: { range: [1, 2], text: "foo" }
+                    }]
+                }
+            );
+        });
+
+        it("should keep suggestion after a removed suggestion.", () => {
+            const reportDescriptor = {
+                node,
+                loc: location,
+                message,
+                suggest: [{
+                    desc: "Suggestion without a fix",
+                    fix: () => null
+                }, {
+                    desc: "Suggestion with a fix",
+                    fix: () => ({ range: [1, 2], text: "foo" })
+                }]
+            };
+
+            assert.deepStrictEqual(
+                translateReport(reportDescriptor),
+                {
+                    ruleId: "foo-rule",
+                    severity: 2,
+                    message: "foo",
+                    line: 2,
+                    column: 1,
+                    nodeType: "ExpressionStatement",
+                    suggestions: [{
+                        desc: "Suggestion with a fix",
+                        fix: { range: [1, 2], text: "foo" }
+                    }]
+                }
+            );
+        });
+
+        it("should remove multiple suggestions that didn't provide a fix and keep those that did.", () => {
+            const reportDescriptor = {
+                node,
+                loc: location,
+                message,
+                suggest: [{
+                    desc: "Keep #1",
+                    fix: () => ({ range: [1, 2], text: "foo" })
+                }, {
+                    desc: "Remove #1",
+                    fix() {
+                        return null;
+                    }
+                }, {
+                    desc: "Keep #2",
+                    fix: () => ({ range: [1, 2], text: "bar" })
+                }, {
+                    desc: "Remove #2",
+                    fix() {
+                        return [];
+                    }
+                }, {
+                    desc: "Keep #3",
+                    fix: () => ({ range: [1, 2], text: "baz" })
+                }, {
+                    desc: "Remove #3",
+                    *fix() {}
+                }, {
+                    desc: "Keep #4",
+                    fix: () => ({ range: [1, 2], text: "quux" })
+                }]
+            };
+
+            assert.deepStrictEqual(
+                translateReport(reportDescriptor),
+                {
+                    ruleId: "foo-rule",
+                    severity: 2,
+                    message: "foo",
+                    line: 2,
+                    column: 1,
+                    nodeType: "ExpressionStatement",
+                    suggestions: [{
+                        desc: "Keep #1",
+                        fix: { range: [1, 2], text: "foo" }
+                    }, {
+                        desc: "Keep #2",
+                        fix: { range: [1, 2], text: "bar" }
+                    }, {
+                        desc: "Keep #3",
+                        fix: { range: [1, 2], text: "baz" }
+                    }, {
+                        desc: "Keep #4",
+                        fix: { range: [1, 2], text: "quux" }
+                    }]
+                }
+            );
+        });
     });
 
     describe("message interpolation", () => {