feat: Introduce a way to suppress violations

[softius]

trafficstars

Summary

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.

Related Issues

• Change Request: Introduce a system to suppress existing errors

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: softius / name: Iacovos Constantinou (f9b39769b586f9961b6e09fe9f6b7376f79fb306, ed01be1d8d909e4ebe00e143f8699e8f33c2e38c, 4d73750973efd107d3ec0b7cc4dd5b171c693562, a99c45634f914e707ca7a6f48ea36eeb36d239c1, c9a718c5650363fe8f63f5564f8cbf446112db01, 4462ce6d8475489123003c75946444b610dc2398, 485f68452c2a1aebf16735069bb6329966bfc9d1, 98779dc8863079a1a1da2c33cc704e8da3ab7579, dc2d94019072055ef243a652dc88242c7c4ff0f6, b343ddb4084e4611dfcdf51a3e24624af5c10752, 9e035f174b5e60d710073d15a5944804ee180e4f, b8a1cf74d831b15004d7e5ba89361fc13bb98792, d5411af1eb7edd8a1fce5567dd781c3cf19d020b, 70a7c569c0c98f5e1cba0ed5ab7814efeed57577, 42d8b95e19f201776a85e47ab62cc3e1fc3aa8b3, ad5343de3f11689f5eaa97eea0bb866e9dcea2e8, bff622da49c90f51f6c7692319f12045758779f5, 0c3d9a48d08bb480d1c9f7a25e0ae47fc0e37eb1, 1a1cbba8b392daa480c14c5d874e36fd24be02b5, 2d4dc84b27fc7849a1f2cfa052be32e5ac8acf3a, 71ee661d6adeb3db2cecbf7f724daa2b4479369f, 4fea00ea16be92239f749d551f7eefb11ef5bb4a, 46cf6ae49e76b688dd56bcaa08c04df12c2b7984, 48e9d0a41369abfbd8275569e2a0679e6b1c0eba, a94a50d6e654410840d6227c7dde8924e600b8ca, 861f1a44489656e3524752d3a39a4550dc43a30f, 566e3b951c15515feb341cb24f0af4345109486f)

[fasttime]

Thanks for the RFC @softius. Can you please sign the CLA as requested in this comment?

[nzakas]

@octogonz Would really like your feedback on this based on your work on Rush Stack Bulk.

[octogonz]

The feature that we designed is part of @rushstack/eslint-patch. Kevin Yang summarized it in this blog post.

A key design question is how to keep bulk suppressions stable across Git merges, since they are stored separately from the associated source code. From a cursory glance, it sounds like this RFC is proposing to count the number of errors per file path. We prototyped that design, as well as a design that tracks line #'s, and also a design that tracks formatted messages ('x' is already defined.) which are more specific than rule names (no-redeclare). We also considered actually storing the suppressions in the source with a different comment syntax.

