allRules = true has no effect when buildUponDefaultConfig = true

[TWiStErRob]

Expected Behavior

One of:

• allRules enables all rules on top of using the default config (which seems to be the code (AllRulesConfig) anyway.
• A build failure is triggered because the CLI throws an exception that conflicting flags were passed in.

Observed Behavior

When configuring DetektExtension

buildUponDefaultConfig = true
allRules = true

bahaves the same as

buildUponDefaultConfig = true

Steps to Reproduce

Will produce if necessary.

Context

I want to go all in, making a reusable strictest configuration I can make, by enabling all checks, but keeping it open that any new Detekt improvements are automatically added.

Your Environment

• Version of detekt used: 1.20.0
• Version of Gradle used (if applicable): 7.4.2
• Gradle scan link (add --scan option when running the gradle task): N/A
• Operating System and version: Windows 10
• Link to your project (if it's a public repository): N/A

[github-actions]

This issue is stale because it has been open 90 days with no activity. Please comment or this will be closed in 7 days.

[BraisGabin]

I was looking at this issue. And I found tests that asserts exactly the behaviour that you describes. If I "fix" this issue I would break those tests. I agree that the behaviour is strange but I don't want to expend more time on this.

If anyone want to go deep in the rabbit hole and find out why we have that behaviour I would happy to read the findings. The configuration of detekt is something that should be rewritten for 2.0.

My findings:

• The test should override active when specified from WorkaroundConfigurationKtSpec defines the curren behaviour.
• An empty config yml and provide no config at all have different behaviours 🤷

My two cents: We should probably remove allRules. #5089 helps out to keep your configuration up to date with the last updates of detekt and you can always enable all the rules there. And it gives you more flexibility to disable rules that you don't like.

[github-actions]

This issue is stale because it has been open 90 days with no activity. Please comment or this will be closed in 7 days.

[mustafaozhan]

Interested +1

[TWiStErRob]

Which of the two expected behaviours would you expect, @mustafaozhan (and any other watchers, if any)?

[mustafaozhan]

My understanding should be as below:

• buildUponDefaultConfig -> Enables all the default detekt configurations.
• allRules -> Enables the experimental, upcoming new rules.

So either of the flags shouldn't override the other one and if a user want's to all in, then they have to enable both.

So when

buildUponDefaultConfig = true
allRules = true

Are set, I expect to use default configuration for stable checks, together with experimental checks.

Let me know if I misunderstood, I just subscribed to this issue because of the title :)

[cortinico]

allRules enables all rules on top of using the default config (which seems to be the code (AllRulesConfig) anyway.

IMHO that should be the preferred behaviour.
This is effectively a bug and should be fixed.

allRules -> Enables the experimental, upcoming new rules.

@mustafaozhan There are no 'experimental' rules though. The allRules should just result in you enabling all the rules that are ever discovered by detekt. buildUponDefaultConfig will instead bring all the custom config keys that are in the default config.

Also, I believe that the semantic of:

buildUponDefaultConfig = true
allRules = true

Should result in having allRules being applied with a higher priority. In the sense that even if a rule is disabled in the default config, it should be enabled. We should probably fire a warning in this case maybe? 🤔

[TWiStErRob]

Agree with @mustafaozhan, he summed it up very nicely. No warnings necessary, because it's the wanted behaviour. I expected point 1 in OP too. allRules = "overlay a config where everything is active: true", and as you said this one should be applied last. The behaviour change should be mentioned in release notes, because some people might get a lot more enabled rules all of a sudden, and that shouldn't be a suprise.

[mustafaozhan]

Yea I think I got confused from the Android Studio plugin, I remember in the past there was experimental term in the config.

Anyway, I think none of the flag really should affect the other one (if we can do...).

What I understand from your comment is
allRules > buildUponDefaultConfig so If I set allRules then buildUponDefaultConfig will be already set, so no need to set buildUponDefaultConfig again, then it seems warning will be useful.

[TWiStErRob]

allRules could imply buildUponDefaultConfig by default, but I would imagine that

allRules = true
buildUponDefaultConfig = false

may be also valid setup, albeit a bit confusing since defaults are not used, but that's what the configurator asked for.

---

If any combination of the flags could be valid, and even though one implies the other (by default if not explicitly set (impl: convention property value)). Setting both to true would just make reading the Gradle configuration easier, if the author chooses to be explicit. Setting both doesn't change the behaviour and doesn't cause any problems, so there is no need for a warning.

[github-actions]

This issue is stale because it has been open 90 days with no activity. Please comment or this will be closed in 7 days.

[TWiStErRob]

What's the next step here?

Is this a candidate for https://github.com/detekt/detekt/milestone/42?
or Can we do it in 1.x?
or This is a won't fix, because ...?

[BraisGabin]

This should be doable on 1.x. To me it doesn't have a high priority but it's up to grab by anyone.