Clarify interactions between settings for generated code, restrictive annotations, etc.

[msridhar]

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 both TreatGeneratedAsUnannotated and AcknowledgeRestrictiveAnnotations are passed, restrictive annotations in generated code are still not acknowledged. Our TreatGeneratedAsUnannotated 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 the AcknowledgeRestrictiveAnnotations 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?

• At the least, we could and should document the above better.
• I wonder if 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.
• I'm not a fan of the subtle interaction between AcknowledgeRestrictiveAnnotations and TreatGeneratedAsUnannotated; maybe we can do better?

[msridhar]

An initial proposal here:

• We replace the AcknowledgeRestrictiveAnnotations flag with something like IgnoreRestrictiveAnnotations, and make acknowledging restrictive annotations the default. This way, "unannotated" and @NullUnmarked mean the same thing by default.
• Maybe rename TreatGeneratedAsUnannotated to TreatGeneratedAsUnannotatedRestrictiveIgnored, for clarity? Not sure about this one
• Right now, the CustomGeneratedCodeAnnotations 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 😅

[lazaroclapp]

• 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.
• 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)

[msridhar]

• 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)

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 becomes IgnoreAnnotationsInGeneratedCode? Now the two settings make sense in any combination, I think.

So this setting currently means two things:

1. Treat code within a @Generated annotation as unmarked
2. Ignore all annotations in @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.