detekt / detekt

Static code analysis for Kotlin
https://detekt.dev
Apache License 2.0
6.3k stars 777 forks source link

New rule: Flag usages like `IntArray(10) { 0 }` in favour of `IntArray(10)` #7761

Open atulgpt opened 3 weeks ago

atulgpt commented 3 weeks ago

Expected Behavior of the rule

We should flag builders which takes lambda returning the same value default value when default initialisation of the data structure uses the same default value.

Non-compliant code

val a = IntArray(10) { 0 }
val c = FloatArray(10) { 0F }
val g = LongArray(10) { 0L }
val i = ShortArray(10) { 0.toShort() }
val j = BooleanArray(10) { false }
val k = IntArray(10) {
    println("Some side-effect")
    0
}
// other constructors like UInt, UShort etc

Compliant code

val b = IntArray(10)
val d = FloatArray(10)
val h = LongArray(10)
val j = ShortArray(10)

Context

Using lambda builder, the default array is created and then runs a for loop without actually changing the value.

val a = IntArray(10) { 0 }

is decompiled to

public final class IntArrayKt {
   @NotNull private static final int[] a;

   @NotNull public static final int[] getA() { return a; }

   static {
      int var0 = 0;
      int[] var1;
      for(var1 = new int[10]; var0 < 10; ++var0) {
         var1[var0] = 0;
      }
      a = var1;
   }
}

There can be a configuration option methods which lists(maybe strings??) down all the constructors methods with lambda which will be disallowed when w/o lambda method is available. methods can

while below code

val a = IntArray(10)

gets decompiled to

public final class IntArrayKt {
   @NotNull private static final int[] a = new int[10];

   @NotNull public static final int[] getA() { return a; }
}
BraisGabin commented 3 weeks ago

We need to include BooleanArray too. And I'm not sure about the one with side effects. If you want that behavior you should use onEach instead. That would make the code easier to read.

cortinico commented 3 weeks ago

When I first read this rule proposal, I thought I really liked it. I then realized that this rule could reduce readability. Especially for the BooleanArray scenario, you might want to be explicit in specifying which value should the array be initialized with.

atulgpt commented 3 weeks ago

We need to include BooleanArray too.

Yes, there are more array types of functions like UShort, UInt etc., I was planning to incorporate all these

I then realized that this rule could reduce readability.

That is true, but if you see the generated code there is a large difference between those so I think that justifies the slight reduction in the readability

Especially for the BooleanArray scenario, you might want to be explicit in specifying which value should the array be initialized with.

These can be configurable where by default we can keep all the constructors excluding the boolean one

And I'm not sure about the one with side effects.

@BraisGabin should this be configurable or should we straightway disallow it as from the code pov doing it separately in the forEach loop doesn't complicate the generated bytecode any further

BraisGabin commented 3 weeks ago

To me this is a rule for the performance rule set. Those rules, in general, have this trade of between performance and readability. So I think that we could have it.

About the side effects I regret my idea. We should have a rule to find side effects in lambdas that shouldn't have side effects. But that should be a different rule. This rule should accept both cases as valid.

atulgpt commented 2 weeks ago

Hi, @BraisGabin, @cortinico updated the original issue with the updated details. PTAL

BraisGabin commented 2 weeks ago

It looks good to me.