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

Artifact with meta-annotations for Java types enhancement #100

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
173 changes: 173 additions & 0 deletions proposals/jvm-meta-annotations-artifact.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,173 @@
# Artifact with meta-annotations for Java types enhancement

* **Type**: Design proposal
* **Author**: Denis Zharkov
* **Contributors**: Andrey Breslav
* **Status**: Submitted
* **Prototype**: Not started
* **Discussion**: [KEEP-99](https://github.com/Kotlin/KEEP/issues/99)
* **Related proposals**: [JSR-305 custom nullability qualifiers](https://github.com/Kotlin/KEEP/blob/master/proposals/jsr-305-custom-nullability-qualifiers.md)

## Summary

Add a separate artifact containing meta-annotations covering use cases of ones from [JSR-305](https://jcp.org/en/jsr/detail?id=305).

## Motivation

- We need to put somewhere `@ApplyToTypeArgumentsAnnotation` meta-annotation (see the [discussion](https://github.com/Kotlin/KEEP/issues/79#issuecomment-336905480)).
- There is a [modules-related issue](https://blog.codefx.org/java/jsr-305-java-9/) with JSR-305 and Java 9.
- It's worth simplifying the way how JSR-305 nullability meta-annotations are being used
and integrating them with Kotlin-specific meta annotations. Namely, `@UnderMigration` and `@ApplyToTypeArguments`.

## Description

This section describes proposed semantics of the new annotations and partly the layout of resulting artifact.

### Root package
Root package for this artifact will be `kotlin.annotations.jvm`.
All classes/packages names are assumed to be placed in the package and only their
relative names are mentioned below.

### Built-in qualifiers
In JSR-305, there is [`@TypeQualifier`](https://aalmiray.github.io/jsr-305/apidocs/javax/annotation/meta/TypeQualifier.html)
annotation that allows to introduce custom qualifiers (like `@Nonnull`).
But that kind of meta-meta level seems to be unnecessary to our needs (at least for now),
the Kotlin compiler supports only nullability qualifier
(and in the nearest future mutability might be supported as well).

So, there will only be fixed number of built-in qualifier annotations:
- `nullability.Nullable`
- `nullability.NotNull`
- `mutability.Mutable`
- `mutability.ReadOnly`

Their target set would be the following:
ElementType.METHOD, ElementType.FIELD,
ElementType.PARAMETER, ElementType.LOCAL_VARIABLE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about TYPE_USE?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes impossible using them with JDK 6


And semantics when being applied to types is just the same as for analogue
from `org.jetbrains.annotations` (see [more](https://github.com/JetBrains/kotlin/blob/master/spec-docs/flexible-java-types.md#more-precise-type-information-from-annotations) about types enhancement)

### Alias annotation
This proposal suggests to introduce `meta.Alias` annotation which would affect the compiler
in a similar way that `@TypeQualifierNickname` [does](https://github.com/Kotlin/KEEP/blob/master/proposals/jsr-305-custom-nullability-qualifiers.md#type-qualifier-nicknames).

In a basic version its declaration may look like
```kotlin
package kotlin.annotations.jvm.meta

@Target(AnnotationTarget.ANNOTATION_CLASS)
annotation class Alias(val qualifier: KClass<*>)
```

and its usages is supposed to look like:
```kotlin
@Alias(kotlin.annotations.jvm.nullability.Nullable::class)
annotation class MyNullable
```

Thus, applying `@MyNullable` should have the same effect as `@Nullable` itself has.
Beside it, `@MyNullable` may have `@UnderMigration` annotation on it that would change its
[migration status](https://github.com/Kotlin/KEEP/blob/master/proposals/jsr-305-custom-nullability-qualifiers.md#undermigration-annotation).

### Default qualifiers options<a name="default-qualifiers"></a>
This proposal has two options how
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we had three:

  1. Alias, ApplyByDefault, UnderMigration
  2. Alias(propagateTo), UnderMigration
  3. Alias(propagateTo, migrationStatus)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your 2d and 3d options are different only by the way how the migration phase is defined.
And this is a different issue described in the next section.

[default qualifiers](https://github.com/Kotlin/KEEP/blob/master/proposals/jsr-305-custom-nullability-qualifiers.md#type-qualifier-default)
semantics can be introduced.

And both of them are assume using special enum class instead of `ElementType`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both of them are assumeboth of them assume

that is used as a parameter type for JSR-305 `@TypeQualifierDefault`.
It might look like:
```kotlin
enum class ApplicabilityKind {
RETURN_TYPE, VALUE_PARAMETER, FIELD, TYPE_USE, TYPE_ARGUMENT
}
```

All elements should work just the same as relevant entries from `ElementType` do for `@TypeQualifierDefault`,
beside `TYPE_ARGUMENT`.
The latter one should have the following effect: if a default qualifier is determined
to be applied to some top-level type (using the same logic as for `@TypeQualifierDefault`)
and the set of applicability kinds contain `TYPE_ARGUMENT` then this qualifier should also be applied
to all of it's type arguments (recursively).

#### Parameter in @Alias (Option 1)
The first option is adding `propagateTo: Array<ApplicabilityKind>` parameter to `meta.Alias` annotation.

Thus, declaration of `AllParametersAreNullableByDefault` would look like this:
```kotlin
@Alias(Nullable::class, propagateTo=[ApplicabilityKind.VALUE_PARAMETER])
annotation class AllParametersAreNullableByDefault
```

*Known pros:*
- It's nice to have one annotation for all issues.
- Probably, for someone such form of default qualifiers would look nicer than one suggested by JSR-305.

*Known cons:*
- It would look a little weird when it's used in cases like the one below:
```kotlin
@Alias(Nullable::class)
@UnderMigration(...)
annotation class MyNullable

@Alias(MyNullable::class, propagateTo=[ApplicabilityKind.PARAMETER])
annotation class MyAllParametersAreNullableByDefault
```
- Also it might be confusing when one alias (probably with non-empty `propagateTo`)
references another alias with non-trivial `propagateTo` argument
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be outlawed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only if they both are written in Koltin


#### Separate annotation (Option 2)
Another option would be a separate annotation `meta.ApplyByDefault` with vararg-parameter
of type `ApplicabilityKind`:
```kotlin
@Target(AnnotationTarget.ANNOTATION_CLASS)
annotation class ApplyByDefault(val qualifier: KClass<*>, vararg val elements: ApplicabilityKind)
```

Basically, it should work just the same as `@TypeQualifierDefault`, e.g. it might be used like:
```kotlin
@ApplyByDefault(MyNullable::class, ApplicabilityKind.PARAMETER)
annotation class MyAllParametersAreNullableByDefault
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like other annotations should be present as well, for comarison


@ApplyByDefault(MyNonnull::class, ApplicabilityKind.PARAMETER)
annotation class MyAllParametersAreNonNullByDefault

@ApplyByDefault(MyNullable::class, ApplicabilityKind.PARAMETER)
annotation class MyAllParametersAreNullableByDefault

@ApplyByDefault(
MyNonnull::class,
ApplicabilityKind.RETURN_TYPE, ApplicabilityKind.VALUE_PARAMETER,
ApplicabilityKind.FIELD, ApplicabilityKind.TYPE_ARGUMENT
)
annotation class MyApiNonNullByDefault
```

### Avoidance of @UnderMigration annotation
After revisiting design for meta-annotations it looks like a natural thing to set up
a migration status for an annotation with its argument instead of another meta-annotation.

Thus, the idea is to add `migrationStatus` parameter to `meta.Alias`
(and to `meta.ApplyByDefault` if we decide to introduce the latter) with default value to be `STRICT`.

This argument should be processed by the compiler in the same way as for [`@UnderMigration`](https://github.com/Kotlin/KEEP/blob/master/proposals/jsr-305-custom-nullability-qualifiers.md#undermigration-annotation) annotation
having the same argument value.

### Details on artifact

There is already an artifact called [kotlin-annotations-jvm](https://mvnrepository.com/artifact/org.jetbrains.kotlin/kotlin-annotations-jvm)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same open question than for the package, isn't Kotlin scope too narrow for such general nullability need?

that might be the best candidate where the new meta annotations may be placed.

Probably, the annotations that are already there should be moved to a different packages:

- `kotlin.annotations.jvm.UnderMigration`, `kotlin.annotations.jvm.MigrationStatus` -> `kotlin.annotations.jvm.meta`
- `kotlin.annotations.jvm.Mutable`, `kotlin.annotations.jvm.ReadOnly` -> `kotlin.annotations.jvm.collections`

## Remaining questions
- Should aliasing JSR-305 qualifiers like `javax.annotation.Nonnull` be allowed?
- What option is the best to choose for supporting [default qualifiers](#default-qualifiers)?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Option 2 looks cleaner to me, but no strong opinion here. Also something to keep in mind is that JVM seems to generates warning when a library (for example Spring) is using enum annotation attributes that are not in the classpath, which was a use case of JSR 305. Maybe this library will be in the projects classpath since it won't have JSR 305 package issue, but I prefer to mention it in order to be sure this is not an issue for projects that will use this library.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but I didn't get the issue with enums and classpath.
Does the warning happen while reading annotations through reflection?
Or it happens when compiling against Spring without JSR-305 in the classpath?

Is it only enum issue, i.e. is everything fine with plain no-arguments annotations?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is that the JVM is lenient about meta-annotations not being in the classpath except when these meta-annotations are using enums not in the classpath for their attribute value.

For example if I compile a Spring project without JSR 305 dependency, and if that project is using @org.springframework.lang.NonNull, no warning when compiling because only @Nonnull and @TypeQualifierNickname meta-annotations are used.

But if I use @org.springframework.lang.Nullable, then I get the following warning during compilation: Warning:java: unknown enum constant javax.annotation.meta.When.MAYBE reason: class file for javax.annotation.meta.When not found because this annotation is meta-annotated with @Nonnull(when = When.MAYBE).

This issue very painful for such usual use case would not occur if JSR 305 was using @Nonnull(when = "MAYBE") or @Nonnull @Maybe, so I would be strongly for the new meta-annotation library you will provide avoiding the same mistake as it will allow all libraries to use your meta-annotations without forcing all users of these libraries to have crappy compilation warnings in their logs if they don't have Jetbrains lib in the classpath. IMO this is a key point for wide adoption.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NB: AFAIK it only happens when using these annotations in a user's project that depends on Spring project but doesn't have jsr305.jar in the classpath. I.e.. when calling a piece of annotated API no warning is emitted.

- Should there be a `migrationStatus` parameter in `@Alias` or migration status
should be tracked through `@UnderMigration` annotation?
- What is the best name for `ApplicabilityKind` enum class?
Would it be better to place it inside the `ApplyByDefault` annotation class (if it will be there)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ApplyByDefault.Scope ?