Other pickling difference errors with explicit nulls and unsafe nulls

[noti0na1]

An error similar to #14947

Minimized code

class C {
  def g: String | Null = ???

  def f =
    import scala.language.unsafeNulls
    try g catch case _ => ""
}

Output (click arrow to expand)

scalac -Xprint-types -Ytest-pickler -Yexplicit-nulls -color never Stest.scala

exception occurred while compiling Stest.scala
Exception in thread "main" class dotty.tools.dotc.reporting.Diagnostic$Error at ?: pickling difference for class C in Stest.scala, for details:

  diff before-pickling.txt after-pickling.txt
        at dotty.tools.dotc.report$.error(report.scala:63)
        at dotty.tools.dotc.transform.Pickler.testSame(Pickler.scala:151)
        at dotty.tools.dotc.transform.Pickler.testUnpickler$$anonfun$2(Pickler.scala:138)
        at scala.runtime.function.JProcedure1.apply(JProcedure1.java:15)
        at scala.runtime.function.JProcedure1.apply(JProcedure1.java:10)
        at scala.collection.IterableOnceOps.foreach(IterableOnce.scala:563)
        at scala.collection.IterableOnceOps.foreach$(IterableOnce.scala:561)
        at scala.collection.AbstractIterable.foreach(Iterable.scala:926)
        at scala.collection.IterableOps$WithFilter.foreach(Iterable.scala:896)
        at dotty.tools.dotc.transform.Pickler.testUnpickler(Pickler.scala:139)
        at dotty.tools.dotc.transform.Pickler.runOn(Pickler.scala:122)
        at dotty.tools.dotc.Run.runPhases$1$$anonfun$1(Run.scala:225)
        at scala.runtime.function.JProcedure1.apply(JProcedure1.java:15)
        at scala.runtime.function.JProcedure1.apply(JProcedure1.java:10)
        at scala.collection.ArrayOps$.foreach$extension(ArrayOps.scala:1328)
        at dotty.tools.dotc.Run.runPhases$1(Run.scala:236)
        at dotty.tools.dotc.Run.compileUnits$$anonfun$1(Run.scala:244)
        at dotty.tools.dotc.Run.compileUnits$$anonfun$adapted$1(Run.scala:253)
        at dotty.tools.dotc.util.Stats$.maybeMonitored(Stats.scala:68)
        at dotty.tools.dotc.Run.compileUnits(Run.scala:253)
        at dotty.tools.dotc.Run.compileSources(Run.scala:186)
        at dotty.tools.dotc.Run.compile(Run.scala:170)
        at dotty.tools.dotc.Driver.doCompile(Driver.scala:35)
        at dotty.tools.dotc.Driver.process(Driver.scala:195)
        at dotty.tools.dotc.Driver.process(Driver.scala:163)
        at dotty.tools.dotc.Driver.process(Driver.scala:175)
        at dotty.tools.dotc.Driver.main(Driver.scala:205)
        at dotty.tools.dotc.Main.main(Main.scala)
class dotty.tools.dotc.reporting.Diagnostic$Error at ?: pickling difference for class C in Stest.scala, for details:

  diff before-pickling.txt after-pickling.txt while compiling Stest.scala

This is similar to the if error before. The try expression gets different types before and after pickling (String and String | Null).

Since the tree seems having correct language imports when unpickling (#14962), I guess the bug is from typing?

cc @odersky

[pikinier20]

The issue will be fixed by #15105

[noti0na1]

@odersky

I can verify you PRs do fix the errors in the minimized code. However, the test BootstrappedOnlyCompilationTests.picklingWithCompiler still fails on the same code.

For example, the AsmUtils.scala is reported to have pickling error. The diff shows the error is at the match expression in instructionString.

def instructionString(instruction: AbstractInsnNode): String = instruction.getOpcode match {
  case -1 => instruction.toString
  case op => scala.tools.asm.util.Printer.OPCODES(op)
}

At the end of the test, the command is given to reproduce the error:

Test 'compiler/src/dotty/tools/backend/jvm/AsmUtils.scala' compiled with 0 error(s) and 0 warning(s),
the test can be reproduced by running from SBT (prefix it with ./bin/ if you
want to run from the command line):

scalac -classpath /root/.cache/coursier/v1/https/scala-webapps.epfl.ch/artifactory/central/org/scala-lang/scala-library/2.13.8/scala-library-2.13.8.jar:/__w/dotty/dotty/library/../out/bootstrap/scala3-library-bootstrapped/scala-3.2.0-RC1-bin-SNAPSHOT-nonbootstrapped/scala3-library_3-3.2.0-RC1-bin-SNAPSHOT.jar:/root/.cache/coursier/v1/https/scala-webapps.epfl.ch/artifactory/central/org/scala-lang/scala-library/2.13.8/scala-library-2.13.8.jar:/root/.cache/coursier/v1/https/scala-webapps.epfl.ch/artifactory/central/org/scala-lang/modules/scala-asm/9.3.0-scala-1/scala-asm-9.3.0-scala-1.jar:/root/.cache/coursier/v1/https/scala-webapps.epfl.ch/artifactory/central/org/jline/jline-terminal/3.19.0/jline-terminal-3.19.0.jar:/root/.cache/coursier/v1/https/scala-webapps.epfl.ch/artifactory/central/org/jline/jline-reader/3.19.0/jline-reader-3.19.0.jar:/root/.cache/coursier/v1/https/scala-webapps.epfl.ch/artifactory/central/org/scala-sbt/compiler-interface/1.3.5/compiler-interface-1.3.5.jar:/__w/dotty/dotty/interfaces/target/scala3-interfaces-3.2.0-RC1-bin-SNAPSHOT.jar:/__w/dotty/dotty/library/../out/bootstrap/scala3-library-bootstrapped/scala-3.2.0-RC1-bin-SNAPSHOT-nonbootstrapped/scala3-library_3-3.2.0-RC1-bin-SNAPSHOT.jar:/__w/dotty/dotty/tasty/../out/bootstrap/tasty-core-bootstrapped/scala-3.2.0-RC1-bin-SNAPSHOT-nonbootstrapped/tasty-core_3-3.2.0-RC1-bin-SNAPSHOT.jar:/__w/dotty/dotty/compiler/../out/bootstrap/scala3-compiler-bootstrapped/scala-3.2.0-RC1-bin-SNAPSHOT-nonbootstrapped/scala3-compiler_3-3.2.0-RC1-bin-SNAPSHOT.jar  -indent -Yno-deep-subtypes -Yno-double-bindings -Yforce-sbt-phases -Xsemanticdb -Xverify-signatures  -pagewidth 120 -color:never -Xtarget 9 -Ycheck:all -Xprint-types -Ytest-pickler -Yprint-pos  -Yprint-pos-syms -Yexplicit-nulls  'compiler/src/dotty/tools/backend/jvm/AsmUtils.scala' 

And I can reproduce the error using the command.

But when I extract the error code to a file, I couldn't reproduce the error anymore :(

import scala.language.unsafeNulls

object AsmUtils {

  def i: Int = ???

  val OPCODES: Array[String | Null] = ???

  def instructionString(instruction: Object): String = i match {
    case -1 => instruction.toString
    case op => OPCODES(op)
  }
}

scalac -color:never -Xprint-types -Ytest-pickler -Yprint-pos -Yexplicit-nulls  Stest.scala

Is this because the pickling test may use tasty files from bootstrap compiling?

[noti0na1]

It seems the bug is from the order of imports. If the unsafeNulls import is not the last one in the scope, the error will happen.

[noti0na1]

There are still some inconsistancy on nullable union in lub and TypeOps.simplify when unsafeNulls is enabled.

import scala.language.unsafeNulls

class C {
  def a(): String | Null = ???
  val b: String | Null = ???

  def f(x: Int) = x match {
    case 0 => b
    case _ => a()
  }

  def g(x: Int) = x match {
    case 0 => a()
    case _ => b
  }
}

For example, the code above, f can pass the pickling test, but g cannot.

Another example is the BTypes, when we change the number or order of the cases, we may get different results.

import scala.language.unsafeNulls
import scala.tools.asm

class BTypes {
  def toASMType(a: String) = a match {
    case "INT"    => asm.Type.INT_TYPE
    case "FLOAT"  => asm.Type.FLOAT_TYPE
    case "MethodBType" => asm.Type.getMethodType(a)
  }
}

I think the problem is: should we get nullable types returned from lub or simplify when unsafeNulls is enabled?

Given the semantic of unsafeNulls, a non-nullable result is more accurate, since Null is a subtype of another side; on the other hand, we should try to preserve the OrNull as much as possible in explicit nulls in general, because the nullable types may give more information globally (outside the unsafe scope).

cc: @odersky @olhotak

[olhotak]

I'm not sure if the following would work, but it would be nice if we can make it work:

I think computation of lub, as well as simplify, should be independent of unsafeNulls. The benefit is consistency of inferred types: I think we want lub and simplify to result in the same type independently of unsafeNulls.

A problem is that one of the relaxations that unsafeNulls is intended to implement is that an expression of type T|Null can be used wherever and expression of type T is expected. It achieves this by making T|Null <: T. But maybe this is the wrong way to achieve this. Maybe the correct way is to distinguish different contexts in which we ask the question whether T|Null <: T. When we ask the question for lub or simplify, we should keep the answer as no to be consistent with when unsafeNulls is off. When we ask the question for subsumption, e.g. in Typer.adapt, we should make the answer yes to implement the relaxation intended by unsafeNulls. In summary, my suggestion is to move the special handling of unsafeNulls out of subtype checking and into more specific places such as Typer.adapt.

But I'm not sure whether this is feasible or whether it will cause significant new problems.

[noti0na1]

Closing this issue since the error is no longer reproducible.