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

Support NonNullApi/NonNullFields #1084

Closed
xenoterracide opened this issue Dec 6, 2024 · 9 comments
Closed

Support NonNullApi/NonNullFields #1084

xenoterracide opened this issue Dec 6, 2024 · 9 comments

Comments

@xenoterracide
Copy link

xenoterracide commented Dec 6, 2024

Support spring style package annotations https://docs.spring.io/spring-framework/reference/core/null-safety.html gradle-api also has NonNullApi, I suspect in lieu of java support this will become more popular. I imagine not supporting it could be problematic when working with these libraries.

Since fields are rarely exposed I could see NonNullApi being treated as a synonym for Nullmarked initially being a safe-ish way forward for consumers. You could document it as such ... "this is partial support"... which would serve my purposes as a consumer.

@msridhar
Copy link
Collaborator

@xenoterracide just confirming, do IntelliJ or other tools already support @NonNullApi in the manner that you're proposing here?

@xenoterracide
Copy link
Author

xenoterracide commented Dec 12, 2024

I'm not certain what you mean by "the manner" and I admit I haven't done comprehensive analysis. It does at least have some support, although I think I've found a bug based on this analysis.... the 2 arg findField I see, both parameters should not be nullable... I think that's an intellij bug that I don't care enough about to report (it might also be looking at kotlin definitions).

Screenshot from 2024-12-12 13-40-15

you can see the package-info is annotated with these annotations. If both are supplied they should be equivalent to putting nullmarked on the package. https://github.com/spring-projects/spring-framework/blob/main/spring-core/src/main/java/org/springframework/util/package-info.java

I care about this detection only for 3rd party packages, but I don't know if you have any way to tell the difference between compiling against a jar vs the code you're compiling. Point being more that I don't think if you're only supporting this for detection on 3rd party libraries I'm not certain you have to worry about the differences between having and not having NonNullFields when using NonNullApi.

@msridhar
Copy link
Collaborator

Right I just meant other tools are treating @NonNullApi and the like as kind of equivalent to @NullMarked. Thanks!

@xenoterracide
Copy link
Author

More or less... As stated there's an interaction with NonNullFields as it's not supposed to imply that. If you only had non-null API then the fields would be considered nullable by default and you can imagine the vice versa as well...

@xenoterracide
Copy link
Author

p.s. I have created tickets for libraries that I'm aware of using NonNullApi to add Nullmarked

@msridhar
Copy link
Collaborator

msridhar commented Dec 15, 2024

@xenoterracide I did a quick hack to add this support in #1095. Could you pull the branch there, install a snapshot (by running ORG_GRADLE_PROJECT_RELEASE_SIGNING_ENABLED=false ./gradlew publishToMavenLocal), and see if it works as you expect? The current snapshot version is 0.12.3-SNAPSHOT

Before landing this change I would need to do some further testing including on Uber code, as this has the potential to make NullAway report a bunch of new warnings. We may have to put it behind a flag at first.

@sdeleuze
Copy link

sdeleuze commented Dec 16, 2024

In Spring Framework 5.x and 6.x, we simply used NullAway:AnnotatedPackages and NullAway:UnannotatedSubPackage to define which packages are annotated. Spring Framework 7.x will migrate its whole codebase to JSpecify annotations.

@NonNullApi/@NonNullField and @NullMarked have subtle but very important differences, as the former apply to parameter parameters/return values/fields while the latter applies to the use of types. The semantics of nullability for arrays, varags and generic types are also totally different.

The proper way to support @NonNullApi / @NonNullFields is IMO to solve #633 since they are meta annotated with JSR 305 @TypeQualifierNickname, that said now that JSpecify is available I would advise to just support JSpecify.

As consequence, I am strongly recommending declining this feature request and just advise projects to migrate to JSpecify.

@msridhar
Copy link
Collaborator

Thanks a lot @sdeleuze! I will then close this in favor of #633. @xenoterracide we can discuss further on #633, but given our limited resources, I am leaning towards not prioritizing support for @TypeQualifierNickname right now.

@xenoterracide
Copy link
Author

xenoterracide commented Dec 16, 2024

I mean I don't have much to say other than that's great for spring internally but that's not great for projects that use spring.

I don't know if Gradle has any plans to do the same thing though (Migrate to Jspecify)

Also last I checked the spring migration to Jspecify was being held up by the fact that jspecify 1.0 decided to remove the meta annotations. Sometimes conversations don't trickle into tickets though; also I could be forgetting that something was changed in that ticket.

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

Successfully merging a pull request may close this issue.

3 participants