Resolve regression for type annotations directly on inner types. #706

lazaroclapp · 2022-12-30T21:09:19Z

This resolves #705 by broadening our slightly overzealous filtering from #702.

We still want to filter type use annotations out when they happen on arrays, 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.

coveralls · 2022-12-30T21:13:02Z

Pull Request Test Coverage Report for Build #1025

0 of 0 changed or added relevant lines in 0 files are covered.
6 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.03%) to 92.96%

Files with Coverage Reduction	New Missed Lines	%
../nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java	6	93.64%

Totals
Change from base Build #1022:	0.03%
Covered Lines:	5189
Relevant Lines:	5582

💛 - Coveralls

lazaroclapp · 2022-12-30T21:11:45Z

nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java

+            "import org.checkerframework.checker.nullness.qual.Nullable;",
+            "class Test {",
+            "  Test.@Nullable Foo f1;",
+            "  @Nullable Test.Foo f2;",


Is it a problem that we treat these two as strictly equivalent for now? Feels like we need to, in order to support the annotationAppliedToInnerTypeImplicitly case.

I think we have to treat them equivalently for now. The former is a correct usage of the annotation, and the latter we must allow for backward compatibility. Do we know why Error Prone complains about the case below but not this one?

lazaroclapp · 2022-12-30T21:13:21Z

nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java

+            "import org.checkerframework.checker.nullness.qual.Nullable;",
+            "class Test {",
+            "  Bar.@Nullable Foo f1;",
+            "  @Nullable Bar.Foo f2;",


Here we could potentially filter the first case, by only allowing inner types if the outer types match the containing scope, but it seems like extra brittleness and I am not sure supporting this is incorrect. Is there a difference in semantics here between the two cases above according to JSpecify or anything else?

Actually, Error Prone does seem to care about this:

error: [NullableOnContainingClass] Type-use nullability annotations should annotate the inner class, not the outer class (e.g., write `A.@Nullable B` instead of `@Nullable A.B`).

Which is annoying, because many devs see the annotation as going on the field, not the type, whether or not it is a type use annotation. So they would write:

@Nullable Bar.Foo f2;

and get an error from that checker.

Still need to look at the whole PR, but yes, according to the spec, type-use annotations should go on the inner type:

https://jspecify.dev/docs/user-guide#type-use-annotation-syntax

We'll have to help developers adjust to this with good error messages.

msridhar

One possible concern

msridhar · 2022-12-31T00:25:01Z

nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java

+    // We care about both annotations directly on the outer type and also those on directly
+    // on an inner type, but wish to discard annotations on arrays, wildcards, or type arguments.
+    return t.position.location.stream()
+        .allMatch(l -> l.tag.equals(TypeAnnotationPosition.TypePathEntryKind.INNER_TYPE));


Why do we need allMatch here? Don't we want to check that there exists an element of the list with tag INNER_TYPE such that it is on the "innermost" type? E.g., what if we have field declaration A.@Nullable B.C foo;; do we want to treat that as a @Nullable reference?

Also we may have a type like A.@Nullable B<@Nullable C>. I think we should treat this as as a @Nullable reference, but allMatch may be too strict? I'm not sure if we'll get a single TypeCompound in two different locations for that case...

Upon further investigation, I think location only has length greater than 1 for multi-dimensional arrays. For a type like A.@Nullable B<@Nullable C> we will get two different TypeCompound objects each of whose position.location lists will be length 1. Update: this is wrong. I think length of location generally relates to level of nesting. Still investigating.

We do need allMatch, because A.B<@Nullable C> shows as having the location array ["INNER_TYPE", "TYPE_ARGUMENT(0)"], so with anyMatch, that one would match.

Think of it like a path: "annotation found at the type argument, position 0, of the inner type".

This is needed, because otherwise stuff like A<@Bar E>.B<C> vs A<E>.B<@Bar C> would be ambiguous if @Bar just says ["TYPE_ARGUMENT(0)"].

So, in your example above, if we see A.@Nullable B<@Nullable C>, what we get is two @Nullable annotations (two calls to isDirectTypeUseAnnotation) one which has the location array ["INNER_TYPE", "TYPE_ARGUMENT(0)"] (the one by C) and one which has location array ["INNER_TYPE"]. We want to match only the second.

We want to match "a path including only a chain of inner types" and that's allMatch.

msridhar · 2022-12-31T00:26:17Z

nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java

+            "import org.checkerframework.checker.nullness.qual.Nullable;",
+            "class Test {",
+            "  Test.@Nullable Foo f1;",
+            "  @Nullable Test.Foo f2;",


I think we have to treat them equivalently for now. The former is a correct usage of the annotation, and the latter we must allow for backward compatibility. Do we know why Error Prone complains about the case below but not this one?

msridhar · 2022-12-31T00:56:36Z

