-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Support NullMarked and NullUnmarked Annotations #26559
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
149a629
7a9519f
834ecd8
1b1e7f9
3836a69
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 |
|---|---|---|
|
|
@@ -81,11 +81,16 @@ object ImplicitNullInterop: | |
| || sym.is(Flags.ModuleVal) then | ||
| return tp | ||
|
|
||
| // In a JSpecify `@NullMarked` scope, unannotated reference types are non-null by default, | ||
| // so the ambient mode becomes `Skip` (nothing is nullified unless an explicit annotation | ||
| // says otherwise). Otherwise the ambient mode is the usual `Default`. | ||
| val ambient = if isNullMarked(sym) then NullMode.Skip else NullMode.Default | ||
|
|
||
| val currentTypeMode = | ||
| // Don't nullify Given/implicit parameters | ||
| if sym.isOneOf(GivenOrImplicitVal) || hasNotNullAnnot(sym) then NullMode.Skip | ||
| else if hasNullableAnnot(sym) then NullMode.Explicit | ||
| else NullMode.Default | ||
| else ambient | ||
|
|
||
| val resultTypeMode = | ||
| // Don't nullify result type of constructors | ||
|
|
@@ -94,9 +99,43 @@ object ImplicitNullInterop: | |
|
|
||
| ImplicitNullMap( | ||
| javaDefined = sym.is(JavaDefined), | ||
| ambient = ambient, | ||
| state = NullMapState(resultTypeMode, currentTypeMode) | ||
| )(tp) | ||
|
|
||
| /** Is `sym` in a JSpecify `@NullMarked` scope? We walk the owner chain (starting at `sym` | ||
| * itself) and let the nearest scope marking win: `@NullMarked` enables the non-null default, | ||
| * `@NullUnmarked` re-enables the implicit-nulls default. | ||
| */ | ||
| private def isNullMarked(sym: Symbol)(using Context): Boolean = | ||
| sym.ownersIterator.map(ownerNullMarking).collectFirst { case Some(marked) => marked }.getOrElse(false) | ||
|
Member
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. According to the document (https://jspecify.dev/docs/api/org/jspecify/annotations/NullMarked.html):
We may want to stop the iteration at package level. |
||
|
|
||
| /** The scope marking declared directly on `owner`, if any: `Some(true)` for `@NullMarked`, | ||
| * `Some(false)` for `@NullUnmarked`, `None` if `owner` declares neither. Per JSpecify, a | ||
| * declaration carrying *both* markers behaves as if it carried neither, so we return `None` and | ||
| * let an enclosing scope decide. | ||
| * | ||
| * For packages the marker is placed on `package-info` (from `package-info.java` / | ||
| * `package-info.class`), which is loaded as a synthetic `package-info` member of the package. | ||
| * We force that member to read its annotations (safe: it only depends on the annotation | ||
| * classes). For all other owners we use `unforcedAnnotation` to avoid forcing symbols that may | ||
| * still be under construction during classfile loading / unpickling. | ||
| */ | ||
| private def ownerNullMarking(owner: Symbol)(using Context): Option[Boolean] = | ||
|
HarrisL2 marked this conversation as resolved.
Member
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. Do we want to also handle |
||
| val (carrier, forced) = | ||
| // For packages the marker lives on the synthetic `package-info` member, which we force. | ||
| if owner.is(Package) then (owner.info.decl(defn.PackageInfoName).symbol, true) | ||
| else (owner, false) | ||
| if !carrier.exists then None | ||
| else | ||
| def has(annots: List[ClassSymbol]): Boolean = | ||
| if forced then annots.exists(carrier.hasAnnotation(_)) | ||
| else annots.exists(carrier.unforcedAnnotation(_).isDefined) | ||
| val marked = has(defn.NullMarkedAnnots) | ||
| val unmarked = has(defn.NullUnmarkedAnnots) | ||
| if marked == unmarked then None // both or neither present: behave as if neither | ||
| else Some(marked) | ||
|
|
||
| private def hasNotNullAnnot(sym: Symbol)(using Context): Boolean = | ||
| defn.NotNullAnnots.exists(sym.unforcedAnnotation(_).isDefined) | ||
|
|
||
|
|
@@ -115,22 +154,29 @@ object ImplicitNullInterop: | |
| ) | ||
|
|
||
| object NullMapState: | ||
| def skipCurrentIf(cond: Boolean)(using Context): NullMapState = | ||
| /** Reset to a nested position: the result type mode becomes the ambient default, and the | ||
| * current-level mode is `Skip` when `cond` holds, otherwise the ambient default. The ambient | ||
| * default is `Default` normally, or `Skip` inside a `@NullMarked` scope. | ||
| */ | ||
| def skipCurrentIf(cond: Boolean, ambient: NullMode): NullMapState = | ||
| NullMapState( | ||
| resultTypeMode = NullMode.Default, | ||
| currentTypeMode = if cond then NullMode.Skip else NullMode.Default | ||
| resultTypeMode = ambient, | ||
| currentTypeMode = if cond then NullMode.Skip else ambient | ||
| ) | ||
|
|
||
| /** A type map that implements the nullification function on types. Given a Java-sourced type or a type | ||
| * coming from Scala code compiled without explicit nulls, this adds `| Null` or `FlexibleType` in the | ||
| * right places to make nullability explicit in a conservative way (without forcing incomplete symbols). | ||
| * | ||
| * @param javaDefined whether the type is from Java source; we always nullify type param refs from Java | ||
| * @param ambient the default mode for unannotated positions: `Default` normally, or `Skip` inside | ||
| * a JSpecify `@NullMarked` scope (where unannotated reference types are non-null). | ||
| * @param state mutable nullification state tracking the current mode for the result type | ||
| * (`resultTypeMode`) and the current nesting level (`currentTypeMode`). | ||
| */ | ||
| private class ImplicitNullMap( | ||
|
Member
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 saw the spec has some special rules for wildcards and type parameters. Are they supported by this PR? If not, I'm not sure if we want to support them? |
||
| val javaDefined: Boolean, | ||
| val ambient: NullMode, | ||
| var state: NullMapState | ||
| )(using Context) extends TypeMap: | ||
|
|
||
|
|
@@ -201,7 +247,7 @@ object ImplicitNullInterop: | |
| case appTp @ AppliedType(tycon, targs) => | ||
| val savedState = state | ||
| // If Java-defined tycon, don't nullify outer level of type args (Java classes are fully nullified) | ||
| state = NullMapState.skipCurrentIf(tp.classSymbol.is(JavaDefined)) | ||
| state = NullMapState.skipCurrentIf(tp.classSymbol.is(JavaDefined), ambient) | ||
| val targs2 = targs.map(this) | ||
| state = savedState | ||
|
|
||
|
|
@@ -213,11 +259,11 @@ object ImplicitNullInterop: | |
| val savedState = state | ||
|
|
||
| // Don't nullify param types for implicit/using sections | ||
| state = NullMapState.skipCurrentIf(mtp.isImplicitMethod) | ||
| state = NullMapState.skipCurrentIf(mtp.isImplicitMethod, ambient) | ||
| val paramInfos2 = mtp.paramInfos.map(this) | ||
|
|
||
| state = NullMapState( | ||
| resultTypeMode = NullMode.Default, | ||
| resultTypeMode = ambient, | ||
| currentTypeMode = savedState.resultTypeMode | ||
| ) | ||
| val resType2 = this(mtp.resType) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| package nmpkg; | ||
|
|
||
| import org.jspecify.annotations.*; | ||
|
|
||
| // No class-level annotation: nullability is decided by the package's `@NullMarked` (package-info). | ||
| public class J { | ||
|
|
||
| // Unannotated -> non-null String (package is null-marked). | ||
| public String get() { | ||
| return ""; | ||
| } | ||
|
|
||
| // Type-use `@Nullable` still reintroduces nullability -> String | Null. | ||
| public @Nullable String getNullable() { | ||
| return null; | ||
| } | ||
|
|
||
| // Unannotated parameter -> non-null String. | ||
| public void set(String s) { | ||
| } | ||
|
|
||
| // `@NullUnmarked` overrides the package marking for this method. | ||
| @NullUnmarked | ||
|
HarrisL2 marked this conversation as resolved.
|
||
| public void unmarkedSet(String s) { | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| package org.jspecify.annotations; | ||
|
|
||
| import java.lang.annotation.*; | ||
|
|
||
| @Target({ElementType.TYPE, ElementType.METHOD, ElementType.CONSTRUCTOR, ElementType.PACKAGE}) | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| public @interface NullMarked { | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| package org.jspecify.annotations; | ||
|
|
||
| import java.lang.annotation.*; | ||
|
|
||
| @Target({ElementType.TYPE, ElementType.METHOD, ElementType.CONSTRUCTOR, ElementType.PACKAGE}) | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| public @interface NullUnmarked { | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| package org.jspecify.annotations; | ||
|
|
||
| import java.lang.annotation.*; | ||
|
|
||
| @Target({ElementType.TYPE_USE, ElementType.METHOD, ElementType.FIELD, ElementType.PARAMETER, ElementType.LOCAL_VARIABLE}) | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| public @interface Nullable { | ||
| } |
|
HarrisL2 marked this conversation as resolved.
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| // Test that a package-level `@NullMarked` (from `package-info.class`) is read from class files. | ||
| // The Java sources above are compiled first (round 0); this file is compiled separately and reads | ||
| // `nmpkg.J` through ClassfileParser. | ||
| //> using options -Yno-flexible-types | ||
|
|
||
| import nmpkg.J | ||
| import nmpkg.U | ||
|
|
||
| // `get` result is non-null because the package is `@NullMarked`. | ||
| def a(j: J): String = j.get() | ||
|
|
||
| // Type-use `@Nullable` still makes the result `String | Null`. | ||
| def b(j: J): String = j.getNullable() // error | ||
|
|
||
| // `set` parameter is non-null, so `null` is rejected. | ||
| def c(j: J): Unit = j.set(null) // error | ||
|
|
||
| // `@NullUnmarked` on the method overrides the package marking, so `null` is accepted here. | ||
| def d(j: J): Unit = j.unmarkedSet(null) | ||
|
|
||
| // `@NullUnmarked` on class `U` overrides the package `@NullMarked`: `get` is `String | Null`. | ||
| def u1(u: U): String = u.get() // error | ||
|
|
||
| // `@NullUnmarked` class: `set` parameter is nullable, so `null` is accepted. | ||
| def u2(u: U): Unit = u.set(null) | ||
|
|
||
| // `@NullMarked` re-marks the method inside the unmarked class, so its result is non-null. | ||
| def u3(u: U): String = u.markedGet() | ||
|
|
||
| // Both markers behave as if neither: `bothGet` inherits `U`'s `@NullUnmarked`, so its result is | ||
| // `String | Null` (it does not become non-null from `@NullMarked`). | ||
| def u4(u: U): String = u.bothGet() // error |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| package nmpkg; | ||
|
|
||
| import org.jspecify.annotations.*; | ||
|
|
||
| // `@NullUnmarked` on a top-level class overrides the package's `@NullMarked`, | ||
| // restoring the implicit-nulls default for its members. | ||
| @NullUnmarked | ||
| public class U { | ||
|
|
||
| // Unmarked -> String | Null. | ||
| public String get() { | ||
| return ""; | ||
| } | ||
|
|
||
| // Unmarked parameter -> nullable. | ||
| public void set(String s) { | ||
| } | ||
|
|
||
| // `@NullMarked` re-marks this method, so its result is non-null again. | ||
| @NullMarked | ||
| public String markedGet() { | ||
| return ""; | ||
| } | ||
|
|
||
| // Both markers behave as if neither is present, so this inherits `U`'s `@NullUnmarked` | ||
| // (its result is `String | Null`), rather than letting `@NullMarked` win. | ||
| @NullMarked | ||
| @NullUnmarked | ||
| public String bothGet() { | ||
| return ""; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| // The whole package is `@NullMarked` via `package-info.java`, so unannotated reference types | ||
| // in this package are non-null. JavaParsers must read this package-level annotation. | ||
| @NullMarked | ||
| package nmpkg; | ||
|
|
||
| import org.jspecify.annotations.*; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| package nmpkg; | ||
|
|
||
| import org.jspecify.annotations.*; | ||
|
|
||
| // No class-level annotation: nullability is decided by the package's `@NullMarked` (package-info). | ||
| public class J { | ||
|
|
||
| // Unannotated -> non-null String (package is null-marked). | ||
| public String get() { | ||
| return ""; | ||
| } | ||
|
|
||
| // Type-use `@Nullable` still reintroduces nullability -> String | Null. | ||
| public @Nullable String getNullable() { | ||
| return null; | ||
| } | ||
|
|
||
| // Unannotated parameter -> non-null String. | ||
| public void set(String s) { | ||
| } | ||
|
|
||
| // `@NullUnmarked` overrides the package marking for this method. | ||
| @NullUnmarked | ||
| public void unmarkedSet(String s) { | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| package org.jspecify.annotations; | ||
|
|
||
| import java.lang.annotation.*; | ||
|
|
||
| @Target({ElementType.TYPE, ElementType.METHOD, ElementType.CONSTRUCTOR, ElementType.PACKAGE}) | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| public @interface NullMarked { | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| package org.jspecify.annotations; | ||
|
|
||
| import java.lang.annotation.*; | ||
|
|
||
| @Target({ElementType.TYPE, ElementType.METHOD, ElementType.CONSTRUCTOR, ElementType.PACKAGE}) | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| public @interface NullUnmarked { | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| package org.jspecify.annotations; | ||
|
|
||
| import java.lang.annotation.*; | ||
|
|
||
| @Target({ElementType.TYPE_USE, ElementType.METHOD, ElementType.FIELD, ElementType.PARAMETER, ElementType.LOCAL_VARIABLE}) | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| public @interface Nullable { | ||
| } |
|
HarrisL2 marked this conversation as resolved.
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| //> using options -Yno-flexible-types | ||
|
|
||
| import nmpkg.J | ||
| import nmpkg.U | ||
|
|
||
| // `get` result is non-null because the package is `@NullMarked`. | ||
| def a(j: J): String = j.get() | ||
|
|
||
| // Type-use `@Nullable` still makes the result `String | Null`. | ||
| def b(j: J): String = j.getNullable() // error | ||
|
|
||
| // `set` parameter is non-null, so `null` is rejected. | ||
| def c(j: J): Unit = j.set(null) // error | ||
|
|
||
| // `@NullUnmarked` on the method overrides the package marking, so `null` is accepted here. | ||
| def d(j: J): Unit = j.unmarkedSet(null) | ||
|
|
||
| // `@NullUnmarked` on class `U` overrides the package `@NullMarked`: `get` is `String | Null`. | ||
| def u1(u: U): String = u.get() // error | ||
|
|
||
| // `@NullUnmarked` class: `set` parameter is nullable, so `null` is accepted. | ||
| def u2(u: U): Unit = u.set(null) | ||
|
|
||
| // `@NullMarked` re-marks the method inside the unmarked class, so its result is non-null. | ||
| def u3(u: U): String = u.markedGet() | ||
|
|
||
| // Both markers behave as if neither: `bothGet` inherits `U`'s `@NullUnmarked`, so its result is | ||
| // `String | Null` (it does not become non-null from `@NullMarked`). | ||
| def u4(u: U): String = u.bothGet() // error |
Uh oh!
There was an error while loading. Please reload this page.