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

Consider nullable annotations in explicit nulls #21953

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions compiler/src/dotty/tools/dotc/core/Definitions.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1125,6 +1125,11 @@ class Definitions {
"reactor.util.annotation.NonNullApi" ::
"io.reactivex.annotations.NonNull" :: Nil)

@tu lazy val NullableAnnots: List[ClassSymbol] = getClassesIfDefined(
HarrisL2 marked this conversation as resolved.
Show resolved Hide resolved
"javax.annotation.Nullable" ::
"org.jetbrains.annotations.Nullable" ::
"org.jspecify.annotations.Nullable" :: Nil)

// convenient one-parameter method types
def methOfAny(tp: Type): MethodType = MethodType(List(AnyType), tp)
def methOfAnyVal(tp: Type): MethodType = MethodType(List(AnyValType), tp)
Expand Down
24 changes: 18 additions & 6 deletions compiler/src/dotty/tools/dotc/core/JavaNullInterop.scala
Original file line number Diff line number Diff line change
Expand Up @@ -66,23 +66,26 @@ object JavaNullInterop {
nullifyExceptReturnType(tp)
else
// Otherwise, nullify everything
nullifyType(tp)
nullifyType(tp, explicitlyNullable = hasNullableAnnot(sym))
}

private def hasNotNullAnnot(sym: Symbol)(using Context): Boolean =
ctx.definitions.NotNullAnnots.exists(nna => sym.unforcedAnnotation(nna).isDefined)

private def hasNullableAnnot(sym: Symbol)(using Context): Boolean =
ctx.definitions.NullableAnnots.exists(nna => sym.unforcedAnnotation(nna).isDefined)

/** If tp is a MethodType, the parameters and the inside of return type are nullified,
* but the result return type is not nullable.
* If tp is a type of a field, the inside of the type is nullified,
* but the result type is not nullable.
*/
private def nullifyExceptReturnType(tp: Type)(using Context): Type =
new JavaNullMap(outermostLevelAlreadyNullable = true)(tp)
new JavaNullMap(outermostLevelAlreadyNullable = true, explicitlyNullable = false)(tp)

/** Nullifies a Java type by adding `| Null` in the relevant places. */
private def nullifyType(tp: Type)(using Context): Type =
new JavaNullMap(outermostLevelAlreadyNullable = false)(tp)
private def nullifyType(tp: Type, explicitlyNullable: Boolean = false)(using Context): Type =
new JavaNullMap(outermostLevelAlreadyNullable = false, explicitlyNullable)(tp)

/** A type map that implements the nullification function on types. Given a Java-sourced type, this adds `| Null`
* in the right places to make the nulls explicit in Scala.
Expand All @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the NotNull/Nullable annotation on the symbol should affect any types nested inside.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

def nullify(tp: Type): Type =
if ctx.flexibleTypes then {
if explicitlyNullable then {
OrNull(tp)
} else {
FlexibleType(tp)
}
} else {
OrNull(tp)
}

/** Should we nullify `tp` at the outermost level? */
def needsNull(tp: Type): Boolean =
Expand Down
50 changes: 50 additions & 0 deletions tests/explicit-nulls/neg/i21629/J.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
package javax.annotation;
import java.util.*;

public class J {

private static String getK() {
return "k";
}

@Nullable
HarrisL2 marked this conversation as resolved.
Show resolved Hide resolved
public static final String k = getK();

@Nullable
public static String l = "l";

@Nullable
public final String m = null;

@Nullable
public String n = "n";

@Nullable
public static final String f(int i) {
return "f: " + i;
}

@Nullable
public static String g(int i) {
return "g: " + i;
}

@Nullable
public String h(int i) {
return "h: " + i;
}

@Nullable
public <T> String[] genericf(T a) {
String[] as = new String[1];
as[0] = "" + a;
return as;
}

@Nullable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should test annotations at parameter positions as well: f(@NotNull String s), g(@Nullable String s)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

public <T> List<T> genericg(T a) {
List<T> as = new ArrayList<T>();
as.add(a);
return as;
}
}
8 changes: 8 additions & 0 deletions tests/explicit-nulls/neg/i21629/Nullable.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
package javax.annotation;

import java.lang.annotation.*;

// A "fake" Nullable Annotation for jsr305
@Retention(value = RetentionPolicy.RUNTIME)
@interface Nullable {
}
15 changes: 15 additions & 0 deletions tests/explicit-nulls/neg/i21629/S.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Test that Nullable annotations are working in Java files.

import javax.annotation.J

class S_3 {
def kk: String = J.k // error
def ll: String = J.l // error
def mm: String = (new J).m // error
def nn: String = (new J).n // error
def ff(i: Int): String = J.f(i) // error
def gg(i: Int): String = J.g(i) // error
def hh(i: Int): String = (new J).h(i) // error
def genericff(a: String | Null): Array[String | Null] = (new J).genericf(a) // error
def genericgg(a: String | Null): java.util.List[String] = (new J).genericg(a) // error
}