Ultimately we settled on a JSON file using a scopeId that loosely identifies regions of code (e.g. ".ExampleClass.example-method") while still being simple enough for humans to interpret without having to consult a specification. This notation is abstracted such that we can reuse the exact same scopeId syntax for tsc/tsconfig.json bulk suppressions (a related feature we're planning to release soon), even though it it is derived from the compiler AST instead of ESLint's AST.

There's probably still a lot that can be improved about our approach. However I can say that Microsoft and TikTok are using it successfully in two very large TypeScript monorepos, where style guide migrations involve hundreds of thousands of source files and megabytes of JSON. So it's worth studying even if you don't adopt it. 🙂

As far as future improvements, a feature I'd like to see is an API that enables the VS Code extension to highlight suppressed violations specially. Currently people either highlight them as normal errors (which I find confusing, since you have to run the CLI to find out whether you really need to fix it or not) or else don't highlight them at all.

@kevin-y-ang

[nzakas]

@octogonz thanks for the insights. I think we're past the point of adopting eslint-bulk wholesale, but would welcome your feedback on this RFC to see if there are any salient points that have been missed or problem areas you've encountered.

[octogonz]

thanks for the insights. I think we're past the point of adopting eslint-bulk wholesale, but would welcome your feedback on this RFC to see if there are any salient points that have been missed or problem areas you've encountered.

Agreed, the eslint-bulk tool was implemented as a monkey patch, therefore it can't be reused directly; the approach needs to be reworked in order to integrate this feature into official ESLint.

I've been really busy lately, but I'd like to help out with the RFC. Let me make some time to read through @softius's doc in more detail, then follow up with feedback.

[fasttime]

@softius just a reminder that there are some suggestions for you to apply. Please, let us know if you need any help.

[Humni]

I really support this RFC - we've been running our own baseline package (just a wrapper around eslint), which takes the same approach. We've been using this implementation on about 30+ prod projects with no complaints from our team. Hopefully, this implementation can assist with the RFC

[nzakas]

@humni that's great! Do you have any specific feedback on the implementation described in this proposal? How does it compare to what you're already using?

[kevin-y-ang]

Hi Nicholas, thanks for checking out the eslint-bulk project!

1. Using the term "suppressions" instead of "baseline" - I think this clarifies what is actually happening. While people may not understand what generating a baseline is, most will understand when something is suppressed.

Other names we considered were exemptions, pardons, and legacy-suppressions.

On a semi-related note, the "baselines" feature seems to very closely resemble the already existing eslint --max-warnings flag, which might be a source of confusion if the two features are both retained.

[nzakas]

On a semi-related note, the "baselines" feature seems to very closely resemble the already existing eslint --max-warnings flag, which might be a source of confusion if the two features are both retained.

@kevin-y-ang I'm not sure I follow. Can you explain what you mean by this?

[kevin-y-ang]

At a conceptual level, both the proposed baseline feature and the --max-warnings feature set a threshold of X errors where the system will alarm if there are >X errors.

At our company, we previously used eslint . --max-warnings X to block merge requests for packages with high warning counts. I believe this was the intended use case when it was originally created?

The message essentially being conveyed here is "Okay there are already X number of ESLint warnings in this package but we won't allow any more", while the message being conveyed with the baseline feature seems to be a more granular version of this: "Okay there are already X number of ESLint errors/warnings for rule R in file F but we won't allow any more."

Anyway, it's just a similarity I see, it's not a big deal.

[Humni]

@Humni that's great! Do you have any specific feedback on the implementation described in this proposal? How does it compare to what you're already using?

Probably the main decision to make for this RFC which style of error matching you use. These would be:

1. Exact Error Line Matching
2. Context Error Matching
3. Count with Context Matching (recommended)

With our current implementation, we use the Exact Error Line Matching, which does have some short falls. It works pretty well, the vast majority of PRs don't require any rework. There are cases where if you modify a line at the top of a file though, will cause all of the existing errors to be exposed. This approach was chosen due to simplicity. For the most part this does push a project to having a smaller and smaller baseline, however it does lead to developers touching the baseline "too much" due to them not wanting to fix errors not directly related to their PR.

We're currently exploring moving to a context aware error matching, so it only comes up if the nearest 3 lines of code are modified - https://github.com/LuminateOne/eslint-baseline/pull/8. This would provide a solution to the issue above, but it's not yet tested but seems like a sensible approach (if it can be done in a performant way).

I would highly recommend an error count approach the same as PHPStan (as per comments from @ondrejmirtes), as that seems to sit with the development team better.

Only other question "feature request" I would add into this is to allow for "flushing" a baseline of excess/resolved errors only (and not add any new ones). Just a QOL improvement though as it's typically easy enough to see new lines added into the diff in the baseline.

[nzakas]

@Humni gotcha, that's helpful. So it sounds like the current RFC, which uses error counting, is what you'd prefer. :+1:

Keep in mind, too, that we generally build things incrementally. Our goal here is to get a baseline (no pun intended) of support for suppressions such that we can continue to build on top of it as feedback comes in.

[nzakas]

@softius are you still working on this? There is some outstanding feedback to review.

[nzakas]

@softius do you intend to finish up this RFC?

[softius]

@nzakas yes, I will go through the outstanding feedback and adjust accordingly.

[nzakas]

@softius thanks! 🙏

[softius]

Thank you all for your valuable feedback!

@nzakas I kept the term "baseline" since it wasn't clear to me whether switching to "suppressions" was a suggestion or something that you would like to enforce. I don't mind doing one more iteration and adjust the terminology, given that this would be easier for the community to grasp. Let me know please.

Also, I think it would be better to leave --suppress-rule <ruleId> and --suppress-all outside of the scope of this RFC. These could be additional improvements once the feature is approved and implemented. Personally, I consider this nice to have since adopters can still edit the baseline file and remove rules manually.

[nzakas]

@softius just wanted to call your attention to this comment: https://github.com/eslint/rfcs/pull/119#pullrequestreview-2094068703

[softius]

@nzakas in regards to --suppress-rule <ruleId>, I understand that this was borrowed as a concept from eslint-bulk but there some fundamentals differences between this RFC and how eslint-bulk works.

In particular, this RFC suggests to use --baseline to generate initially the baseline. The same command can be used afterwards when new errors are added in .eslintrc, but also to remove any unmatched rules. As per my understanding, eslint-bulk uses --suppress-rule <ruleId> so that you can incrementally suppress more and more rules. To clear unmatched rules you need to execute eslint suppress prune - which means that we will need one more extra flag i.e. --prune-suppressions if we decide to go down that path.

Some examples:

# Generate the baseline (or overwriting an existing)
eslint --suppress-all ./src
eslint --suppress-all --suppressions-location /home/user/project/mycache ./src

# Create the baseline with one rule only (or add this rule to the existing baseline)
eslint --suppress-rule "@typescript-eslint/no-explicit-any"
eslint --suppress-rule "@typescript-eslint/no-explicit-any" --suppressions-location /home/user/project/mycache

# Remove unmatched rules without adding any new rules
eslint --prune-supressions
eslint --prune-supressions --suppressions-location /home/user/project/mycache

What do you think?

[softius]

At a conceptual level, both the proposed baseline feature and the --max-warnings feature set a threshold of X errors where the system will alarm if there are >X errors.

At our company, we previously used eslint . --max-warnings X to block merge requests for packages with high warning counts. I believe this was the intended use case when it was originally created?

The message essentially being conveyed here is "Okay there are already X number of ESLint warnings in this package but we won't allow any more", while the message being conveyed with the baseline feature seems to be a more granular version of this: "Okay there are already X number of ESLint errors/warnings for rule R in file F but we won't allow any more."

Anyway, it's just a similarity I see, it's not a big deal.

@kevin-y-ang the baseline doesn't take warnings into consideration or alter warnings in any way. So the message conveyed is ""Okay there are already X number of ESLint errors for rule R in file F but we won't allow any more.""

[nzakas]

The same command can be used afterwards when new errors are added in .eslintrc, but also to remove any unmatched rules.

Right, and this was my concern -- it feels like --baseline is doing a lot that isn't apparent to users. Specifically, I just don't think the word "baseline" correctly indicates any of what it's doing vs. explicit flags.

To clear unmatched rules you need to execute eslint suppress prune - which means that we will need one more extra flag i.e. --prune-suppressions if we decide to go down that path.

I think this is preferable because it clearly indicates what is happening.

[softius]

@fasttime @nzakas I updated the terminology and concept from baseline to suppressions. The new options are now documented for the ESLint CLI. I also separated the execution details from the CLI options for clarity. Implementation notes have been updated too.

Having a closer look at the source code, I have noticed that it messages suppressed with eslint-disable. Should we handle messages suppressed by this RFC the same way ?

[softius]

Minor points addressed.

[nzakas]

@eslint/eslint-tsc looking for another approval on this.

[mdjermanovic]

Having a closer look at the source code, I have noticed that it messages suppressed with eslint-disable. Should we handle messages suppressed by this RFC the same way ?

That's a good question. I think it makes sense to insert lint messages suppressed by this feature into LintResult#suppressedMessages before passing lint results to the formatter. @nzakas @fasttime thoughts?

[nzakas]

I think it makes sense to insert lint messages suppressed by this feature into LintResult#suppressedMessages before passing lint results to the formatter

I agree. Makes sense. :+1:

[fasttime]

I think it makes sense to insert lint messages suppressed by this feature into LintResult#suppressedMessages before passing lint results to the formatter.

Makes sense. I think that errors suppressed by the suppressions file will have no suppressions array attached to them, so tools should be able to tell them apart from errors suppressed per eslint-disable directive because of that.

[nzakas]

@softius I think all that's left is to update the RFC to mention suppressedMessages (see previous three comments). Can you take a look?