Annotation wrapping before explicit constructor (ktlint_official)

[meyertime]

Expected Behavior

Our Android project contains many class declarations that look like this:

class Foo @Inject constructor(
    private val someDependency: SomeDependency,
    private val anotherDependency: AnotherDependency,
) : Bar {
    //...
}

In my opinion, this should pass linting using the ktlint_official style. The official Kotlin and Android style guides actually contain no examples of annotations on a constructor, so I don't think they even thought about it when writing the style guide. However, the Kotlin guide does say, "A single annotation without arguments may be placed on the same line as the corresponding declaration," and the Android guide says something similar. Therefore, the above example should pass.

Observed Behavior

Starting with ktlint v0.49.0, the above code produces the following linting errors:

Linting error > [standard:annotation] Expected newline before annotation
Linting error > [standard:annotation] Expected newline after last annotation

Further, an auto-correction of the above code looks like this:

class Foo
    @Inject
    constructor(
        private val someDependency: SomeDependency,
        private val anotherDependency: AnotherDependency,
    ) : Bar {
        //...
    }

This is really ugly and hard to read. Linting should make our code better, not worse. The issue that introduced this feature suggests the following in the case of a short parameter list:

class Foo
    @Inject
    constructor(private val someDependency: SomeDependency): Bar {
    //...
}

This is even worse. The indentation obscures where the opening brace is.

Steps to Reproduce

Run ktlint v0.49.0 or newer against the above code example with the ktlint_official style.

Your Environment

• Version of ktlint used: 0.49.1
• ktlint_official
• Name and version (or code for custom task) of integration used (Gradle plugin, Maven plugin, command line, custom Gradle task): org.jmailen.kotlinter version 3.15.0
• Version of Gradle used (if applicable): 8.2.1
• Operating System and version: Arch Linux

[paul-dingemans]

In my opinion, this should pass linting using the ktlint_official style. The official Kotlin and Android style guides actually contain no examples of annotations on a constructor, so I don't think they even thought about it when writing the style guide. However, the Kotlin guide does say, "A single annotation without arguments may be placed on the same line as the corresponding declaration," and the Android guide says something similar. Therefore, the above example should pass.

ktlint_official aims to produce more consistent code than the current code styles. Annotations with arguments are for example always wrapped, regardless on which construct they are used. To avoid inconsistencies between constructors with an annotation not having parameters, and constructors with an annotation having parameters, I have decided to wrap explicit constructors preceded by an annotation or modifier.

class Foo
    @Inject
    internal constructor()

and

data class Foo
    @Bar1
    @Bar2("bar")
    @Bar3
    @Bar4
    constructor(private val foobar: Int) {
        fun foo(): String = "foo"
    }

This is really ugly and hard to read. Linting should make our code better, not worse.

So, indeed the code looks quite different from before and it might need time to get used to it. Qualifiers like 'ugly', 'hard', 'better' and 'worse' are subjective and always related to what you are used to see. Also, I am still getting used to this new format.

class Foo
    @Inject
    constructor(private val someDependency: SomeDependency): Bar {
    //...
}

This example, I have been doubting about. In some other context, the Kotlin style guide suggest two possibilties:

Option 1:

For classes with a long supertype list, put a line break after the colon and align all supertype names horizontally:

class MyFavouriteVeryLongClassHolder :
    MyLongHolder<MyFavouriteVeryLongClass>(),
    SomeOtherInterface,
    AndAnotherOne {

    fun foo() { /*...*/ }
}

or

Option 2:

To clearly separate the class header and body when the class header is long, either put a blank line following the class header (as in the example above), or put the opening curly brace on a separate line:

class MyFavouriteVeryLongClassHolder :
    MyLongHolder<MyFavouriteVeryLongClass>(),
    SomeOtherInterface,
    AndAnotherOne
{
    fun foo() { /*...*/ }
}

I have chosen the first option. Due to the rule that removes blank lines at the start of a class the result becomes:

class MyFavouriteVeryLongClassHolder :
    MyLongHolder<MyFavouriteVeryLongClass>(),
    SomeOtherInterface,
    AndAnotherOne {
    fun foo() { /*...*/ }
}

Allowing a blank line in at start of class above would not be consistent with that rule.

