KEEP icon indicating copy to clipboard operation
KEEP copied to clipboard

Improvements to annotation use-site targets on properties

Open serras opened this issue 1 year ago • 10 comments

This is an issue to discuss changing the defaulting rule for annotations and the new all meta-target. The full text of the proposal can be found here.

serras avatar Nov 20 '24 11:11 serras

The link with the text "defaulting rule" goes to https://kotlinlang.org/docs/annotations.html#java-annotations. Should it instead be https://kotlinlang.org/docs/annotations.html#annotation-use-site-targets?

klitoskyriacou avatar Nov 20 '24 14:11 klitoskyriacou

I like this idea. It's how I expected annotations to behave.

As additional examples, there is a lot of code in the wild that looks like:

@Serializable
class Foo(
    @ObjectId
    @SerialName("_id")
    val id: String
)

I wonder though if it will break some frameworks. Issues in annotation-based frameworks are in general difficult to detect. What if there exists a framework that behaves differently based on whether the annotation is specified on the constructor parameter or on both the constructor parameter and fields. Would there be a way to detect this? I doubt it, so the best we can do is migrate slowly and hope users detect issues easily?

CLOVIS-AI avatar Nov 21 '24 17:11 CLOVIS-AI

This is great, I've definitely shot myself in the foot with the wrong target before.

The all target may not be used with multiple annotations. It is unclear what the behavior should be when the multiple annotations have different targets.

This seems overly restrictive. IMO it's obvious that @all:[A B] would apply A to all targetsA can be applied to, and B to all targetsB can be applied to.

Was all considered as the default target, instead of param + property/field? I'm curious why you wouldn't want it to be.

rnett avatar Nov 21 '24 18:11 rnett

What if there exists a framework that behaves differently based on whether the annotation is specified on the constructor parameter or on both the constructor parameter and fields. Would there be a way to detect this?

This is why we are giving some interim period with a warning, so that people can reflect on their use-site. Actually, in many cases we think this may reveal that some annotation is not applied in the way they were thinking (especially when using validation frameworks).

serras avatar Nov 21 '24 19:11 serras

This seems overly restrictive. IMO it's obvious that @all:[A B] would apply A to all targets A can be applied to, and B to all targets B can be applied to.

Unfortunately this doesn't seem to be so clear cut. During the design different people came up with different answers to what @all:[A B] should do. Given how rare muti-annotations are in Kotlin, it doesn't seem to be worth the possible confusion.

Was all considered as the default target, instead of param + property/field? I'm curious why you wouldn't want it to be.

For me, the key point here is that Kotlin does not work that way. It has never applied annotations in properties to the accessors, for example. We don't want to change completely how Kotlin handles annotations, only tweak one of the rules that we think causes a lot of confusion.

serras avatar Nov 21 '24 19:11 serras

Hi, I'm just wondering, purely out of curiosity, if there has been any progress to this issue?

jesperancinha avatar Mar 14 '25 08:03 jesperancinha

@jesperancinha yes, this is currently being implemented, and we expect these new features to appear in 2.2

serras avatar Mar 14 '25 08:03 serras

I can't wait for this to become available. It's a huge pitfall at the moment, and even though I'm aware of it, I sometimes forget to add the proper annotation target...

MartinHaeusler avatar Apr 01 '25 08:04 MartinHaeusler

@MartinHaeusler @jesperancinha this feature is to be fully announce in 2.2.0, but there's a experimental version available in 2.1.20 under the -Xannotation-default-target=param-property flag.

serras avatar Apr 01 '25 10:04 serras

The KEEP has been merged, and the feature should be fully announced by 2.2.0, as experimental. This PR will remain open for some time after that release to gather additional feedback.

serras avatar Apr 07 '25 14:04 serras