An alternative feature to UnsafeNulls: UnsafeJavaReturn

[noti0na1]

This PR introduces a new language feature for explicit nulls, unsafeJavaReturn. It is similar to unsafeNulls, but less aggresive. This is an experiment to address the pain point 2 in #14622.

Under unsafeJavaReturn, all | Null in Java methods' return types and members are replced by a new annotation @CanEqualNull. For example, a Java method def f(x: String | Null): Array[String | Null] | Null becomes def f(x: String | Null): Array[String @CanEqualNull] @CanEqualNull.

String @CanEqualNull is a same type asString. But when a reference type has this annotation, its values can be compared with null.

This feature is very useful when working with Java methods with long chain calling. For example, "aaab".substring(2).trim().length(). The null safety is only weakened for Java methods. Other part of code is not affect. Without this feature, we have add .nn everywhere, or use unsafeNulls to weaken the null safety in the whole scope.

A longer example:

import java.util.List;

class JC {

  public int a;

  public String b;

  public int f() {
    return 0;
  }

  public String g() {
    return null;
  }

  public List<String> h() {
    return null;
  }

  public String[] h2() {
    return null;
  }
}

import scala.language.unsafeJavaReturn
import java.{util => ju}

def test[T >: Null <: AnyRef | Null](jc: JC) = {
  val a: Int = jc.a

  val b: String = jc.b // it returns String @CanEqualNull
  val bb = jc.b == null // it's ok to compare String @CanEqualNull with Null
  val b1 = jc.b.trim().length() // String @CanEqualNull is just String, unsafe selecting

  val f: Int = jc.f()

  val g: String = jc.g()
  val gg = jc.g() == null
  val gl = jc.g().trim().length()

  val h: ju.List[String] = jc.h()

  val h2 = jc.h2()
  val h2a = jc.h2() == null
  val h2b = jc.h2().map(_ == null)
}

def test2 = {
  val s: String = null // error: null safety is not affect here
}

[noti0na1]

@odersky

What is the correct way to make an annotation more sticky to a type?

I tried to create an annotation extending RefiningAnnotation, so the type inference can preserve the annotation, but this will change the subtyping for the annotated type.

[olhotak]

@odersky

What is the correct way to make an annotation more sticky to a type?

I tried to create an annotation extending RefiningAnnotation, so the type inference can preserve the annotation, but this will change the subtyping for the annotated type.

Do you have minimized example of source code for which this is needed (i.e. where the annotation is lost)? That might help to determine the most practical solution.

[noti0na1]

@olhotak

If an annotation is a normal static annotation, it will be stripped when we use dealiasKeepRefiningAnnots and several other places in TypeComparer.

Suppose xs has type Array[String @annot], for xs.map(x => ???), we want to infer String @annot for x. If the annotation is lost in TypeComparer, the expression will become refArrayOps[String](xs).map(x => ???), and the infered type for x becomes String.

Update:

I end up adding a special rule for CanEqualNull, so that T =:= T @CanEqualNull.

[olhotak]

This now LGTM, but I would like @odersky to approve particularly the fact that CanEqualNull is a RefiningAnnotation and the change in TypeComparer that makes it a special RefiningAnnotation.

[noti0na1]

While I was fixing the errors, I found a bug in TreeChecker I need to fix first.

It seems the ctx for checking is not initialized properly. The val checkingCtx uses setMode(Mode.ImplicitsEnabled), which removes other modes in the ctx. This causes Mode.SafeNulls removed from it.

[olhotak]

Seems it doesn't work for the following minimized case:

import scala.language.unsafeJavaReturn

val s = "foo"
val methods: Array[java.lang.reflect.Method] = s.getClass.getMethods

-- [E007] Type Mismatch Error: Test.scala:4:47 -------------------------------------------------------------------------------------------------------------------------------------------------------------------
4 |val methods: Array[java.lang.reflect.Method] = s.getClass.getMethods
  |                                               ^^^^^^^^^^^^^^^^^^^^^
  |                                               Found:    Array[java.lang.reflect.Method | Null] | Null
  |                                               Required: Array[reflect.Method]
  |
  | longer explanation available when compiling with `-explain`
1 error found

[olhotak]

Here's another bug I came across in the community build:

import language.unsafeJavaReturn
import java.math.MathContext, MathContext._
val x: MathContext = DECIMAL32             // Error
val y: MathContext = MathContext.DECIMAL32 // OK

I'd expect both to be OK.

[olhotak]

And another bug from the community build:

import language.unsafeJavaReturn
val zids: java.util.Set[String] = java.time.ZoneId.getAvailableZoneIds
val zarr: Array[String] = java.time.ZoneId.getAvailableZoneIds.toArray(Array.empty[String])

