Consider nullable annotations in explicit nulls #21953
Conversation
| return as; | ||
| } | ||
|
|
||
| @Nullable |
There was a problem hiding this comment.
We should test annotations at parameter positions as well: f(@NotNull String s), g(@Nullable String s)
There was a problem hiding this comment.
It seems like @NotNull is currently not working in parameter position.
Also, I am not sure about how to test @Nullable in parameter position, since we should be testing that the parameter is a (String | Null) instead of a String?.
I discussed with @olhotak, and one possible way is through overriding, where the following code would fail
public class JavaClass {
public int f(@Nullable String s)
}
class ScalaClass extends JavaClass {
override def f(s: String): Int
}
However we handle overriding logic through a special case for explicit nulls, so this currently doesn't fail. We might have to change the behavior in some other files.
There was a problem hiding this comment.
I discussed offline with @noti0na1 . We agreed that we should try to remove the special cases in the overriding logic for explicit nulls. Flexible types would make them unnecessary. I've create a new issue #22071 about this.
After that's done, it should be possible to create such tests here using overriding.
| @@ -95,8 +98,17 @@ object JavaNullInterop { | |||
| * This is useful for e.g. constructors, and also so that `A & B` is nullified | |||
| * to `(A & B) | Null`, instead of `(A | Null & B | Null) | Null`. | |||
| */ | |||
| private class JavaNullMap(var outermostLevelAlreadyNullable: Boolean)(using Context) extends TypeMap { | |||
| def nullify(tp: Type): Type = if ctx.flexibleTypes then FlexibleType(tp) else OrNull(tp) | |||
| private class JavaNullMap(var outermostLevelAlreadyNullable: Boolean, explicitlyNullable: Boolean)(using Context) extends TypeMap { | |||
There was a problem hiding this comment.
I don't think the NotNull/Nullable annotation on the symbol should affect any types nested inside.
There was a problem hiding this comment.
I'm not sure what you mean by this, I add the explicitlyNullable flag here to distinguish the case between a flexible type and an OrNull. Are you referring to the places where we call this in apply?
Fixes #21629
Not sure if the tests are sufficient.