Exports generate broken code depending on location (scope) of said code

[lbialy]

This is related to VirtusLab/besom#430.

This issue is really mind-bending: in repro in Besom repo I have this working fine if object exporting these implicit methods is in test scope but failing steadily if it's in main scope. Running scala-cli test . in this repro shows the same behavior: in main crashes while in test and local both work. If you do however run tests from metals in main and in test tests fail but local works!

Compiler version

3.3.1

Minimized code

https://github.com/lbialy/besom-issue-430

Output

Run scala-cli test . in the main folder of this repo and you will get:

repro.Suite:
==> X repro.Suite.in main  0.006s java.lang.ClassCastException: class scala.collection.immutable.$colon$colon cannot be cast to class scala.collection.View (scala.collection.immutable.$colon$colon and scala.collection.View are in unnamed module of loader 'app')
    at scala.collection.BuildFrom$$anon$8.fromSpecific(BuildFrom.scala:85)
    at repro.Output$.sequence(output.scala:20)
    at repro.OutputExtensionsFactory$OutputSequenceOps.sequence(output.scala:26)
    at repro.OutputExtensionsFactory$OutputTraverseOps.traverse(output.scala:30)
    at repro.Suite.$init$$$anonfun$1(suite.test.scala:19)
  + in test 0.001s
  + local 0.001s

Expectation

I would expect that Array("a", "b").toList.traverse(str => ...) would infer CC type param in traverse as List and not View:

toList infers List[File]:

but then traverse infers Output[View[X]]:

[lbialy]

Addendum: we have went a bit on a merry chase with this one with @prolativ (thx buddy!). The only place that seems to be a possible source of the View type would be via this declaration of extension method:

  implicit final class OutputTraverseOps[A, CC[X] <: Iterable[X]](coll: CC[A]):
    def traverse[B, To](f: A => Output[B])(using BuildFrom[CC[Output[B]], B, To], Context): Output[To] =
      coll.map(f).asInstanceOf[CC[Output[B]]].sequence

with constraint of CC[X] <: Iterable[X] where map is defined on Iterable this way:
def map[B](f: A => B): CC[B] = iterableFactory.from(new View.Map(this, f))

[prolativ]

A better minimization: https://github.com/prolativ/besom-issue-430/tree/prolativ-simplified

[prolativ]

Metals are not to blame for anything here: in test works after a clean compilation, but it crashes when compiled incrementally with scala-cli (after modifying suite.test.scala only). With --server=false in test works too.

[prolativ]

In general the crash in runtime seems to be caused by the compiler picking up the wrong instance of BuildFrom (buildFromView instead of buildFromIterableOps), which is a result of wrong type inference. Note that in the scope of in main the compiler doesn't refuse to compile OutputSequenceOps[Int, scala.collection.View](ints).sequence, which should be invalid because ints is a List[Int] and List[Int] is not a subtype of View[Int]

[prolativ]

A minimization as a (currently failing) Vuilpix-style neg test:

Lib_1.scala:

object Foo:
  def xyz[A, CC[X] <: Iterable[X]](coll: CC[A]): Unit = ()

object Bar:
  export Foo.xyz

Test_2.scala:

object Test:
  val ints = List(1)
  Foo.xyz[Int, List](ints)
  Foo.xyz[Int, scala.collection.View](ints) // error
  Bar.xyz[Int, List](ints)
  Bar.xyz[Int, scala.collection.View](ints) // error

The first error is not reported, although it should. However it is reported if we rename Test_2.scala to Test_1.scala, making all the sources to be compiled together instead of separately

[odersky]

How I fixed it: This was quite hard to diagnose, despite the great minimization. At some point, given a type parameter
CC[X] <: Iterable[X], what should have been a CC[A] got replaced by Iterable[A]. I had no idea why, and looking at many places did not help. It seems the instantiate method produced the wrong types, but the code looked simple and correct. As a last resort, I instrumented the code that created applied types in object Types.AppliedType to fail with an AssertionError if the type so constructed was of the form Iterable[A] with the right uniqid for A. That seemed to work in the sense that the stack trace was from the region where the error happened, so it probably did not give me a spurious error for something that was correct. But the stacktrace alone still did not tell me much, it was basically just the usual set of operations called from instantiate. I then instrumented along the call trace by catching AssertionErrors, printing the failed call and rethrowing the exception. After about 6 steps like this I found something suspicious. In tryReduce in the appliedTo method of TypeApplication we first saw the suspicious Iterable instead of CC. Further investigation revealed that tryReduce produced Iterable from CC by calling safeDealias. And safeDealias was defined like this:

  /** Dealias type if it can be done without forcing the TypeRef's info */
  def safeDealias(using Context): Type = self match {
    case self: TypeRef
    if self.denot.exists && self.symbol.isAliasType && !self.symbol.isProvisional =>
      self.superType.stripTypeVar.safeDealias
    case _ =>
      self
  }

It turned out that the isAliasType test claimed that the type parameter CC was an alias type, so it could replace it by its underlying type, Iterable. Then I looked at isAliasType, which essentially said that a type symbol is an alias type if it is neither a class nor an abstract type. And the isAbstractType test just tested whether the Deferred flag was set. So, by that logic, types that only have the Param flag set are alias types! There is another method, isAbstractOrParamType, which should have been called by isAliasType instead.

So why did anything work before if we got the logic wrong in such a basic part? It's because in Namer we also set the Deferred flag for type parameters. So in most cases the test worked. But it did not work for exports, since forwarder parameters only had the Param flag set.

The following actions were taken to avoid a problem like this in the future:

• Drop the dangerous isAbstractType method, and keep only isAbstractOrParamType`.
• Don't_ set the additional Deferred flag in Namer. That way, we fail fast for any wrong logic that mistakenly assumes that all parameters are Deferred.