Allow ForbiddenSuppress to disallow suppressing ForbiddenSuppress

[mgroth0]

According to the docs and PR, ForbiddenSuppress cannot disallow supression of itself.

Expected Behavior

This should be allowed, because otherwise there is no way to strictly enforce a rule.

Current Behavior

There is no way to strictly enforce a rule.

Context

If ForbiddenSuppress cannot disallow supressing itself, then as the PR says this rule does not really forbid any suppression, it just discourages it.

I do not know if this limitation is by design, because of a technical limitation, or both.

If it is only by design, I would argue that the developer should be allowed to strictly enforce a rule in their project by forbidding suppression of it completely. Its a decision for the developer, not for this library to make.

If it is a technical limitation, then I would propose that a workaround is implemented. One idea for a workaround, if required, is that this could be a special option for the detekt engine itself, and not considered a regular rule. That way, the suppresion mechaism doesn't take precedence.

One idea for how it could look in the yaml is this:

build:
    forbidSuppressingForbiddenSuppression: true

I personally find the tongue twister fun, but it could of course be replaced by something simpler such as forbidSuppressionsStrictly.

The point is that if there is a technical limitation, a special configuration option could be added that is not technically part of any rule, but is an option on detekt itself. This special option would report issues just like a normal detekt rule, but it would not itself be suppressible. It would still be easy to disable by setting forbidSuppressingForbiddenSuppression: false, but this would be up to the developer. While it might sound radical to bake a issue-reporter into detekt itself without being in the form of a rule, I think this would be the single case where it might be justified.

[cortinico]

I do not know if this limitation is by design, because of a technical limitation, or both.

I believe this was due to a technical limitation.
We would have to add an exception inside the Detekt's core suppression logic to ignore ForbiddenSuppress suppressions at all

[BraisGabin]

I see two options to implement this:

1. Add the option on the configuration to make a rule "no-suppressable" so it will skip the @Suppress() step check if it is enabled.
2. Nico's idea: hardcode inside the core of detekt that ForbiddenSuppress should skip that step.

I vote for the first option. With the current state of core this should be kind of easy to accomplish. All the logic that needs to change should be in the Analyzer.

Once we have that on the core then there is still a task to finish this issue: change the default configuration to make ForbiddenSuppress no-suppresable by default.

@mgroth0 do you want to give it a try?

[mgroth0]

@BraisGabin Both of your implentation ideas seem good to me.

To clarify, for the first option would "no-suppressable" be an option on all rules, or just on the ForbiddenSuppress rule? I'm slightly confused by what you suggested, only because I am having a thought that if "no-suppressable" were added as an option to all rules, wouldn't that mean we should just get rid of ForbiddenSuppress entirely since it is redundant? Or is the idea that ForbiddenSuppress still has a use case here because it could forbid suppressions of things outside of detekt?

Once we have clarification and a consensus on how to do it, I can give it a shot.

[mgroth0]

Another thought I am having is that if we have both ForbiddenSuppress as well as a "no-suppressable" on all rules, this creates a scenario where a rule suppression can be forbidden in two different ways. I am wondering if the redundancy there could be undesirable

[mgroth0]

We could make "no-suppressable" an option only on ForbiddenSuppress?

[BraisGabin]

I just realised that what I think it was already on core it is not. So we should wait for #7101 to be merged first.

I about the no suppress feature I was thinking about adding it to all the rules. I didn't realise that I was just deprecating the rule itself.

If we go in that direction, and I like it, then we have another problem. If you want to forbid all the to suppress all the rules you need to make it explicit for every single rule. And if a rule is added to detekt you need to remember to add it for that new rule too. That doesn't seem right.

The other option is to forbid the suppress by default on all the rules and if someone wants to suppress it then it needs to configure it to allow that. Because we are working on 2.0 that's a breaking change that we could assume.

I like this last option for how I use detekt but I'm not sure if that's a good default value for the new users of detekt.

The next option is to have the default value as a configuration and then, if a rule defines it the default value would be overwritten. The problem of this las version is that it increase complexity because we need also a way to configure the default value.

[mgroth0]

The other option is to forbid the suppress by default on all the rules and if someone wants to suppress it then it needs to configure it to allow that. Because we are working on 2.0 that's a breaking change that we could assume.

I like this last option for how I use detekt but I'm not sure if that's a good default value for the new users of detekt.

@BraisGabin you and I use Detekt in the same way in this regard. I didn't know this was an option, but personally it makes the most sense to me. The valuable principle here I think is that strictness should be the default; it should of course be allowed to suppress but that would need to be explictly allowed first in the configuration file. This would be very valuable because then you can quickly scan a configuration file and instantly know which rules are applied everywhere and which are sometimes suppressed. In the alternative case, where "no-suppression" is off by default, it is less clear in the configuration file if a rule is being applied strictly or not. That is because likely many people will want to keep their configuration files as short as possible, so they won't even add "no-suppression" to the configuration file, even if they never do in fact suppress the rule.

Hope that all made sense. In any case, I can also see why some might argue that they want suppression to be enabled by default so it is easier to do.

The next option is to have the default value as a configuration and then, if a rule defines it the default value would be overwritten. The problem of this las version is that it increase complexity because we need also a way to configure the default value.

I like this too. Personally I don't need it if the above option is possible, but maybe this is more accommodating.

Here are the possible implentations we have discussed, sorted by my personal preference

1. (most preferred) disallow suppression globally by default, including ForbiddenSuppress
2. make the global default itself configurable
3. Require explicitly disallowing suppression for each rule

In any case, we still need to be able to forbiden suppression of ForbiddenSuppress itself, which is what this issue is.

[BraisGabin]

TL;DR: we were talking about adding an option on each rule to define if that rule can be suppressed or not.

I see 5 options:

1. Add a config like suppressable defaulted to true on all the rules. This will keep the current behavior of detekt but if you want to forbid suppress on all the rules you need to define it on every single rule.
2. Add a config like suppressable defaulted to false on all the rules. More strict. In general all the suppression is disabled and you need to enable that option for each rule that you want to allow it. I like this but probably is not a good idea for the new users of detekt.
3. Add a config like suppressable and then another config at detekt level to define its default value. This will allow you to run detekt in the default mode or in a more strict one. The cons of this is that it increases the complexity a bit more but this covers both uses cases.
4. Don't implement that. Just hardcode on core that this rule can't be suppressed. I dint like this one. Core shouldn't have this type of logic.
5. Close this issue as won't fix.

@detekt/maintainers I would like to have more opinions on this topic. I vote for option 3.

[TWiStErRob]

+1 for 3), however the complexity shouldn't increase as we already have an "inherited" setting: enabled, right? Can these two use the same mechanism? Could it be enough to define suppressible only on the ruleset and rule level, not fully globally?

[schalkms]

I vote for 5 or 4 if we really want this behavior implemented.

While I agree for 4 that this logic shouldn't be in core for architectural reasons, it isn't the end of the world as it's rather a small change with very limited impact.

[cortinico]

While I agree for 4 that this logic shouldn't be in core for architectural reasons, it isn't the end of the world as it's rather a small change with very limited impact.

Same. I initially mentioned this solution as we're practically speaking about a 1 if-then-else statement inside core. ForbiddenSuppress is a rather specific use case.

Users can then use the ForbiddenSuppress to forbid other suppresses (so implement the logic suggested in 1,2,3).