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

Migrate to JSpecify annotations #24767

Open
jvandort opened this issue Apr 14, 2023 · 11 comments
Open

Migrate to JSpecify annotations #24767

jvandort opened this issue Apr 14, 2023 · 11 comments
Assignees
Labels
a:feature A new functionality in:building-gradle gradle/gradle build
Milestone

Comments

@jvandort
Copy link
Member

Expected Behavior

Since we have now merged Kotlin 1.8.20 support into Gradle, we should now be able to migrate to JSpecify annotations.

Current Behavior (optional)

We currently use javax.annotation.Nonnull and org.jetbrains.annotations.NotNull annotations on Gradle's own @NonNullApi. We should investigate if we can instead annotate this annotation with one of JSPecify's annotations, or see if we can completely migrate to an alternative provided by JSpecify.

Context

Some things to consider:

  • How would this change interact with javax.annotation.Nullable annotations we use on methods and fields in the codebase?
  • What is the migration path to using JSpecify annotations instead of javax annotations throughout the codebase?
  • Will switching to JSpecify annotations affect Kotlin integration in any way in terms of source or binary compatibility.
@jvandort jvandort added the a:feature A new functionality label Apr 14, 2023
@jvandort jvandort added this to the 8.3 RC1 milestone Apr 14, 2023
@octylFractal
Copy link
Member

We should migrate to the NullMarked annotation instead of using @NonNullApi.

@eskatos
Copy link
Member

eskatos commented Apr 18, 2023

The way the support for provider.map { null } was done in #19088 actually rely on a Kotlin compiler bug in handling nullability annotations. With jsr305 or jetbrains annotations it is currently impossible to represent that use case. JSpecify should work.

As part of this work we should make sure we can remove the workaround introduced in c3168f2 that adds a kotlin compiler flag to restore the compiler bug (along with loooots of type system imprecisions).

@lptr
Copy link
Member

lptr commented Jun 9, 2023

Is this actually planned for 8.3? Would be lovely.

@octylFractal
Copy link
Member

Yes, it is.

@vlsi
Copy link
Contributor

vlsi commented Jun 9, 2023

https://github.com/jspecify/jspecify#jspecify

Any and all aspects of the API and specifications are subject to change prior to the 1.0 release. Using the annotations in a released library is not recommended.

:(

@octylFractal
Copy link
Member

See the more recently-updated https://github.com/jspecify/jspecify/wiki/adoption, we feel it is safe to adopt now. Assuming there are no major issues and it improves Kotlin DSL nullness, it should be a net positive overall.

@kevinb9n
Copy link

kevinb9n commented Jun 9, 2023

Whoops, I should have fixed that wording on the main github page a while ago.

The jar is very safe to depend on. The absolute most we might change in the actual artifact at this point would be to remove @Target(METHOD) from @NullMarked and @NullUnmarked. Even if you'd been using them on methods, it wouldn't cause you any trouble (worse than having avoided them in the first place) and should be a nonevent to clients as well. No other removals or renames (or the bizarre addition of a mandatory default-less attribute) will be considered.

@octylFractal octylFractal self-assigned this Jun 9, 2023
@octylFractal
Copy link
Member

Blocked by https://youtrack.jetbrains.com/issue/KT-59352 and another potentially related issue.

@eskatos
Copy link
Member

eskatos commented May 17, 2024

KT-59352 and the related blocking issue are both fixed in Kotlin 2.0.0-RC1.

@eskatos
Copy link
Member

eskatos commented May 17, 2024

It seems that even if it could theoretically be possible to express this using JSpecify, there's no implementation commitment on the Kotlin side. IOW, migrating to JSpecify will not allow to resolve #12388.

@cpovirk
Copy link

cpovirk commented May 17, 2024

Is there an easy way for me to help out from the JSpecify side? My guess would be that #12388 could be fixed by adding @NullMarked to Transformer without any further changes, since #23164 did most of what I would have done. (I could also make some other changes that I don't think are necessary. But I'd start simple unless those changes turn out to be more necessary than I would think :)) I have tried to browse through various related issue, but I don't know if there's a standalone test that I could run easily with a Gradle built from head, so I'm not sure how easily I can test my claims.

For what it's worth, Kotlin has implemented enough JSpecify support that I would expect that much to work today. I know that you've seen some other issues (as have those of us at Google in our usage of the JSpecify annotations with Kotlin), so I could understand if you want to wait until Kotlin 2+ is widely adopted before switching to JSpecify. I would only add that we have found the support to be good enough to use widely in our codebase, and the Kotlin developers have been of help when we've encountered problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:feature A new functionality in:building-gradle gradle/gradle build
Projects
None yet
Development

No branches or pull requests

9 participants