Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvements to annotation use-site targets on properties #402

Open
serras opened this issue Nov 20, 2024 · 5 comments
Open

Improvements to annotation use-site targets on properties #402

serras opened this issue Nov 20, 2024 · 5 comments

Comments

@serras
Copy link
Contributor

serras commented Nov 20, 2024

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
Copy link

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
Copy link

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
Copy link

rnett commented Nov 21, 2024

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
Copy link
Contributor Author

serras commented Nov 21, 2024

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
Copy link
Contributor Author

serras commented Nov 21, 2024

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 serras changed the title Change defaulting rule for annotations + new all meta-target Improvements to annotation use-site targets on properties Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants