Fix #2677: Several fixes involving super selection #2768

odersky · 2017-06-15T10:26:08Z

This was much harder than I thought. In the end, there are several fixes that interact in interesting ways with each other.

We fix the way baseclasses are computed in AndTypes.
We fix the way members of supertypes are computed.
We keep the order of operands of a & invariant.

The problems in (1) and (3) conspired to cancel each other out. So once I fixed (1) the problem caused by (3) became apparent, but in a very indirect way. Details are in the commit message of
0f708eb.

List union is clearly wrong, as it is simply the same as ++.

It used to return the joint denotation, but the correct answer is the denotation of the symbol in the joint denotation as seen from the super prefix.

OlivierBlanvillain · 2017-06-15T13:57:33Z

Failure reproduced on my machine with sbt dotty-bootstrapped/vulpix t1751

Testing ../tests/pos-java-interop/t1751
scala.MatchError: Typed(SeqLiteral(List(),TypeTree[TypeVar(ParamRef(scala$Array$apply$$T) -> RefinedType(TypeRef(ThisType(TypeRef(NoPrefix,lang)),Class), java$lang$Class$$T, TypeBounds(TypeRef(ThisType(TypeRef(NoPrefix,scala)),Nothing), TypeRef(ThisType(TypeRef(NoPrefix,scala)),Any))))]),TypeTree[RefinedType(TypeRef(ThisType(TypeRef(NoPrefix,scala)),<repeated>), scala$<repeated>$$T0, TypeAlias(TypeVar(ParamRef(scala$Array$apply$$T) -> RefinedType(TypeRef(ThisType(TypeRef(NoPrefix,lang)),Class), java$lang$Class$$T, TypeBounds(TypeRef(ThisType(TypeRef(NoPrefix,scala)),Nothing), TypeRef(ThisType(TypeRef(NoPrefix,scala)),Any)))), 1))]) (of class dotty.tools.dotc.ast.Trees$Typed)
        at dotty.tools.backend.jvm.DottyBackendInterface.emitArgument(DottyBackendInterface.scala:237)
        at dotty.tools.backend.jvm.DottyBackendInterface.$anonfun$31(DottyBackendInterface.scala:279)
        at scala.compat.java8.JProcedure1.apply(JProcedure1.java:18)
        at scala.compat.java8.JProcedure1.apply(JProcedure1.java:10)
        at scala.collection.immutable.List.foreach(List.scala:392)
        at dotty.tools.backend.jvm.DottyBackendInterface.emitArgument(DottyBackendInterface.scala:279)
        at dotty.tools.backend.jvm.DottyBackendInterface.emitAssocs$$anonfun$2(DottyBackendInterface.scala:315)
        at scala.compat.java8.JProcedure1.apply(JProcedure1.java:18)
        at scala.compat.java8.JProcedure1.apply(JProcedure1.java:10)
        at scala.collection.TraversableLike$WithFilter$$anonfun$foreach$1.apply(TraversableLike.scala:733)
        at scala.collection.immutable.List.foreach(List.scala:392)
        at scala.collection.TraversableLike$WithFilter.foreach(TraversableLike.scala:732)
        at dotty.tools.backend.jvm.DottyBackendInterface.emitAssocs(DottyBackendInterface.scala:315)
        at dotty.tools.backend.jvm.DottyBackendInterface.emitAnnotations$$anonfun$5(DottyBackendInterface.scala:308)
        at scala.compat.java8.JProcedure1.apply(JProcedure1.java:18)
        at scala.compat.java8.JProcedure1.apply(JProcedure1.java:10)
        at scala.collection.TraversableLike$WithFilter$$anonfun$foreach$1.apply(TraversableLike.scala:733)
        at scala.collection.immutable.List.foreach(List.scala:392)
        at scala.collection.TraversableLike$WithFilter.foreach(TraversableLike.scala:732)
        at dotty.tools.backend.jvm.DottyBackendInterface.emitAnnotations(DottyBackendInterface.scala:309)
        at scala.tools.nsc.backend.jvm.BCodeHelpers$BCAnnotGen.emitAnnotations(BCodeHelpers.scala:282)
        at scala.tools.nsc.backend.jvm.BCodeSkelBuilder$PlainSkelBuilder.initJClass(BCodeSkelBuilder.scala:161)
        at scala.tools.nsc.backend.jvm.BCodeSkelBuilder$PlainSkelBuilder.genPlainClass(BCodeSkelBuilder.scala:102)
        at dotty.tools.backend.jvm.GenBCodePipeline$Worker1.visit(GenBCode.scala:199)
        at dotty.tools.backend.jvm.GenBCodePipeline$Worker1.run(GenBCode.scala:152)
        at dotty.tools.backend.jvm.GenBCodePipeline.buildAndSendToDisk(GenBCode.scala:367)
        at dotty.tools.backend.jvm.GenBCodePipeline.run(GenBCode.scala:333)
        at dotty.tools.backend.jvm.GenBCode.run(GenBCode.scala:54)
        at dotty.tools.dotc.core.Phases$Phase.runOn$$anonfun$2(Phases.scala:283)
        at scala.collection.immutable.List.map(List.scala:284)
        at dotty.tools.dotc.core.Phases$Phase.runOn(Phases.scala:285)
        at dotty.tools.dotc.Run.compileUnits$$anonfun$1$$anonfun$1(Run.scala:82)
        at scala.compat.java8.JProcedure1.apply(JProcedure1.java:18)
        at scala.compat.java8.JProcedure1.apply(JProcedure1.java:10)
        at scala.collection.IndexedSeqOptimized$class.foreach(IndexedSeqOptimized.scala:33)
        at scala.collection.mutable.ArrayOps$ofRef.foreach(ArrayOps.scala:186)
        at dotty.tools.dotc.Run.compileUnits$$anonfun$1(Run.scala:90)
        at scala.compat.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.java:12)
        at dotty.tools.dotc.util.Stats$.monitorHeartBeat(Stats.scala:76)
        at dotty.tools.dotc.Run.compileUnits(Run.scala:95)
        at dotty.tools.dotc.Run.compileSources(Run.scala:64)
        at dotty.tools.dotc.Run.compile(Run.scala:48)
        at dotty.tools.dotc.Driver.doCompile(Driver.scala:26)
        at dotty.tools.vulpix.ParallelTesting$$anon$20.doCompile$$anonfun$1(ParallelTesting.scala:367)
        at dotty.tools.vulpix.ParallelTesting$$anon$20.ntimes$$anonfun$1(ParallelTesting.scala:362)
        at scala.collection.TraversableOnce$$anonfun$foldLeft$1.apply(TraversableOnce.scala:157)
        at scala.collection.TraversableOnce$$anonfun$foldLeft$1.apply(TraversableOnce.scala:157)
        at scala.collection.immutable.Range.foreach(Range.scala:160)
        at scala.collection.TraversableOnce$class.foldLeft(TraversableOnce.scala:157)
        at scala.collection.AbstractTraversable.foldLeft(Traversable.scala:104)
        at scala.collection.TraversableOnce$class.$div$colon(TraversableOnce.scala:151)
        at scala.collection.AbstractTraversable.$div$colon(Traversable.scala:104)
        at dotty.tools.vulpix.ParallelTesting$$anon$20.ntimes(ParallelTesting.scala:362)
        at dotty.tools.vulpix.ParallelTesting$$anon$20.doCompile(ParallelTesting.scala:369)
        at dotty.tools.dotc.Driver.process(Driver.scala:124)
        at dotty.tools.dotc.Driver.process(Driver.scala:93)
        at dotty.tools.vulpix.ParallelTesting$Test.compile(ParallelTesting.scala:377)
        at dotty.tools.vulpix.ParallelTesting$PosTest$$anon$17.$anonfun$131(ParallelTesting.scala:444)
        at scala.collection.immutable.List.map(List.scala:288)
        at dotty.tools.vulpix.ParallelTesting$PosTest$$anon$17.checkTestSource$$anonfun$2(ParallelTesting.scala:445)
        at scala.compat.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.java:12)
        at dotty.tools.vulpix.ParallelTesting$Test.tryCompile(ParallelTesting.scala:311)
        at dotty.tools.vulpix.ParallelTesting$PosTest$$anon$17.checkTestSource(ParallelTesting.scala:464)
        at dotty.tools.vulpix.ParallelTesting$Test$LoggedRunnable.run(ParallelTesting.scala:212)
        at java.util.concurrent.ForkJoinTask$AdaptedRunnableAction.exec(ForkJoinTask.java:1386)
        at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:289)
        at java.util.concurrent.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1056)
        at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1692)
        at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:157)

