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

JSR-305 custom nullability qualifiers #78

Merged
merged 12 commits into from Feb 14, 2018

Conversation

Projects
None yet
3 participants
@dzharkov
Copy link
Collaborator

dzharkov commented Aug 14, 2017

No description provided.

}
@Target(AnnotationTarget.ANNOTATION_CLASS)
annotation class Migration(val status: MigrationStatus)

This comment has been minimized.

@udalov

udalov Aug 29, 2017

Contributor

Why such a generic name? Is it supposed to be used for something else in the future?

This comment has been minimized.

@udalov

udalov Aug 29, 2017

Contributor

If yes, maybe it makes sense to rename -Xjsr305-annotation-migration to something like -Xoverride-migration-status

This comment has been minimized.

@dzharkov

dzharkov Aug 29, 2017

Collaborator

The name is a subject for discussion as usual, but the flag name you proposed seems to be more appropriate here

This comment has been minimized.

@udalov

udalov Aug 29, 2017

Contributor

I mean, what other use cases for this annotation were considered?

If none were, then it probably should reflect that it's about JSR-305 in its name, e.g. kotlin.jvm.Jsr305Migration, and the compiler argument should indeed be named something like -Xjsr305-annotation-migration.

to `ERROR`, then it should become an error.

It's important that migration annotation works only for its immediate usages
on types or through `TypeQualifierDefault` annotation.

This comment has been minimized.

@udalov

udalov Aug 29, 2017

Contributor

It says above

it can be only used for custom nullability qualifier nicknames

, so this line is a bit confusing. Can I use it with TypeQualifierDefault or not? Maybe provide an example

When determining immediate nullability of a type the Kotlin compiler should look
for nullability annotation on the type itself at first.
- If there is one, then it should use it
- Otherwise, nullability is determined by the closest annotation marked as

This comment has been minimized.

@udalov

udalov Aug 29, 2017

Contributor

It may be unclear to users what "closest" means here

This comment has been minimized.

@dzharkov

dzharkov Aug 29, 2017

Collaborator

@udalov How do you think, would the phrase "the innermost enclosing element annotated with TypeQualifierDefault" be more clear in the context?

This comment has been minimized.

@udalov

udalov Aug 29, 2017

Contributor

Yes, that's clearer

It effectively defines the migration status behavior only for nullability
qualifier nicknames that haven't yet been annotated with `kotlin.Migration`.

This annotation is necessary, since using all of the custom nullability

This comment has been minimized.

@udalov

udalov Aug 29, 2017

Contributor

This annotation -> This compiler flag?

I can't make sense of this sentence either way though

This comment has been minimized.

@dzharkov

dzharkov Aug 29, 2017

Collaborator

Of course, it's all about the compiler flag
This sentence is intended to describe the motivation behind its introduction (a lot of already annotated API that we haven't processed properly until now)

Sometimes it might be necessary to manage a migration phase for a particular
library. That could be done with the flag `-Xjsr305-user-annotation`. It
accepts fully-qualified name of a nullability qualifier annotation and its
desired status in a format `<fq.name>:ignore/warn/error`.

This comment has been minimized.

@udalov

udalov Aug 29, 2017

Contributor

Please provide a concrete usage example of this compiler argument here in the proposal

Current proposal is supposed to introduce additional ways of types enhancement
and instruments to control their migration status.

### Type qualifier nicknames

This comment has been minimized.

@abreslav

abreslav Sep 1, 2017

Contributor

This part is very hard to understand for people having no background in how JSR-305 is supposed to work. Given that there's no comprehensive doc on JSR-305, I think we should include a brief description of its relevant parts

This comment has been minimized.

@abreslav

abreslav Sep 1, 2017

Contributor

Having read a bit further, I realize that this part is giving the necessary background, but it's not clear upfront. We should add a comment on this, along the lines of "JSR 305 has this and that semantics, and we support the following part of it"

corresponding `when` argument. Namely:
- `When.ALWAYS` makes the type not-nullable
- `When.MAYBE` makes the type nullable
- `When.NEVER/When.UNKNOWN` doesn't affect type's nullability

This comment has been minimized.

@abreslav

abreslav Sep 1, 2017

Contributor

Why do we treat NEVER this way?

Current proposal is supposed to introduce additional ways of types enhancement
and instruments to control their migration status.

### Type qualifier nicknames

This comment has been minimized.

@abreslav

abreslav Sep 1, 2017

Contributor

Having read a bit further, I realize that this part is giving the necessary background, but it's not clear upfront. We should add a comment on this, along the lines of "JSR 305 has this and that semantics, and we support the following part of it"

- `ElementType.METHOD` for return type of methods
- `ElementType.PARAMETER` for value parameters
- `ElementType.FIELD` for fields
- `ElementType.TYPE_USE` for any top-level type

This comment has been minimized.

@abreslav

abreslav Sep 1, 2017

Contributor

Why only top-level?


Note, that `javax.annotation.ParametersAreNonnullByDefault` annotation should
work automatically
by applying the rules above.

This comment has been minimized.

@abreslav

abreslav Sep 1, 2017

Contributor

It would be great to include its declaration here

This comment has been minimized.

@abreslav

abreslav Sep 1, 2017

Contributor

And a link to its JavaDoc or some other description would be great too

}
```
```java
// FILE: test/package-info.java

This comment has been minimized.

@abreslav

abreslav Sep 1, 2017

Contributor

I'd suggest to put each file in a separate code block

### `@Migration` annotation
Current proposal suggests to add the following meta-annotation in
the `kotlin.annotation` package:
```kotlin

This comment has been minimized.

@abreslav

abreslav Sep 1, 2017

Contributor

I'd add a package statement here too


### `@Migration` annotation
Current proposal suggests to add the following meta-annotation in
the `kotlin.annotation` package:

This comment has been minimized.

@abreslav

abreslav Sep 1, 2017

Contributor

Did we discuss the package much? Looks like it should be something jvm-specific, e.g. kotlin.annotation.jvm


Its aim is to define a migration status of an annotation it's been applied to.
To the moment it can be only used for custom nullability qualifier
[nicknames](#Type qualifier nicknames) introduced in this proposal as well.

This comment has been minimized.

@abreslav

abreslav Sep 1, 2017

Contributor

Link is broken


When the annotation is applied to a nullability nickname its argument specifies
how the compiler handles the nickname:
- `MigrationStatus.ERROR` makes annotation work just the same way as any plain

This comment has been minimized.

@abreslav

abreslav Sep 1, 2017

Contributor

We need to explain why we need this

errors
for
inappropriate usages of an annotated type
- `MigrationStatus.IGNORE` makes compiler to ignore the annotation completely

This comment has been minimized.

@abreslav

abreslav Sep 1, 2017

Contributor

It's unclear which annotation is ignored. A motivating use case would help

@dzharkov dzharkov force-pushed the jsr-305 branch from 93baaf6 to 41091f1 Sep 12, 2017


### `@UnderMigration` annotation
Current proposal suggests to add the following meta-annotation in
the `kotlin.annotations.jvm` package:

This comment has been minimized.

@udalov

udalov Sep 12, 2017

Contributor

You should probably also highlight that it's in a separate library, not in kotlin-stdlib

custom nullability qualifiers) without `jsr305.jar` as a dependency.

# Open questions/issues
- Should nullability default qualifiers with `TYPE_USE` applicability enhance

This comment has been minimized.

@udalov

udalov Sep 13, 2017

Contributor

Not an open question anymore, since we agreed to do this?

@dzharkov dzharkov merged commit 2351d76 into master Feb 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment