Compilation order dependencies in compileStdLib #5433
Comments
milessabin
changed the title
compileStdLib
|
After updating I no longer see a local failure unless I explicitly sort the sources, so local and CI appear to be on a par now. I've attempted to minimize the problem and come up with the following, Compiling the three However, with all four the compiler crashes with, |
|
This seems like a different issue: to compile Stream we unpickle StreamViewLike from a Scala 2 classfile, which then tries to find some symbol in SeqViewLike that we're also trying to compile. This sort of Dotty->Scala2->Dotty dependency chain is not something that should ever happen in a real project (that is, until we make Scala 2 use Dotty as a backend) so we shouldn't try to make it work. If you add StreamViewLike to the list of files you're compiling, do you still get a crash ? |
Perhaps. But then removing any source file from the full |
No it doesn't. |
One way around that would be to first change the top level package (easiest way to do it is to add "package foo" at the top of every file) then do the minimization. |
TBQH, that's what I think the full test really ought to be. |
|
Well we do need to test the actual standard library as we would compile it as part of the bootstrapping process, but since we're moving to the 2.13 library anyway, it'd make sense to import the sources of the 2.12 library in the dotty repo, add an extra top package, and make it a regular test. |
|
Reduced to 4 files: |
|
And here is a single file reproduction: package scala.collection
trait IterableViewLike[+A, +Coll, +This <: IterableView[A, Coll] with IterableViewLike[A, Coll, This]]
extends TraversableView[A, Coll] {
trait TransformedI[+B] extends IterableView[B, Coll] with super.TransformedT[B]
}
trait SeqViewLike[+A, +Coll, +This <: SeqView[A, Coll] with SeqViewLike[A, Coll, This]]
extends IterableView[A, Coll] with IterableViewLike[A, Coll, This] {
trait TransformedS[+B] extends SeqView[B, Coll] with super.TransformedI[B]
abstract class AbstractTransformedS[+B] extends Seq[B] with super[IterableViewLike].TransformedI[B] with TransformedS[B]
}
trait TraversableViewLike[+A, +Coll, +This <: TraversableView[A, Coll] with TraversableViewLike[A, Coll, This]] {
trait TransformedT[+B] extends TraversableView[B, Coll]
}
trait IndexedSeqView[A, +Coll] extends IndexedSeq[A]
with IndexedSeqOptimized[A, IndexedSeqView[A, Coll]]
with SeqView[A, Coll]
with SeqViewLike[A, Coll, IndexedSeqView[A, Coll]] {
trait TransformedX[B] extends IndexedSeqView[B, Coll] with super.TransformedS[B]
abstract class AbstractTransformedX[B] extends super.AbstractTransformedS[B] with TransformedX[B]
} |
|
Minimization: import scala.collection._
abstract class ATS[B] extends Seq[B]
trait ISV[A, +Coll] extends IndexedSeq[A]
with IndexedSeqOptimized[A, ISV[A, Coll]] {
abstract class Fail[B] extends ATS[B] with ISV[B, Coll]
} |
|
@allanrenucci Your reproduction (and my minimization) both fail to compile using scalac with: i5433.scala:5: error: overriding method newBuilder in trait TraversableLike of type => scala.collection.mutable.Builder[A,ISV[A,Coll]];
method newBuilder in trait GenericTraversableTemplate of type => scala.collection.mutable.Builder[A,IndexedSeq[A]] has incompatible type
trait ISV[A, +Coll] extends IndexedSeq[A]
^
one error foundSo they're not actually valid reproduction of the original problem, but point to a separate issue in Dotty (missing check in RefChecks) |
|
Here's a minimization that:
class X
class Y extends X
trait A[+T] {
def foo: T = null.asInstanceOf[T]
}
trait B extends A[X] {
override def foo: X = new X
}
trait C extends A[Y] {
override def foo: Y = new Y
super.foo
}
class Fail extends B with C
object Test {
def main(args: Array[String]): Unit = {
new Fail // ClassCastException: X cannot be cast to Y
}
}@adriaanm is this a known issue ? ^ |
|
To elaborate, the problem is that the call to def C$$super$foo: Ywhich is then implemented in def C$$super$foo: Y = super[B].foowhich does not typecheck since the result type of Looking at the spec for super-calls, I don't think this situation was ever considered. |
|
Note that I checked and scala/scala#7439 doesn't save us here. |
|
I think the correct thing to do here would be for def C$$super$foo: Y = super[A[Y]].fooThat is, pick the first element in the linearization order that typechecks, so |
|
@smarter just showed me this and it looks pretty scary... with the current runtime semantics for superaccessors, It's not obvious what would be a good fix. Strictly speaking, a more accurate type would be a (not-yet-existing)
That seems sound, but it seems a concerning chance in semantics for this case, tho it might be OK in practice. |
Relying on `matchingDenotation` is not enough as demonstrated by i5433.scala: in `Fail`, `B#foo` matches `C$$super$foo` but it cannot implement it since `X` is a supertype of `Y` Note that scalac seems to have the same bug (but at least in Dotty this is detected by -Ycheck:all).
Relying on `matchingDenotation` is not enough as demonstrated by i5433.scala: in `Fail`, `B#foo` matches `C$$super$foo` but it cannot implement it since `X` is a supertype of `Y` Note that scalac seems to have the same bug (but at least in Dotty this is detected by -Ycheck:all). For reference, here's what the spec says on resolving super accessors: A reference super.m refers statically to a method or type m in the least proper supertype of the innermost template containing the reference. It evaluates to the member m′ in the actual supertype of that template which is equal to m or which overrides m. This seems like it should be clarified to indicate that the member m' may be found not only in "the actual supertype" (defined elsewhere) but in any supertype of the actual supertype.
My first reaction would be to still make this a |
|
I don't have strong feelings either way, but emitting an error here means that implementation details (which traits need what kind of super-accessors) affect how traits can be composed, and since users don't fully control the linearization order they might be stuck, e.g. writing: class Fail extends B with A[Y] with Cdoesn't help. |
|
Thankfully it looks like the 2.13 library doesn't need many super-accessors, it could still be useful to review all the |
Relying on `matchingDenotation` is not enough as demonstrated by i5433.scala: in `Fail`, `B#foo` matches `C$$super$foo` but it cannot implement it since `X` is a supertype of `Y` Note that scalac seems to have the same bug (but at least in Dotty this is detected by -Ycheck:all). For reference, here's what the spec says on resolving super accessors: A reference super.m refers statically to a method or type m in the least proper supertype of the innermost template containing the reference. It evaluates to the member m′ in the actual supertype of that template which is equal to m or which overrides m. This seems like it should be clarified to indicate that the member m' may be found not only in "the actual supertype" (defined elsewhere) but in any supertype of the actual supertype.
|
@odersky’s idea might be the least bad here.
That’s a good point and an annoying failure of separate typechecking. But we already fail this sort of guarantee to some extent: in some cases you cannot reliably mix together traits from independent maintainers. If later both add a method with the same name, you’re stuck. In some scenarios that’s even more annoying. If you use the cake pattern with interfaces, you can have two “module interfaces” that compose together, and two “implementations” that don’t link together because of such extra methods. This is still a very annoying pitfall. What I don’t get is how was the impact so low nobody knew? |
Relying on `matchingDenotation` is not enough as demonstrated by i5433.scala: in `Fail`, `B#foo` matches `C$$super$foo` but it cannot implement it since `X` is a supertype of `Y` Note that scalac seems to have the same bug (but at least in Dotty this is detected by -Ycheck:all). For reference, here's what the spec says on resolving super accessors: A reference super.m refers statically to a method or type m in the least proper supertype of the innermost template containing the reference. It evaluates to the member m′ in the actual supertype of that template which is equal to m or which overrides m. This seems like it should be clarified to indicate that the member m' may be found not only in "the actual supertype" (defined elsewhere) but in any supertype of the actual supertype.
Relying on `matchingDenotation` is not enough as demonstrated by i5433.scala: in `Fail`, `B#foo` matches `C$$super$foo` but it cannot implement it since `X` is a supertype of `Y` Note that scalac seems to have the same bug (but at least in Dotty this is detected by -Ycheck:all). For reference, here's what the spec says on resolving super accessors: A reference super.m refers statically to a method or type m in the least proper supertype of the innermost template containing the reference. It evaluates to the member m′ in the actual supertype of that template which is equal to m or which overrides m. This seems like it should be clarified to indicate that the member m' may be found not only in "the actual supertype" (defined elsewhere) but in any supertype of the actual supertype.
Relying on `matchingDenotation` is not enough as demonstrated by i5433.scala: in `Fail`, `B#foo` matches `C$$super$foo` but it cannot implement it since `X` is a supertype of `Y` Note that scalac seems to have the same bug (but at least in Dotty this is detected by -Ycheck:all). For reference, here's what the spec says on resolving super accessors: A reference super.m refers statically to a method or type m in the least proper supertype of the innermost template containing the reference. It evaluates to the member m′ in the actual supertype of that template which is equal to m or which overrides m. This seems like it should be clarified to indicate that the member m' may be found not only in "the actual supertype" (defined elsewhere) but in any supertype of the actual supertype.
Relying on `matchingDenotation` is not enough as demonstrated by i5433.scala: in `Fail`, `B#foo` matches `C$$super$foo` but it cannot implement it since `X` is a supertype of `Y` Note that scalac seems had the same bug (but at least in Dotty this is detected by -Ycheck:all), now fixed based on the logic in this PR by scala/scala#7641. For reference, here's what the spec says on resolving super accessors: A reference super.m refers statically to a method or type m in the least proper supertype of the innermost template containing the reference. It evaluates to the member m′ in the actual supertype of that template which is equal to m or which overrides m.
Relying on `matchingDenotation` is not enough as demonstrated by i5433.scala: in `Fail`, `B#foo` matches `C$$super$foo` but it cannot implement it since `X` is a supertype of `Y` Note that scalac seems had the same bug (but at least in Dotty this is detected by -Ycheck:all), now fixed based on the logic in this PR by scala/scala#7641. For reference, here's what the spec says on resolving super accessors: A reference super.m refers statically to a method or type m in the least proper supertype of the innermost template containing the reference. It evaluates to the member m′ in the actual supertype of that template which is equal to m or which overrides m.
Relying on `matchingDenotation` is not enough as demonstrated by i5433.scala: in `Fail`, `B#foo` matches `C$$super$foo` but it cannot implement it since `X` is a supertype of `Y` Note that scalac seems had the same bug (but at least in Dotty this is detected by -Ycheck:all), now fixed based on the logic in this PR by scala/scala#7641. For reference, here's what the spec says on resolving super accessors: A reference super.m refers statically to a method or type m in the least proper supertype of the innermost template containing the reference. It evaluates to the member m′ in the actual supertype of that template which is equal to m or which overrides m.
Relying on `matchingDenotation` is not enough as demonstrated by i5433.scala: in `Fail`, `B#foo` matches `C$$super$foo` but it cannot implement it since `X` is a supertype of `Y` Note that scalac seems had the same bug (but at least in Dotty this is detected by -Ycheck:all), now fixed based on the logic in this PR by scala/scala#7641. For reference, here's what the spec says on resolving super accessors: A reference super.m refers statically to a method or type m in the least proper supertype of the innermost template containing the reference. It evaluates to the member m′ in the actual supertype of that template which is equal to m or which overrides m.
Fix #5433: check that the implemented super-accessor is valid
Running
testCompilationlocally fails for me (Linux, Oracle JDK 1.8.0_192) due to what appears to be a compilation order dependency incompileStdLib. This isn't normally visible in CI runs, presumably due to platform dependency of the compilation order. Sorting the sources, as done in #5432 causes local and CI builds to fail in the same way.The text was updated successfully, but these errors were encountered: