Enable rules by default for 1.21

[BraisGabin]

All the rules that we add to detekt start disable by default. That's a good default because the early-adopters can give us feedback about them and we don't create a lot of noise for those users that prefer more stability.

But from time to time we should promote those rules and enable them by default. An example of rule that we could enable by default:

• InvalidPackageDeclaration

Which other rules do you think we should enable by default?

Note: There are some rules that are "controversial" and they will never become enable by default.

[marschwar]

Thank you for taking the initiative. There are so many great rules that are not active by default right now. Here is the list of my candidates for 1.21. I am aware that some of them might not make the final cut.

Edit: Trying to keep track of the current state of discussion here.

• MethodOverloading
• InstanceOfCheckForException
• InvalidPackageDeclaration
• NoNameShadowing
• AvoidReferntialEquality
• ImplicitUnitReturnType - I assume this might be controversial and break some builds 😉
• MissingPackageDeclaration
• UnreachableCatchBlock
• I also like the idea of CanBeNonNullable but do not know how battle tested it is Too new
• ClassOrdering - would definitely break builds, but it enforces the official coding guidelines
• ExplicitItLambdaParameter
• RedundantHigherOrderMapUsage, UnnecessaryFilter and UseAnyOrNoneInsteadOfFind - Love the rules. Have we seen many false positives?
• UnusedImports
• UseArrayLiteralsInAnnotations
• UseCheckNotNull and UseRequireNotNull
• UseCheckOrError and UseRequire
• UseIsNullOrEmpty
• UseOrEmpty
• HasPlatformType

[cortinico]

Note: There are some rules that are "controversial" and they will never become enable by default.

IIRC we decided to annotate them but we never did so right?

[hfhbd]

I would like add HasPlatformType to the discussion as well.

[marschwar]

Note: There are some rules that are "controversial" and they will never become enable by default.

IIRC we decided to annotate them but we never did so right?

Correct, we never did. Should we at least create the annotation so We have the option to do so in the future?

[BraisGabin]

@marschwar thanks you so much for that list with all the links! It's so easy to follow it!

• MethodOverloading

I'm not 100% bought with this one... I know in general it's a bad idea to have too much overload methods and probably that you are not using the polymorphism correctly. But to be honest 6 seems like a really good threshold to me.

• ImplicitUnitReturnType - I assume this might be controversial and break some builds 😉

This one will break lots of builds. This patter is really common: @Test fun foo() = runBlockingTest {}

And I don't agree with this part of its description:

Changing the type of the expression accidentally, changes the functions return type.

That issue is not related with functions returning implicit Unit that's generic for any implicit return value.

Edit: Thinking about this one... maybe if we exclude test by default on this one it have more sense to enable it.

• I also like the idea of CanBeNonNullable but do not know how battle tested it is

I agree that it would be really nice but it was introduced in 1.20.0-RC1 so I'm not sure... we could talk about it once 1.20 is released.

• ClassOrdering - would definitely break builds, but it enforces the official coding guidelines

I think this one is controversial too. I for sure will disable this one in my projects.

• RedundantHigherOrderMapUsage, UnnecessaryFilter and UseAnyOrNoneInsteadOfFind - Love the rules. Have we seen many false positives?

+1! I have them enabled in my code and they work perfectly fine.

---

The other ones that I didn't comment here I agree 100% that we should enable them

[marschwar]

I agree with your concerns. I updated my comment above to keep track of the current state of the discussion. Hope that more users chime in as well.

[BraisGabin]

My own list (I'm not adding the onces that were mentioned already)

• LabeledExpression
• NamedArguments
• ReplaceSafeCallChainWithRun I'm not 100% bought with this one... In general I don't like to chain multiple safe calls because it's not clear what can be null but It's a bit too strict some times...
• All the rules from the rule set Coroutines. They are great
  • Maybe we could deprecate GlobalCoroutineUsage now that you need to annotate its usages with @OptIn(DelicateCoroutinesApi::class). How can we deprecate a rule? I think that's not a thing right now on detekt.
• NotImplementedDeclaration
• ObjectExtendsThrowable
• MatchingDeclarationName
• DoubleMutabilityForCollection
• IgnoredReturnValue
• LateinitUsage, this one could be really polemic on the android world but lateinit is the same as a null on java...
• MapGetWithNotNullAssertionOperator
• NullableToStringCall
• UnreachableCatchBlock
• UnusedUnaryOperator
• UselessPostfixExpression
• DestructuringDeclarationWithTooManyEntries
• ForbiddenVoid
• ObjectLiteralToLambda
• RedundantHigherOrderMapUsage
• I would love to enable UnnecessaryLet but I think that it's not ready yet...

And I would vote to disable UnsafeCallOnNullableType. I saw that it make a lot of devs to abuse from the safe call operator (?.) and to me that's even worst. I prefer a NPE than an unexpected behaviour that goes under the radar. But I suppose that this is very controversial so I'm ok maintaining the "status quo".

[hfhbd]

I am surprised NotImplementedDeclaration is not enabled by default :D

[marschwar]

I could get behind most of your suggestions, other than:

strongly oppose -> candidates for @Controversial?

• ReplaceSafeCallChainWithRun - it really depends on the use case if it improves the code or makes it harder to understand
• NotImplementedDeclaration - while trying to use continuous delivery it is helpful to be able commit stubs that are not yet used
• NullableToStringCall - never understood the rule

mildly oppose as that may cause many builds to break

• IgnoredReturnValue
• LateinitUsage

And I would vote to disable UnsafeCallOnNullableType.

To me this should stay active by default.

[BraisGabin]

I would like to give a more inside about two of those rules and why I think that they should be enabled:

NullableToStringCall: Usually, when you use toString() is to show that to the user in some way. But it's really strange to want to show "null" to the user. But kotlin is completely fine with this: null.toString(). What this rule does is to reduce that issue. I think that this one is important.

IgnoredReturnValue: I agree, it could fail a lot of buids. But what this rule is to "empower" the library authors to force the users to use theri library properly. By default we just flag the ignored values from functions that were explicitily annotated. I think that the false positives of this rule should be really low and it should find lots of real issues. But I'm fine with keeping it disabled until the next "bunch of enabled rules".

(I agree with you in the other ones and I crosslined them)

[marschwar]

I guess our use cases differ. I usually work on the server side of things and when ever I use the implicit toString in string Interpolation in a log message, I actually want to see the "null" string.

[BraisGabin]

Good point. I crosslined it from the list.

[BraisGabin]

I'm changing my mind about enabling this rules:

• UnusedImport: I think that we should keep it disable for now because of False positive on UnusedImport with const variable import #4689
• NamedArguments: I like this one but I think that it's controversial and it should raise a lot of issues.
• UnnecessaryLet: It's not ready.