More IDE work #2885
More IDE work #2885
Conversation
| @@ -62,6 +62,9 @@ object Interactive { | |||
| sourceSymbol(sym.owner) | |||
| else sym | |||
|
|
|||
| private def safely[T](op: => List[T]): List[T] = | |||
| @@ -131,7 +131,7 @@ object Interactive { | |||
| * @param includeReferences If true, include references and not just definitions | |||
| */ | |||
| def namedTrees(trees: List[SourceTree], includeReferences: Boolean, treePredicate: NameTree => Boolean) | |||
| (implicit ctx: Context): List[SourceTree] = { | |||
| (implicit ctx: Context): List[SourceTree] = safely { | |||
There was a problem hiding this comment.
It's a bit of a shame that this means we'll discard all results when the problem might be localized. Alternatively, we could move the try/catch around traverseChildren I think.
There was a problem hiding this comment.
I think it's pretty rare that we would get a TypeError, so not a big deal either way.
| @@ -2146,13 +2146,15 @@ object Types { | |||
| } | |||
|
|
|||
| case class LazyRef(refFn: () => Type) extends UncachedProxyType with ValueType { | |||
| private var myRefFn: () => Type = refFn | |||
There was a problem hiding this comment.
I think this would be slightly nicer as case class LazyRef(private var refFn: () => Type), this way you cannot accidentally capture refFn outside of the constructor.
| @@ -2146,13 +2146,15 @@ object Types { | |||
| } | |||
|
|
|||
| case class LazyRef(refFn: () => Type) extends UncachedProxyType with ValueType { | |||
| private var myRefFn: () => Type = refFn | |||
There was a problem hiding this comment.
Also, another way to avoid the leak would be to write refFn: Context => Type at the cost that equals could not be made to force the reference anymore. I tried it before and it seemed to work, but I haven't benchmarked it.
There was a problem hiding this comment.
I am not sure it would work in general, though. Seems generally dicy to evaluate in a different context. We'd have to check every site where we create a LazyRef whether it is permissible.
Same principle as for completions applies.
This prevented many local functions from being lifted out to static scope, and thereby caused memory leaks. The memory leak found in the IDE was caused an anonymous function in NamerContextOps (a base trait of Context) that was kept as a non-private private method of NamerContextOps, taking a `this` referring to a context.
The previous attempt did not actually remove the closure, since LazyRef is a case class.
| @@ -61,14 +61,15 @@ object Annotations { | |||
| def tree(implicit ctx: Context) = body | |||
| } | |||
|
|
|||
| case class LazyBodyAnnotation(bodyExpr: Context => Tree) extends BodyAnnotation { | |||
| case class LazyBodyAnnotation(private var bodyExpr: Context => Tree) extends BodyAnnotation { | |||
There was a problem hiding this comment.
It's suspicious that a Context => Tree function would capture a Context.
There was a problem hiding this comment.
Oh actually I guess this is intentional: https://github.com/lampepfl/dotty/blob/master/compiler/src/dotty/tools/dotc/typer/Inliner.scala#L193
(Same principle as for completions applies).