Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Breaking: Require meta.hasSuggestions for rules with suggestions #14573

Merged
merged 10 commits into from Aug 5, 2021
1 change: 1 addition & 0 deletions .eslintrc.js
Expand Up @@ -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",
Expand Down
4 changes: 2 additions & 2 deletions Makefile.js
Expand Up @@ -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);

Expand Down Expand Up @@ -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");
Expand Down
13 changes: 10 additions & 3 deletions docs/developer-guide/working-with-rules.md
Expand Up @@ -62,14 +62,17 @@ 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.

* `fixable` (string) is either `"code"` or `"whitespace"` if the `--fix` option on the [command line](../user-guide/command-line-interface.md#--fix) automatically fixes problems reported by the rule

**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.
Expand Down Expand Up @@ -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:
Expand All @@ -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
btmills marked this conversation as resolved.
Show resolved Hide resolved
},
create: function(context) {
// ...
Expand Down Expand Up @@ -438,7 +444,8 @@ module.exports = {
messages: {
unnecessaryEscape: "Unnecessary escape character: \\{{character}}.",
removeEscape: "Remove `\\` before {{character}}.",
}
},
hasSuggestions: true
},
create: function(context) {
// ...
Expand Down
8 changes: 8 additions & 0 deletions lib/linter/linter.js
Expand Up @@ -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);
}
}
Expand Down
5 changes: 3 additions & 2 deletions lib/rules/no-nonoctal-decimal-escape.js
Expand Up @@ -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: {
Expand Down
5 changes: 3 additions & 2 deletions lib/rules/no-unsafe-negation.js
Expand Up @@ -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",
Expand Down
5 changes: 3 additions & 2 deletions lib/rules/no-useless-escape.js
Expand Up @@ -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.",
Expand Down
5 changes: 3 additions & 2 deletions lib/rules/radix.js
Expand Up @@ -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"]
Expand Down
18 changes: 16 additions & 2 deletions tests/fixtures/testers/rule-tester/suggestions.js
@@ -1,6 +1,7 @@
"use strict";

module.exports.basic = {
meta: { hasSuggestions: true },
create(context) {
return {
Identifier(node) {
Expand All @@ -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 {
Expand Down Expand Up @@ -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') }]
});
}
};
}
};
74 changes: 58 additions & 16 deletions tests/lib/linter/linter.js
Expand Up @@ -4799,21 +4799,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" } });

Expand All @@ -4838,7 +4841,8 @@ var a = "test2";
messages: {
suggestion1: "Insert space at the beginning",
suggestion2: "Insert space at the end"
}
},
hasSuggestions: true
},
create: context => ({
Program(node) {
Expand Down Expand Up @@ -4875,6 +4879,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", () => {
Expand Down
11 changes: 11 additions & 0 deletions tests/lib/rule-tester/rule-tester.js
Expand Up @@ -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", () => {
Expand Down