Fix cyclic reference errors in collection strawman #2914
Conversation
Need to keep cycle breakers
Same principle as reading from Scala2 pickles applied = in the face of F-bounded polymorphism we cannot assume that the type parameters are known when we unpickle a type application.
| val rdr = fork | ||
| def readUnderlying() = rdr.readType() | ||
| skipTree() | ||
| LazyRef(readUnderlying) |
There was a problem hiding this comment.
This means LazyRefs will keep the context from which they were unpickled around, if these LazyRefs are subsequently stored in some cache, that could lead to space leaks, no?
There was a problem hiding this comment.
Yes, correct. Maybe the best option is to allow a context to be passed into a LazyRef completer. In the case of unpickling, this is fine; we don't need to hang on to more than the reader (I think, at least that's what we do for the other completers).
| case symbol => symbol.showFullName | ||
| } | ||
| catch { | ||
| case NonFatal(ex) => fntpe.show |
There was a problem hiding this comment.
If this is only for cyclic references, could we do case ex: CyclicReference instead?
There was a problem hiding this comment.
I think we by default we should not throw any exception from printing. It detracts from the underlying cause.
There was a problem hiding this comment.
Sure, but we shouldn't be throwing any kind of exceptions, and silently catching them all might hide issues.
|
@odersky, instead of pickling lazy refs, could we create them at un-pickling similarly to how we create them at typer? |
I tried that, but it didn't help. Did not get to find out why. Anyway, pickling LazyRefs is more efficient than re-scanning types to reconstruct them. |
This avoids memory leaks in unpicklers, where lazy refs don't capture the context anymore. Other lazyrefs still need to capture context, though.
|
Merging now to get the strawman unstuck, but I opened an issue to revert the LazyRef addition if possible. /cc @julienrf |
|
Thanks @odersky! |
This should make collection strawman #137 build. There were some "interesting" F-bounded types which triggered a series of problems when read back with Tasty unpicklers. First, the underlying error message ("wrong number of arguments") was hidden because it caused a CRE. Next, the wrong number of argument problems demanded that we are more careful reading applicatons. Once that was done we got Stackoverflows. To avoid these, we have to maintain any LazyRefs we have in the original types in the pickle information.
Review by @szeiger or @julienrf?