Ability to correctly annotate without immediately breaking callers (e.g. @Migrating)

[wmdietlGC]

Motivation

There is a huge body of existing Java code that only sparingly uses annotations, e.g. some usage of @Nullable.
How can we make it as easy as possible to transition such code to Java with codeanalysis-annotations, where tools enforce correct usage.

Let's take an un-annotated Java file:

class Demo {
  Object m(Object p) ...
}

This class can be used from a context that enforces, e.g., null safety.

@NotNullByDefault
class User {
  void use(Demo d) {
    d.m(null).toString();
  }
}

The code analysis can now either make optimistic or conservative assumptions about the signature of Demo or introduce platform types to check correct usage.
Let us assume that User produces no compile-time errors.

We now want to convert Demo to also be null safe:

@NotNullByDefault
class Demo {
  @Nullable Object m(Object p) ...
}

The code analysis can now ensure that the implementation of Demo is safe.
However, now the usage in User will produce two errors:

• passing null to m, which now defaults to @NotNull
• dereferencing the @Nullable return value of m

Because Demo is now fully annotated (considering the `@NotNullByDefault and explicit annotations) no optimistic defaults would be assumed or no platform types would be generated.

If there are many uses of an API, we need a way to help with this migration.

Proposal

We introduce a new declaration annotation tentatively named @UnderMigration, which is applicable to declarations of fields, methods, type parameters, types, and packages.

In our example, we could annotate Demo as:

@UnderMigration(since = "2019-01-30)
@NotNullByDefault
class Demo {
  @Nullable Object m(Object p) ...
}

This marks Demo as being newly annotated and gives code analysis tools the additional information that the new signatures might produce many warnings.
For example, a tool could decide to initially continue to use platform types, then issue warnings instead of errors for such APIs, and then finally treat the API as final.

The new @UnderMigration annotation separates the semantic information of all codeanalysis-annotations from the migration issues.
The API is annotated with the semantically correct annotations and one annotation can be used for migration instead of mixing this concern with every check.

The annotation would look roughly like:

@interface UnderMigration {
  // yyyy-mm-dd since when the API is under migration.
  String since();

  // Checkers that are under migration.
  // Empty default signifies all checkers.
  // Strings identify checkers, in the same format as used for @SuppressWarnings
  String[] checkers() default {};
}

The since attribute conveys the date when migration began.
Tool users can use this information to decide how to use the signatures.

The checkers attribute conveys which checkers are under migration.
For example, some API might have been converted for null safety, but not yet for @CheckReturnValue.
The strings follow the same format as what @SuppressWarnings would use to suppress warnings from a particular checker.

Applicability to methods, classes, and packages allows nested specification of APIs, e.g. this whole package hasn't been transitioned for null-safety and this class additionally hasn't been transitioned for CRV.

Discussion

How should migration status be designated?

The proposal above includes a single since date that allows users to decide how to handle signatures.
This has the advantage that the API doesn't need to be changed and users are free to decide on severity.

Alternatives considered:

• Use some enum constants to mark how far along the API is. E.g. initially something would be @UnderMigration(status = MigrationStatus.ALPHA), then change to @UnderMigration(status = MigrationStatus.BETA), before becoming @UnderMigration(status = MigrationStatus.FINAL), which is equivalent to having no annotation.
  Compared to the since date this has the disadvantage that the API needs to change to use different migration statuses.

• Similarly, use an enum that describes severity e.g. Usage.INFO, Usage.WARN, and Usage.ERROR. This has the disadvantage that it prescribes tool behavior, which we don't want to prescribe in the API.

Instead use attributes on the analysis annotations

Instead of marking an API as @UnderMigration the type qualifiers could convey migration status, e.g. as @Nullable(since = "2019-01-28").
This has several disadvantages:

• we need to add the same attributes to all code analysis annotations, leading to duplication and maintenance efforts
• type use annotations should generally be short
• it seems error prone to require every parameter/return type to specify different migration levels

Instead, the checkers attribute on @UnderMigration gives us the possibility to mark API as under migration for a particular checker.

Instead use three possible qualifiers

Alternatively, we could give developers the option to specify the "third option" explicitly, e.g. by using @LikelyNonNull or some such. Tools could then choose to interpret these as platform types or handle them optimistically or conservatively.
This has several advantages:

• we need a migration option for every code analysis check
• we mix migration and specification issues
• it seems hard when to choose the third option relative to the other options.

Separating migration issues into @UnderMigration gives us one concept that applies across all code analysis checkers.

Why allow use on type parameter declarations?

This allows to also migrate the annotations on a type parameter bound:

class C<@UnderMigration(since = "2019-01-10") T extends @Nullable Data> {}

Changing the upper bound of a type parameter has an impact on possible instantiations and wildcard uses.
The alternative would require marking at least the whole class as under migration.

Usage in the JDK or Android sources

The @UnderMigration annotations should be usable instead of using @RecentlyNullable/@RecentlyNonNull.
An API would be annotated with a migration date and tools can decide when they want to start enforcing the semantics of these annotations correctly.

[donnerpeter]

I wouldn't rush with this.

First, I haven't observed a need for this in personal experience inside codebases I've worked with.

Second, since specific checkers might treat these annotations a bit differently (e.g. issue errors or warnings) and have their own additional annotations, the migration strategy for them might also be different, and they can add their own annotation with similar semantics describing their own unique needs.

Third (minor), date formats are different in different countries and the annotation shouldn't probably rely on any one specific.

[eaftan]

@donnerpeter I think this is already be a problem for Kotlin interoperating with Java code. If the Java code is initially unannotated, Kotlin code can treat its return value and parameters as either non-null or nullable. If that code is later annotated, then it can break Kotlin callers.

I believe this was the motivation for @RecentlyNullable in the Android SDK -- they didn't want to break existing Kotlin callers, so they created this new annotation and a policy that newly-annotated APIs will start with @RecentlyNullable and then be migrated to @Nullable in the following SDK version.

[donnerpeter]

I see, then probably someone more familiar with Android and Kotlin should evaluate this proposal.

[wmdietlGC]

We discussed this some more in the meeting today. There seems to be a general agreement that having the migration dimension is good, but attaching a date or a version might be too fine grained.

[kevinb9n]

Here's my latest observation about supporting the migration use case.

The best outcome is if the API owner is able to annotate their API fully and correctly, and then make the fewest/simplest orthogonal tweaks possible to account for the migration situation. Later, that tweak can be untwuck.

If we feel confident enough that we can probably reach a solution with that property, then that may give us the freedom to decide that are not going to worry about this requirement yet and layer it in later. It's a risk, but how big a risk is the question. Right now we feel we need a way to prioritize the requirements so we don't have to design everything at once.

[kevinb9n]

Ignoring migration, we've come up with a relatively clean design around three nullness categories (nullable, non-nullable, and nullness-unknown).

Adding @NullnessInMigration (or whatever) to an API element permits dependents to be compiled in one of two different modes:

1. Unmigrated mode: treat all annotations in the encompassed signatures as if they are @NullnessUnknown.
2. Migrated mode: ignore the migration annotation.

Compiling the annotated code itself always ignores the migration annotation, since it must fully validate that the annotations are actually correct.

(NOTE: I have dropped several elements of Werner's original proposal, like a since-date or severity levels, in search of "the simplest thing that can possibly work". I think what I described just now should be enough, but I may be wrong.)

What is still unclear is how to give the user some control over the granularity of this switch - consider different APIs that are migrating on different schedules.

I am slowly switching to viewing this annotation as being nullness-specific. (This could be good or just a symptom of the fact that we're all thinking too much about only nullability.)

[stephan-herrmann]

I very much like the idea of @UnderMigration.

I briefly thought about the correspondence to enhanced deprecation, but

• their since attribute is restricted to JDK versions, so not usable for us
• their forRemoval attribute has no equivalent in static analysis.

Ergo, not much similarity to enhanced deprecation.

I don't follow @kevinb9n in seeing this tied to nullness. Doesn't every annotation that elaborates a kind of API contract benefit from migration support if added after the fact? Hence, I support the idea to use a token like in @UnderMigration("null") (reminds me, that we (=Java tool smiths) still don't have a forum for coordinating suppress warnings tokens as suggested even in JLS).

I wouldn't put more information into the annotation. Rather, interested checkers could offer an option to map certain API to problem severities. For illustration

 -warn:migration=com.library1.* -ignore:migration=org.library1.Foo

This could express that problems relating to API under migration would be reported as warnings for any code under the first pattern, with the exception of class Foo, for which migration related problems should be suppressed. Each checker can choose, which severities it offers for selection (e.g. ignore,info,warning,error).

I believe that some details of how checkers consume this annotations, and what options they provide to users, can be left unspecified, if only the intention is clear.

[stephan-herrmann]

For clarification: how does the discussion in this issue relate to code ownership:

• is it about code owned by the same party where just some internal API have been annotated, while other code hasn't yet reacted to those changes?
• or is it (also) about gradually adopting updated API from 3rd party libraries (with null specifications added between versions)?

It seems these might be different animals altogether?

[kevinb9n]

If JSpecify-type nullness analysis becomes widespread in builds in the future, then this feature request will be suddenly very important to library owners. I'm not 100% sure whether it will start out important to them or not. I think we have been looking at this FR as "if and when libraries start needing it, we'll look more into it then". But if that's likely to happen soon then we'll wish we'd gotten ahead of it a bit.

I'm sorry I dropped responding to the last comments.

Last first: it is definitely most important to provide something 3rd party libraries can use; I don't know the reasons why it wouldn't be useful for your first case too.

I think Stephan is also right that we'd eventually regret a one-off @NullnessInMIgration annotation. @Migrating({key1, key2}) should be the smart move and I don't especially see a problem with it.

The "keys" in that illustration should be knowable and standardized, so I think the obvious choice is class literals of the annotation classes the library code is already importing anyway.

@Migrating(Nullable.class)
@Nullable Foo bar(@Nullable baz);

But, I don't love the idea of having to put @Migrating({Nullable.class, NullMarked.class}) on a scope, so loosely, I'd like for "annotation families" to become a proper concept.

But all the above only works if the end users' tools let them configure mode1 / mode2 (#26 (comment)). Fortunately, I suspect Stephan's right that they're free to each do it their own way and it's only mildly annoying. But if we don't get agreement from at least most tools to support this somehow, the whole idea has no legs.

[cpovirk]

One thing that Artem raised but that I don't think was specifically mentioned here: Kotlin inserts runtime checks (when passing arguments or receiving return values that are annotated as non-null, IIRC). Presumably, any sort of @UnderMigration annotation would not only silence errors/warnings but also disable the insertion of runtime checks.

That said, there wasn't (if I'm reading the room right) a ton of enthusiasm in the meeting today for per-API configuration as much as for per-compilation configuration. (Maybe per-API is more appealing for Android, which has a ton of users who don't all have monorepos and teams responsible for applying updates?) That's not to say that we won't offer per-API configuration, but I think we've rightly labeled this issue as "Post 1.0."

I was going to say that per-compilation configuration is more the domain of individual tools, but it does tie into any work we do to standardize @SuppressWarnings strings -- though that helps only tools that consume Java source, not Kotlin source or Java bytecode.

[kevinb9n]

(Except as revised in my last comment, I still have been seeing this the way I described in #26 (comment), pretty much.)

I'm a little fuzzy on the per-API/per-compilation distinction. Is that just about what scope this annotation sits at? Or is it on the same topic as the second-to-last paragraph ("...granularity...") in the linked comment? Or something else?

In particular, I'm not clear on what makes this post-1.0 even though we seem to have the need for it ourselves internally.

[cpovirk]

The per-API/per-compilation distinction came from a meeting discussion that I probably can't recap fully accurately, so I invite others to correct me, as always.

People have found that, when they annotate an API for nullness, they see nullness violations in tests pretty frequently. So there's a desire to be able to disable nullness checking when building the affected tests. That's the "per-compilation" model: The compilation job for (at least some) tests ignores nullness annotations, but prod code respects them.

Contrast this to a "per-API" model, where no code respects the new annotations yet (or maybe it respects them only in a "warnings mode" or something) because those specific annotations are in a "not yet" mode. That's the @UnderMigration/@Migrating model that we've been otherwise been discussing here.

[cpovirk]

RE: post-1.0: I was under the impression that you and David had come to the conclusion it was post-1.0, and then so did I. But maybe we're now all reversing course :)

(Usual caveat for 1.0 discussions between Kevin and me: I'm setting aside the larger debate of what 1.0 "means" (e.g., whether it should include any "additive" features like this one).)

My historical position has been that a @Migrating annotation is not essential in a Google-like environment: We just update all callers before we submit the CL that contains the annotations. (This is the implicit contrast I was making to Android above.) We've had success with this.

But as Kevin observes, I just today sent him a doc in which I argued that something like this would be useful :) In short, I'm coming to believe that it's important to let library owners annotate their libraries (which is hard enough already!) and submit the changes without also having to block on adding suppressions (or sometimes making manual changes!) in their callers. (Then we need robots and humans (whether the library owners themselves or someone else -- but hopefully it's mainly robots) to finish the job.) I hope that that model is more scalable.

That means we'd want an annotation "like this." But that's a little different from how I'd been envisioning usage of @Migrating in the past: Previously, I'd been looking at it as the annotation for people who need to release a public library and give their users time to update manually (again, the "Android" case). We should give some thought to how those two use cases relate.

[kevinb9n]

We made it post-1.0 due to having such low chatter. Then, chattter!

Your doc reminded me of my primary concern: that I really don't want anything to block anyone from annotating an API correctly. That makes this issue feel essential to me.

I'm going to provisionally 1.0 it based on the new energy around it, and because it's okay if things move back and forth a few times.

[kevinb9n]

Previously, I'd been looking at it as the annotation for people who need to release a public library and give their users time to update manually (again, the "Android" case). We should give some thought to how those two use cases relate.

Oh, I didn't pick up on a difference between these cases.

Here's a new straw-design:

• @Migrating at any scope applies to all participating annotations in that scope
• @Migrating(Nullable.class) applies only to annotations in the scope that consider @Nullable to be part of their family (I am wondering if we can get away with this without super-over-formalizing the concept of a family; not sure)
• Perhaps make it repeatable sooner or later (same reason as @Implies syntax (array or repeatable?) [working decision: array] #215)
• Include ANNOTATION_TYPE target, to support transitive annotation pattern (so someone could define @NullMigrating)
• Could it possibly be that the only effect this needs to have is to make generated findings respect a different suppression string? Then the problem reduces to the same general problem of users needing flexible suppression of any prefix at any scope. I might not be thinking clearly here.

[cpovirk]

Compiling the annotated code itself always ignores the migration annotation, since it must fully validate that the annotations are actually correct.

This is one thing I've started wondering about. How much have you (or others) thought about it?

It seems like the ideal is that @Migrating annotations in "my project" are ignored when compiling "my project," but of course "my project" is an ill-defined concept.

One possibility is that the each individual annotation establishes both the scope of "what has unspecified nullness" and the scope of "what ignores the unspecified-ness," if that makes any sense. That at least gives people the ability to have a whole Java package fully checked for nullness even while @Migrating (albeit with all the usual downsides of package-level annotations).

(Maybe that's tricky for implementations, since the nullness of a given type usage might vary, even within the same file, depending on where it's used from?)

Another possibility is to have different behavior based on whether the annotations are read from source or bytecode, though that feels icky.

[cpovirk]

Since Kevin recently linked here from a discussion inside Google, I'll chime in to say that I think this comment is on point:

What is still unclear is how to give the user some control over the granularity of this switch - consider different APIs that are migrating on different schedules.

I see ~3 conclusions that we could come to:

(1) Users are at the mercy of library owners. We'd try our best to convince library owners to always publish one release with @UnderMigration @Nullable and then another release with just @Nullable. (The syntax is an interesting question but not what I'm trying to get into now :)) But how reliably will users upgrade only one version at a time, always addressing all new failures before the next upgrade?

(2) Control should be left entirely up to users. Thus, there's little point to @UnderMigration. In its place, we'd try our best to convince tool authors to provide a way to say "For the nullness annotations on this package/class/method/parameter/type, issue only warnings" (or even "ignore them entirely"). But are users really going to want to configure, e.g., kotlinc, IntelliJ, and NullAway all to match? And how flexible is the configuration syntax?

(2a) We already have been talking about a way for users to view libraries as annotated differently than they currently are -- stub/overlay files. If we tell users to use those, we don't need for JSpecify or individual tool vendors to design an additional format/mechanism for "which code is under migration": We'd have one format that we'd like to think we can eventually use with all tools (even if only by providing tools to rewrite library jars to contain the desired annotations). Naturally, this conclusion would increase the importance of supporting stub files. (And maybe we'd even still would want @UnderMigration, with the understanding that one way it will be used will be in stub files by users who want to migrate at their own pace.)

We may well end up with a combination of approaches....

[cpovirk]

I realized something: I'd been viewing this feature as enabling the following (among other things):

• I want to add @Nullable to this return type, so I'll add @Migrating @Nullable first.

And it does. But I see now that @wmdietl may have suggested an additional possibility in #27 (comment):

• I want to annotate this return type as non-nullable, but I can't yet because it returns null in some edge case.

(Or maybe that use case is better served by @LessEnforced. Or maybe we should provide a single annotation instead of providing both @LessEnforced and @Migrating :) I overall do not feel that I have a good picture of what we actually want here. In an ideal world, we'd get a lot of time to experiment with any proposed new annotation(s) in some internal codebases for a while before committing to anything.)