Oof, this is a bit worse than we thought, see the test in f300953. Right now we report an error for the @Nullable Object[] foo = null; line, since that @Nullable annotation has a location of ARRAY kind (i.e., it is saying that the elements of the array are @Nullable according to the spec). To say that the field itself is @Nullable you have to write Object @Nullable[] foo2 = null;.

Again for backward compatibility I think we want to treat both positions as making the field @Nullable for now. It feels weird to only accept the "wrong" version, but I guess we could do that also. WDYT @lazaroclapp?

lazaroclapp · 2023-01-02T04:08:31Z

Oof, this is a bit worse than we thought, see the test in f300953. Right now we report an error for the @Nullable Object[] foo = null; line, since that @Nullable annotation has a location of ARRAY kind (i.e., it is saying that the elements of the array are @Nullable according to the spec). To say that the field itself is @Nullable you have to write Object @Nullable[] foo2 = null;.

Again for backward compatibility I think we want to treat both positions as making the field @Nullable for now. It feels weird to only accept the "wrong" version, but I guess we could do that also. WDYT @lazaroclapp?

We specifically don't have JSpecify-like semantics for arrays yet, do we? I think this is an orthogonal concern to the purpose of this PR, but happy to have the test there if we currently handle the "correct" case. I think whenever we support the nullability of elements of arrays, that will be a major breaking change either way, since we always used the opposite semantics here from JSpecify.

Edit: Actually, it very much looks like we don't handle the Object @Nullable[] foo case at all. I think that's potentially a good thing? Would be easier to migrate and less confusing if we introduce the new semantics carefully when adding support for nullability of array elements, rather than in this PR. We might consider a two step approach where we first ask everyone to migrate to the Object @Nullable[] foo style upon upgrade, before we add support for nullability of elements (so we can report an error in that case), but I think we need to be careful there either way, and not sure that should be part of this change.

msridhar · 2023-01-02T21:56:10Z

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:

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)).
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 Map.Entry foo1 = null; or @Nullable Object[] foo2 = null;, as it changed NullAway to only look for the annotations in the syntactically-correct place (i.e., Map.@Nullable Entry foo1 or Object @Nullable [] foo2).

For inner types, I think I'm fine with the solution currently implemented in this PR. 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:

NullAway/nullaway/src/test/java/com/uber/nullaway/NullAwayCoreTests.java

Lines 1113 to 1117 in 1b1a5b1

    
           "  // 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?

Ok, those are all my thoughts for now. It's a messy situation but it's good that we are at least understanding the scope of the issue finally.

lazaroclapp · 2023-01-03T18:28:10Z

Just as a note:

2. 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 Map.Entry foo1 = null; [...] , as it changed NullAway to only look for the annotations in the syntactically-correct place.

I don't think this is true (see the original test case). The change is only for arrays, because we are explicitly excluding arrays as a location, but it would be fine when annotating the outermost type, because that's just the location path [].

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?

We could. This could be as easy as requiring that either all components of the location are inner types or all components of the location are arrays, but not a mix of both. There is an annoying edgecase where:

class Test {
  Test.@Nullable Foo[][] fs;
  class Foo { }
}

given explicitly does produce path [ARRAY, ARRAY, INNER_TYPE] for fs.

But:

class Test {
  @Nullable Foo[][] fs;
  class Foo { }
}

shows up as just [ARRAY, ARRAY] (which means we would allow it with our backward compatibility considerations).

Maybe the later is fine from the point of view of developer ergonomics? It's still a bit confusing, though...

msridhar · 2023-01-03T20:34:00Z

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 Map.Entry foo1 = null; [...] , as it changed NullAway to only look for the annotations in the syntactically-correct place.

I don't think this is true (see the original test case). The change is only for arrays, because we are explicitly excluding arrays as a location, but it would be fine when annotating the outermost type, because that's just the location path [].

Yes I think you're right; my mistake.

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?

We could. This could be as easy as requiring that either all components of the location are inner types or all components of the location are arrays, but not a mix of both. There is an annoying edgecase where:
class Test {
  Test.@Nullable Foo[][] fs;
  class Foo { }
}
given explicitly does produce path [ARRAY, ARRAY, INNER_TYPE] for fs.

But:
class Test {
  @Nullable Foo[][] fs;
  class Foo { }
}
shows up as just [ARRAY, ARRAY] (which means we would allow it with our backward compatibility considerations).

Maybe the later is fine from the point of view of developer ergonomics? It's still a bit confusing, though...

Everything we do here is going to be confusing for a while 🙂 I think allowing for all inner types or all arrays is probably the best thing to do for now.

msridhar

Mostly LGTM! Approved assuming minor comments below are addressed

msridhar · 2023-01-03T22:33:52Z

nullaway/src/test/java/com/uber/nullaway/NullAwayTypeUseAnnotationsTests.java

+            "import org.checkerframework.checker.nullness.qual.Nullable;",
+            "class A { class B { class C {} } }",
+            "class Test {",
+            "  // At some point, we should not treat foo1, foo2, or foo4 as @Nullable.",


