Skip to content
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

Proper support for type use annotation locations + develop a deprecation path for incorrect locations #708

Open
lazaroclapp opened this issue Jan 3, 2023 · 1 comment
Assignees

Comments

@lazaroclapp
Copy link
Collaborator

For context, see #706 and particularly this summary by @msridhar (slightly edited from here):

Let me try to summarize the overall situation here so we can think about potential solutions and their tradeoffs. #702 addressed an issue where writing List<@Nullable String> list made list itself @Nullable, due to the type use annotation on the type argument (identified in #674 (comment)). The change there was to only consider a type-use @Nullable annotation to be relevant a field / parameter / return type if the annotation was not in an array, inner type, wildcard, or type argument position.

Unfortunately, this change was bad for a couple of reasons:

  1. As shown in [0.10.6] @Nullable ignored and warns as non-null field #705, sometimes type use annotations are in an inner type position even when it doesn't syntactically look like it (see example in [0.10.6] @Nullable ignored and warns as non-null field #705 (comment)).
  2. Syntactically, the "correct" location for a type use annotation is not what NullAway users (who mostly use declaration annotations) are used to. Relevant discussion is in the JSpecify user guide. The change in Fix logic for @Nullable annotation on type parameter #702 unintentionally broke cases where users used a type use annotation (like the Checker Framework or JSpecify @Nullable) and wrote @Nullable Object[] foo2 = null;, as it changed NullAway to only look for the annotations in the syntactically-correct place (Object @Nullable [] foo2).

For inner types, we have a fix, with caveats, in #706. It fixes point 1 above and gets rid of the surprising change in checking behavior for inner types from point 2. It does lead to a bit of weirdness, as shown in here:

" // At some point, we should not treat the foo1 and foo2 as @Nullable.",
" // For now we do, for ease of compatibility.",
" @Nullable A.B.C foo1 = null;",
" A.@Nullable B.C foo2 = null;",
" A.B.@Nullable C foo3 = null;",

In particular, writing A.@Nullable B.C foo2 makes foo2 @Nullable. But this seems like a corner case that shouldn't affect users much right now.

Dealing with arrays is a bit more complicated, since users may want to write annotations to express the nullability of array contents. If we adopt a solution like the one used in this PR for inner types, writing either @Nullable Object[] foo or Object @Nullable[] foo would make foo @Nullable. But, this would mean that if foo were intended to be a @NonNull reference to an array containing @Nullable Objects, a user could not write the correct annotation for this case (@Nullable Object[] foo) without adverse impacts from NullAway (since foo itself would then be treated as @Nullable). Such NullAway users would be forced to either not write the correct annotations for the nullability of array contents for now or to deal with references becoming unintentionally @Nullable (the latter seems worse). A similar issue is seen in #674 for varargs arrays. While this downside is unfortunate, it does match the behavior of NullAway before #702 landed.

We could also just stick with requiring users to write type use nullability annotations for arrays in the right spot. But that kind of breaking change seems weird to introduce by accident, and I'm not sure of other unintended consequences. That seems like a change we should introduce deliberately and possibly behind a flag at first to let users adopt gradually.

So, I think our best option is to handle inner type and array type use annotations in the same way for now, as inner types are currently being handled in this PR. I'm not sure about a combination of these, e.g., A.B.@Nullable C [][] foo = null;. This annotation does not make foo @Nullable under proper usage of type use annotations nor in a backward compatibility scenario. So I guess we can just treat foo as @NonNull for this case?

The stop gap fix for the issues above implemented in #706 does the following:

  • Interpret an annotation on any inner or outer type of the field/argument declaration as annotating the field/argument itself (equivalently: as an annotation on the innermost inner type in proper type use annotation semantics)
  • Interpret an annotation on any sequence of array locations (including @Nullable Foo [][], Foo @Nullable [][], and Foo [] @Nullable []) as marking the array itself as being nullable.
  • We don't support type use annotations on generics or wildcards
  • We don't support type use annotations in locations that involve both inner classes and arrays (so, for A.B.C [] foo the only two valid places for the annotation are @Nullable A.B.C [] foo and A.B.C @Nullable [] foo)

Still, this means we fail to correctly follow standard semantics for type use annotations, which will only get more confusing once we want to introduce JSpecify-like support for annotating the nullability of array elements.

We should, relatively soon, introduce a breaking change intentionally that prevents adding @Nullable in the wrong position for arrays and inner types. To do this, we probably need to refactor some of our location parsing logic in NullabilityUtil to provide more information about what is being annotated/queried. We also want a reasonable UX for developers upgrading past this breaking change and/or switching between declaration and type use annotations at any point after the change (i.e. If possible, we should avoid a situation where changing the imports for @Nullable to a different annotation silently causes nullability to switch from applying to the array reference itself to applying to the elements! Though I am not whether we can do something here...)

At any rate, the version introducing this breaking change should bump the "minor" release number, not the "patch" number (not that NullAway follows semver, per se)

@lazaroclapp lazaroclapp self-assigned this Jan 3, 2023
lazaroclapp added a commit that referenced this issue Jan 4, 2023
This resolves #705 by broadening our slightly overzealous filtering from #702.

We still want to filter type use annotations out when they happen on wildcards or
type parameters. However, we can handle annotations on inner types (i.e. `Foo.@nullable Bar`).
More importantly, these inner types appear implicitly in natural code involving inner classes,
such as:

```
class Bar {
   @nullable Foo foo; // <- this is, implicitly Bar.@nullable Foo !!
   class Foo { }
}
```

Not handling this case leads to confusing an unintuitive errors where a `@Nullable` annotation
with a type use location will seemingly stop working for certain fields at random, if those fields
are of an inner type.

Additionally, as part of this fix, we restore the handling of type use annotations at the start of
an array type as meaning the array object itself is `@Nullable` (i.e. `@Nullable Foo[] arr`, in
addition to the correct type use form `Foo @nullable[] arr`). This is technically incorrect, but
prohibiting it would break backwards compatibility with older versions of NullAway and surprise
users, specially those switching from declaration annotations to type use annotations.

See #708 for a full discussion of the tradeoffs and the path forward towards the proper semantics
for type use annotations.

Co-authored-by: Manu Sridharan <msridhar@gmail.com>
msridhar added a commit that referenced this issue Oct 25, 2023
Added unit tests for annotations on array in JSpecify mode and modified
behavior.

Currently, NullAway does not support `@Nullable` annotations on array
elements (#708). Both `@Nullable String[]` and `String @nullable []` are
currently treated identically, and NullAway considers both annotations
as the array itself might be null.

This is the first step to bring the annotation syntax in line with
JSpecify norms, which treats `@Nullable String[]` as the array elements
could be null while `String @nullable []` implies the array itself could
be null.

After this change, NullAway completely ignores type-use annotations on array element types, such
as `@Nullable String[]`, and only considers type-use annotations in the correct position for the top-level type, i.e.,
`String @nullable []`. This is an intermediary change, and support for
type annotations will be extended eventually. Additionally, this
**only** applies to **JSpecify mode**

Unit tests currently conform to the current behavior of NullAway after
the changes and some tests have TODOs until we support annotations on
type and are able to detect the nullability of array elements.

---------

Co-authored-by: Manu Sridharan <msridhar@gmail.com>
Co-authored-by: Lázaro Clapp <lazaro.clapp@gmail.com>
@cpovirk
Copy link

cpovirk commented Nov 28, 2023

(When you do revisit this, you might find cushon's recent google/error-prone@5123cd5 to be helpful in handling annotations on inner types.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants