Open msridhar opened 2 months ago
An initial proposal here:
AcknowledgeRestrictiveAnnotations
flag with something like IgnoreRestrictiveAnnotations
, and make acknowledging restrictive annotations the default. This way, "unannotated" and @NullUnmarked
mean the same thing by default.TreatGeneratedAsUnannotated
to TreatGeneratedAsUnannotatedRestrictiveIgnored
, for clarity? Not sure about this oneCustomGeneratedCodeAnnotations
setting only has any effect if TreatGeneratedAsUnannotated
is also set, which is kind of confusing. I propose to make them independent, and to change the name of the option to something like UnannotatedRestrictiveIgnoredAnnotations
, i.e., annotations that indicate the class is unannotated with restrictive @NonNull
/ @Nullable
annotations ignored.Thoughts? Hoping someone can come up with better names 😅
IgnoreRestrictiveAnnotations
would be a bit of a silly name, the original name was based on "accept annotations here iff they are more restrictive than our optimistic defaults", if our/JSpecify's default is to acknowledge annotations in @NullUnmarked
code but not assume @NonNull
, then the notion of "restrictive annotations" makes no sense. I suggest IgnoreAnnotationsInUnmarkedCode
or similar (have a term other than annotated
for included/@NullMarked
code)TreatGeneratedAsUnannotated
maybe becomes IgnoreAnnotationsInGeneratedCode
? Now the two settings make sense in any combination, I think.@Generated
annotations, because it is generally useful to know when code is generated. We have very little control over the execution here due to Error Prone, but generally it can be useful to: a) skip analysis of generated code, b) skip reporting inside generated code (see Lombok) regardless of what annotations we consider, etc.(Anecdotally, "is this code generated" has been something that has repeatedly been useful to know the answer for in other analysis tools, so I wouldn't go out of my way to remove that semantic knowledge from NullAway, assuming the use case is still exclusively generated code vs the many other ways we have to specify unmarked code)
IgnoreRestrictiveAnnotations
would be a bit of a silly name, the original name was based on "accept annotations here iff they are more restrictive than our optimistic defaults", if our/JSpecify's default is to acknowledge annotations in@NullUnmarked
code but not assume@NonNull
, then the notion of "restrictive annotations" makes no sense. I suggestIgnoreAnnotationsInUnmarkedCode
or similar (have a term other thanannotated
for included/@NullMarked
code)
I like IgnoreAnnotationsInUnmarkedCode
! It will require introducing the term "unmarked" to our users but we need to write docs on that anyway so I think it's fine. We should probably describe this as a backwards-compatibility / not-recommended setting, as if its enabled, we will go against the JSpecify spec by ignoring annotations in @NullUnmarked
code.
TreatGeneratedAsUnannotated
maybe becomesIgnoreAnnotationsInGeneratedCode
? Now the two settings make sense in any combination, I think.
So this setting currently means two things:
@Generated
annotation as unmarked@Generated
code(I.e., the previous default meaning of "unannotated".) Do we think it's reasonable to have a setting IgnoreAnnotationsInGeneratedCode
that means both of the above, given that we're trying to align the default meaning of "unannotated" with @NullUnmarked
? Maybe yes, since it doesn't really make sense to ignore annotations in @NullMarked
code?
- We still want custom
@Generated
annotations, because it is generally useful to know when code is generated. We have very little control over the execution here due to Error Prone, but generally it can be useful to: a) skip analysis of generated code, b) skip reporting inside generated code (see Lombok) regardless of what annotations we consider, etc.(Anecdotally, "is this code generated" has been something that has repeatedly been useful to know the answer for in other analysis tools, so I wouldn't go out of my way to remove that semantic knowledge from NullAway, assuming the use case is still exclusively generated code vs the many other ways we have to specify unmarked code)
I agree. One thing I am trying to accomplish, though, is to make the custom @Generated
annotations setting independent of the TreatGeneratedAsUnannotated
setting. The reason is that the latter is sometimes too coarse, and we have scenarios where we may want to treat code within @Generated
annotations from some packages as annotations-ignored, but not from others. So, to make this setting understandable on its own, I feel the name should somehow convey that whatever code has the specified annotation will be treated as annotations-ignored. But I don't have a good idea for a name.
Historically, NullAway has had settings for "annotated" vs "unannotated" packages and classes. Annotated code is assumed to be annotated according to NullAway's conventions. For unannotated code, we make no such assumption, and further, by default we completely ignore any annotations present in that code. This default can be changed via the
AcknowledgeRestrictiveAnnotations
setting; when provided, this setting causes@Nullable
annotations on return types and@NonNull
annotations on parameter types to be acknowledged even within unannotated code.NullAway also has a setting
TreatGeneratedAsUnannotated
which causes all code within a@Generated
annotation (or any custom generated code annotation) to be treated as unannotated. Crucially (and non-obviously), if bothTreatGeneratedAsUnannotated
andAcknowledgeRestrictiveAnnotations
are passed, restrictive annotations in generated code are still not acknowledged. OurTreatGeneratedAsUnannotated
setting is rather coarse, and in trying to make it more flexible, I ran into this subtle interaction that I hadn't recalled.Also, NullAway now supports
@NullMarked
and@NullUnmarked
annotations.@NullMarked
is equivalent to "annotated" (I think), whereas@NullUnmarked
corresponds to "unannotated" with theAcknowledgeRestrictiveAnnotations
setting enabled. There is no standard null-markedness setting that corresponds to our default "unannotated" config, where all annotations are ignored.So what is the point of this issue?
AcknowledgeRestrictiveAnnotations
should be on by default, with a setting to disable it? This way, "unannotated" and@NullUnmarked
would be equivalent out of the box, modulo generated code.AcknowledgeRestrictiveAnnotations
andTreatGeneratedAsUnannotated
; maybe we can do better?