Just foo1 or foo2? Looks like foo4 is now not treated as @Nullable.

msridhar · 2023-01-03T22:35:33Z

nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java

+    // proper deprecation of the incorrect behaviors for type use annotations when their
+    // semantics don't match those of a declaration annotation in the same position.
+    // See https://github.com/uber/NullAway/issues/708
+    boolean location_has_inner_types = false;


Stick with standard lowerCamelCase?

Suggested change

boolean location_has_inner_types = false;

boolean locationHasInnerTypes = false;

Ugh, sorry, the perils of alt-tabbing between Python and Java code 😅

msridhar · 2023-01-03T22:35:52Z

nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java

+    // semantics don't match those of a declaration annotation in the same position.
+    // See https://github.com/uber/NullAway/issues/708
+    boolean location_has_inner_types = false;
+    boolean location_has_array = false;


Similarly:

Suggested change

boolean location_has_array = false;

boolean locationHasArray = false;

…es. (uber#706)" This reverts commit c5e1a79.

Resolve regression for type annotations directly on inner types.

ff722ef

lazaroclapp requested a review from msridhar December 30, 2022 21:09

More explicit testing.

214cfcd

lazaroclapp commented Dec 30, 2022

View reviewed changes

lazaroclapp mentioned this pull request Dec 30, 2022

[0.10.6] @Nullable ignored and warns as non-null field #705

Closed

msridhar reviewed Dec 31, 2022

View reviewed changes

add test involving arrays

f300953

msridhar force-pushed the lazaro_fix_705 branch from 1ad02f3 to f300953 Compare December 31, 2022 00:53

more tests and comments

f43a69a

msridhar added 2 commits January 2, 2023 13:30

add a multi-level inner type test

6e26322

another test

1b1a5b1

msridhar mentioned this pull request Jan 3, 2023

Update to newer version of NullAway wala/WALA#1200

Merged

lazaroclapp mentioned this pull request Jan 3, 2023

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

Open

lazaroclapp added 2 commits January 3, 2023 17:13

Array fixes and refactor tests

69d4e65

Move test class to correct package, and add copyright header

0adb703

msridhar approved these changes Jan 3, 2023

View reviewed changes

Fixes from review

bf3fdb4

lazaroclapp merged commit c5e1a79 into master Jan 4, 2023

msridhar deleted the lazaro_fix_705 branch January 4, 2023 00:23

msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 18, 2023

Revert "Resolve regression for type annotations directly on inner typ…

490e955

…es. (uber#706)" This reverts commit c5e1a79.

msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 19, 2023

Revert "Resolve regression for type annotations directly on inner typ…

7265611

…es. (uber#706)" This reverts commit c5e1a79.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve regression for type annotations directly on inner types. #706

Resolve regression for type annotations directly on inner types. #706

lazaroclapp commented Dec 30, 2022

coveralls commented Dec 30, 2022 •

edited

Loading

lazaroclapp Dec 30, 2022

msridhar Dec 31, 2022

lazaroclapp Dec 30, 2022

lazaroclapp Dec 30, 2022

msridhar Dec 30, 2022

msridhar left a comment

msridhar Dec 31, 2022

msridhar Dec 31, 2022 •

edited

Loading

lazaroclapp Jan 2, 2023 •

edited

Loading

msridhar Dec 31, 2022

msridhar commented Dec 31, 2022

lazaroclapp commented Jan 2, 2023 •

edited

Loading

msridhar commented Jan 2, 2023 •

edited

Loading

lazaroclapp commented Jan 3, 2023

msridhar commented Jan 3, 2023

msridhar left a comment

msridhar Jan 3, 2023

msridhar Jan 3, 2023

lazaroclapp Jan 3, 2023

msridhar Jan 3, 2023

	boolean location_has_inner_types = false;
	boolean locationHasInnerTypes = false;

	boolean location_has_array = false;
	boolean locationHasArray = false;

Resolve regression for type annotations directly on inner types. #706

Resolve regression for type annotations directly on inner types. #706

Conversation

lazaroclapp commented Dec 30, 2022

coveralls commented Dec 30, 2022 • edited Loading

Pull Request Test Coverage Report for Build #1025

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msridhar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msridhar Dec 31, 2022 • edited Loading

Choose a reason for hiding this comment

lazaroclapp Jan 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msridhar commented Dec 31, 2022

lazaroclapp commented Jan 2, 2023 • edited Loading

msridhar commented Jan 2, 2023 • edited Loading

lazaroclapp commented Jan 3, 2023

msridhar commented Jan 3, 2023

msridhar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 30, 2022 •

edited

Loading

msridhar Dec 31, 2022 •

edited

Loading

lazaroclapp Jan 2, 2023 •

edited

Loading

lazaroclapp commented Jan 2, 2023 •

edited

Loading

msridhar commented Jan 2, 2023 •

edited

Loading