-
Notifications
You must be signed in to change notification settings - Fork 299
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 JSR305 meta annotations TypeQualifierNickname and TypeQualifierDefault #633
Comments
Thanks for the suggestion! But I'm not sure I completely follow. Look at a @javax.annotation.meta.TypeQualifierNickname
@javax.annotation.Nonnull(when = javax.annotation.meta.When.UNKNOWN) But, our solution in #629 ended up treating |
There was some discussion on google/guava#6126 about changing the meta-annotation for If there are unrelated use cases that would benefit of us supporting That said, it is certainly doable, if there is a need beyond just Guava's Now that we support a slower mode for code with mixed-checking for No idea how to support |
Thanks for the thoughtful responses! I guess we need to agree on what this annotation means:
To me, that means it may be null, because, while there are situations where it's nonnull, it's "unknown" what these situations are. Is that how we're supposed to read the |
I'm not sure how where there's good official documentation for the jsr305 annotations. My understanding is that If the goal is instead to tell tools to issue warnings appropriate to a value that might be Here's at least some evidence that others have interpreted it that way:
|
That's interesting. I think from the NullAway standpoint, I wasn't even thinking of the subtleties of
There are two ways to interpret
Either way, that's only with respect to NullAway's internal mental model. If we decide that actually implementing JSR305 is something we care about, rather than basically hijacking its annotations for our purposes, then I think going by the interpretations @cpovirk shared makes the most sense. Personally, I am not against more faithful JSR305 support if it doesn't compromise NullAway's performance on code that doesn't use these |
Thanks for explaining your rationale Lázaro & Chris! Fortunately, I believe the spec and documentation for JSR305 answers all of our questions! In JSR305, From the source code: @TypeQualifierNickname
@Nonnull(when=UNKNOWN)
public @interface Nullable {} It has no semantics of its own; in fact, So it seems safe and correct to treat |
Oh, wow, thanks for those jsr305 slides! Treating them equivalently would definitely be the technically correct thing to do. My understanding (which might or might not be what you're advocating for) is that the technically correct way to resolve that difference would be to change how the jsr305
As the slides you found say, I assume that changing NullAway's handling of jsr305 |
I'm filing this as a follow-up to #628. (Note that fixing this will allow undoing the temporary workaround in #629)
If you read the bug at #628 (and google/guava#6126),
@ParametricNullness
is not be doing anything novel or weird – it is just usingjavax.annotation.meta
annotations which were specified as part of JSR 305 fifteen years ago in 2007. Unfortunately, unlike IntelliJ and TCF, NullAway does not support these.The ideal fix for #628 would be for NullAway to support
TypeQualifierNickname
andTypeQualifierDefault
. This would obviate the need for special cases to handle these Guava annotations.I would guess (not knowing much about NullAway architecture) it should not be a huge effort to implement this (though it may be hard to not degrade performance). Roughly:
@Nullable
(e.g. variable declarations, JSR308 annotated types):@TypeQualifierNickname
, interpret that class's annotations as being on the original AST element@TypeQualifierDefault
, and interpret that node's annotations as being on the original AST elementThe text was updated successfully, but these errors were encountered: