More consistent results for union types in pickling

[noti0na1]

This PR tries to fix some of the pickling errors in #14946.

#15105 was an attempt to solve the errors of different union types in unpickler, by simplifying the type of match and try nodes.
We thought Typer performs a simplification step after type assigning a tree, so we should do the same step in unpickler. But this is not correct, because the tree before Pickler phase is posttyper, which does not perform a simplification.
The reason why #15105 fixes some of the errors because lub does not return a consistant result. But this also introduces more errors in the test.

Let's consider a case: lub(List(String | Null, String, Null)). Inside lub we have:

val t1 = mergeIfSuper(tp1, tp2, canConstrain)
if t1.exists then return t1

val t2 = mergeIfSuper(tp2, tp1, canConstrain)
if t2.exists then return t2

So, depends on the order of arguments, we may get String or String | Null.

This PR adds a TreeTransformer to the Pickling, so it can overwrite the types of If, Match, and Try nodes before and after pickling with the simplified version. Hopefully, we can always get consistent types during comparison.

[noti0na1]

test performance please

[dottybot]

performance test scheduled: 1 job(s) in queue, 0 running.

[dottybot]

Performance test finished successfully:

Visit https://dotty-bench.epfl.ch/15515/ to see the changes.

Benchmarks is based on merging with main (5a8a61d)

[olhotak]

For the statement:
"So, depends on the order of arguments, we may get String or String | Null."
do you mean without -Yexplicit-nulls?

I'm confused because with -Yexplicit-nulls, the lub cannot be String, but without -Yexplicit-nulls, we don't have the pickling errors in #14946.

I agree that lub(t1,t2) == lub(t2,t1) is a desirable property, that it's difficult to ensure with equivalent but unequal types such as A|B and B|A, and that simplifying the result of lub might be one way to get closer to it, but I don't know whether: 1) it is even possible to achieve lub(t1,t2) == lub(t2,t1) in the general case or 2) simplifying the result of lub is the best way to get there.

[olhotak]

Oh, I see, it's with -Yexplicit-nulls but with unsafeNulls. Then the lub can be String.

A desirable property (though it might not be achievable) would be for unsafeNulls to not affect computed lubs. More generally, for unsafeNulls to not affect inferred types, but only to relax certain subtyping checks.

[odersky]

I don't think lub should call simplified. That's really not supported by the architecture.

[noti0na1]

It is difficult to relax the pickling test just for unsafe nulls. We are not comparing the individual types in the test, because the strings of the two trees are compared directly.

It is also difficult to trick the printer to ignore | Null, since it doesn't use the import information.

cc: @odersky @olhotak

[olhotak]

Could the printer ignore |Null always, even without the unsafeNulls language import?

[noti0na1]

Could the printer ignore |Null always, even without the unsafeNulls language import?

I don't think it is a good idea for explicit nulls (without unsafe nulls). The nullable types are important in this case.

[olhotak]

Could we canonicalize the types in the trees before printing them to strings in the unpickler testing code? This would require a tree transformation that creates a new tree with transformed types.

[noti0na1]

Could we canonicalize the types in the trees before printing them to strings in the unpickler testing code? This would require a tree transformation that creates a new tree with transformed types.

We can call simplify on the types of original tree and hope the types from both trees are simplified in the same way, but this is an expensive step, and I'm not sure this can handle all the issues caused by lub.

[noti0na1]

@olhotak @odersky
I removed simplified from lub. I added a TreeTransformer to the Pickling, so it can overwrite the types of If, Match, and Try nodes before and after pickling with the simplified version.

[noti0na1]

I don't think adding a TreeMap and running it on the whole tree twice is a good idea. So I delete it and add type simplify to PostTyper, which should achieve the same result.

The tests here are all passed, then I tested #14946 based on this PR. There are still some pickling errors.
I think they may be related to the order in which the trees are completed in unpickler.

If a tree is loaded by TreeReader.readTopLevel, the import information is correctly processed. If a tree is being completed by TreeUnpickler.Completer later, it couldn't find the imports (unsafeNulls) outside, so the simplified will return a different result.

For example:

S1.scala

MainGenericRunner.processClasspath("")

S2.scala

import scala.language.unsafeNulls

object MainGenericRunner {
  def processClasspath(cp: String) =
    val singleEntryClasspath: Boolean = ???
    // The type of the If node is `String | Null`, 
    // it should be simplified to `String` in unsafe nulls 
    val globdir: String = if singleEntryClasspath then cp.replaceAll("[\\\\/][^\\\\/]*$", "") else ""
    globdir
}

If we compile S2 only: scalac -Yexplicit-nulls -Xprint-types -Ytest-pickler S2.scala, there is no error.

If we compile S1 and S2 together: scalac -Yexplicit-nulls -Xprint-types -Ytest-pickler S1.scala S2.scala, when processClasspath is completed, the unsafeNulls import is missing, so the type of If expreesion is not simplified, which causes pickling error.

I tried to read Imports in indexStats in advance. This can fix the simple example above, but will crush on complicated examples due to Cyclic reference.

[olhotak]

Good catch.

I thought the TreeMap just for the pickling tests was fine, but if simplifying in PostTyper can fix all the failures, I agree that's less clunky.

So this is another example in the line of issues caused by language imports not being considered everywhere. I don't know enough about the deferred unpickling to suggest how to fix this. Is the cyclic reference specific for unsafeNulls (after you already know that the import is unsafeNulls) or does it happen when trying to resolve the symbol in the import? More generally, I wonder whether language imports should be pickled as something more specific than just general imports.

[ckipp01]

Hey @noti0na1, it's been a while here. Do you plan on returning, or are you waiting for a review from someone? How can I help you move this forward?

[noti0na1]

Close this PR for now. Plan to revisite this bug soon.