Fix incorrect ctx chain in TreeUnpickler

[OlivierBlanvillain]

In this example:

object Test {
  def length(l: LIST): Boolean =
    l eq l

  sealed trait LIST
}

trait LIST used to have the context of method length as its outer context. After this PR TreeUnpickler follows the same pattern used in Namer.

[OlivierBlanvillain]

The failing test is non deterministic and can only be reproduced in the REPL. Here is a minimization:

class Inv

abstract class C {
  val s = locally(1)
}

@allanrenucci Any idea how to investigate that?

[allanrenucci]

Can be reproduced (it seems in a deterministic way) with:

scala> 1                                                                                                                                                                                                        
val res0: Int = 1

scala> locally                                                                                                                                                                                                  
Exception in thread "main" java.lang.AssertionError: assertion failed: denotation module scala invalid in run 2. ValidFor: Period(1..68, run = 3)
	at scala.Predef$.assert(Predef.scala:219)

The REPL and the IDE create multiple runs of the compiler. However some symbols (such as the ones in Definitions) are shared between run and completed lazily with a context from the current run.

Looking at your proposed fix, it seems that a symbol's completer is created in run 1 and then completed in run 2 with a context that derives from run 1. Hope this helps

[odersky]

That's unfortunately a rather sweeping change, which will create severe memory leaks. For this reason, it was intentional that completers in TreeUnpicklers do not store their context.

So the description of the problem is actually what is intended. Is there an issue where things go wrong with the status quo?

[OlivierBlanvillain]

Is this really intended? While unpickling, type assigner will see trait LIST as if it was nested in method length. It looks like:

object Test {
  def length(l: LIST): Boolean = {
    l eq l
    sealed trait LIST
  }
}

Instead of what's in the source (where LIST is inside object TEST, and has nothing to do with length).

This seems clearly wrong. It might be the case that typing LIST with an additional random outer context (method length) doesn't change anything, but it looks really fragile.

In our case we use the chain outer context to know whether or not we are in a transparent context, the addition of method length in that chain changes things when length is itself transparent.

[odersky]

Leaving out the context from the unpickler is necessary. It's true that it's fragile, but I don't see another way. We can't leave contexts (which have about a 4MB transitive footprint) hanging in completers because these completers might not be triggered until many runs later. So this would prevent the compiler from being resident at all, which would make the IDE impossible.

[OlivierBlanvillain]

Ok makes sense, we will try to find a workaround for our transparent methods.

[Blaisorblade]

Instead of closing over contexts, could one complete symbols after unpickling "outside-in" following the pattern in Namer?

[smarter]

In our case we use the chain outer context to know whether or not we are in a transparent context

Could you use the chain of owners instead?

[smarter]

As a side-note, the fact that the unpickling completers have these weird contexts where you can't trust everything is a pretty big trap, maybe we could set a flag on them or something so that any attempt to read fields like ctx.outer results in a runtime crash.

[OlivierBlanvillain]

test performance please

[dottybot]

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

[dottybot]

performance test failed:

Please check http://lamppc37.epfl.ch:8000/pull-4889-08-23-16.54.out for more information

[Blaisorblade]

@OlivierBlanvillain You might want to restore this branch if you want to test its performance 😉

[OlivierBlanvillain]

test performance please

[dottybot]

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

[dottybot]

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/4889/ to see the changes.

Benchmarks is based on merging with master (33afaa2)