I am not inclined to change the formatting at this moment. I will leave the issue open for a while to collect other opinions.

[lyallcooper]

Also coming from a project that heavily uses @Inject-annotated constructors, I agree with @meyertime that this new formatting feels like a regression. While it's definitely hard to ignore the "I don't like change!" aspect of a something like this, I do think multi-line formatting decreases readability here, apart from just being unfamiliar, so I'll try to justify that.

Taking the original example:

class Foo @Inject constructor(
    private val someDependency: SomeDependency,
    private val anotherDependency: AnotherDependency,
) : Bar {
    //...
}

In terms of token saliency when reading, I would argue class, Foo, someDependency: SomeDependency, anotherDependency: AnotherDependency and Bar are top, while @Inject, constructor, private and val are all lower.

Looking at the post-formatted example, I would argue that @Inject and constructor specifically are given undue prominence. The two extra lines used also means the eye has to jump further to get to the next important pieces of information after the class name.

class Foo
    @Inject
    constructor(
        private val someDependency: SomeDependency,
        private val anotherDependency: AnotherDependency,
    ) : Bar {
        //...
    }

Finally, this approach also means the whole class contents are shifted over by an additional indent, which feels a little wasteful.

In our codebase, the ...@Bar1 @Bar2("bar") @Bar3 @Bar4 constructor... pattern of having a bunch of annotations on the constructor is very uncommon, while the above pattern of a single @Inject annotation is extremely common, so the readability/aesthetics vs. consistency tradeoff would not be worth it for us.

I would suggest maybe enforcing the wrapping maybe only if more than one annotation is present.

[paul-dingemans]

Also coming from a project that heavily uses @Inject-annotated constructors, I agree with @meyertime that this new formatting feels like a regression. While it's definitely hard to ignore the "I don't like change!" aspect of a something like this, I do think multi-line formatting decreases readability here, apart from just being unfamiliar, so I'll try to justify that.

...

In terms of token saliency when reading, I would argue class, Foo, someDependency: SomeDependency, anotherDependency: AnotherDependency and Bar are top, while @Inject, constructor, private and val are all lower.

I like your style of reasoning. Despite that, there are other arguments not to allow to make an exception for the case of a single annotation.

From a viewpoint of consistency, the rules which are applied on functions, are also applied on constructors. I know that constructors and functions are different beasts. But as with cars, they have both four wheels and some doors.

The case of the @Inject constructor can not be compared to all other possible types of annotations which might a bigger impact and therefore would require more attention. Some other examples, that I have considered are:

class SecondLevelBeanFactoryQuery @Autowired(required = false) constructor(internal val topLevelValue: String)

data class GraphQLBatchRequest @JsonCreator constructor(@get:JsonValue val requests: List<GraphQLRequest>)

public class SerializersModuleBuilder @PublishedApi internal constructor() : SerializersModuleCollector

