Revisit concept of Severity

[marschwar]

Context

It has been stated multiple times, that the detekt severity model is to complicated. The upcoming 2.0 is a chance to rework this concept without necessarily keeping backward compatibility.

Proposal

More information of what has already been discussed can be found in:

• Configurable severity per rule #3274
• Support configurable severity per ruleset/rule in XML and Sarif output #3310

The following list tries to consolidate what seems to be consensus as well as make a proposal for the implementation.

• The current values of the severity enum is replaced with the severity levels error, warning and info.
• The default severity of an all rules is error.
• The severity of a rule instance must be configurable via configuration file.
• It should be possible to configure to the severity at rule set level to apply it to all rules of the rule set (unless overwritten at rule level)
• The severity will be moved from the Issue to the Finding as it depends on a configurable rule instance.
• The concept of maxIssues will be removed. The build fails if (and only if) there is at least one issue with the severity error.
• It must be possible to reduce the level that has the build fail. Something similar to warningsAsError but also allowing the threshold to be lowered to info.
• The concept of weighted issues (build > weights) will be removed.
• The SeverityLevel will be removed.

Ideas for additional changes

The configuration option active will be removed

• There is an additional severity ignore
• Inactive rules by default have the severity 'ignored` and are not executed.

Configuration Example

style:
  severity: error
  MagicNumber:
    severity: info

[marschwar]

As mentioned above, this is just a proposal. I assume some of the points will be controversial.

Here is some of my reasoning:

The current valus of the severity enum is replaced with the severity levels error, warning, info and ignore.

There was the idea of allowing any string as severity. I do not see much benefit in that.

The configuration option active will be removed. active: false -> severity: ignore.

IMO it would be inconsistent to have both unless we actually want to run the rule and still apply the ignore label to it so post processors are able to process it. But I do not see the point in doing so.

The concept of maxIssues will be removed. The build fails if (and only if) there is at least one issue with the severity error.
The concept of weighted issues (build > weights) will be removed.

I think this will make the interface much cleaner. What is the use case for allowing five issues? Once there are four issues in the code base, every additional issue becomes a blocker.

[BraisGabin]

Thanks for this recap and the proposal. It is really useful.

I like your proposal. Every point looks reasonable to me.

Open questions:

• Right now we decide if a rule is enabled or disabled by default. Should we now decide between those 4 levels or just error/ignore?
• What happen with the baseline? Should we add the findings of all the levels to it? Just errors? Should we allow the user to decide?

[marschwar]

Right now we decide if a rule is enabled or disabled by default. Should we now decide between those 4 levels or just error/ignore?

I am not sure that I understand your question. But let me expand a little on the active/inactive part.

During default config file generation we set the severity to error or ignored depending of the presence/absence of the @ActiveByDefault annotation. The rule itself would no longer set a default severity. During rule execution we check the configured severity and run the rule unless it is set to ignored.

What happen with the baseline? Should we add the findings of all the levels to it? Just errors? Should we allow the user to decide?

That is actually a really good question. Generally I would add all findings to the baseline. Can you think of a use case where one would only want issues of a specific severity in the baseline?

[BraisGabin]

First point solved.

About baselines: their use case is to be able to introduce detekt on projects with issues because, otherwise, you would need to fix all of them before you add it.

If the issues are warnings you don't need to ignore them. They will not fail your compilation, so there is no need for them to be there. That's the reason.

But other tools as everything to the baseline, so maybe we should do the same.

[cortinico]

Should we add the findings of all the levels to it? Just errors? Should we allow the user to decide?

My 2 c: we should add both warnings and errors there. Whatever is in the baseline should not be exposed to the end user.

If the issues are warnings you don't need to ignore them. They will not fail your compilation, so there is no need for them to be there. That's the reason.

I actually believe there is value in having them there, as if you introduce detekt in a big codebase, you might have thousands of warnings polluting your inspection output. Having them in the baseline allows you to organically clean things up

[marschwar]

The default severity of an active rule is error.

Looking into this some more, I realized, that for the sarif report the default severity of warning is assumed. Since I would like to reuse the already supported severity property, this would change the sarif reports. Do you thing this would be an issue @chao2zhang ?

[chao2zhang]

I don't think this is an issue, as the severity is optional and seem to be used for display purpose only.

I noticed that our definition of severity needs to be mapped to Sarif severity:

• "warning": The rule specified by ruleId was evaluated and a problem was found.
• "error": The rule specified by ruleId was evaluated and a serious problem was found.
• "note": The rule specified by ruleId was evaluated and a minor problem or an opportunity to improve the code was found.
• "none": The concept of “severity” does not apply to this result because the kind property (§3.27.9) has a value other than "fail".

[schalkms]

My opinion is pretty much unchanged, as I wrote in another issue.

I would start small with just the first idea leaving out more advanced and complex options.

Each approach is better than the current severity feature.
Hence, I'm fine with either approach, but I wouldn't touch the active property on rules.

[marschwar]

Thank you for your feedback.

I think it would be a good start to leave the active feature as it is in the first iteration and see how that feels. I am afraid that mixing severity: ignore and active: true could become confusing. But maybe it allows us to differentiate which rules to run and which to report.

[BraisGabin]

I think that remove active will be a win. But I also agree that it could be done in another iteration.

[chao2zhang]

The default severity of an inactive rule is ignore.

If the inactive rule is equivalent to an ignore severity, does it still makes sense to have the ignore severity as the rule won't be run in the execution path?

[marschwar]

No, that assumes that we remove the active flag. I will update the proposal.

[BraisGabin]

I know that nearly all the static tools have this feature and that some users asked for it. But, did it provide enough value for the complexity that it adds to the project and to the configuration for our users.

Something is an issue or it is not. I don't see on which use cases you want warnings instead of errors.

If you need time to integrate a rule: use the baseline.

[jbarr21]

I like this approach of allowing detekt to report issues as Kotlin errors. We use Buck and currently only errors are shown. Also when using the detekt kotlin compiler plugin, we'd like to see issues reported as Kotlin errors so the build can fail early and provide faster feedback to devs (like errorprone) instead of needing to run an additional task to check for issues.

One thing that I'd like to see considered is an ability to baseline new detectors when using the detekt kotlin compiler plugin. We should be able to temporarily not fail the build & keep compiling, but still collect all of the issues that should be errors during compilation and output them to a file somewhere so that we can baseline new rules more easily instead of having to keep building, updating baseline, and repeating until successful compiliation.

[BraisGabin]

The idea of the baseline is to have the technical debt that you have already on your project. It is not meant to be changed often. Which is your use case so you need to be adding/removing things from there?

[jbarr21]

We write a lot of custom rules that usually have existing tech debt in our monorepo. Also detekt updates sometimes add new detectors or fixing existing ones that bring up new errors. Being able to baseline these additions is very important to us