Without -Yexplicit-nulls, it compiles fine. With -Yexplicit-nulls, I get:

3 |val zarr: Array[String] = java.time.ZoneId.getAvailableZoneIds.toArray(Array.empty[String])
  |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |None of the overloaded alternatives of method toArray in trait Set with types
  | [T](x$0: Array[Object & T | Null] | Null): Array[Object & T | Null] | Null
  | (): Array[Object | Null] | Null
  | [T]
  |  (x$0: java.util.function.IntFunction[Array[Object & T | Null]] | Null): Array[Object & T | Null] | 
  |    Null
  |match arguments (Array[String])
1 error found

Not sure if that's a bug in this PR or a limitation of the feature implemented by the PR.

[noti0na1]

And another bug from the community build:

import language.unsafeJavaReturn
val zids: java.util.Set[String] = java.time.ZoneId.getAvailableZoneIds
val zarr: Array[String] = java.time.ZoneId.getAvailableZoneIds.toArray(Array.empty[String])

Without -Yexplicit-nulls, it compiles fine. With -Yexplicit-nulls, I get:

3 |val zarr: Array[String] = java.time.ZoneId.getAvailableZoneIds.toArray(Array.empty[String])
  |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |None of the overloaded alternatives of method toArray in trait Set with types
  | [T](x$0: Array[Object & T | Null] | Null): Array[Object & T | Null] | Null
  | (): Array[Object | Null] | Null
  | [T]
  |  (x$0: java.util.function.IntFunction[Array[Object & T | Null]] | Null): Array[Object & T | Null] | 
  |    Null
  |match arguments (Array[String])
1 error found

Not sure if that's a bug in this PR or a limitation of the feature implemented by the PR.

Since we don't modify the parameter types, the arguments should be nullable:

val zarr: Array[String] = ZoneId.getAvailableZoneIds.toArray(Array.empty[String | Null])

Here's another bug I came across in the community build:

import language.unsafeJavaReturn
import java.math.MathContext, MathContext._
val x: MathContext = DECIMAL32             // Error
val y: MathContext = MathContext.DECIMAL32 // OK

I'd expect both to be OK.

Fixed

[olhotak]

The contents of the PR now LGTM, but I'm concerned about merging it into main. This is an experimental feature and the PR introduces a language import and a change to the standard library (CanEqualNull). Both of these would be hard to remove if we change our minds about how to do this. We want to merge it so we can experiment with it on the compiler itself and on the community build; would it be possible to merge it not into main but into a branch like the cc-experiment branch?

[odersky]

If we add a language import and something to the standard library we have to make them experimental. I.e.

  import language.experimental.unsafeJavaReturn

  @experimental 
  final class CanEqualNull...

[noti0na1]

If we add a language import and something to the standard library we have to make them experimental. I.e.

@odersky I have moved the new language feature inside the experimental object, but the CI test Dotty / MiMa still fails:

Error:  scala3-library-bootstrapped: Failed binary compatibility check against org.scala-lang:scala3-library_3:3.1.3! Found 2 potential problems (filtered 24)
Error:   * static field unsafeJavaReturn in object scala.runtime.stdLibPatches.language#experimental does not have a correspondent in other version
Error:     filter with: ProblemFilters.exclude[MissingFieldProblem]("scala.runtime.stdLibPatches.language#experimental.unsafeJavaReturn")
Error:   * object scala.runtime.stdLibPatches.language#experimental#unsafeJavaReturn does not have a correspondent in other version
Error:     filter with: ProblemFilters.exclude[MissingClassProblem]("scala.runtime.stdLibPatches.language$experimental$unsafeJavaReturn$")
Error:  java.lang.RuntimeException: Failed binary compatibility check against org.scala-lang:scala3-library_3:3.1.3! Found 2 potential problems (filtered 24)

Do I need to change some configration?

[odersky]

I think the person who knows best would be @dwijnand. In your place I would just add the filters that were suggested to the MiMa excludes file.

[dwijnand]

Yeah, you can either add the filters or annotate it with the scala.annotation.experimental annotation. In the future we can @experimental annotate the scala.language.experimental object and that will apply to everything in it, but that isn't handled by mima atm.

[olhotak]

I'm happy with this now to be merged, but I would still like approval from @odersky even though it's now all experimental.

[ckipp01]

@noti0na1 I'm assuming you were still waiting on a review here? If so, are you able to rebase and I can try to get you a review?

[noti0na1]

@ckipp01 I will have time recently to rebase this PR.

We should consider and compare the alternative approach in #17369 at the same time.