Error while emitting A2_1.scala
Typed(SeqLiteral(List(),TypeTree[TypeVar(ParamRef(scala$Array$apply$$T) -> RefinedType(TypeRef(ThisType(TypeRef(NoPrefix,lang)),Class), java$lang$Class$$T, TypeBounds(TypeRef(ThisType(TypeRef(NoPrefix,scala)),Nothing), TypeRef(ThisType(TypeRef(NoPrefix,scala)),Any))))]),TypeTree[RefinedType(TypeRef(ThisType(TypeRef(NoPrefix,scala)),<repeated>), scala$<repeated>$$T0, TypeAlias(TypeVar(ParamRef(scala$Array$apply$$T) -> RefinedType(TypeRef(ThisType(TypeRef(NoPrefix,lang)),Class), java$lang$Class$$T, TypeBounds(TypeRef(ThisType(TypeRef(NoPrefix,scala)),Nothing), TypeRef(ThisType(TypeRef(NoPrefix,scala)),Any)))), 1))]) (of class dotty.tools.dotc.ast.Trees$Typed)
-- [E006] Unbound Identifier Error: /home/olivier/workspace/dotty/compiler/../tests/pos-java-interop/t1751/A1_2.scala:1:28
1 |@SuiteClasses(Array(classOf[A2]))
  |                            ^^
  |                            not found: type A2
[error] Test dotty.tools.dotc.CompilationTests.posTwice failed: java.lang.AssertionError: Expected no errors when compiling, but found: 2, took 2.121 sec
[error]     at dotty.tools.vulpix.ParallelTesting$CompilationTest.checkCompile(ParallelTesting.scala:845)
[error]     at dotty.tools.dotc.CompilationTests.posTwice(CompilationTests.scala:139)
[error]     ...

