Updates to better match main-eisop reference checker
#505
Conversation
| @@ -43,7 +43,7 @@ interface SuperSuper<T extends @Nullable Object> { | |||
|
|
|||
| void checkNeverNull(Lib<? extends Object> lib); | |||
|
|
|||
| <U> void checkUnspecNull(Lib<@NullnessUnspecified U> lib); | |||
| <U extends @Nullable Object> void checkUnspecNull(Lib<@NullnessUnspecified U> lib); | |||
There was a problem hiding this comment.
Without the more flexible bound, there are warnings about invalid inferred method type arguments.
As the bound of U doesn't really matter, I think this doesn't change anything substantially.
Let me know if this isn't correct.
There was a problem hiding this comment.
I think that
jspecify/samples/ComplexParametric.java
Lines 21 to 25 in 8989d39
| static final NullnessUnspecifiedTypeParameter<@Nullable Object> A2 = | ||
| // :: error: jspecify_nullness_mismatch |
There was a problem hiding this comment.
In the really old version of the CF main uses, the error was apparently on the =.
For a long while, it is now on the right-hand side of the assignment, which is here on a different line.
As these are tests for a particular tool, I think it's fine to move the expected error.
For the conformance tests, we should look into ensuring such tests are always on one line.
There was a problem hiding this comment.
Moving it SGTM.
Putting everything on one line in the future sounds good if we can get away with it.
| @@ -43,7 +43,7 @@ interface SuperSuper<T extends @Nullable Object> { | |||
|
|
|||
| void checkNeverNull(Lib<? extends Object> lib); | |||
|
|
|||
| <U> void checkUnspecNull(Lib<@NullnessUnspecified U> lib); | |||
| <U extends @Nullable Object> void checkUnspecNull(Lib<@NullnessUnspecified U> lib); | |||
There was a problem hiding this comment.
I think that
jspecify/samples/ComplexParametric.java
Lines 21 to 25 in 8989d39
| return x; | ||
| } | ||
|
|
||
| <T extends @Nullable Object & @Nullable Lib> Object x8(@NullnessUnspecified T x) { | ||
| // :: error: jspecify_nullness_mismatch | ||
| // :: error: jspecify_nullness_not_enough_information |
There was a problem hiding this comment.
| // :: error: jspecify_nullness_not_enough_information | |
| // :: error: jspecify_nullness_mismatch jspecify_but_expect_warning |
Under our current substitution rules, I think that jspecify_nullness_mismatch is correct: @NullnessUnspecified T can currently "add" nullness to the given type but not "remove" nullness from it. So, if T is a nullable type (as it can be, thanks to the bounds), @NullnessUnspecified T can be nullable, too.
This may change when we look at #248 again, but for now, it's what our rules say, and it's what other samples are set up for.
There was a problem hiding this comment.
(Err, I was assuming above that the checker was currently producing a warning—hence "jspecify_but_expect_warning." But my main point is about why I think "jspecify_nullness_mismatch" is correct. Then we can fill in an "override" like "jspecify_but_expect_warning" or "jspecify_but_expect_nothing" if the checker currently has one of those behaviors and we want to encode it here.)
There was a problem hiding this comment.
I've undone this change.
| static final NullnessUnspecifiedTypeParameter<@Nullable Object> A2 = | ||
| // :: error: jspecify_nullness_mismatch |
There was a problem hiding this comment.
Moving it SGTM.
Putting everything on one line in the future sounds good if we can get away with it.
No description provided.