Skip to content

Commit 6eaad96

Browse files
wdougKai Cataldo
wdoug
authored and
Kai Cataldo
committed
New: Add suggestions API (#12384)
* New: Initial suggestions support in the linter * Docs: Update some docs with info about suggestions * Chore: Add some tests for suggestions * New: Add support for suggestion messageId-based descriptions * New: WIP add suggestions for no-useless-escape * Chore: Update no-useless-escape suggestions to use messageIds * New: Support output option for rule-tester tests of suggestions * Chore: Respond to PR feedback * Chore: Update rule-tester suggestion testing capabilities and tests * Fix: Bug in no-useless-escape suggestions * Chore: Use messageId more for suggestion example * Chore: Update rule-tester suggestion support * Chore: Remove rule-tester support for testing fix object
1 parent b336fbe commit 6eaad96

12 files changed

Lines changed: 1521 additions & 73 deletions

File tree

‎docs/developer-guide/nodejs-api.md‎

Lines changed: 43 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ The most important method on `Linter` is `verify()`, which initiates linting of
108108
* `preprocess` - (optional) A function that [Processors in Plugins](/docs/developer-guide/working-with-plugins.md#processors-in-plugins) documentation describes as the `preprocess` method.
109109
* `postprocess` - (optional) A function that [Processors in Plugins](/docs/developer-guide/working-with-plugins.md#processors-in-plugins) documentation describes as the `postprocess` method.
110110
* `filterCodeBlock` - (optional) A function that decides which code blocks the linter should adopt. The function receives two arguments. The first argument is the virtual filename of a code block. The second argument is the text of the code block. If the function returned `true` then the linter adopts the code block. If the function was omitted, the linter adopts only `*.js` code blocks. If you provided a `filterCodeBlock` function, it overrides this default behavior, so the linter doesn't adopt `*.js` code blocks automatically.
111-
* `disableFixes` - (optional) when set to `true`, the linter doesn't make the `fix` property of the lint result.
111+
* `disableFixes` - (optional) when set to `true`, the linter doesn't make either the `fix` or `suggestions` property of the lint result.
112112
* `allowInlineConfig` - (optional) set to `false` to disable inline comments from changing ESLint rules.
113113
* `reportUnusedDisableDirectives` - (optional) when set to `true`, adds reported errors for unused `eslint-disable` directives when no problems would be reported in the disabled area anyway.
114114

@@ -170,6 +170,7 @@ The information available for each linting message is:
170170
* `endColumn` - the end column of the range on which the error occurred (this property is omitted if it's not range).
171171
* `endLine` - the end line of the range on which the error occurred (this property is omitted if it's not range).
172172
* `fix` - an object describing the fix for the problem (this property is omitted if no fix is available).
173+
* `suggestions` - an array of objects describing possible lint fixes for editors to programmatically enable (see details in the [Working with Rules docs](./working-with-rules.md#providing-suggestions)).
173174

174175
Linting message objects have a deprecated `source` property. This property **will be removed** from linting messages in an upcoming breaking release. If you depend on this property, you should now use the `SourceCode` instance provided by the linter.
175176

@@ -437,9 +438,23 @@ The return value is an object containing the results of the linting operation. H
437438
column: 13,
438439
nodeType: "ExpressionStatement",
439440
fix: { range: [12, 12], text: ";" }
441+
}, {
442+
ruleId: "no-useless-escape",
443+
severity: 1,
444+
message: "disallow unnecessary escape characters",
445+
line: 1,
446+
column: 10,
447+
nodeType: "ExpressionStatement",
448+
suggestions: [{
449+
desc: "Remove unnecessary escape. This maintains the current functionality.",
450+
fix: { range: [9, 10], text: "" }
451+
}, {
452+
desc: "Escape backslash to include it in the RegExp.",
453+
fix: { range: [9, 9], text: "\\" }
454+
}]
440455
}],
441456
errorCount: 1,
442-
warningCount: 0,
457+
warningCount: 1,
443458
fixableErrorCount: 1,
444459
fixableWarningCount: 0,
445460
source: "\"use strict\"\n"
@@ -865,6 +880,7 @@ In addition to the properties above, invalid test cases can also have the follow
865880
* `column` (number): The 1-based column number of the reported location
866881
* `endLine` (number): The 1-based line number of the end of the reported location
867882
* `endColumn` (number): The 1-based column number of the end of the reported location
883+
* `suggestions` (array): An array of objects with suggestion details to check. See [Testing Suggestions](#testing-suggestions) for details
868884

869885
If a string is provided as an error instead of an object, the string is used to assert the `message` of the error.
870886
* `output` (string, optional): Asserts the output that will be produced when using this rule for a single pass of autofixing (e.g. with the `--fix` command line flag). If this is `null`, asserts that none of the reported problems suggest autofixes.
@@ -880,6 +896,31 @@ Any additional properties of a test case will be passed directly to the linter a
880896

881897
If a valid test case only uses the `code` property, it can optionally be provided as a string containing the code, rather than an object with a `code` key.
882898

899+
#### Testing Suggestions
900+
901+
Suggestions can be tested by defining a `suggestions` key on an errors object. The options to check for the suggestions are the following (all are optional):
902+
* `desc` (string): The suggestion `desc` value
903+
* `messageId` (string): The suggestion `messageId` value for suggestions that use `messageId`s
904+
* `output` (string): A code string representing the result of applying the suggestion fix to the input code
905+
906+
Example:
907+
908+
```js
909+
ruleTester.run("my-rule-for-no-foo", rule, {
910+
valid: [],
911+
invalid: [{
912+
code: "var foo;",
913+
errors: [{
914+
suggestions: [{
915+
desc: "Rename identifier 'foo' to 'bar'",
916+
messageId: "renameFoo",
917+
output: "var bar;"
918+
}]
919+
}]
920+
}]
921+
})
922+
```
923+
883924
### Customizing RuleTester
884925

885926
`RuleTester` depends on two functions to run tests: `describe` and `it`. These functions can come from various places:

‎docs/developer-guide/unit-tests.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,10 @@ This automatically starts Mocha and runs all tests in the `tests` directory. You
1212

1313
If you want to quickly run just one test, you can do so by running Mocha directly and passing in the filename. For example:
1414

15-
npm test:cli tests/lib/rules/no-wrap-func.js
15+
npm run test:cli tests/lib/rules/no-wrap-func.js
1616

1717
Running individual tests is useful when you're working on a specific bug and iterating on the solution. You should be sure to run `npm test` before submitting a pull request.
1818

1919
## More Control on Unit Testing
2020

21-
`npm test:cli` is an alias of the Mocha cli in `./node_modules/.bin/mocha`. [Options](https://mochajs.org/#command-line-usage) are available to be provided to help to better control the test to run.
21+
`npm run test:cli` is an alias of the Mocha cli in `./node_modules/.bin/mocha`. [Options](https://mochajs.org/#command-line-usage) are available to be provided to help to better control the test to run.

‎docs/developer-guide/working-with-rules.md‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ The source file for a rule exports an object with the following properties.
6262
* `category` (string) specifies the heading under which the rule is listed in the [rules index](../rules/)
6363
* `recommended` (boolean) is whether the `"extends": "eslint:recommended"` property in a [configuration file](../user-guide/configuring.md#extending-configuration-files) enables the rule
6464
* `url` (string) specifies the URL at which the full documentation can be accessed
65+
* `suggestion` (boolean) specifies whether rules can return suggestions (defaults to false if omitted)
6566

6667
In a custom rule or plugin, you can omit `docs` or include any properties that you need in it.
6768

@@ -344,6 +345,79 @@ Best practices for fixes:
344345

345346
* This fixer can just select a quote type arbitrarily. If it guesses wrong, the resulting code will be automatically reported and fixed by the [`quotes`](/docs/rules/quotes.md) rule.
346347

348+
### Providing Suggestions
349+
350+
In some cases fixes aren't appropriate to be automatically applied, for example, if a fix potentially changes functionality or if there are multiple valid ways to fix a rule depending on the implementation intent (see the best practices for [applying fixes](#applying-fixes) listed above). In these cases, there is an alternative `suggest` option on `context.report()` that allows other tools, such as editors, to expose helpers for users to manually apply a suggestion.
351+
352+
In order to provide suggestions, use the `suggest` key in the report argument with an array of suggestion objects. The suggestion objects represent individual suggestions that could be applied and require either a `desc` key string that describes what applying the suggestion would do or a `messageId` key (see [below](#suggestion-messageids)), and a `fix` key that is a function defining the suggestion result. This `fix` function follows the same API as regular fixes (described above in [applying fixes](#applying-fixes)).
353+
354+
```js
355+
context.report({
356+
node: node,
357+
message: "Unnecessary escape character: \\{{character}}.",
358+
data: { character },
359+
suggest: [
360+
{
361+
desc: "Remove the `\\`. This maintains the current functionality.",
362+
fix: function(fixer) {
363+
return fixer.removeRange(range);
364+
}
365+
},
366+
{
367+
desc: "Replace the `\\` with `\\\\` to include the actual backslash character.",
368+
fix: function(fixer) {
369+
return fixer.insertTextBeforeRange(range, "\\");
370+
}
371+
}
372+
]
373+
});
374+
```
375+
376+
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 confirm to user preferences on presence/absence of semicolumns. All of those things can be corrected by multipass autofix when the user triggers it.
377+
378+
Best practices for suggestions:
379+
380+
1. Don't try to do too much and suggest large refactors that could introduce a lot of breaking changes.
381+
1. As noted above, don't try to conform to user-defined styles.
382+
383+
#### Suggestion `messageId`s
384+
385+
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:
386+
387+
```js
388+
module.exports = {
389+
meta: {
390+
messages: {
391+
unnecessaryEscape: "Unnecessary escape character: \\{{character}}.",
392+
removeEscape: "Remove the `\\`. This maintains the current functionality.",
393+
escapeBackslash: "Replace the `\\` with `\\\\` to include the actual backslash character."
394+
}
395+
},
396+
create: function(context) {
397+
// ...
398+
context.report({
399+
node: node,
400+
messageId: 'unnecessaryEscape',
401+
data: { character },
402+
suggest: [
403+
{
404+
messageId: "removeEscape",
405+
fix: function(fixer) {
406+
return fixer.removeRange(range);
407+
}
408+
},
409+
{
410+
messageId: "escapeBackslash",
411+
fix: function(fixer) {
412+
return fixer.insertTextBeforeRange(range, "\\");
413+
}
414+
}
415+
]
416+
});
417+
}
418+
};
419+
```
420+
347421
### context.options
348422
349423
Some rules require options in order to function correctly. These options appear in configuration (`.eslintrc`, command line, or in comments). For example:

‎lib/linter/report-translator.js‎

Lines changed: 73 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ const interpolate = require("./interpolate");
2626
* @property {Object} [data] Optional data to use to fill in placeholders in the
2727
* message.
2828
* @property {Function} [fix] The function to call that creates a fix command.
29+
* @property {Array<{desc?: string, messageId?: string, fix: Function}>} suggest Suggestion descriptions and functions to create a the associated fixes.
2930
*/
3031

3132
/**
@@ -34,14 +35,15 @@ const interpolate = require("./interpolate");
3435
* @property {string} ruleId
3536
* @property {(0|1|2)} severity
3637
* @property {(string|undefined)} message
37-
* @property {(string|undefined)} messageId
38+
* @property {(string|undefined)} [messageId]
3839
* @property {number} line
3940
* @property {number} column
40-
* @property {(number|undefined)} endLine
41-
* @property {(number|undefined)} endColumn
41+
* @property {(number|undefined)} [endLine]
42+
* @property {(number|undefined)} [endColumn]
4243
* @property {(string|null)} nodeType
4344
* @property {string} source
44-
* @property {({text: string, range: (number[]|null)}|null)} fix
45+
* @property {({text: string, range: (number[]|null)}|null)} [fix]
46+
* @property {Array<{text: string, range: (number[]|null)}|null>} [suggestions]
4547
*/
4648

4749
//------------------------------------------------------------------------------
@@ -182,6 +184,29 @@ function normalizeFixes(descriptor, sourceCode) {
182184
return fix;
183185
}
184186

187+
/**
188+
* Gets an array of suggestion objects from the given descriptor.
189+
* @param {MessageDescriptor} descriptor The report descriptor.
190+
* @param {SourceCode} sourceCode The source code object to get text between fixes.
191+
* @param {Object} messages Object of meta messages for the rule.
192+
* @returns {Array<SuggestionResult>} The suggestions for the descriptor
193+
*/
194+
function mapSuggestions(descriptor, sourceCode, messages) {
195+
if (!descriptor.suggest || !Array.isArray(descriptor.suggest)) {
196+
return [];
197+
}
198+
199+
return descriptor.suggest.map(suggestInfo => {
200+
const computedDesc = suggestInfo.desc || messages[suggestInfo.messageId];
201+
202+
return {
203+
...suggestInfo,
204+
desc: interpolate(computedDesc, suggestInfo.data),
205+
fix: normalizeFixes(suggestInfo, sourceCode)
206+
};
207+
});
208+
}
209+
185210
/**
186211
* Creates information about the report from a descriptor
187212
* @param {Object} options Information about the problem
@@ -192,6 +217,7 @@ function normalizeFixes(descriptor, sourceCode) {
192217
* @param {string} [options.messageId] The error message ID.
193218
* @param {{start: SourceLocation, end: (SourceLocation|null)}} options.loc Start and end location
194219
* @param {{text: string, range: (number[]|null)}} options.fix The fix object
220+
* @param {Array<{text: string, range: (number[]|null)}>} options.suggestions The array of suggestions objects
195221
* @returns {function(...args): ReportInfo} Function that returns information about the report
196222
*/
197223
function createProblem(options) {
@@ -221,9 +247,47 @@ function createProblem(options) {
221247
problem.fix = options.fix;
222248
}
223249

250+
if (options.suggestions && options.suggestions.length > 0) {
251+
problem.suggestions = options.suggestions;
252+
}
253+
224254
return problem;
225255
}
226256

257+
/**
258+
* Validates that suggestions are properly defined. Throws if an error is detected.
259+
* @param {Array<{ desc?: string, messageId?: string }>} suggest The incoming suggest data.
260+
* @param {Object} messages Object of meta messages for the rule.
261+
* @returns {void}
262+
*/
263+
function validateSuggestions(suggest, messages) {
264+
if (suggest && Array.isArray(suggest)) {
265+
suggest.forEach(suggestion => {
266+
if (suggestion.messageId) {
267+
const { messageId } = suggestion;
268+
269+
if (!messages) {
270+
throw new TypeError(`context.report() called with a suggest option with a messageId '${messageId}', but no messages were present in the rule metadata.`);
271+
}
272+
273+
if (!messages[messageId]) {
274+
throw new TypeError(`context.report() called with a suggest option with a messageId '${messageId}' which is not present in the 'messages' config: ${JSON.stringify(messages, null, 2)}`);
275+
}
276+
277+
if (suggestion.desc) {
278+
throw new TypeError("context.report() called with a suggest option that defines both a 'messageId' and an 'desc'. Please only pass one.");
279+
}
280+
} else if (!suggestion.desc) {
281+
throw new TypeError("context.report() called with a suggest option that doesn't have either a `desc` or `messageId`");
282+
}
283+
284+
if (typeof suggestion.fix !== "function") {
285+
throw new TypeError(`context.report() called with a suggest option without a fix function. See: ${suggestion}`);
286+
}
287+
});
288+
}
289+
}
290+
227291
/**
228292
* Returns a function that converts the arguments of a `context.report` call from a rule into a reported
229293
* problem for the Node.js API.
@@ -242,17 +306,17 @@ module.exports = function createReportTranslator(metadata) {
242306
*/
243307
return (...args) => {
244308
const descriptor = normalizeMultiArgReportCall(...args);
309+
const messages = metadata.messageIds;
245310

246311
assertValidNodeInfo(descriptor);
247312

248313
let computedMessage;
249314

250315
if (descriptor.messageId) {
251-
if (!metadata.messageIds) {
316+
if (!messages) {
252317
throw new TypeError("context.report() called with a messageId, but no messages were present in the rule metadata.");
253318
}
254319
const id = descriptor.messageId;
255-
const messages = metadata.messageIds;
256320

257321
if (descriptor.message) {
258322
throw new TypeError("context.report() called with a message and a messageId. Please only pass one.");
@@ -267,6 +331,7 @@ module.exports = function createReportTranslator(metadata) {
267331
throw new TypeError("Missing `message` property in report() call; add a message that describes the linting problem.");
268332
}
269333

334+
validateSuggestions(descriptor.suggest, messages);
270335

271336
return createProblem({
272337
ruleId: metadata.ruleId,
@@ -275,7 +340,8 @@ module.exports = function createReportTranslator(metadata) {
275340
message: interpolate(computedMessage, descriptor.data),
276341
messageId: descriptor.messageId,
277342
loc: normalizeReportLoc(descriptor),
278-
fix: metadata.disableFixes ? null : normalizeFixes(descriptor, metadata.sourceCode)
343+
fix: metadata.disableFixes ? null : normalizeFixes(descriptor, metadata.sourceCode),
344+
suggestions: metadata.disableFixes ? [] : mapSuggestions(descriptor, metadata.sourceCode, messages)
279345
});
280346
};
281347
};

0 commit comments

Comments
 (0)