Ktlint: `: Unit =` formatting

[JLLeitschuh]

If I have an interface like this in java:

public interface Plugin<T> {
    void apply(T target);
}

If I override this in kotlin:

class PluginImpl : Plugin<Cat> {
    override fun apply(target : Cat) : Unit =
        doSomething(target)

    private fun doSomething(target: Cat) : String {
        // Do some other complex logic...
        return "Cat"
    }
}

The compiler requires you to have the return type of Unit or it won't compile.

[shyiko]

@JLLeitschuh I believe you meant (something similar to)

class PluginImpl : Plugin<Cat> {
    override fun apply(target : Cat) : Unit =
        target.run { doSomething(target) }

    private fun doSomething(target: Cat) : String {
        // Do some other complex logic...
        return "Cat"
    }
}

(without target.run { ... } it won't compile due to Type Mismatch: Required Unit, Found String (at least using [email protected])).

Anyway, I'm actually surprised that

    override fun apply(target : Cat) : Unit =
        target.run { doSomething(target) }

compiles fine when

    override fun apply(target : Cat) : Unit =
        doSomething(target)

doesn't (return type of target.run { doSomething(target) } and doSomething(target) is the same after all = String).
Given this inconsistency I think it fair to say that we have one more reason for not allowing explicit : Unit return type (unless I'm missing something of course). In other words,

    override fun apply(target : Cat) : Unit =
        target.run { doSomething(target) }

should be changed to

    override fun apply(target : Cat) {
        target.run { doSomething(target) } // target.run { } is obviously not necessary here 
    }

(feel free to reopen if I missed something)

[JLLeitschuh]

You may be right.
The example that I have might be wrong.

Here's an example where I know the behaviour.
This compiles fine:

class KotlinScriptBasePlugin : Plugin<Project> {
    override fun apply(project: Project) : Unit =
        project.run {
            rootProject.apply<KotlinScriptRootPlugin>()
            task<PrintAccessors>("kotlinDslAccessorsReport")
        }
}

This does not compile:

class KotlinScriptBasePlugin : Plugin<Project> {
    override fun apply(project: Project) =
        project.run {
            rootProject.apply<KotlinScriptRootPlugin>()
            task<PrintAccessors>("kotlinDslAccessorsReport")
        }
}

This is under kotlin version 1.1.4-3

I can't re-open as you're an admin on the repo and closing the issue prevents me from doing so.

[shyiko]

My point is - it shouldn't compile, because

class KotlinScriptBasePlugin : Plugin<Project> {
    override fun apply(project: Project) : Unit =
        task<PrintAccessors>("kotlinDslAccessorsReport")
}

won't (see my previous comment).
Notice that return type of

project.run {
    rootProject.apply<KotlinScriptRootPlugin>()
    task<PrintAccessors>("kotlinDslAccessorsReport")
}

and

task<PrintAccessors>("kotlinDslAccessorsReport")

is the same (Task?).
To me it looks like something is wrong with type inference.
Replace : Unit with { ... } and you should be good to go.

[JLLeitschuh]

Huh... Interesting.
So this does not compile:

class KotlinScriptBasePlugin : Plugin<Project> {
    override fun apply(project: Project) : Unit =
        project.task<PrintAccessors>("kotlinDslAccessorsReport")
}

I think what the compiler is secretly doing is turning this:

project.run {
    rootProject.apply<KotlinScriptRootPlugin>()
    task<PrintAccessors>("kotlinDslAccessorsReport")
}

Into this:

project.run {
    rootProject.apply<KotlinScriptRootPlugin>()
    task<PrintAccessors>("kotlinDslAccessorsReport")
    return
}

I would guess that this is so that you can write small lines of code like that and not have to worry about adding a return statement at the end.

---

As it stands this does compile:

class KotlinScriptBasePlugin : Plugin<Project> {
    override fun apply(project: Project) : Unit =
        project.run {
            rootProject.apply<KotlinScriptRootPlugin>()
            task<PrintAccessors>("kotlinDslAccessorsReport")
        }
}

Any code formatter should have the behaviour of taking an input that compiles and returning an output that also compiles. This is not currently the behaviour of ktlint in this case.

[shyiko]

@JLLeitschuh fair enough

[shyiko]

So basically what we need to do here: make https://github.com/shyiko/ktlint/blob/master/ktlint-ruleset-standard/src/main/kotlin/com/github/shyiko/ktlint/ruleset/standard/NoUnitReturnRule.kt rewrite : Unit = to { ... } instead of simply dropping : Unit.

[JLLeitschuh]

I would argue that's not the correct solution. The correct solution in my mind would be to not remove the Unit return if the method if the method uses the = notation.

A little bit more explicitly:

• : Unit {} In this case : Unit should be removed
• : Unit = in this case : Unit should not be removed as doing so changes the return type of the method. If the Unit is removed in this case then the formatter is changing the method signature.

If we try to go down your route by adding { ... }then you have to get into the business of getting the indentation correct for code inside of that block. That seems like a path that is going to lead to nothing but pain and suffering.

[shyiko]

@JLLeitschuh agreed

[shyiko]

Fixed in 0.9.2 (should become available through Maven Central within the next ~25min).
Thank you, as usual :wink: