From d6a761f9b6582e9f71705161be827ca303ef183f Mon Sep 17 00:00:00 2001 From: Bryan Mishkin <698306+bmish@users.noreply.github.com> Date: Thu, 5 Aug 2021 12:59:59 -0700 Subject: [PATCH] Breaking: Require `meta.hasSuggestions` for rules with suggestions (#14573) * Breaking: Require `meta.hasSuggestions` for rules with suggestions * Docs: add another note about hasSuggestions * Update docs/developer-guide/working-with-rules.md Co-authored-by: Milos Djermanovic * Update docs/developer-guide/working-with-rules.md Co-authored-by: Milos Djermanovic * enable eslint-plugin/require-meta-has-suggestions rule internally * update property in Makefile Co-authored-by: Milos Djermanovic --- .eslintrc.js | 1 + Makefile.js | 4 +- docs/developer-guide/working-with-rules.md | 13 +++- lib/linter/linter.js | 8 ++ lib/rules/no-nonoctal-decimal-escape.js | 5 +- lib/rules/no-unsafe-negation.js | 5 +- lib/rules/no-useless-escape.js | 5 +- lib/rules/radix.js | 5 +- .../testers/rule-tester/suggestions.js | 18 ++++- tests/lib/linter/linter.js | 74 +++++++++++++++---- tests/lib/rule-tester/rule-tester.js | 11 +++ 11 files changed, 118 insertions(+), 31 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index b405b82fccd..9ebb5f6b3cb 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -73,6 +73,7 @@ module.exports = { "eslint-plugin/prefer-replace-text": "error", "eslint-plugin/report-message-format": ["error", "[^a-z].*\\.$"], "eslint-plugin/require-meta-docs-description": "error", + "eslint-plugin/require-meta-has-suggestions": "error", "eslint-plugin/require-meta-schema": "error", "eslint-plugin/require-meta-type": "error", "eslint-plugin/test-case-property-ordering": "error", diff --git a/Makefile.js b/Makefile.js index 6d1ee39198e..e84d21abf2c 100644 --- a/Makefile.js +++ b/Makefile.js @@ -198,7 +198,7 @@ function generateRuleIndexPage() { description: rule.meta.docs.description, recommended: rule.meta.docs.recommended || false, fixable: !!rule.meta.fixable, - hasSuggestions: !!rule.meta.docs.suggestion + hasSuggestions: !!rule.meta.hasSuggestions }, category = categoriesData.categories.find(c => c.name === rule.meta.docs.category); @@ -671,7 +671,7 @@ target.gensite = function(prereleaseVersion) { const rule = rules.get(ruleName); const isRecommended = rule && rule.meta.docs.recommended; const isFixable = rule && rule.meta.fixable; - const hasSuggestions = rule && rule.meta.docs.suggestion; + const hasSuggestions = rule && rule.meta.hasSuggestions; // Incorporate the special portion into the documentation content const textSplit = text.split("\n"); diff --git a/docs/developer-guide/working-with-rules.md b/docs/developer-guide/working-with-rules.md index 77e05da1034..3da1731d5f1 100644 --- a/docs/developer-guide/working-with-rules.md +++ b/docs/developer-guide/working-with-rules.md @@ -62,7 +62,6 @@ The source file for a rule exports an object with the following properties. * `category` (string) specifies the heading under which the rule is listed in the [rules index](../rules/) * `recommended` (boolean) is whether the `"extends": "eslint:recommended"` property in a [configuration file](../user-guide/configuring/configuration-files.md#extending-configuration-files) enables the rule * `url` (string) specifies the URL at which the full documentation can be accessed (enabling code editors to provide a helpful link on highlighted rule violations) - * `suggestion` (boolean) specifies whether rules can return suggestions (defaults to false if omitted) In a custom rule or plugin, you can omit `docs` or include any properties that you need in it. @@ -70,6 +69,10 @@ The source file for a rule exports an object with the following properties. **Important:** the `fixable` property is mandatory for fixable rules. If this property isn't specified, ESLint will throw an error whenever the rule attempts to produce a fix. Omit the `fixable` property if the rule is not fixable. +* `hasSuggestions` (boolean) specifies whether rules can return suggestions (defaults to `false` if omitted) + + **Important:** the `hasSuggestions` property is mandatory for rules that provide suggestions. If this property isn't set to `true`, ESLint will throw an error whenever the rule attempts to produce a suggestion. Omit the `hasSuggestions` property if the rule does not provide suggestions. + * `schema` (array) specifies the [options](#options-schemas) so ESLint can prevent invalid [rule configurations](../user-guide/configuring/rules.md#configuring-rules) * `deprecated` (boolean) indicates whether the rule has been deprecated. You may omit the `deprecated` property if the rule has not been deprecated. @@ -376,6 +379,8 @@ context.report({ {% endraw %} ``` +**Important:** The `meta.hasSuggestions` property is mandatory for rules that provide suggestions. ESLint will throw an error if a rule attempts to produce a suggestion but does not [export](#rule-basics) this property. + Note: Suggestions will be applied as a stand-alone change, without triggering multipass fixes. Each suggestion should focus on a singular change in the code and should not try to conform to user defined styles. For example, if a suggestion is adding a new statement into the codebase, it should not try to match correct indentation, or conform to user preferences on presence/absence of semicolons. All of those things can be corrected by multipass autofix when the user triggers it. Best practices for suggestions: @@ -397,7 +402,8 @@ module.exports = { unnecessaryEscape: "Unnecessary escape character: \\{{character}}.", removeEscape: "Remove the `\\`. This maintains the current functionality.", escapeBackslash: "Replace the `\\` with `\\\\` to include the actual backslash character." - } + }, + hasSuggestions: true }, create: function(context) { // ... @@ -438,7 +444,8 @@ module.exports = { messages: { unnecessaryEscape: "Unnecessary escape character: \\{{character}}.", removeEscape: "Remove `\\` before {{character}}.", - } + }, + hasSuggestions: true }, create: function(context) { // ... diff --git a/lib/linter/linter.js b/lib/linter/linter.js index 3d445bd7ba3..51ba02aea29 100644 --- a/lib/linter/linter.js +++ b/lib/linter/linter.js @@ -928,6 +928,14 @@ function runRules(sourceCode, configuredRules, ruleMapper, parserOptions, parser if (problem.fix && rule.meta && !rule.meta.fixable) { throw new Error("Fixable rules should export a `meta.fixable` property."); } + if (problem.suggestions && !(rule.meta && rule.meta.hasSuggestions === true)) { + if (rule.meta && rule.meta.docs && typeof rule.meta.docs.suggestion !== "undefined") { + + // Encourage migration from the former property name. + throw new Error("Rules with suggestions must set the `meta.hasSuggestions` property to `true`. `meta.docs.suggestion` is ignored by ESLint."); + } + throw new Error("Rules with suggestions must set the `meta.hasSuggestions` property to `true`."); + } lintingProblems.push(problem); } } diff --git a/lib/rules/no-nonoctal-decimal-escape.js b/lib/rules/no-nonoctal-decimal-escape.js index 78cc39815c6..deaafac550f 100644 --- a/lib/rules/no-nonoctal-decimal-escape.js +++ b/lib/rules/no-nonoctal-decimal-escape.js @@ -32,10 +32,11 @@ module.exports = { description: "disallow `\\8` and `\\9` escape sequences in string literals", category: "Best Practices", recommended: true, - url: "https://eslint.org/docs/rules/no-nonoctal-decimal-escape", - suggestion: true + url: "https://eslint.org/docs/rules/no-nonoctal-decimal-escape" }, + hasSuggestions: true, + schema: [], messages: { diff --git a/lib/rules/no-unsafe-negation.js b/lib/rules/no-unsafe-negation.js index a9c2ee74f2c..f1d5c9c4e31 100644 --- a/lib/rules/no-unsafe-negation.js +++ b/lib/rules/no-unsafe-negation.js @@ -54,10 +54,11 @@ module.exports = { description: "disallow negating the left operand of relational operators", category: "Possible Errors", recommended: true, - url: "https://eslint.org/docs/rules/no-unsafe-negation", - suggestion: true + url: "https://eslint.org/docs/rules/no-unsafe-negation" }, + hasSuggestions: true, + schema: [ { type: "object", diff --git a/lib/rules/no-useless-escape.js b/lib/rules/no-useless-escape.js index 512c93a8bc0..436a7531639 100644 --- a/lib/rules/no-useless-escape.js +++ b/lib/rules/no-useless-escape.js @@ -85,10 +85,11 @@ module.exports = { description: "disallow unnecessary escape characters", category: "Best Practices", recommended: true, - url: "https://eslint.org/docs/rules/no-useless-escape", - suggestion: true + url: "https://eslint.org/docs/rules/no-useless-escape" }, + hasSuggestions: true, + messages: { unnecessaryEscape: "Unnecessary escape character: \\{{character}}.", removeEscape: "Remove the `\\`. This maintains the current functionality.", diff --git a/lib/rules/radix.js b/lib/rules/radix.js index d1b67e5e280..f1a7c1af265 100644 --- a/lib/rules/radix.js +++ b/lib/rules/radix.js @@ -82,10 +82,11 @@ module.exports = { description: "enforce the consistent use of the radix argument when using `parseInt()`", category: "Best Practices", recommended: false, - url: "https://eslint.org/docs/rules/radix", - suggestion: true + url: "https://eslint.org/docs/rules/radix" }, + hasSuggestions: true, + schema: [ { enum: ["always", "as-needed"] diff --git a/tests/fixtures/testers/rule-tester/suggestions.js b/tests/fixtures/testers/rule-tester/suggestions.js index df97e6dd138..ecedde04637 100644 --- a/tests/fixtures/testers/rule-tester/suggestions.js +++ b/tests/fixtures/testers/rule-tester/suggestions.js @@ -1,6 +1,7 @@ "use strict"; module.exports.basic = { + meta: { hasSuggestions: true }, create(context) { return { Identifier(node) { @@ -25,7 +26,8 @@ module.exports.withMessageIds = { avoidFoo: "Avoid using identifiers named '{{ name }}'.", unused: "An unused key", renameFoo: "Rename identifier 'foo' to '{{ newName }}'" - } + }, + hasSuggestions: true }, create(context) { return { @@ -57,4 +59,16 @@ module.exports.withMessageIds = { } }; - +module.exports.withoutHasSuggestionsProperty = { + create(context) { + return { + Identifier(node) { + context.report({ + node, + message: "some message", + suggest: [{ desc: "some suggestion", fix: fixer => fixer.replaceText(node, 'bar') }] + }); + } + }; + } +}; diff --git a/tests/lib/linter/linter.js b/tests/lib/linter/linter.js index 057019cc366..b2724ae20f9 100644 --- a/tests/lib/linter/linter.js +++ b/tests/lib/linter/linter.js @@ -4801,21 +4801,24 @@ var a = "test2"; describe("suggestions", () => { it("provides suggestion information for tools to use", () => { - linter.defineRule("rule-with-suggestions", context => ({ - Program(node) { - context.report({ - node, - message: "Incorrect spacing", - suggest: [{ - desc: "Insert space at the beginning", - fix: fixer => fixer.insertTextBefore(node, " ") - }, { - desc: "Insert space at the end", - fix: fixer => fixer.insertTextAfter(node, " ") - }] - }); - } - })); + linter.defineRule("rule-with-suggestions", { + meta: { hasSuggestions: true }, + create: context => ({ + Program(node) { + context.report({ + node, + message: "Incorrect spacing", + suggest: [{ + desc: "Insert space at the beginning", + fix: fixer => fixer.insertTextBefore(node, " ") + }, { + desc: "Insert space at the end", + fix: fixer => fixer.insertTextAfter(node, " ") + }] + }); + } + }) + }); const messages = linter.verify("var a = 1;", { rules: { "rule-with-suggestions": "error" } }); @@ -4840,7 +4843,8 @@ var a = "test2"; messages: { suggestion1: "Insert space at the beginning", suggestion2: "Insert space at the end" - } + }, + hasSuggestions: true }, create: context => ({ Program(node) { @@ -4877,6 +4881,44 @@ var a = "test2"; } }]); }); + + it("should throw an error if suggestion is passed but `meta.hasSuggestions` property is not enabled", () => { + linter.defineRule("rule-with-suggestions", { + meta: { docs: {}, schema: [] }, + create: context => ({ + Program(node) { + context.report({ + node, + message: "hello world", + suggest: [{ desc: "convert to foo", fix: fixer => fixer.insertTextBefore(node, " ") }] + }); + } + }) + }); + + assert.throws(() => { + linter.verify("0", { rules: { "rule-with-suggestions": "error" } }); + }, "Rules with suggestions must set the `meta.hasSuggestions` property to `true`."); + }); + + it("should throw an error if suggestion is passed but `meta.hasSuggestions` property is not enabled and the rule has the obsolete `meta.docs.suggestion` property", () => { + linter.defineRule("rule-with-meta-docs-suggestion", { + meta: { docs: { suggestion: true }, schema: [] }, + create: context => ({ + Program(node) { + context.report({ + node, + message: "hello world", + suggest: [{ desc: "convert to foo", fix: fixer => fixer.insertTextBefore(node, " ") }] + }); + } + }) + }); + + assert.throws(() => { + linter.verify("0", { rules: { "rule-with-meta-docs-suggestion": "error" } }); + }, "Rules with suggestions must set the `meta.hasSuggestions` property to `true`. `meta.docs.suggestion` is ignored by ESLint."); + }); }); describe("mutability", () => { diff --git a/tests/lib/rule-tester/rule-tester.js b/tests/lib/rule-tester/rule-tester.js index 71225611dfe..3eef7982306 100644 --- a/tests/lib/rule-tester/rule-tester.js +++ b/tests/lib/rule-tester/rule-tester.js @@ -2279,6 +2279,17 @@ describe("RuleTester", () => { }); }, /Invalid suggestion property name 'outpt'/u); }); + + it("should throw an error if a rule that doesn't have `meta.hasSuggestions` enabled produces suggestions", () => { + assert.throws(() => { + ruleTester.run("suggestions-missing-hasSuggestions-property", require("../../fixtures/testers/rule-tester/suggestions").withoutHasSuggestionsProperty, { + valid: [], + invalid: [ + { code: "var foo = bar;", output: "5", errors: 1 } + ] + }); + }, "Rules with suggestions must set the `meta.hasSuggestions` property to `true`."); + }); }); describe("naming test cases", () => {