Less eager dealiasing of type aliases

[pweisenburger]

This PR aims to keep more type aliases during type checking, which are currently dealised. Whether a type is dealiased or not affects error messages, type inspection in metaprogramming and (probably most importantly) type unification.

The compiler dealiases types rather eagerly (quite some time ago I reported #4565). In particular, even types that are explicitly ascribed in code are also dealiased. So this behavior does not only affect inference.

A further problem in this context is that, in the current implementation, deprecated annotations on type aliases are lost during dealiasing in some situations (cf. test case tests/neg-custom-args/type-alias.scala; previously reported in #9825). There were actually a couple of uses of deprecated collections (e.g., Traversable) in the compiler, which should have been compiler errors under -Xfatal-warnings but were not. After the changes of this pull request, these correctly become errors. I added a commit that fixes the collection usages.

Another issue due to over-eager dealiasing is #14171. In general, it seems it is a good idea to keep aliases as long as possible since they may have an effect on type checking.

I'm aware that when to dealias and when not is a subtle matter and in general type inference does not guarantee that aliases are preserved. But I think it would be good to be a bit more conservative with dealiasing. This pull request contains a couple of commits that try to keep aliases in more cases if possible.

Some detailed comments on the main changes

1. In some places (OrderingConstraint.scala, TypeOps.scala, Types.scala), calls to dealias were dropped. TypeSizeAccumulator and CoveringSetAccumulator in Types.scala and extractParamTypess in Signatures.scala now need to dealias types themselves (previously they could assume a completely dealiased type instead).

2. One source for eager dealiasing is Config.simplifyApplications. After switching this off, some tests failed, notably hk-alias-unification.scala. This not necessarily a bug. The chosen instantiations for type variables are correct but unintuitive. I recovered the the intuitive solution by deferring the dealiasing until performing the subtype test in necessarySubType: We now first dealias the left type until its symbol matches the symbol of the right type (or one of the right type's aliases). Going into recur with the dealised type then yields the expected solution:

   https://github.com/lampepfl/dotty/blob/a4b11d1b7474fe2713a67c3d6c06ada3f06786e0/compiler/src/dotty/tools/dotc/core/TypeComparer.scala#L136-L153

   Incidentally, this strategy is insensitive to the order of type parameters in contrast to the current approach. It finds intuitive instantiations also in the following cases (which the current approach does not):

   https://github.com/lampepfl/dotty/blob/a4b11d1b7474fe2713a67c3d6c06ada3f06786e0/tests/run/hk-alias-unification.scala#L28-L29

3. The last change restores an alias if recur followed the alias of the left type and the right type is a type parameter. After returning from the recursive call to recur, we replace the dealiased type in the constraints by the alias (if it was added to the constraints):

   https://github.com/lampepfl/dotty/blob/24463aa0c7daebbb00e3c20513ffa03d3b9d319f/compiler/src/dotty/tools/dotc/core/TypeComparer.scala#L410-L428

4. Tests where the inferred type shows up were adapted to reflect the changes.

Independently of the changes above, but also concerning type aliasing, I adapted how the compiler special-treats EmptyTuple.type and its EmptyTuple alias:

1. Compiler-generated code uses the canonical EmptyType alias.

2. I fixed tupleArity and tupleElementTypes to work with both EmptyTuple.type and EmptyTuple:

   https://github.com/lampepfl/dotty/blob/569e4b926c0a40a573add38babf89ac6be6c3489/compiler/src/dotty/tools/dotc/transform/TypeUtils.scala#L52-L81

   Previously, the methods returned the expected values for a tuple of the form T *: EmptyTuple.type. A type T *: EmptyTuple, however, was not treated as a tuple. I assume that this is not intentional? It also led to following strange situation:

   def test0: Int *: EmptyTuple.type = ???  // erases to `Tuple1`
   def test1: Int *: EmptyTuple = ???       // erases to `Product`

   This can also be observed in the standard library's Tuple.apply method, which is erased to:

   public static <T> java.lang.Object apply(T);
     descriptor: (Ljava/lang/Object;)Lscala/Product;
   

   Shouldn't this erase to Tuple1 instead of Product (also for Java interop)? Also, why does the Java signature states that the method returns an Object?

   Anyway, I'm not really an expert on this and I'm not sure what changing this would mean for binary compatibility. So I kept the old behavior of treating EmptyTuple and EmptyTuple.type differently (but only) under erasure.

3. Again, tests were adapted to the changes.

Notes

In the last commit, I temporarily disabled some community build tests for the scalatest, izumi-reflect and zio projects since they are expected not to run successfully because they assume dealiased types. The fixes to these projects for the tests are trivial.

jvm/scalatest-test/src/test/scala/org/scalatest/AssertionsSpec.scala
jvm/scalatest-test/src/test/scala/org/scalatest/DirectAssertionsSpec.scala
- assert(e.message == Some(wasNotInstanceOf(s1, if (ScalaTestVersions.BuiltForScalaVersion.startsWith("3.")) "scala.collection.immutable.List[scala.Int]" else "scala.List")))
+ assert(e.message == Some(wasNotInstanceOf(s1, if (ScalaTestVersions.BuiltForScalaVersion.startsWith("3.")) "scala.List[scala.Int]" else "scala.List")))

izumi-reflect/izumi-reflect/src/main/scala_3/izumi/reflect/dottyreflection/Inspector.scala
- tpe match {
+ tpe.dealias match {

Results

This pull request fixes lost deprecated annotations (type-alias.scala), improves type inference for certain cases (like hk-alias-unification.scala), fixes the type inference issue of #4565 (but does improve error reporting by showing both the type alias and dealiased type), fixes #5177, fixes #9421, fixes #9825, fixes #14171, fixes #15304 and might supersede #14232 (both the succeeding and failing test of that PR succeed with the changes of this PR).

[pweisenburger]

We still need to simplify type applications if they are type aliases that are declared private since we lose access to the type alias' symbol at the later point, rendering the new approach in necessarySubType inapplicable. I discovered this through a test case in the "endpoints4s" community build project that uses Akka Http, which has this definition: https://github.com/akka/akka-http/blob/v10.2.8/akka-http/src/main/scala/akka/http/scaladsl/marshalling/PredefinedToResponseMarshallers.scala#L23. I wonder whether this code should be legal since the private type escapes its scope?

[smarter]

I wonder whether this code should be legal since the private type escapes its scope?

Depends on where the private type alias appears: https://github.com/lampepfl/dotty/blob/e05af52bcf9f5cfcbda532c6dfaee4deac9928ac/compiler/src/dotty/tools/dotc/typer/Checking.scala#L565-L585

[pweisenburger]

Ah, I see. I wasn't aware of that but this makes sense.

[pweisenburger]

We only try to restore the type alias if the symbol is static because then it is guaranteed that it is accessible. Otherwise it might not be. But I suppose even if the symbol is not static, it might be accessible in the current context, right? Is there a better check?

[smarter]

There's isAccessibleFrom but doing accessibility checks in the subtype checker seems like too much coupling to me.

[pweisenburger]

Thanks for the hint! Then I guess, isStatic might be good approximation.

[bishabosha]

test performance please

[odersky]

I am very worried about performance here. The changes to TypeComparer look quite problematic in that regard. If we see an option that was turned on because it makes JUnit tests 6% faster (as the comment claims), we cannot simply turn it off without analyzing the performance question in depth.

[odersky]

Seems wrong that EmptyTupleType and EmptyTupleModule are obtained by different means.

[pweisenburger]

I'm not sure I get the problem. Could you elaborate? This seems very similar to me to how CanThrow and the related CanThrow alias are obtained: https://github.com/lampepfl/dotty/blob/3.1.1/compiler/src/dotty/tools/dotc/core/Definitions.scala#L836-L837

If there is a better way, I suppose this can be changed and shouldn't be a fundamental issue.

[pweisenburger]

I investigated a bit which changes in this PR are the most performance-critical and added a commit to improve performance (changes are unrelated to the Config.simplifyApplications option), while still fixing all the linked issues.

My measurements did not really show any impact of Config.simplifyApplications contrary to what the comments claims. The comment is also almost 6 years old (f50cb20) and the simplifications switched on by this option are way more conservative now (6c26344, b159489) than they were back then. Also, the unit tests evolved a lot. So, it may be that the comment does not apply anymore and could even be removed.

It would help to know how exactly the claimed performance impact was measured for reproduction. Is there an established way to reasonably benchmark these changes in the compiler?

What I did was running the JUnit test suite and some benchmarks (compiler, stdlib, re2, int sums) locally. The results were a bit inconclusive due to high variance. I even experienced rather a slight speedup instead of a slowdown (running on my local machine). Considering the variations in the results, I did not find evidence that performance changes substantially.

Comparison before/after this PR

 dotty compiler benchmark
 Benchmark       Mode  Cnt      Score     Error  Units
-Worker.compile  avgt   20  19281.165 ± 433.447  ms/op
+Worker.compile  avgt   20  18389.194 ± 125.928  ms/op

 stdlib benchmark
 Benchmark       Mode  Cnt     Score     Error  Units
-Worker.compile  avgt   20  9121.527 ± 146.814  ms/op
+Worker.compile  avgt   20  8964.328 ± 173.365  ms/op

 re2 benchmark
 Benchmark       Mode  Cnt    Score    Error  Units
-Worker.compile  avgt   20  857.137 ± 31.739  ms/op
+Worker.compile  avgt   20  840.604 ± 29.982  ms/op

 sums of constant integer types benchmark
 Benchmark       Mode  Cnt     Score    Error  Units
-Worker.compile  avgt   20  1299.250 ± 46.820  ms/op
+Worker.compile  avgt   20  1303.119 ± 42.433  ms/op

 scala3-compiler-bootstrapped/compile
-Total time: 87 s (01:27)
+Total time: 85 s (01:25)

 scala3-compiler-bootstrapped/test
-Total time: 1639 s (27:19)
+Total time: 1619 s (26:59)

I would also argue that performing dealiasing in TypeComparer (where we know both types for which to test the subtype relation) is more principled than dealiasing some cases when applying type arguments in a more ad-hoc way in TypeApplications.

[odersky]

I think overall this needs more study to be able to decide whether this can go in. It feels very risky to me, but I don't have a lot of data to back it up. I won't have the time to follow this through, unfortunately, so somebody else will have to take this on. Somebody with a good performance background, preferably.

[pweisenburger]

rebased onto master; extended benchmark results (in previous comment); happy to discuss and address any concerns

[smarter]

I think this is really interesting work, but I won't be able to look into it more for several months at least so I just left a couple comments for now, also:

• Would this address Inline macros dealias function parameter types too eagerly #15304 ?
• If you have the time, I suggest sending PRs upstream to fix projects that rely on eager dealiasing, even if this PR doesn't end up being merged it would make them more robust.

[smarter]

It would also be interesting to run this on the bigger, out-of-CI community build from VirtusLab, but I don't know if there's a way to do that for an arbitrary PR yet /cc @WojciechMazur

[WojciechMazur]

@smarter It's not automated yet, but it is possible, however, we must remember that there are still plenty of projects that fail community build already for a different reason that might not be connected to changes in this PR.

The instructions to run arbitrary compiler branch are not yet written down, so I'll leave them here:

1. Go to the https://scala3.westeurope.cloudapp.azure.com/job/runBuild/build and log in using GitHub, members of the Dotty Core team should have an access to run new builds, everyone has the read access.
2. Change repo of the compiler, update following fields:

• scalaRepoUrl - insert here URL to your repo/fork, eg. github.com/WojciechMazur/dotty
• scalaRepoBranch - name of the branch with the PR
  Community Build would build the compiler and Scala3 stdlib from that repo

3. Optionally set maxProjectsCount and minStarsCount (minimal amount of GitHub star project needs to have to be included in the build) to limit amount of the projects that would be run

I've already run the build for this PR: https://scala3.westeurope.cloudapp.azure.com/blue/organizations/jenkins/runBuild/detail/runBuild/66/

[smarter]

Thanks!

there are still plenty of projects that fail community build already for a different reason that might not be connected to changes in this PR.

Do you have an example of a run of the community build on the main branch to compare against the results of the run for this PR?

[WojciechMazur]

Sure
Latest nightly (it's actually from the 26th of May, because during the last few days I've not seen that CB was broken): https://scala3.westeurope.cloudapp.azure.com/blue/organizations/jenkins/runBuild/detail/runBuild/61/pipeline
3.1.3-RC3: https://scala3.westeurope.cloudapp.azure.com/blue/organizations/jenkins/runBuild/detail/runBuild/51/pipeline

[pweisenburger]

I think this is really interesting work

Thanks!

Would this address Inline macros dealias function parameter types too eagerly #15304 ?

It does now :-) It now handles also that case in TypeComparer.

If you have the time, I suggest sending PRs upstream to fix projects that rely on eager dealiasing, even if this PR doesn't end up being merged it would make them more robust.

Makes sense. I'm planning on doing that when I find the time.

however, we must remember that there are still plenty of projects that fail community build already for a different reason that might not be connected to changes in this PR.
I've already run the build for this PR: https://scala3.westeurope.cloudapp.azure.com/blue/organizations/jenkins/runBuild/detail/runBuild/66/

Thanks :-) I will have a more detailed look. If I see this correctly, it seems at least the number of failing projects overall is similar to the main branch.