feat: Suppression support

[Yiwei-Ding]

Summary

Violations can be suppressed by inline directive comments. We propose to add an --output-suppression CLI option for adding suppression information, including its kind and justification, in the output of ESLint.

Related Issues

eslint/eslint#14784 Add suppression information in the output of ESLint

[nzakas]

Thanks, this is very helpful. I think this is a potentially useful feature. I left some comments on the areas where I think we need some clarification. We will also need some feedback from the rest of the team.

[nzakas]

Thanks. I left a few more comments after reviewing this a bit more.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Yiwei-Ding (c3677d5, a74b654, 8de3626, 376b9f6)

[ljharb]

i don't think the word "suppressed" or "suppression" is appropriate here. inline comments can also enable rules, or reconfigure them.

What happens with this CLI option when an inline comment causes an error to appear that wouldn't have otherwise done so?

[nzakas]

We are only tracking when a rule is disabled for this RFC. Enabling a rule with a directive doesn’t change how this behaves.

Is there a better word you can suggest?

[ljharb]

So If i reconfigure an enabled rule such that it provides fewer errors, that would not be caught by this arg, despite that being conceptually the same kind of "suppression" as disabling a rule entirely?

[nzakas]

Correct. We care only about rules that are completely turned off

[ljharb]

Then this isn't about warning suppression at all, it's just about disabled rules (which, incidentally, seems much much less useful than caring about "a comment that reduced the number of observed warnings).

Perhaps --track-inline-disabled-rules, since that's all it's doing?

[nzakas]

It’s also tracking rules disabled in configs, not just inline.

And really, it’s not tracking that the rule is disabled but rather the messages that would have been produced by a disabled rule. “Suppression” refers to the message being squashed rather than the rule being disabled.

[ljharb]

Good to know it’s not just inline - in that case it’s only about disabled rules - meaning, eslint somehow tracks which rules are configured to be on, with an “off” trampling it later?

--track-disabled-rules still seems like a better option to me in that case.

What about rules that would error but are set to warn instead, when --quiet is enabled?

[nzakas]

eslint somehow tracks which rules are configured to be on, with an “off” trampling it later?

You may want to read the rest of the RFC. 😄

I just don’t think “track disabled rules” is an accurate phrase for what this is doing.

[ljharb]

I have, altho it’s long and i may have missed something, but what i read implies that disabling a rule in a config file isn’t something eslint tracks. Is this rfc attempting to change that?

[Yiwei-Ding]

We might have to change the logic of skipping rules disabled in configuration files, as described in "Reserve external suppression" section of the RFC.

[aladdin-add]

As an alternative: you can create an eslint rule to report the usage of eslint-disable-*. eslint-plugin-eslint-comments sounds like a good place to have.

[Yiwei-Ding]

We'd like to track external suppressions as well. Reporting the usage of eslint-disable-* seems not all we want exactly.

[aladdin-add]

as external suppressions, you can just read the eslintrc, to find which rules were disabled:

config.rules.filter([rulename, ruleOptions] => ruleOptions === "off" || ruleOptions === 0);

[Yiwei-Ding]

I want to clarify that we are not going to track what rules are suppressed (or switched to 'off'). What we want is tracking violations (or warnings/errors) in the code. For example:

a = 1;
console.log('hello');
b = 2;

rules: {
  "no-undef": "error"
}

We'd like to get the suppression info of line 1 (a = 1) and line 3 (b = 2) instead of the message that the rule 'no-undef' is switched to 'off'. No errors or warnings would be output, but these suppressions are expected to be recorded. As described in the RFC, ESLint would not skip rules disabled in configuration files if --track-suppressions is applied.

[ljharb]

@Yiwei-Ding when what switches it to off? another config, or only an inline override comment?

[Yiwei-Ding]

For inline suppressions, we would keep the current logic basically. Violations (or problems in ESLint) disabled by inline directive comments are expected to be tracked but would not cause errors/warnings if '--track-suppressions'.

For external suppressions (where rules are switched to 'off' in configuration files), ESLint would not skip rules switched to 'off' if '--track-suppressions' is used. All rules would be ran and the generated problems belonging to disabled rules would be tagged 'Globally disabled'. In this RFC, when we talk about something is switched to 'off', we are talking about external suppressions (rules are switched to 'off' in configuration files).

if '--track-suppressions' is not used, we'd like to keep the current behavior and logic of ESLint.

Does it answer your questions?

[aladdin-add]

thanks, I see.

so, the alternative can be creating a new eslintrc based the original:

const base = require('./eslintrc.js');  // this is your original config

base.rules = find_disabled_rules(base);  // find global-disabled rules in the config, and set to "error"
base.rules['no-eslint-disable'] = "error"; // an eslint rule to report all the usage "eslint-disable-*"

modules.exports = base;

thus, you can run eslint -c to get all suppressions. does it match your use case?

[ljharb]

What happens when a rule is switched off (from a shared config) in a config file because it crashes eslint, or because it's very slow?

[Yiwei-Ding]

@aladdin-add I didn't get your idea..

Engineers prefer to suppress warnings or errors that would break CI rather than fix them. These suppressions are inevitable but we want to reduce them as much as possible to improve code quality. That is why we are going to track these suppressions.

So in this RFC, suppressions are expected to output from formatters (JSON, SARIF or any else) instead of causing warnings/errors to break pipelines. "no-eslint-disable": "error" would report inline directive comments as errors, right? That is not what we want.

[btmills]

I can also see this being useful. I can see how tracking inline comment directives will work, but I have some reservations about rules that are disabled via config files and CLI flags.

[ljharb]

How does this change solve your use case? A developer could still use an override to set a rule to “off”, but now track-suppressions won’t track it.

[Yiwei-Ding]

Thanks for all comments and suggestions! We decided to remove external suppression in this RFC. Will update the doc soon.

[ljharb]

With the latest changes, what does this give you that https://www.npmjs.com/package/eslint-plugin-eslint-comments does not?

[Yiwei-Ding]

@ljharb Thanks for the concern.

We are not going to analyze directive comments. Our goal is to track suppressions for auditing purposes. The action that directive comments work on violations is considered as suppressions.

If a rule is turned off in configuration files, the related violations (problems) will not be created, where the corresponding directive comments will not work as well. In this case, no suppressions happened and nothing should be considered as suppressions to be recorded.

[ljharb]

Right - but that eslint plugin demonstrates how you can track the occurrence of inline override comments, so can’t you assume that N comments means N suppressions? (there’s a chance a comment won’t be suppressing anything, but there’s already a config switch to show those warnings, so you can collect them too and subtract them out)

[Yiwei-Ding]

That's not what we expect. Only there's a violation, there could be a suppression. If there's not a violation, there would not be a suppression.

As described in the RFC, if multiple directive comments happen to work on the same line and the same rule, the output suppression info should be integrated into one suppression list. In this case, one violation is suppressed once by the list.

"messages":[
      {
        "ruleId":"no-undef",
        "severity":2,
        "message":"'a' is not defined.",
        "line":1,
        "column":1,
        "nodeType":"Identifier",
        "suppressions":[
          { "kind":"directive", "justification":"Sample justification message." },
          { "kind":"directive", "justification":"Another justification message." }
        ],
        "messageId":"undef",
        "endLine":1,
        "endColumn":2
      }
    ]

And I believe the new feature could be helpful to ESLint users :)

