Configurable severity per rule

[chao2zhang]

This topic was previously discussed in #3129 and #533, but I would like to make a proposal with new information.

Some well-known static analysis tools allow the users to configure severity on rule or ruleset:

• Checkstyle allows configuring the severity of each rule to ERROR, WARNING, or IGNORE
• Android Lint allows configuring the severity of each rule to Fatal, Error, Warning, Information, Ignore

This configurable severity level allows the tool itself to control the output and the exit code of the process. In addition, it also cooperates with other tools:

• Github checks may use SARIF output, which has four levels: error, warning, note, none. For example, Error means blocking the PR from being merged. Warning is informational.
• Danger as mentioned in Override existing rule severity #3129, also defines behavior based on the severity level
• As mentioned in Severity level for rules #533, we also want to control the output as well as the result for detekt based on severity level
• At my company, we also have internal tooling to treat these severity levels
  • For example, ERROR means a PR cannot introduce any new violations with ERROR
  • WARNING means that a PR cannot increase the total number of WARNING in the codebase unless it is approved by a specific group of people

Now to support all the usecases, we need to be able to configure the severity per rule in the XML output as well as SARIF output. So far, the only configurable option I can see is config.warningsAsErrors, which is all or nothing. And the severity mapping for XmlOutputReport and SarifOutputReport are not flexible enough.

In my opinion, this feature should be implemented before we close #533 and #3045

[chao2zhang]

Note: I am willing to contribute PRs for this feature request, #3045 and #533. But I would like to get a general consensus before moving on.

[schalkms]

Thanks for submitting the proposal and willing to contribute this feature.
The proposal is fine for me, although I have one question. On which part exactly do you need a consensus?

But I would like to get a general consensus before moving on.

[chao2zhang]

It sounds like there is no concern, so we are already at a consensus.

I think the weakest point is probably In my opinion, this feature should be implemented before we close #533 and #3045

[schalkms]

I'm fine with adding a config option here.

detekt/detekt-core/src/main/resources/default-detekt-config.yml

Lines 10 to 14 in fc5c993

	config:
	validation: true
	warningsAsErrors: false
	# when writing own rules with new properties, exclude the property path e.g.: 'my_rule_set,.*>.*>[my_property]'
	excludes: ''

[chao2zhang]

To support configurable severity per rule:

• One idea is a new config option for each rule group and each rule.
• Another idea is to combine the severity level with active: So severity: ignore is equal to active:false

To implement #533, yes we can add a config option as well as a cli argument

detekt/detekt-core/src/main/resources/default-detekt-config.yml

Lines 10 to 14 in fc5c993

	config:
	validation: true
	warningsAsErrors: false
	# when writing own rules with new properties, exclude the property path e.g.: 'my_rule_set,.*>.*>[my_property]'
	excludes: ''

[schalkms]

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

[BraisGabin]

I agree about adding a config for each rule. But, what should be the default severity?

I think that all of them should be consider errors and then give the users the option to low the severity if they want.

About the severity I think that 4 levels are enough: error, warning, info, ignore (I don't care that much of the names them selfs, but the idea)

And what do we do if we have an active rule with ignore severity? Should we treat severity ignore as active false?

[schalkms]

But, what should be the default severity?

There's already a default severity configured for each rule. At the moment it's hardcoded into the rule implementation. For the first version using the default value for each severity is fine.

[chao2zhang]

In my opinion,

• Error = Failed the gradle task / cli command
• Warning = Count it in the total number of found issues, but the gradle task / cli command should succeed
• Info = Print out in the console report / XML / HTML, but do not report in the total number of found issues.
• Ignore = This should be equivalent to active: false

[cortinico]

I generally agree on the feature. Thanks @chao2zhang for tackling this.

Warning = Count it in the total number of found issues, but the gradle task / cli command should succeed

I would like to challenge this statement. We currently support a maxIssues: N field in the config file.

When N is 0, that's equivalent to having all the rules with severity: error.
When N is >0, Detekt reports a failure if the total number of findings is above maxIssues.

So ideally we should have those 4 levels:

• Error = Failed the gradle task / cli command as soon as one is reported
• Warning = Count it in the total number of found issues, but the gradle task / cli will fail if the #of findings is above maxIssues <- That would be the default level.
• Info = Print out in the console report / XML / HTML, but do not report in the total number of found issues (will not count towards the maxIssues threshold)
• Ignore = This should be equivalent to active: false

Moreover, we also have a warningAsErrors in the config file:

detekt/detekt-core/src/main/resources/default-detekt-config.yml

Lines 10 to 12 in 2e89bd4

	config:
	validation: true
	warningsAsErrors: false

That is used only for config file validation. If we introduce configurable severity, I suppose users will be confused by this warningAsErrors here.

[schalkms]

I guess warningAsErrors shall be removed, after this new feature has been introduced. It does not provide value anymore.

[chao2zhang]

In general, this feature can be implemented as:

• Backwards compatible: We can keep active, warningsAsErrors, maxIssues`, but that may end up some confusion
• Backwards incompatible: Remove active, warningsAsErrors and maxIssues

I will probably publish a PR soon for backward-incompatible change, and we can later evaluate whether it will be worth breaking the compatibility.

[cortinico]

I will probably publish a PR soon for backward-incompatible change, and we can later evaluate whether it will be worth breaking the compatibility.

Agree. I'll be for having this as backward incompatible for Detekt 2.x

I think we should also consider behavioral compatibility. Detekt users expect to work with the maxIssues threshold. We should discuss if we plan to remove that behavior entirely (if so we should properly document it) or not.

[arturbosch]

As @cortinico rightly pointed out config>warningsAsErrors is used for config validation.
If we deprecate config properties, we print them as warnings. Users wanted a way to fail the build so they migrate their properties asap.
So this feature is independent of the severity concept.

I think I was inspired by sonarqube when choosing the current severity values. Other tools which @chao2zhang mentioned above use a much simpler "severity scale".
For now we only use severity for the xml report. We map our values to checkstyle values.

We might want to deprecate our severity scale in favor of a simpler one in a future major release.
For this and related linked issues I propose to introduce a new field mappedSeverity of type string which defaults to warning.
This value can be changed via the yaml configuration:

MagicNumber:
  severity: 'info'

We can use this new field for the output reports. A Finding decorator can overwrite this field in the report function.
This would basically be the same "hack" like we do with aliases and suppressions with ruleSetId.

I will test my implementation idea update this comment later.

Edit:
related #2272

[BraisGabin]

I really like the idea of a String to give maximum flexibility. This the users can add a post-processor and change them to their will.

I'm thinking: can a rule raise two issues with different severities? Does it have sense? I think that the api should allow that.

[schalkms]

I like the string configuration example from above.

I'm thinking: can a rule raise two issues with different severities? Does it have sense? I think that the api should allow that.

Can you name some current rules in the detekt rule set, where this would actually make sense?
I don't think that this should be implemented at first. It makes the implementation more complex without adding too much value, in my humble opinion.