diff --git a/designs/2024-baseline-support/README.md b/designs/2024-baseline-support/README.md index f5bec80c..1613d294 100644 --- a/designs/2024-baseline-support/README.md +++ b/designs/2024-baseline-support/README.md @@ -3,13 +3,13 @@ - RFC PR: (leave this empty, to be filled in later) - Authors: [Iacovos Constantinou](https://github.com/softius) -# Introduce baseline system to suppress existing errors +# Introduce a way to suppress violations ## Summary -Declare currently reported errors as the "baseline", so that they are not being reported in subsequent runs. It allows developers to enable one or more rules as `error` and be notified when new ones show up. +Suppress existing violations, so that they are not being reported in subsequent runs. It allows developers to enable one or more lint rules and be notified only when new violations show up. ## Motivation @@ -18,7 +18,7 @@ outcome? --> Enabling a new lint rule as `error` can be painful when the codebase has many violations and the rule isn't auto-fixable. A good example is [`@typescript-eslint/no-explicit-any`](https://typescript-eslint.io/rules/no-explicit-any/). Unless the rule is enabled during the early stages of the project, it becomes harder and harder to enable it as the codebase grows. Existing violations must be resolved before enabling the rule, but while doing that other violations might creep in. -This can be counterintuitive for enabling new rules as `error`, since the developers need to address the violations before-hand in one way or another. The suggested solution suppress existing violations, allowing the developers to address these at their own pace. It also reports any new violations making it easier to identify and address future violations. +This can be counterintuitive for enabling new rules as `error`, since the developers need to address the violations before-hand in one way or another. The suggested solution suppress existing violations, allowing the developers to address these at their own pace. It also reports any new violations making it easier to identify and address them. ## Detailed Design @@ -31,11 +31,11 @@ This can be counterintuitive for enabling new rules as `error`, since the develo used. Be sure to define any new terms in this section. --> -To keep track of the all the errors that we would like to ignore, we are introducing the concept of the baseline file; A JSON file containing the number of errors that must be ignored for each rule in each file. By design, the baseline is disabled by default and it doesn't affect existing or new projects, unless the baseline file is generated. +To keep track of the all the violations that we would like to suppress, we are storing these into a separate file; A JSON file containing the number of errors that must be ignored for each rule in each file. By design, no violations are suppressed - in other words, this feature doesn't affect existing or new projects, unless the developers explicitly suppress one or more violations. -### Baseline format +### File format -The baseline files includes details about the file where the error found, the rule name and the number of errors. Here is what the baseline file looks like. This indicates that the file `"src/app/components/foobar/foobar.component.ts"` has one error for the rule `@typescript-eslint/no-explicit-any` that is acceptable to be ignored. All paths are relative to CWD, for portability reasons. +The JSON file includes details about the file where the violations found, the rule name and the number of violations. As an example, the following indicates that the file `"src/app/components/foobar/foobar.component.ts"` has one violation for the rule `@typescript-eslint/no-explicit-any` that we want to suppress. All paths are relative to CWD, for portability reasons. ``` { @@ -47,161 +47,191 @@ The baseline files includes details about the file where the error found, the ru } ``` -### Generating the baseline +The file is stored in `.eslint-suppressions.json` , unless otherwise specified. -A new option `--baseline` wil be introduced to ESLint CLI. When provided, the baseline is generated and saved in `.eslint-baseline.json`. If the file already exists, it gets over-written. Note that this is a boolean flag option (no values are accepted). Here is an example how to generate the baseline: +### Suppressing all violations + +A new option `--suppress-all` wil be introduced to ESLint CLI. When provided, the JSON file is generated and saved in `.eslint-suppressions.json`. If the file already exists, it gets over-written. Note that this is a boolean flag option (no values are accepted). ``` bash -eslint --baseline ./src +eslint --suppress-all ./src ``` -The above goes through each result item and messages, and counts the number of errors (`severity == 2`). The necessary details are stored in the baseline file. By default, the baseline file is saved at `.eslint-baseline.json` . To control where the baseline is saved, another option will be introduced `--baseline-location`. That is a string argument specifying where the baseline must be saved. Here is an example -how to generate the baseline and save the results to `/home/user/project/mycache`. +### Suppressing violations of a specific rule + +A new option `--suppress-rule [RULE1]` will be introduced to ESLint CLI. When provided, the existing suppressions file will be updated to include any existing violation of the provided rule. The suppressions file will be created if not already exists. Note that this is a string flag option (value is required). ``` bash -eslint --baseline --baseline-location /home/user/project/mycache +eslint --suppress-rule '@typescript-eslint/no-explicit-any' ./src ``` -To introduce the above-mentioned options, we will need to: +### Changing the location of the suppressions file -* add the new options in `default-cli-options.js`. -* adjust the config for optionator. -* add the new options as comments and arguments for eslint. -* update documentation to explain the newly introduced options. +A new option `--suppression-location` will be introduced to ESLint CLI. When provided, the suppressions file will be loaded and saved to the provided location. Note that this is a string flag option (value is required). -On top of that, we will need to adjust `cli.js` to check if `--baseline` was provided and set to true, right after the fixes are done and the warnings are filtered out, to avoid counting errors that are about to be fixed. A new method `generateBaseline` will be introduced, which must be called only and only if `--baseline` was provided and set to true. Please refer to "Implementation Details" for more details on this. +``` bash +eslint --suppress-all --suppressions-location /home/user/project/mycache ./src +``` -### Matching against the baseline +### Maintaining a lean suppressions file -The suggested solution always compares against the baseline, given that the baseline file exists. By default the baseline file is picked up from `.eslint-baseline.json`, unless the option `--baseline-location` is provided. This makes it easier for existing and new projects to adopt the baseline without the need to adjust scripts in `package.json` and CI/CD workflows. As mentioned above, `--baseline-location` is a string argument specifying where the baseline was previously saved. +When working with suppressed violations, there is a chance that a violation is addressed but the suppressions file is not updated. This might allow new violations to creep in without noticing. To ensure that the suppressions file is always up to date, `eslint` can exit with an error code when there are suppressed violations that do not occur anymore. -This will go through each result item and message, and check each rule giving an error (`severity == 2`) against the baseline. By design, we do not take warnings into consideration (regardless of whether quite mode is enabled or not), since warnings do not cause eslint to exit with an error code and they already serve a different purpose. If the file and rule are part of the baseline, means that we can remove and ignore the result message. +Consider the following scenario: -To implement this, we will need to adjust further `cli.js` to adopt the following operations: +* The developer executes `eslint --supress-all ./src` which creates the suppressions file. +* Then `eslint ./src` is executed which reports no violations, with an exit status of 0. +* The developer addresses an error violation. While the violation is addressed is still part of the suppressions file. +* The developer then executes `eslint ./src` again. While it still reports no violations, it exits with a non-zero status code. That is to indicate that the suppressions file needs to be updated. -* Check if the `--baseline` option is passed - * If yes, we need to generate the baseline based on `resultsToPrint`. - * If no, we need to check if the baseline file already exists taking `--baseline-location` into consideration -* Assuming that a baseline was found or generated, we need to match the errors found against the baseline. In particular, for each error found: - * We need to check whether both file and error are part of the baseline - * If yes, we reduce the `count` by 1 and ignore the current error. If `count` has already reach zero we keep the error. - * If no, we keep the error. +To address this, a new option `--prune-suggestions` will be introduced to ESLint. Note that this is a boolean flag option (no values are accepted). When provided, violations in suppressions file that no longer occur will be removed, but no new violations will be added (in contrary to when executing `--suppress-all`). -Note that the error detection in `cli.js` happens before the error counting. This allow us to create the baseline, before it is time to count errors. Please refer to the last example of the "Implementation details". +``` bash +eslint --prune-suppressions ./src +eslint --prune-suppressions --suppressions-location /home/user/project/mycache ./src +``` -We can also keep track of which errors from baseline were not matched, that is useful for the next section. +### Execution details -### Maintaining a lean baseline +The suggested solution always compares against the suppressions file, given that the file already exists. By default the file is picked up from `.eslint-suppressions.json`, unless the option `--suppressions-location` is provided. This makes it easier for existing and new projects to adopt this feature without the need to adjust scripts in `package.json` and CI/CD workflows. -When using the baseline, there is a chance that an error is resolved but the baseline file is not updated. This might allow new errors to creep in without noticing. To ensure that the baseline is always up to date, eslint can exit with an error code when there are ignored errors that do not occur anymore. To fix this, the developer can regenerate the baseline file. +This will go through each result item and message, and check each rule giving an error (`severity == 2`) against the suppressions. By design, we do not take warnings into consideration (regardless of whether quite mode is enabled or not), since warnings do not cause eslint to exit with an error code and they already serve a different purpose. If the file and rule are part of the suppressions file, it means that we can remove and ignore the result message. -Consider the following scenario: +To implement this, we will need to adjust mainly `cli.js` to adopt the following operations: -* The developer executes `eslint --baseline ./src` which updates the baseline file. -* Then `eslint ./src` is executed which gives no violations, with an exit status of 0. -* The developer addresses an error violation. While the error is addressed is still part of the baseline. -* The developer then executes `eslint ./src` again. While it still gives no violations, it exits with a non-zero status code. That is to indicate that the baseline needs to be re-generated by executing `eslint --baseline ./src`. +* Check if the `--suppress-all` or `--suppress-rule` option is passed + * If both are passed exit with an error, since the two options are mutually exclusive. + * If either option was passed, we need to update the suppressions file based on `results`. + * If none option was passed, we need to check if the suppressions file already exists taking `--suppressions-location` into consideration +* Assuming that a suppressions file was found or generated, we need to match the errors found against the violations included in the suppressions file. In particular, for each error found: + * We need to check whether both file and error are part of the suppressions file. + * If yes, we reduce the `count` by 1 and ignore the current error. If `count` has already reach zero we keep the error. + * If no, we keep the error. +* Exit with a non-zero status code if there are unmatched violations. Depending on the verbose mode we can display the list of errors that were left unmatched. +* Otherwise list the remaining errors as usual. -To implement this, we will need to extend `applyBaseline` to return the unmatched rules. In particular, we will need to check if one or more rules are left unmatched, and exit with an error code. Depending on the verbose mode we can display the list of errors that were left unmatched. +Note that the error detection in `cli.js` happens before the error counting. This allow us to update the suppressions file and modify the errors, before it is time to count errors. Please refer to the last example of the "Implementation notes" for more details. -### ESLint Cache +Furthermore, ESLint cache (`--cache`) must contain the full list of detected violations, even those matched against the suppressions file. This approach has the following benefits: -ESLint cache (`--cache`) must contain the full list of detected errors, even those matched against the baseline. This approach has the following benefits: +- Generating the suppressions file can be based on the cache file and should be faster when the cache file is used. +- Allows developers to re-generate the suppressions file or even adjust it manually and re-lint still taking the same cache into consideration. +- It even allows developers to delete the suppressions file and still take advantage of the cached file in subsequent runs. -- Generating the baseline can be based on the cache file and should be faster when the cache file is used. -- Allows developers to re-generate the baseline or even adjust it manually and re-lint still taking the same cache into consideration. -- It even allows developers to delete the baseline and still take advantage of the cached file in subsequent runs. +# Implementation notes -### Implementation Details +To introduce the above-mentioned options, we will need to: -First of all we need to introduce a new type to hold the individual baseline result: +* add the new options in `default-cli-options.js`. +* adjust the config for optionator. +* add the new options as comments and arguments for eslint. +* update documentation to explain the newly introduced option + +A new type must be created: ``` js /** - * @typedef {Record} BaselineResult + * @typedef {Record} SuppressedViolation */ ``` -One way to approach this is to introduce a Manager class handling the baseline results - in particular, both generating a baseline and validating the results against a baseline. - ``` js -class BaselineResultManager { +class SuppressedViolationsManager { /** - * Creates a new instance of BaselineResultManager. - * @param {string} baselineLocation The location of the baseline file. + * Creates a new instance of SuppressedViolationsManager. + * @param {string} suppressionsLocation The location of the suppressions file. */ - constructor(baselineLocation) {} + constructor(suppressionsLocation) {} /** - * Generates the baseline from the provided lint results. + * Updates the suppressions file based on the current violations + * * @param {LintResult[]} results The lint results. - * @returns BaselineResult[] + * @returns SuppressedViolation[] */ - generateBaseline(results) + suppressAll(results) /** - * Checks the provided baseline results against the lint results. - * It filters and returns the lint results that are not in the baseline. - * It also returns the unmatched baseline results. + * Updates the suppressions file based on the current violations and the provided rule * * @param {LintResult[]} results The lint results. - * @param {BaselineResult[]} baseline The baseline. + * @param {string} rule The rule to suppress. + */ + suppressByRule(results, rule) + + /** + * Removes old suppressions that do not occur anymore. + * @returns void + */ + prune() + + /** + * Checks the provided suppressions against the lint results. + * It filters and returns the lint results that are not in the suppressions file. + * It also returns the unmatched suppressions. + * + * @param {LintResult[]} results The lint results. + * @param {SuppressedViolation[]} suppressions The suppressions. * @return { * results: LintResult[], - * unmatched: BaselineResult[] + * unmatched: SuppressedViolation[] * } */ - applyBaseline(results, baseline) + applySuppressions(results, suppressions) /** - * Loads the baseline from the file. - * @returns BaselineResult[] + * Loads the suppressions file. + * @returns SuppressedViolation[] */ - loadBaseline() + load() /** - * Saves the baseline to the file. - * @param {BaselineResult[]} + * Updates the suppressions file. + * @param {SuppressedViolation[]} * @returns void * @private */ - saveBaseline(baseline) + save() } ``` -The resolution of the baseline location must happen outside of the above class. An idea is to make `getCacheFile` in `lib/eslint/eslint-helpers.js` a bit more abstract so that we can inject the prefix i.e. `.cache_` or `.baseline` when a directory is provided. This way both `cache-location` and `baseline-location` are consistent and following the same pattern. +The resolution of the suppressions file must happen outside of the above class. An idea is to make `getCacheFile` in `lib/eslint/eslint-helpers.js` a bit more abstract so that we can inject the prefix i.e. `.cache_` or `.suppressions_` when a directory is provided. This way both `cache-location` and `suppressions-location` are consistent and following the same pattern. Once the above are in place, `cli.js` should look something like: ``` js // ... -const { baseline } = require("../conf/default-cli-options"); -// ... -if (options.quiet) { - debug("Quiet mode enabled - filtering out warnings"); - resultsToPrint = ActiveESLint.getErrorResults(resultsToPrint); +if (options.fix) { + debug("Fix mode enabled - applying fixes"); + await ActiveESLint.outputFixes(results); } -const baselineFileLocation = getCacheFile(baseline, cwd, 'baseline_'); -if (options.baseline || fs.existsSync(baseline)) { - const baselineManager = new BaselineResultManager(baselineFileLocation); - let loadedBaselineRecords = []; - if (options.baseline) { - loadedBaselineRecords = baselineManager.generateBaseline(resultsToPrint); +const suppressionsFileLocation = getCacheFile(options.suppressionsLocation, cwd, 'suppressions_'); +if (options.suppressAll || options.suppressRule || options.pruneSuppressions || fs.existsSync(suppressionsFileLocation)) { + const suppressionsManager = new SuppressedViolationsManager(suppressionsFileLocation); + if (options.suppressAll) { + results = suppressionsManager.suppressAll(results); + } else if (options.suppressRule) { + results = suppressionsManager.suppressByRule(results, options.suppressRule); + } else if (options.pruneSuppressions) { + results = suppressionsManager.prune(); } else { - loadedBaselineRecords = baselineManager.loadBaseline(); - } - - const baselineResults = await baselineManager.applyBaseline(resultsToPrint, loadedBaselineRecords); + const suppressionResults = suppressionsManager.applySuppressions(results, suppressionsManager.load()); + if (suppressionResults.unmatched.length > 0) { + // exit with a non-zero code + } - if (baselineResults.unmatched.length > 0) { - // exit with errors + results = suppressionResults.results; } +} + +let resultsToPrint = results; - resultsToPrint = baselineResults.results; +if (options.quiet) { + debug("Quiet mode enabled - filtering out warnings"); + resultsToPrint = ActiveESLint.getErrorResults(resultsToPrint); } -const resultCounts = countErrors(results); //... ``` @@ -212,7 +242,7 @@ const resultCounts = countErrors(results); on the ESLint blog to explain the motivation? --> -We should update [Command Line Interface Reference](https://eslint.org/docs/latest/use/command-line-interface) to document the newly introduced option. A dedicated section should be added in Documentation to explain how baseline works. +We should update [Command Line Interface Reference](https://eslint.org/docs/latest/use/command-line-interface) to document the newly introduced options. A dedicated section should be added in Documentation to explain how the new suppression system works. ## Drawbacks @@ -227,7 +257,7 @@ We should update [Command Line Interface Reference](https://eslint.org/docs/late implementing this RFC as possible. --> -The baseline can be generated and used only when linting files. It can not be leveraged when using `stdin` since it relies on file paths. +The suggested solution can be used only when linting files. It can not be leveraged when using `stdin` since it relies on file paths. ## Backwards Compatibility Analysis @@ -237,11 +267,11 @@ The baseline can be generated and used only when linting files. It can not be le to existing users? --> -If the baseline file is not generated, ESLint CLI behavior will not change. This change is therefore backwards compatible to start with. +If the suppressions file does not exist, ESLint CLI behavior will not change. This change is therefore backwards compatible to start with. -If the baseline file is generated, ESLint CLI will compare the errors against the errors included in the baseline. Hence it might report less errors than before and someone might argue that this is not backwards compatible since the behavior changes for them. However, as discussed earlier this should facilitate the adoption of the baseline without worrying about adjusting scripts in `package.json` and CI/CD workflow. Plus, the baseline can be easily deleted and cancel the new behavior. +If the suppressions file is already generated, ESLint CLI will compare the errors against the violations included in the suppressions file. Hence it might report less errors than before and someone might argue that this is not backwards compatible since the behavior changes for them. However, as discussed earlier this should facilitate the adoption of the suggested solution without worrying about adjusting scripts in `package.json` and CI/CD workflow. Plus, the suppressions file can be easily deleted and cancel the new behavior. -Furthermore, we are adding one more reason to exit with an error code (see "Maintaining a lean baseline"). This might have some negative side-effects to wrapper scripts that assume that error messages are available when that happens. We could introduce a different exit code, to differentiate between exiting due to unresolved errors or ignored errors that do not occur anymore. +Furthermore, we are adding one more reason to exit with an error code (see "Maintaining a lean suppressions file"). This might have some negative side-effects to wrapper scripts that assume that error messages are available when that happens. We could introduce a different exit code, to differentiate between exiting due to unresolved errors or ignored errors that do not occur anymore. ## Alternatives @@ -252,7 +282,7 @@ Furthermore, we are adding one more reason to exit with an error code (see "Main projects have already implemented a similar feature. --> -Unfortunately existing approaches do not address the issue at its core and come with their own set of drawbacks. It is worth mentioning that the suggested solution is based on [how baseline works in PHPStan](https://phpstan.org/user-guide/baseline). +Unfortunately existing approaches do not address the issue at its core and come with their own set of drawbacks. It is worth mentioning that the suggested solution is based on [how baseline works in PHPStan](https://phpstan.org/user-guide/baseline) and bulk suppressions from [@rushstack/eslint-bulk](https://www.npmjs.com/package/@rushstack/eslint-bulk). The following sections are extracted from [Change Request: Introduce a system to suppress existing errors](https://github.com/eslint/eslint/issues/16755) where [@jfmengels](https://github.com/jfmengels) did a detailed analysis about existing approaches and their drawbacks. @@ -321,12 +351,13 @@ I expect to implement this change. ### Does this count warnings? -No, we are only counting errors when generating the baseline. Also only errors are considered when checking against the baseline. +No, we are only counting errors when updating the suppressions file. Also only errors are considered when checking against the suppressions file. ## Related Discussions * [Change Request: Introduce a system to suppress existing errors](https://github.com/eslint/eslint/issues/16755) * [PHPStan - The Baseline](https://phpstan.org/user-guide/baseline) +* [@rushstack/eslint-bulk](https://www.npmjs.com/package/@rushstack/eslint-bulk)