odersky · 2017-06-16T09:21:21Z

The problem is we need to know in what order the files for dotc itself were compiled. Unfortunately it's that order that makes a difference between a compiler that works and a compiler that crashes. Can you pull that out of the logs on your machine?

We previously could get the following: Given: class A trait B extends A trait C extends A Then (A & B) & C simplified to C & B This is usually not a problem, *except* for typing super calls, where we pick the symbol of the right operand. The problem was masked because due to the wrong computation of base classes for AndTypes, we always ended up picking the leftmost symbol in a joint denotation instead of the rightmost one. So we got a double swap! The problem is reproducible in the test case. It originally manifested itself only in the bootstrap, and only under a particular order of compiling files, which was exercised by the CI but initially not by my local setup. The summary of the problem is as follows: ElimRepeated has the structure of ER in the test case. Because of the problem, a super call in `transform` of `ElimRepeated` went to `InfoTransformers` instead of `AnnotationTransformers`. Consequently, the elim repeated transform was not done on annotations, which led to backend crashes of the bootstrapped compiler for those annotations that had repeated arguments. It took me two full days to get to the bottom of this. Must rank right at the top of the list of hard-to-find bugs for me. Note: It would seem logical to apply a similar technique to OrTypes. But this currently fails some tests. Need to investigate later.

smarter · 2017-06-19T13:07:02Z

tests/pos/Orderings.scala

+  class Context
+  implicit val ctx: Context
+
+  f(2)               // OK


The definition of f appears to be missing from this test.

smarter · 2017-06-19T13:57:38Z

compiler/src/dotty/tools/dotc/core/Types.scala

+            else
+              if (bcs1set contains bc2)
+                if (bc2.is(Trait)) bc2 :: recur(bcs2rest)
+                else bcs2rest 


Shouldn't this be bcs2 ? Otherwise we miss bc2 even though it's contained in the baseclasses of both operands

Good catch!

smarter

LGTM, but it seems like we really need more tests exercising the base classes of an OrType :)

odersky added 4 commits June 15, 2017 12:23

Fix baseClasses computation of AndTypes

347a3aa

List union is clearly wrong, as it is simply the same as ++.

Fix findMember of SuperType

9c46f72

It used to return the joint denotation, but the correct answer is the denotation of the symbol in the joint denotation as seen from the super prefix.

Fix tests to conform to new behavior

f479be1

test case

953e8db

odersky force-pushed the fix-@2677-2 branch from 049199a to a4e9d28 Compare June 16, 2017 16:25

odersky force-pushed the fix-@2677-2 branch from a4e9d28 to 0f708eb Compare June 16, 2017 17:08

odersky changed the title ~~WIP Another partial fix of #2677~~ Fix #2677: Several fixes involving super selection Jun 16, 2017

odersky requested a review from smarter June 16, 2017 17:12

This was referenced Jun 16, 2017

Fix #2677 V3 #2775

Closed

[WIP] Fix baseClasses computation of AndTypes #2767

Closed

Fix #2677: Fix type of super selection #2757

Closed

smarter mentioned this pull request Jun 17, 2017

Super-call in intersection type not type safe #2677

Closed

Make computation of baseClasses for And/OrTypes more efficient.

0fde265

smarter reviewed Jun 19, 2017

View reviewed changes

One more test

243abad

odersky force-pushed the fix-@2677-2 branch from 6a00b2e to 243abad Compare June 19, 2017 13:36

smarter requested changes Jun 19, 2017

View reviewed changes

Fix logic error in baseClasses computation

b03cb94

smarter approved these changes Jun 19, 2017

View reviewed changes

odersky merged commit ea582a7 into scala:master Jun 20, 2017

allanrenucci deleted the fix-@2677-2 branch December 14, 2017 19:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix #2677: Several fixes involving super selection #2768

Fix #2677: Several fixes involving super selection #2768

Uh oh!

odersky commented Jun 15, 2017 •

edited

Loading

Uh oh!

OlivierBlanvillain commented Jun 15, 2017 •

edited

Loading

Uh oh!

odersky commented Jun 16, 2017

Uh oh!

smarter Jun 19, 2017

Uh oh!

smarter Jun 19, 2017

Uh oh!

odersky Jun 19, 2017

Uh oh!

smarter left a comment

Uh oh!

Uh oh!

Fix #2677: Several fixes involving super selection #2768

Fix #2677: Several fixes involving super selection #2768

Uh oh!

Conversation

odersky commented Jun 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OlivierBlanvillain commented Jun 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

odersky commented Jun 16, 2017

Uh oh!

smarter Jun 19, 2017

Choose a reason for hiding this comment

Uh oh!

smarter Jun 19, 2017

Choose a reason for hiding this comment

Uh oh!

odersky Jun 19, 2017

Choose a reason for hiding this comment

Uh oh!

smarter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

odersky commented Jun 15, 2017 •

edited

Loading

OlivierBlanvillain commented Jun 15, 2017 •

edited

Loading