VarCouldBeVal reported on private lateinit var

[rli]

Steps to Reproduce

class A {
    private lateinit var label: JLabel
}

Context

Per #4664 (comment), I'm not sure our use-case is mainstream enough to warrant a change in detekt, but I'd like to bring it up as a datapoint anyways in case there there is additional discussion in the future.

As part of my work I develop plugins for JetBrains IDEs.
We have a number of older Kotlin source files backing visual forms designed using the JetBrains GUI Designer. I'm fuzzy on all the details, but how this generally works is that there's instrumentation at compile time that injects constructors and all the necessary method calls needed to setup the Swing components correctly.

For example, the code snippet above would result in decompiled bytecode that might look like the following:

class A {
    private lateinit var label: JLabel

    private fun `$$$setupUI$$$`() {
        label = JLabel("some label text")
        label.setIcon(Icon("and maybe we have a /path/to/icon declared here too"))
    }

    init {
        `$$$setupUI$$$`() 
    }
}

Obviously since detekt acts on source code and not bytecode, it can't know that the fields actually end up initialized.

The VarCouldBeVal rule is still very useful for us, so as a workaround to this regression we'd likely either mark the ~60 or so instances in our codebase with an annotation or throw it in a baseline file and call it a day.

Your Environment

• Version of detekt used: 1.20.0
• Link to your project (if it's a public repository): https://github.com/aws/aws-toolkit-jetbrains

[geojakes]

I'm getting the same issue

[BraisGabin]

Do those classes have any type of difference from any other? An annotation, an interface, a name pattern... Something?

[vladimirfx]

Same issue in simplest test class:

internal class LoaderTest {

    private lateinit var wireMockServer: WireMockServer

    // test methods there
}

Type resolution enabled. Field initialized via reflection (not in init {} ). I think lateinit modifier should disable this rule.

[rli]

Do those classes have any type of difference from any other? An annotation, an interface, a name pattern... Something?

In my case, the only difference is the presence of a .form file with the same name of the class.

[BraisGabin]

I don't like the idea to just ignore lateinit because there could be false negatives. It seems a very broad suppression.

But if there is not other way to suppres those we could add a flag ignoreLateinitVar. false by default. Do you have the time to implement it?

[vladimirfx]

I don't like the idea to just ignore lateinit because there could be false negatives. It seems a very broad suppression.

Actually, I can't imagine such real false negatives:
There is no such thing as lateinit val (unfortunately). Lateinit by it mining - 'I can't initialize this now and do it somehow later'. 90% of our lateinit usages it is some kind of dependency injection and other 'magic'. So it was all hit by this false positive.
One potentially usefull case:

class C {
  private lateinit var a: String
    init {
        a = "1"
    }
}

But there the problem is not var usage but lateinit usage which is checked by the special rule LateinitUsage.

So because there is no lateinit val I can't see value of this rule for lateinit case. Anyway ignoreLateinitVar is also OK.

[ZacSweers]

I agree with @vladimirfx, I can't see any use case for lateinit var to be intelligently convertible to a val, its use is explicitly for cases where something can't reasonably be known as a val at compile-time. If anything, ignoreLateinitVar should be true by default.

[BraisGabin]

A lot of android devs use lateinit var on Activities to set properties at onCreate. Later they can refactor its code and remove the set. This rule should find that type of issues. I imagine that, in general, this rule finds issues related with refactors and no with new code.

If a project uses a library that forces you to use lateinit without setters it's a one time configuration. But, if you don't use such a library, you will never change that configuration to true, there are far too much configurations on detekt.

For that reason I think that false is safer as default value.

[ZacSweers]

The only time I ever see it used is with Dagger(-esque) member injection. What other cases have you seen?

Expecting use of a setter is a bit strange as then it must be nullable and changes semantics. Or they make get() possibly throw if it's not been set yet, and which point it's just reimplementing lateinit var

[BraisGabin]

The use cases that I usually see are like this:

private lateinit var itemId: String

fun onCreate(...) {
  itemId = intent.getExtra(...)
}

I agree that that usage of lateinit isn't good but that kind of things happend. I usually have LateinitUsage enabled because of this.

And, for this rule, I think that it's better to add ignoreAnnotated: ['Inject'] than enable ignoreLateinitVar. We added it because there are libraries that don't give you other option but I think that there are not that much.