public class JsonConfiguration @OptIn(ExperimentalSerializationApi::class) internal constructor(

I do agree that the identation feels awkward. But I would wrap, and keep IntelliJ IDEA formatting, the result would be:

class Foo
@Inject
constructor(
    private val someDependency: SomeDependency,
    private val anotherDependency: AnotherDependency,
) : Bar {
    //...
}

In this way the most important thing class Foo got lost. Especially when it would be preceded by another annotation.

[meyertime]

Qualifiers like 'ugly', 'hard', 'better' and 'worse' are subjective and always related to what you are used to see.

It's true that "ugly" is subjective, but my main point is that it is "hard to read", or in other words, "less readable", and that is not entirely subjective. "Better" and "worse" are also not entirely subjective in my opinion, as there are patterns that have proven to be more or less readable regardless of what people are used to.

Looking at the post-formatted example, I would argue that @Inject and constructor specifically are given undue prominence. The two extra lines used also means the eye has to jump further to get to the next important pieces of information after the class name.

Finally, this approach also means the whole class contents are shifted over by an additional indent, which feels a little wasteful.

Agreed! Thanks @lyallcooper for articulating this.

ktlint_official aims to produce more consistent code than the current code styles.

I understand the desire for consistency, but it is really a secondary concern. The primary concern is readability. Consistency generally improves readability, which is what makes consistency worthwhile. However, if something is done just to improve consistency and it actually reduces readability, then it defeats the purpose.

A machine will never be able to make the best decision for human readability in 100% of cases. There are always going to be some cases where a human will have to make a judgment call. The Kotlin guide says as much in the way it is worded. I previously quoted: "A single annotation without arguments may be placed on the same line as the corresponding declaration." Here it says "may", denoting that it is an option for the human programmer to decide, not a strict rule. I am suggesting that this be made an option in harmony with the Kotlin official guidance, not that it be enforced to be on the same line in that case.

From a viewpoint of consistency, the rules which are applied on functions, are also applied on constructors. I know that constructors and functions are different beasts. But as with cars, they have both four wheels and some doors.

Generally, yes, constructors are similar to functions. However, I could easily argue that Kotlin primary constructors are very different and have a much stronger link to the class definition, but I do not have to, because even in the case of a regular function, the Kotlin guide says that a single annotation without arguments can be placed on the same line as the function declaration, even offering this example:

@Test fun foo() { /*...*/ }

Further, the Android guide also states, "When only a single annotation without arguments is present, it may be placed on the same line as the declaration," and offers this example:

@Test fun selectAll() {
    // …
}

Obviously, there's a reason these guides agree and both saw fit to include this as an explicit option. There are definitely cases where it is most readable to put the single annotation without arguments on the same line, and my @Inject constructor case is one such example.

class Foo
@Inject
constructor(
    private val someDependency: SomeDependency,
    private val anotherDependency: AnotherDependency,
) : Bar {
    //...
}

Honestly, of all the different indentation examples where the annotation is on a separate line, I think this one is the most readable. class Foo does not get lost to me, because it's the first line, but as you said, if it were preceded by another annotation, it might be different.

However, I think every such example given here is deficient in some way. I don't think we can clearly say of any of them, "This is the best way to indent things in 100% of cases," so I don't think it makes sense to force it upon everyone. But this issue is really secondary to me, because I can avoid it altogether if I can put @Inject on the same line.

[paul-dingemans]

Let's agree to disagree on this topic for now. As documented:

This ktlint_official code style combines the best elements from the Kotlin Coding conventions and Android's Kotlin styleguide. This code style also provides additional formatting on topics which are not (explicitly) mentioned in those conventions and style guide.

For now, I will mark the issue as parked so that it can gather more opinions.

[eygraber]

This looks like a regression to me, for many of the same reasons stated in the comments above.

My primary reason for using ktlint is to format my code automatically so that it's readable. Of course consistency is important, but not at the expense of readability.

It's also concerning that consistency is decided in a vacuum. @paul-dingemans you mention that:

I know that constructors and functions are different beasts. But as with cars, they have both four wheels and some doors.

which sounds nice, but comes with the presupposition that constructors and functions are both cars. Considering that's a matter of opinion, this probably shouldn't have been decided on unilaterally (or if there was more involvement, then it should've been in the open).

[ephemient]

We are using .editorconfig

[*.{kt,kts}]
ktlint_standard_annotation = disabled

because this part of this rule is simply unusable for our codebase.

[meyertime]

We've gotten around this for now by using the android_studio style rather than ktlint_official, but it seems like it's not strict enough in some ways. It would be nice if it was more configurable. The stricter you get, the more people are going to disagree on which way it should be. At least with the intellij_idea and android_studio styles, you could point to the official style guides to resolve any dispute.

It's also concerning that consistency is decided in a vacuum. @paul-dingemans you mention that:

I know that constructors and functions are different beasts. But as with cars, they have both four wheels and some doors.

which sounds nice, but comes with the presupposition that constructors and functions are both cars. Considering that's a matter of opinion, this probably shouldn't have been decided on unilaterally (or if there was more involvement, then it should've been in the open).

Not only are constructors and functions "different beasts", primary constructors in Kotlin are even further removed from normal functions for several reasons:

• You can define class-level properties in primary constructors, but not in normal functions.
• There can only be one primary constructor for a class, whereas you can have many functions.
• You can only define the primary constructor as part of the class header, whereas functions can only be in the class body.
• A primary constructor cannot have a function body. What you find syntactically in place of the primary constructor's function body is actually the class body. Instead, property initializers and any init blocks are combined behind the scenes to generate the function body.

Any of these would be good reason for potentially different formatting rules to apply to a primary constructor than to a function. And, indeed, the fact that it has to be part of the class header necessitates it.

[paul-dingemans]

Considering that's a matter of opinion, this probably shouldn't have been decided on unilaterally (or if there was more involvement, then it should've been in the open).

In past I have tried to get feedback on proposal for changes via the issue tracker and via Slack. This resulted in almost no feedback at all. The feedback collected in this issue is valuable (that is the reason that the issue was not closed), but unfortunately also not representative to the entire user group. Users that disagree with decisions made are much more likely to reach out than people who agree.

I don't see any problem in unilatterly deciding about the new code style ktlint_official. The idea has been openly announced in this issue in November 2022. Also, all issues related to this code style haven been labeled and as of that were easily to follow or to provide feedback on.
Next to that, it is not affecting the existing code styles that people are used to. Finally, considering that I am the developer who puts countless number of hours in evolving ktlint with almost no help from the community, I feel that I have a big say towards the direction that ktlint is evolving.

[paul-dingemans]

We've gotten around this for now by using the android_studio style rather than ktlint_official, but it seems like it's not strict enough in some ways. It would be nice if it was more configurable.

Please see https://pinterest.github.io/ktlint/0.50.0/faq/#can-a-new-toggle-be-added-to-optionally-enabledisable-format-code-in-a-particular-way. Also note that when your android_studio code style, that you still can enable rules of ktlint_official that you would like to use. But this will not help for the constructor annotation case.

The stricter you get, the more people are going to disagree on which way it should be.

Yes, I am aware of that. Code style ktlint_official values consistency more important than the other code styles.

At least with the intellij_idea and android_studio styles, you could point to the official style guides to resolve any dispute.

Please feel free to create a PR which provides this clarity.

Not only are constructors and functions "different beasts", primary constructors in Kotlin are even further removed from normal functions for several reasons:

You can define class-level properties in primary constructors, but not in normal functions.
There can only be one primary constructor for a class, whereas you can have many functions.
You can only define the primary constructor as part of the class header, whereas functions can only be in the class body.
A primary constructor cannot have a function body. What you find syntactically in place of the primary constructor's function body is actually the class body. Instead, property initializers and any init blocks are combined behind the scenes to generate the function body.
Any of these would be good reason for potentially different formatting rules to apply to a primary constructor than to a function. And, indeed, the fact that it has to be part of the class header necessitates it.

Above are indeed differences between constructors and functions. But the root cause of this issue is the handling of annotations on the constructor and how to format those annotations. According to the Kotlin Coding conventions:

Place annotations on separate lines before the declaration to which they are attached, and with the same indentation:

[eygraber]

This resulted in almost no feedback at all.

That's just the way it is. Most people don't think about the tool that's going to format their code, and just assume it will work.

Users that disagree with decisions made are much more likely to reach out than people who agree.

True, and it's also an effective way of getting feedback, but it also alienates users and lowers trust in the tool.

I don't see any problem in unilatterly deciding about the new code style ktlint_official.

Android Kotlin Style Guide
Jetbrains Kotlin Style Guide

Neither of those are/were built with one person having unilateral decision power. It's also not very visible how much time and methodology went into making the decisions that they did. There's no way one person could have the context or info needed to make a decision that would apply across the ecosystem. I think you'll end seeing a lot more issues like this one crop up as more and more people adopt this style.

Of course it would be good to get community involvement in the decision making process, but it's not a topic that's ever going to get broad community involvement. It's a rock and a hard place situation.

considering that I am the developer who puts countless number of hours in evolving ktlint with almost no help from the community

💯 and personally I'm very thankful that you do this. I try to contribute by testing new versions and filing issues when I could, but most of the time I have to work on my projects and don't want to spend time thinking about the formatting tool.

I feel that I have a big say towards the direction that ktlint is evolving

Absolutely, but with great power comes great responsibility.

[meyertime]

Above are indeed differences between constructors and functions. But the root cause of this issue is the handling of annotations on the constructor and how to format those annotations. According to the Kotlin Coding conventions:

Place annotations on separate lines before the declaration to which they are attached, and with the same indentation:

Yes, this is the general rule in the Kotlin coding conventions, but it then proceeds to explain two explicit exceptions in that same section:

Annotations without arguments may be placed on the same line:

And:

A single annotation without arguments may be placed on the same line as the corresponding declaration:

And this is probably where you're going to bring up "consistency" and we'll be going in circles. But we can just as easily be consistent by enforcing that a single annotation without arguments always goes on the same line. These are called out in the coding conventions for a reason, so it should at least be an option (enforce separate line, enforce same line, or do not enforce either way).

[hantsy]

How can I use the original format? The new line break before/after annotation and intend make the codes look so ugly.

[hantsy]

Any lint tools instead? @meyertime

[meyertime]

@hantsy Use the intellij_idea or android_studio style. See https://pinterest.github.io/ktlint/1.0.0/rules/code-styles/

[PauGuillamon]

Comment copied from #2115, since I had missed this thread:

Giving my feedback on this topic, I find it quite strange that classes with annotated constructor causes the class body to have 2 indents.

All of a sudden I find some classes with 2 tabs and others with 1 tab. Instead of all classes looking similar, it seems quite random and does not give a feel of coherent code formatting in the project. Usually one does not look at the class constructor.

Additionally because of the 2 tabs indent some lines now reach the max length and are formatted completely different, even though it's the same code within the same amount of scope levels.

---

END of original comment.

I don't even care that much about how the constructor looks like. Speaking of consistency mentioned in the thread, some classes having 2 tabs and some classes having 1 tab is not consistent and decreases readability. Indentation levels are usually a hint of scope level which is broken here.

[realdadfish]

Coming from an earlier ktlint version this also looks like a big regression for me, but not so much because I (subjectively) dislike the new style; rather because once the code base is automatically reformatted (and our devs use that feature a LOT), we basically break git blame history on the respective files, and that's simply a no-go.

Certainly, no one expected that a 0.x version of a software is still 100% compatible with a 1.x version, but if that 0.x version was out for years and people relied on it formatting their code in one particular way now see that the same tool in a stable version reformats their code completely, then this is not acceptable.

Sorry, I see your intent and all the work that went into this, but please, please make this configurable. Thanks!

[paul-dingemans]

Certainly, no one expected that a 0.x version of a software is still 100% compatible with a 1.x version, but if that 0.x version was out for years and people relied on it formatting their code in one particular way now see that the same tool in a stable version reformats their code completely, then this is not acceptable.

I do understand that this is unwanted. Did you notice that the default code style of Ktlint was changed with publication of 1.0? The default code style is now ktlint_official which indeed has a some big changes in the code style. You can try whether sticking on the old intellij_idea code style results in less violations and a better backwards compatibility. See https://pinterest.github.io/ktlint/latest/rules/code-styles/

[realdadfish]

I had weird results locally when changing the different .editorconfig settings, because none seem to have had any effect. Eventually this was caused by the plugin I used (kotlinter), because once I forced single-daemon mode, deactivating the offending rule worked, so I could keep otherwise ktlint-official "compliant".

[paul-dingemans]

I had weird results locally when changing the different .editorconfig settings, because none seem to have had any effect. Eventually this was caused by the plugin I used (kotlinter), because once I forced single-daemon mode, deactivating the offending rule worked, so I could keep otherwise ktlint-official "compliant".

I am not familiar with the Kotlinter integration with ktlint. But I am happy for you that it works.

[erdi]

The default code style is now ktlint_official which indeed has a some big changes in the code style. You can try whether sticking on the old intellij_idea code style results in less violations and a better backwards compatibility.

Even though it's my fault for not reading the changelog prior to upgrading I would like to flag the this was the crucial bit that I missed. My motivation for upgrading to 1.x was to include a fix for #2343 so I was surprised that so many new violations are being raised after upgrading. After reconfiguring back to using intellij_idea style post upgrade the violation this issue is about went away.

[ericloe-cash]

I don't even care that much about how the constructor looks like. Speaking of consistency mentioned in the thread, some classes having 2 tabs and some classes having 1 tab is not consistent and decreases readability. Indentation levels are usually a hint of scope level which is broken here.

This

Please fix it, there is no rationale.
Experimental rule class-signature is also unusable because of the behavior with extends/implements in a new line causing double indent on the class body.

#2423