Kotlin / KEEP

Kotlin Evolution and Enhancement Process
Apache License 2.0
3.42k stars 362 forks source link

Change defaulting rule for annotations + new `all` meta-target #402

Open serras opened 5 days ago

serras commented 5 days ago

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.

klitoskyriacou commented 5 days ago

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?

CLOVIS-AI commented 4 days ago

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?

rnett commented 4 days ago

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.

serras commented 4 days ago

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 commented 4 days ago

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.