[mdjermanovic]

We currently don't have this info - only one directive applies, while the others are marked and reported as unused. I think this is an edge case that doesn't warrant the added complexity and confusion that would be caused by directives that are at the same time used (as their justifications appear here) and unused (as they're reported as such).

[Yiwei-Ding]

Yes, currently if a violation is suppressed by multiple directives, the first suppression will work while the rest of them will be treated as unusedDisableDirectives, as described in the section Suppression in ESLint.

What we propose to do is that when using the new option --track-suppressions, these suppressions for one violation could be gathered into a suppression list. Without the option, everything works as it is currently. This changes in code would be in function applyDirectives(options).

[mdjermanovic]

ESLint already marks redundant directives as "unused", and that's observable when the --report-unused-disable-directives flag is used. Furthermore, they cause exit code 1 if this flag is used.

What should be the behavior when both flags are used at the same time? It doesn't make sense to report a directive as unused and to include the same directive in the list of directives that have applied.

[Yiwei-Ding]

If --track-suppression used, directives that do suppress violations will be considered as "used". But those don't suppress violations will still be tagged as "unused".

Fox example:

/* eslint-disable eqeqeq -- Justification 1 */
// eslint-disable-next-line no-undef -- Justification 2
a=1; // eslint-disable-line no-undef -- Justification 3

In this code, // eslint-disable-next-line no-undef -- Justification 2 and // eslint-disable-line no-undef -- Justification 3 are used to suppress the violation "'a' is not defined". /* eslint-disable eqeqeq -- Justification 1 */ does nothing so it will be "unused" and be reported when --report-unused-disable-directives is used.

I'll update the RFC to clarify it if you are good to this design.

[mdjermanovic]

I think the purpose of this new flag should only be to provide more information.

Consider this example:

// eslint-disable-next-line no-undef -- Justification 1
a=1; // eslint-disable-line no-undef -- Justification 2

If we run eslint foo.js --report-unused-disable-directives, the outcome is exit code 1.

If we run eslint foo.js --report-unused-disable-directives --track-suppressions, the outcome would be exit code 0.

So, a flag used to output additional info about the linting has side-effects on linting: it removes an error (the error on unused disable directive) and alters the linting outcome.

[Yiwei-Ding]

Hi @mdjermanovic , we will keep the current behavior. In this example:

// eslint-disable-next-line no-undef -- Justification 1
a=1; // eslint-disable-line no-undef -- Justification 2

the unused-error should still be throw out if we run eslint foo.js --report-unused-disable-directives --track-suppressions.

And we'd like to gather both used and unused suppressions, i.e., we will get a suppression list with 2 items for the above code if we run eslint foo.js --track-suppressions:

"suppressions": [
  { "kind": "directive", "justification": "Justification 1" },
  { "kind": "directive", "justification": "Justification 2" }
]

[mdjermanovic]

// eslint-disable-next-line no-undef -- Justification 1
a=1; // eslint-disable-line no-undef -- Justification 2

the unused-error should still be throw out if we run eslint foo.js --report-unused-disable-directives --track-suppressions.

What would be in "suppressions": [] in this case?

[Yiwei-Ding]

Same as eslint foo.js --track-suppressions.

--report-unused-disable-directives will report the "unused" error, --track-suppressions will gather and output suppressions. They would be independent, right?

[mdjermanovic]

--report-unused-disable-directives will report the "unused" error, --track-suppressions will gather and output suppressions. They would be independent, right?

Yes, in that case the options would work independently of each other, which looks good.

What still looks confusing is that the same directive appears as both used and unused in the same output.

[Yiwei-Ding]

The directive is suppressing but is unused at the same time 😄

[mdjermanovic]

Can you provide more details on how it should be modified? Should it filter out suppressed problems from the results?

[Yiwei-Ding]

Yes, formatters should filter out those suppressed problems.

Take stylish as an example, messages passed into stylish could have a suppressions property when using --track-suppressions. Currently, stylish just gets the messages and displays them on CLI, regardless they are suppressed or not. We have to modify it to filter out suppressed ones.

[mdjermanovic]

Third-party formatters might become incompatible, depending on how we implement this feature.

[Yiwei-Ding]

Depending on how third-party formatters process LintMessage. Only using the new option --track-suppressions, there could be incompatible problems. Without the option, nothing will be affected.

[mdjermanovic]

If user runs eslint with --track-suppressions and an existing third-party formatter, it will show suppressed errors as if they were not suppressed. I think this is a compatibility issue we should address.

[Yiwei-Ding]

Maybe add a support formatter list for --track-suppressions and drive the owners of third-party formatters to support the suppressions property?

When using a formatter that --track-suppressions not supports, ESLint would throw a warning message like "The formatter is not supported by the option --track-suppressions" and keep working as the incompatible mode.

[mdjermanovic]

I'm not sure what would be the best approach.

Some thoughts:

• Instead of mixing suppressed and non-suppressed problems in messages, maybe we could add a new property suppressedMessages (array). In that case, an option might even be unnecessary? (we could just always provide them and it would be up to the formatter to merge and show all, or entirely ignore suppressedMessages as all the existing formatters are doing now).
• Adding an ability for formatters to define features they support, and throw an error if the given formatter doesn't support suppressions. If the formatter doesn't support supressions, that wouldn't mean that it is incompatible and that it needs to release a new version, just that it isn't designed to be used with --track-suppressions.

[Yiwei-Ding]

If I'm a JSON formatter user, I would feel strange that so many suppressed messages come to my output after updating ESLint... I suppressed so I don't want to see those violations.

Maybe an explicit option could give users a clear idea whether tracking suppressions is working.

[nzakas]

I think it’s okay to always return the suppressed messages. The JSON formatter is primarily used by automation to transform the results output into another format so it’s unlikely to cause problems.

[DSIE63]

So seems like we have an agreement here, which is we will not need to add the --track-suppressions switch and always output the suppressions to a separate property (suppressedMessages). Formatters that know about this new property can merge and handle the messages accordingly, and for formatters that don't then it is an NoOp.

Agreed?

If we all agree, we will move forward to making the code changes and submit PR for reviews when we are ready.

Thanks,

Daniel Sie

[Yiwei-Ding]

Hi @mdjermanovic @nzakas @DSIE63 , the RFC is updated and the option related things are removed now.

[nzakas]

@DSIE63 please wait until the RFC has been approved before submitting code.

[Yiwei-Ding]

It is nearly 90 days since the RFC created. Could we move forward?

[nzakas]

I’m happy with where we are with this RFC now. I’d like to suggest we move to the final commenting period.

[nzakas]

We can’t change these methods to return an object. That would be a significant breaking change to the API. The options are:

1. Return an array with an added suppressedMessages property.
2. Add a getSupressedMessages() method to Linter that can retrieve the suppressed messages from the previous run.

[ljharb]

Arrays with added properties are very weird; aside from regexp match objects, they rarely happen, and that's a good thing - out of those two options, the second seems far better (and also avoids penalizing the common case of not needing this info)

[nzakas]

I like it. 👍

[Yiwei-Ding]

Could we move to the Final Commenting period now?😊

[nzakas]

Moving to final commenting.

[btmills]

I’m happy with where this ended up!

[nzakas]

Approved! Thanks for everyone’s patience on this. I know it took a while but we ultimately ended up with what I think is a good solution.