Annotation to negate @NullMarked at a narrower scope

[cpovirk]

That is, should we have a @DefaultNullnessUnspecified/@NotNullAware annotation that overrides any @DefaultNonNull/@NullAware annotation that is in scope?

We have been proceeding as if the answer is "yes" (e.g., in #41). Maybe we can settle that quickly. Doing so would simplify the remaining discussions about partially annotated code. Now seems like as good a time as any to summarize those discussions (building on kmb's table):

• whether to have a @NonNull annotation ([moved to #230] Decide whether to include a @NonNull annotation #4; name already chosen in Choose non-null annotation name #44)
• whether to have a @NullnessUnspecified annotation (Ability to apply unspecified-nullness to an individual type usage (e.g. a type-use annotation like @UnspecifiedNullness) #137; naming discussion in Naming the third nullness annotation (the one that matches the behavior of unannotated code) #32; some use cases we haven't endorsed in Address the "findViewById problem" (generally nullable, but in *common* specific cases not) #71)
• whether the defaulting annotations can be applied at levels above (@NullMarked (and its negation) includes module and package targets #34) or below (Allow annotating null-markedness at method/constructor level? #43) a whole class
• the names of these defaulting annotations (Name(s) of the annotation(s) expressing "in the nullness club" or "not" #87), based on the naming scheme for defaulting annotations in general (Common pattern for a set of exclusive scope-annotations (e.g. null-marked and null-unmarked) #109)

I'll try to dump some thoughts before leaving for the day.

[cpovirk]

First, let's set up an assumption. The concern: This annotation would make less sense if defaulting annotations were permitted only at the class level. In that case, the only way for one defaulting annotation to override another would be for one to be located on a class nested inside the other. However, it seems likely that we will allow annotations on other scopes. And even if we don't, there may be at least a little value in providing users with a way to explicitly label every top-level class as either "in the club" or "out of the club." (Users might choose to enforce this with presubmit checks.) So, for purposes of this discussion, let's proceed under the following assumption:

• The defaulting annotations will be permitted on some additional scope besides just class scope.

[cpovirk]

With that out of the way... some pros and cons:

＋ makes it possible to "flip the default" for new code in a package/class

• That is: I can put @DefaultNonNull/@NullAware on package/class but then override it for all existing APIs by annotating each with @DefaultNullnessUnspecified/@NotNullAware. Then, new APIs must be annotated for nullness, and old ones can be cleaned up at my leisure.
• We have had success with this approach at Google with Error Prone's @CheckReturnValue checker.
• I also like the idea that, as you annotate more code for nullness, you remove annotations like @DefaultNullnessUnspecified rather than add annotations like @DefaultNonNull. It provides a way for code to get less noisy as you annotate for nullness (partially offsetting the noise added by @Nullable, <T extends @Nullable Object>, etc.).

＋ consistent with a "cascading" pattern we're likely to employ for non-nullness-related annotations

• My go-to example, again, is @CheckReturnValue+@CanIgnoreReturnValue.

＋ lets users explicitly label APIs as "intentionally not annotated for nullness, at least for now" rather than leaving open the question of whether someone just forgot to consider annotating them

• (This is part of what I discussed in my "assumption" post above.)
• [edit: Users could also express this with a plain comment: // TODO(cpovirk): Add @DefaultNonNull. (Arguably users should include a TODO comment even if they do include @DefaultNullnessUnspecified!) Still, an annotation may be easier for some tools to detect. Plus, an annotation is authoritative: If a class has @DefaultNullnessUnspecified on it, then that is the annotation in effect for that file. In contrast, if it has // TODO(cpovirk): Add @DefaultNonNull, it's possible that the class is in fact already covered by a package- or module-level @DefaultNonNull.]

－ one more thing for users to understand

• Hypothetical user says: "Hmm, @DefaultNullnessUnspecified. I thought that 'unspecified' was the default. So what is the default?"
• Other hypothetical user says: "Hmm, @DefaultNullnessUnspecified, @DefaultNonNull. I guess the default for code with neither of those is @Nullable?"
• [edit: Note that both of these particular confusions may be avoidable by picking a different name, as discussed under the next bullet.]

－ one more thing for us to name (previously, previously)

• Note that including this "negative," "not in the club" annotation may also constrain our choices of names for the "positive," "in the club" annotation: If we want a clear mapping between the two, then that might be a reason to prefer names like @NullExplicit+@NotNullExplicit over names like @DefaultNonNull+@DefaultNullnessUnspecified.
• [edit: And of course naming things takes time, though hopefully not too much time.]

[kevin1e100]

It's linked from the previous comment, but for more explicit reference, #87 was arguably filed under the assumption that the answer to this here issue is, yes, there should be a defaulting annotation to undo "in the club".

That issue also proposes a solution, InTheClub(OFF), InTheClub(false), or InTheClub.Off, that could minimize the two minuses in the previous post. Curious for your thoughts on that @cpovirk.

[cpovirk]

Filling #137 reminded me that Android wanted findViewById to return a type with unspecified nullness (at least until requireViewById catches on). If we want to support that, then we'll need at least one of:

• a @NullnessUnspecified type annotation (Ability to apply unspecified-nullness to an individual type usage (e.g. a type-use annotation like @UnspecifiedNullness) #137)
• a @NotInTheClub defaulting annotation (this issue) applicable at the method level (Allow annotating null-markedness at method/constructor level? #43)
• (least convenient but technically sufficient) some way to annotate all other types in the Activity class while leaving these ones untouched (such as the ability to apply @InTheClub to every method except for this one (again, Allow annotating null-markedness at method/constructor level? #43))

A fuller discussion of that may belong on #71 or a findViewById-specific issue that we fork off that.

[cpovirk]

I wonder if this will also be useful for generated code: If I want to put @InTheClub on my package/module but some of my sources are generated by a JSpecify-oblivious code generator, then it would be nice for the generator to be able to annotate them as @NotInTheClub. (Well, it would be nice for the generator to full support JSpecify, but that's not going to be realistic to expect all the time.) But of course we can't necessarily expect the generator to do even that. So this may end up being a case in which users need to avoid setting a default above the class level in the first place :( I'll note as much in #34.

[kevinb9n]

I've been assuming this would probably be called @NullUnmarked, now.

It seems important to me, for supporting migration.

[netdpb]

I think having such an annotation simplifies migration and other use cases and presents no real additional complexity to the model, and so we should.

[sdeleuze]

+1 from me as well, that will help for migration but also will help the project and tooling support to be "unplugged" for advanced use cases where the behavior is not the one expected. That will allow to wait until it is identified if:

• That's a spec issue
• That's a tooling bug
• That's due to the user project having to upgrade semantics and documentation to comply with the spec

That will avoid people to be blocked when the tooling is configured to throw errors not warnings which is what we plan to do.

[kevin1e100]

Ergonomics-wise, I like having an annotation for this among other things to make it easier to to annotate a package or module `@NullMarked`. Specifically, it avoids having to remember annotating each new class in a package or module that has some files that aren't ready to be opted in yet. Using this annotation on something like an inner class (or a method) could also be useful but does seem to create some complexity (or at least room for misunderstanding) if the outer class has type parameters. In particular, the question becomes how to interpret usages of the outer class's type parameters in the inner class. Minimally we'll have to define that case, but I'm also simply not sure what most users will expect to happen in that case, and how close we can and should get to that expectation.

[kevinb9n]

Good point - I copied it to #43.

[kevinb9n]

This might be fairly resolvable as a design question, but I suppose the issue can just stay open until we actually check it in and plumb it through all the samples and docs.

(In general we might benefit from separating design issues from impl issues more -- and it is already the case that design issues have been marked closed where we still need to go back and make sure we actually worked them into everything where needed.)

[cpovirk]

Sorry, there's some history that I failed to record here: This came up in one of our meetings last year, and there were no objections raised to eventually adding it. The meeting conclusion was a bit more subtle than that, though: We said that we'd try to get some actual users on board and then wait for them to report a need for this to us.

I personally think that that need is going to arise for all the reasons discussed above.

Still, I see some appeal in taking the more empirical approach so as to set a precedent for other feature requests. (I'm confident that we'll get some "biggest pain point" feedback on this if we just wait long enough.)

On the other hand, I also see some strategic value in "locking in" this annotation that we're highly confident that we'll need, since that lets us provide users with the annotation, and that advances the conversation to "Which annotation do we need next?" (And it's worth noting that this process will take months.)

On the other other hand, I have been pushing to defer issues where practical: Every moment we spend on followup issue #109 (which in turn requires discussion of #200) is another moment not spent on, e.g., preparations for opening up a presence to the Internet at large. (But that's probably a more general prioritization discussion than we should have on this thread. Plus, again, if the process of deciding and implementing these issues is going to take a long time, I'm not sure if that's a point in favor of starting them now or a point against!)

For purposes of this GitHub issue, I think I'm personally happy to just turn all the above into a "yes" at this point, especially knowing that all decisions can be revisited given new information -- probably meaning, in this case, a shocking lack of information about anyone who needs this annotation. But if anyone has been reading this and thinking "What happened to what we said last October?" then please do speak up. Again, I'm sorry for not recording that here at the time.

(Aside: Good point about design issue vs. impl issues.)

[kevinb9n]

I believe it has essentially been resolved that we do want this, for a while.

My reasoning is this: this is an adoptability feature, which replaces stair-step hurdles with a smoother ramp that's easier to slide up a bit at a time. In particular, this feature would mean that partly-migrated files could nevertheless already have the property that new methods added to them are automatically in the club, and I think this is very good.

Furthermore: features that exist to make adoption easier are not good choices for things "we can just add later", because it's in OUR interest to get as much adoption and thus feedback early (before it's too late) as we can. So, the time to make it easy to adopt is now.

We'll attempt to decide its exact shape in #109, together with a couple other examples of other sets of exclusive scope-annotations we might add.

[netdpb]

Just a note that tool providers (including @lazaroclapp) have raised the issue that supporting arbitrary negative/positive containers might be hard to do efficiently. I think that's worth thinking about from an implementation perspective, but I still think the value of this feature is great enough that we should figure out how to make it feasible.

[cpovirk]

I gave my thumbs-up emoji to having support for this.

2 quick comments:

• RE: tool efficiency: I had gotten the impression from @lazaroclapp that the performance concerns might fundamentally exist already: You can already put @NullMarked on a nested class, so tools' logic already potentially has to look the whole way up the chain of nested classes for any @NullMarked annotation. In particular, in the case in which code has no @NullMarked annotation, tools are going to have to look all the way up the chain in order to confirm that.
• RE: new methods: If we want the ability to put @NullMarked on a class and @NotNullMarked on its methods, then we also also need to Allow annotating null-markedness at method/constructor level (#43). Without that, a @NotNullMarked annotation is still useful but "only" for the case of letting a class override a module-level or package-level (or enclosing-class-level, but that's less likely) @NullMarked.

[kevin1e100]

Just to be clear, I'd like to avoid this annotation if possible, and I certainly would like to keep it out of 1.0. There are a lot of reasons but among other things I'm simply not seeing that this has that much bearing on adoptability. I'm not sure what the protocol is but I'll reopen this issue for now.

[cpovirk]

Thanks for the update! One question: Is that...

• Performance should be acceptable for a class as long as @NullMarked/@NullUnmarked appears only on the top-level class, or...
• Performance should be acceptable for a class only if NullAway never looks for @NullMarked/@NullUnmarked on nested classes at all?

I'm not sure if I asked that clearly :\ What I'm trying to get a sense of is whether we can tell users "If you annotate non-top-level classes, your performance will be bad" or "If you annotate non-top-level classes, NullAway 'won't work'" (in which case we would have more reason to avoid offering such a feature in the first place).

[msridhar]

Hi @cpovirk, it would be more of a case of "If you annotate non-top-level classes, NullAway performance may deteriorate a bit, for those compilation units only." The annotations would still be supported in those cases, though.

My main concern before was avoiding a perf regression for NullAway users who are using NullAway's extant mechanisms (command-line options) for indicating which code is marked, and I now think we can achieve this.

[kevinb9n]

Given the potential complications we'd have to work through (listed above and possibly more), why not hold off until there's more data available?

I'll resummarize the case I see:

• This feature eases cliffs in the adoption curve
• A smoother adoption curve means more adoption (not obvious: perhaps cliffs get people to "just push through" and end up adopting more; I don't think this is one of them)
• We want to ease adoption early, not late, because the more uptake the better before we freeze forever

As for disadvantages:

• Any disadvantages to users of partly-migrated files are best resolved by continuing the migration, so don't constitute disadvantages of the feature
• Disadvantages of the feature seem mainly hypothetical at this point

Of course, not doing this is a valid option. We can not do it. I'm only explaining why it looks better to do it.

[netdpb]

I suppose one argument against such an annotation is that we expect/want its usage to go to zero in the long term.

[kevin1e100]

From what I'm hearing the case for this annotation rests on it being critically needed for adoption.

But what I've heard so far is that it could be useful in some scenarios for some hypothetical users, and there is a workaround. Specifically the use case, as I understand it, would be someone wanting to mark a class (package, respectively) as @NullMarked but exempt some of that class's methods (package's classes, respectively), so that methods added in the future are automatically opted in. (But there is no concrete case known where someone wants to do that.) To me it simply seems that:

• this will only come up for a small subset of users, since several intersecting conditions (some but not all methods couldn't initially be considered null-marked + desire to follow JSpecify's conventions going forward + expectation that more methods will be added) have to align
• there is a trivial workaround (though it may in some cases be syntactically unfortunate)

The combination of these two simply makes me think this is at best nice to have. In particular I'm honestly not seeing evidence that the annotation would critically enable JSpecify adoption for a lot of users.

As @kevinb9n conceded above, the burden of proof is in introducing the annotation, so I'd suggest we take this to an issue doc and hash it out more completely, instead of through comments addressing one concern or another, as I'm admittedly continuing to do here.

[kevinb9n]

I suppose one argument against such an annotation is that we expect/want its usage to go to zero in the long term.

Uh oh. This led me to

• It's fair to assume that some codebases would want to ban this annotation.

(Note: I'm using "fair to assume" to mean "technically hypothetical, but doesn't really warrant skepticism" and people are obviously allowed to disagree with it, it is just my judgment.)

In fact for any fully migrated or new codebase we would basically want them to ban it.

But that means we would end up wanting to support them, with something like @NullMarked(okay_to_negate_it = false). RIght here is where I get that special queasy "slippery-slope" sensation 100% (I'm not making a claim about that being rational or irrational, I just do).

[kevinb9n]

This issue requires restructuring and resummarizing before we can make further progress on it. I'm signing up to do that... in the meantime, anyone is welcome to offer quick thoughts but don't necessarily invest much time in trying to follow all the twists and turns.

[lazaroclapp]

Some thoughts based on the meeting notes (sorry I missed it today) and conversations with Manu:

1. While annotating a few large targets at Uber, I did run into a use case for @NullUnmarked, primarily around wanting to exclude a few top-level classes from annotation (I don't have a single example were it makes sense to do this for a single method). Generally, this was due to initialization:

• The largest class were a couple of classes whose constructor was passed a large number of @Nullable values as a result of some JSON deserialization code and directly assigned those to fields. I am not the author of the original code and I didn't have enough domain knowledge to distinguish between: "these are mandatory fields of the JSON, expected to be non-null, and thus the field should be marked @NonNull and a parsing error injected to guard against malformed JSON" and "this is an optional field in the JSON and thus a @Nullable field in the class". Marking all fields as @Nullable would have caused undue propagation of errors. In this case, it would have been useful to be able to mark these few classes as @NullUnmarked and enroll the rest of the target, then file tickets with the code owners about those specific cases. (A suppression on the constructor works too, but @NullUnmarked might be a better way to say "I don't know the nullability of this fields right now" vs "I know they are @NonNull but the checker doesn't").
• A smaller case was two classes without constructors and a few named public fields (basically used as named tuples). It was clear that the fields were all meant to be assigned non-null before usage, but since there was no constructor, NullAway couldn't prove they would be initialized. Our solution was a NullAway specific mechanism to suppress warnings about initialization, but I can imagine a different tool marking them as @NullUnmarked. In either case, the next step will be making a follow up code change that adds constructors and getters and makes this fields private unless there is a good reason not to.
• I guess more broadly the point to make is: sometimes, when the person enrolling the code is not the original author, it makes sense to punt on some parts of the target, and it's usually enough to be able to punt at the top-level class level.

2. For NullAway and other performance-sensitive tools, supporting @NullUnmarked doesn't seem to be more costly from an implementation standpoint compared to supporting only @NullMarked at the same sites. For code being called outside the current compilation unit, either case requires us to traverse the symbol hierarchy up looking for the closest enclosing @NullMarked/@NullUnmarked annotation (likely with an aggressive cache). For source code under analysis, we can handle this by having a fast mode for code that uses these annotations only at the package or top-level class level, and falling back to more expensive tree path traversals when we see such annotation in an inner-class (or method), which we expect to be rare. Theoretically we can support these at method-level too, in a fairly similar way, but see (1) above, and every extra place we support this annotation at adds complexity/overhead.

[kevinb9n]

TODO: reopen this with new summary, as this is one of the 3 most vertical issues we have open for 1.0 currently.

[cpovirk]

One thing I just said in an internal discussion:

Background: One problem with package-level @NullMarked is that it applies not only to any prod code in the package but also to any tests. That's just how packages work. This behavior will not necessarily be obvious to users. Plus, we can expect many users to focus on annotating their prod code first, leaving a window during which a package's prod code is annotated but its test code is not. Yet tools (like IntelliJ, I recently saw) will consider the annotations to apply to the test code, leading to justified but unhelpful findings (like claiming that a field is never null because it's enclosed by @NullMarked and not annotated with @Nullable).

After seeing this in practice, I realized that we could make package-level @NullMarked a little more usable by putting @NullUnmarked on test classes. This still provides the benefit of flipping the default for a package while letting tests "escape." (And if people forget @NullUnmarked, the effect is limited to tests.) Of course, it's verbose, too. But it has the advantage of putting that verbosity in tests (which already have annotations like @Test and which are probably looked at less often).

This would still leave issues with package-level annotations (which are probably discussed somewhere in #30 and/or #34 and/or elsewhere). But if people really want to use package-level annotations (and I'm sure some will), then their lives would get a little better if they had the option of subsequently applying @NullUnmarked to their test classes.

[lazaroclapp]

Plus, we can expect many users to focus on annotating their prod code first, leaving a window during which a package's prod code is annotated but its test code is not. Yet tools (like IntelliJ, I recently saw) will consider the annotations to apply to the test code, leading to justified but unhelpful findings.

I agree this is a good use case for @NullUnmarked. One side note of this, though, is that depending on the tool, you can also solve the above situation by having both tests and prod code be @NullMarked, but only running your checker tool on prod code. Arguably, no prod code for the current target or anything from a different target should be depending on the code for unit tests for the current target (let alone, say, third-party code depending on unit test code for a library), so this would be doable for a checker like NullAway, where the checker might be entirely disabled at the level of each javac invocation based on build system logic (.bzl files). In fact, internally, we consider having NullAway enabled for both prod and tests (our Android default) to be a stricter configuration than just for prod code, which is the default for server-side Java code. Though, as you note, this might be less practical for IDEs that show different hints/warnings to developers for @NullMarked vs @NullUnmarked code by default.

[cpovirk]

Thanks. That makes sense, and it makes me see that package-level @NullMarked could interact badly with "What if a tool provided an option to check only @NullMarked code?": Under a tool that implemented that option, a package-level @NullMarked would turn on checking for tests, too. (Now, I could imagine that tools could refine their behavior to "check only @NullMarked code that is not a test." Or again, people could use @NullUnmarked. But that's drifting further from the "simple" approach I'd hoped for.) [edit: Oops, I had already said that in #236 (comment), but I forgot :)]

Another possible issue is tests written in Kotlin: Kotlin is always applying nullness checking, so there's no option to run it only on prod code. Fortunately, Kotlin code also is "annotated for nullness" :) So the issue would arise only if all the following conditions were met:

• A package has a @NullMarked annotation to indicate that its prod Java code has been annotated.
• The test directory for the package contains Java code that has not been annotated.
• The Java code in the test directory is used from Kotlin code (presumably because the Java code contains "test utilities").

Then we could see the Kotlin compiler reject code or insert runtime null checks that cause problems—at least until someone applies @NullUnmarked to the test utilities.

(I could also imagine similar problems with a hypothetical Java bytecode rewriter that automatically inserts null checks similar to the ones that Kotlin inserts automatically. But I think we are quite far from the time at which anyone would try that. And anyone who did try it would likely have to make exceptions for code that is clearly test code, among the many, many other exceptions that would likely be necessary in real-world code.)

[lazaroclapp]

Under a tool that implemented that option, a package-level @NullMarked would turn on checking for tests, too.

Only if the tool runs on test code at all, mind you.

Now, I could imagine that tools could refine their behavior to "check only @NullMarked code that is not a test."

That would be somewhat hard to do within the context of a javac plugin, I think. At least "from the inside" of the plugin. The configuration I describe above is much simpler: the NullAway checker is not anywhere in any relevant classpath when compiling test code. So it isn't, "the tool gracefully handles the prod/test distinction", but "due to how our build system is set up, the tool doesn't even exist when compiling test code". At a high enough level where the build system config + NullAway is "the tool", then your description matches our situation too, though.

That said, in the general case: @NullUnmarked and maybe @Test => @NullUnmarked might be the general principled fix that doesn't depend on the build system configuration internals. Of course, test utilities would need their own @NullUnmarked sub-package or individual annotations, whether the main test fixtures are in Kotlin or tool-checked Java (for broad definitions of tool-checked, including the IDE).

p.s. Re: bytecode rewriters and such: One of the areas were NullAway does have a lot of edge cases handling is in detecting generated code (generated .java files created unmarked in specific filesystem directories or packages, @Generated annotated code, Lombok AST mutations during compilation in memory, etc, etc). @NullUnmarked can make that easier if and only if the code generator is aware of our annotations and uses them correctly, but presumably, in an ideal world, tools that know enough about JSpecify to declare their generated code as @NullUnmarked could just generate @Nullable annotated code 😅 .

[kevinb9n]

We are comfortable with a Working Decision to allow @NullUnmarked at the same scopes as @NullMarked. A new issue should be filed to discuss changes to this plan and should summarize the best arguments given here.

[netdpb]

Current decision: Yes, this feature is important to enable incremental migration of legacy code.

Argument for changing: None was made other than it may not be necessary.

Timing: This must be decided before version 1.0 of the jar.

Proposal for 1.0: Finalize the current decision. If you agree, please add a thumbs-up emoji (👍) to this comment. If you disagree, please add a thumbs-down emoji (👎) to this comment and briefly explain your disagreement. Please only add a thumbs-down if you feel you can make a strong case why this decision will be materially worse for users or tool providers than an alternative.

Results: This proposal received 6 👍s and no other votes.

[netdpb]

Decision: Yes, JSpecify will have an annotation to negate @NullMarked at narrower scope.