-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Consider nullable annotations in explicit nulls #21953
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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 = | ||
|
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should test annotations at parameter positions as well: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like Also, I am not sure about how to test I discussed with @olhotak, and one possible way is through overriding, where the following code would fail
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After looking into this, currently, parameter annotations are not parsed from source or class files, so this issue may be left to a separate task. |
||
public <T> List<T> genericg(T a) { | ||
List<T> as = new ArrayList<T>(); | ||
as.add(a); | ||
return as; | ||
} | ||
} |
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 { | ||
} |
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 | ||
} |
Uh oh!
There was an error while loading. Please reload this page.