Skip to content

Commit

Permalink
SI-10026 Fix endless cycle in runtime reflection
Browse files Browse the repository at this point in the history
56f23af introduced a call to `baseTypeSeq` of `scala.collection.mutable.ArrayOps.ofRef[?T]{}`
in `findMember`. This exposed a latent bug in the synchronized wrapper of `BaseTypeSeq`,
demonstrated below with an older version of Scala:

```
Welcome to Scala 2.11.8 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_112).
Type in expressions for evaluation. Or try :help.

scala> val symtab = reflect.runtime.universe.asInstanceOf[scala.reflect.internal.SymbolTable]
symtab: scala.reflect.internal.SymbolTable = scala.reflect.runtime.JavaUniverse@458544e0

scala> import symtab._
import symtab._

scala> val ArrayOps_ofRef_Class = symtab.symbolOf[scala.collection.mutable.ArrayOps.ofRef[AnyRef]]
ArrayOps_ofRef_Class: symtab.TypeSymbol = class ofRef

scala> appliedType(symbolOf[Set[Any]], symbolOf[Set[Any]].typeParams.map(TypeVar(_)))
res2: symtab.Type = Set[?A]

scala> .narrow
res3: symtab.Type = <none>.<refinement>.type

scala> .baseTypeSeq
java.lang.StackOverflowError
  at scala.reflect.runtime.Gil$class.gilSynchronized(Gil.scala:21)
  at scala.reflect.runtime.JavaUniverse.gilSynchronized(JavaUniverse.scala:16)
  at scala.reflect.runtime.SynchronizedOps$SynchronizedBaseTypeSeq$class.map(SynchronizedOps.scala:27)
  at scala.reflect.runtime.SynchronizedOps$SynchronizedBaseTypeSeq$$anon$2.map(SynchronizedOps.scala:34)
  at scala.reflect.runtime.SynchronizedOps$SynchronizedBaseTypeSeq$class.lateMap(SynchronizedOps.scala:34)
  at scala.reflect.runtime.SynchronizedOps$SynchronizedBaseTypeSeq$$anon$2.lateMap(SynchronizedOps.scala:34)
  at scala.reflect.internal.BaseTypeSeqs$MappedBaseTypeSeq.map(BaseTypeSeqs.scala:235)
  at scala.reflect.runtime.SynchronizedOps$SynchronizedBaseTypeSeq$$anon$2.scala$reflect$runtime$SynchronizedOps$SynchronizedBaseTypeSeq$$super$map(SynchronizedOps.scala:34)
  at scala.reflect.runtime.SynchronizedOps$SynchronizedBaseTypeSeq$$anonfun$map$1.apply(SynchronizedOps.scala:27)
  at scala.reflect.runtime.SynchronizedOps$SynchronizedBaseTypeSeq$$anonfun$map$1.apply(SynchronizedOps.scala:27)
  at scala.reflect.runtime.Gil$class.gilSynchronized(Gil.scala:19)
  at scala.reflect.runtime.JavaUniverse.gilSynchronized(JavaUniverse.scala:16)
  at scala.reflect.runtime.SynchronizedOps$SynchronizedBaseTypeSeq$class.map(SynchronizedOps.scala:27)
  at scala.reflect.runtime.SynchronizedOps$SynchronizedBaseTypeSeq$$anon$2.map(SynchronizedOps.scala:34)
  at scala.reflect.runtime.SynchronizedOps$SynchronizedBaseTypeSeq$class.lateMap(SynchronizedOps.scala:34)
  at scala.reflect.runtime.SynchronizedOps$SynchronizedBaseTypeSeq$$anon$2.lateMap(SynchronizedOps.scala:34)
  at scala.reflect.internal.BaseTypeSeqs$MappedBaseTypeSeq.map(BaseTypeSeqs.scala:235)
```

The infinite cycle involves:

```
class MappedBaseTypeSeq(orig: BaseTypeSeq, f: Type => Type) extends BaseTypeSeq(orig.parents map f, orig.elems) {
    ...
    override def map(g: Type => Type) = lateMap(g)
    override def lateMap(g: Type => Type) = orig.lateMap(x => g(f(x)))
  }

  trait SynchronizedBaseTypeSeq extends BaseTypeSeq {
    ...
    override def map(f: Type => Type): BaseTypeSeq = gilSynchronized { super.map(f) }
    override def lateMap(f: Type => Type): BaseTypeSeq =
      // only need to synchronize BaseTypeSeqs if they contain refined types
      if (map(f).toList.exists(_.isInstanceOf[RefinedType])) new MappedBaseTypeSeq(this, f) with SynchronizedBaseTypeSeq
      else new MappedBaseTypeSeq(this, f)
  }
```

This commit creates a new factory method for `MappedBaseTypeSeq`-s to break the cycle.

As an independent change, I have also removed the attempt to conditionally synchronize them,
as the condition was eagerly applying the map function (and throwing away the result!).

I've appeased MiMa with new whitelist entries, but I'm confident that this is deep enough
in the bowels of our runtime reflection implementation that there is no way that user code
will be calling these methods directly.
  • Loading branch information
retronym committed Feb 19, 2017
1 parent 147e5dd commit 777a0e5
Show file tree
Hide file tree
Showing 6 changed files with 38 additions and 6 deletions.
8 changes: 8 additions & 0 deletions bincompat-backward.whitelist.conf
Expand Up @@ -33,6 +33,14 @@ filter {
matchName="scala.sys.process.ProcessImpl#CompoundProcess.getExitValue"
problemName=DirectMissingMethodProblem
},
{
matchName="scala.reflect.runtime.SynchronizedOps.scala$reflect$runtime$SynchronizedOps$$super$newMappedBaseTypeSeq"
problemName=ReversedMissingMethodProblem
},
{
matchName="scala.reflect.runtime.SynchronizedOps#SynchronizedBaseTypeSeq.lateMap"
problemName=DirectMissingMethodProblem
},
{
matchName="scala.collection.immutable.HashMap.contains0"
problemName=DirectMissingMethodProblem
Expand Down
8 changes: 8 additions & 0 deletions bincompat-forward.whitelist.conf
Expand Up @@ -43,6 +43,14 @@ filter {
matchName="scala.util.hashing.MurmurHash3.wrappedArrayHash"
problemName=DirectMissingMethodProblem
},
{
matchName="scala.reflect.runtime.SynchronizedOps.newMappedBaseTypeSeq"
problemName=DirectMissingMethodProblem
},
{
matchName="scala.reflect.runtime.JavaUniverse.newMappedBaseTypeSeq"
problemName=DirectMissingMethodProblem
},
{
matchName="scala.collection.immutable.HashMap.contains0"
problemName=DirectMissingMethodProblem
Expand Down
5 changes: 4 additions & 1 deletion src/reflect/scala/reflect/internal/BaseTypeSeqs.scala
Expand Up @@ -33,6 +33,9 @@ trait BaseTypeSeqs {
protected def newBaseTypeSeq(parents: List[Type], elems: Array[Type]) =
new BaseTypeSeq(parents, elems)

protected def newMappedBaseTypeSeq(orig: BaseTypeSeq, f: Type => Type) =
new MappedBaseTypeSeq(orig, f)

/** Note: constructor is protected to force everyone to use the factory method newBaseTypeSeq instead.
* This is necessary because when run from reflection every base type sequence needs to have a
* SynchronizedBaseTypeSeq as mixin.
Expand Down Expand Up @@ -125,7 +128,7 @@ trait BaseTypeSeqs {
newBaseTypeSeq(parents, arr)
}

def lateMap(f: Type => Type): BaseTypeSeq = new MappedBaseTypeSeq(this, f)
def lateMap(f: Type => Type): BaseTypeSeq = newMappedBaseTypeSeq(this, f)

def exists(p: Type => Boolean): Boolean = elems exists p

Expand Down
11 changes: 6 additions & 5 deletions src/reflect/scala/reflect/runtime/SynchronizedOps.scala
Expand Up @@ -18,6 +18,12 @@ private[reflect] trait SynchronizedOps extends internal.SymbolTable
if (elems.exists(_.isInstanceOf[RefinedType])) new BaseTypeSeq(parents, elems) with SynchronizedBaseTypeSeq
else new BaseTypeSeq(parents, elems)

override protected def newMappedBaseTypeSeq(orig: BaseTypeSeq, f: Type => Type) =
// MappedBaseTypeSeq's are used rarely enough that we unconditionally mixin the synchronized
// wrapper, rather than doing this conditionally. A previous attempt to do that broke the "late"
// part of the "lateMap" contract in inspecting the mapped elements.
new MappedBaseTypeSeq(orig, f) with SynchronizedBaseTypeSeq

trait SynchronizedBaseTypeSeq extends BaseTypeSeq {
override def apply(i: Int): Type = gilSynchronized { super.apply(i) }
override def rawElem(i: Int) = gilSynchronized { super.rawElem(i) }
Expand All @@ -28,11 +34,6 @@ private[reflect] trait SynchronizedOps extends internal.SymbolTable
override def exists(p: Type => Boolean): Boolean = gilSynchronized { super.exists(p) }
override lazy val maxDepth = gilSynchronized { maxDepthOfElems }
override def toString = gilSynchronized { super.toString }

override def lateMap(f: Type => Type): BaseTypeSeq =
// only need to synchronize BaseTypeSeqs if they contain refined types
if (map(f).toList.exists(_.isInstanceOf[RefinedType])) new MappedBaseTypeSeq(this, f) with SynchronizedBaseTypeSeq
else new MappedBaseTypeSeq(this, f)
}

// Scopes
Expand Down
1 change: 1 addition & 0 deletions test/files/run/t10026.check
@@ -0,0 +1 @@
List(1, 2, 3)
11 changes: 11 additions & 0 deletions test/files/run/t10026.scala
@@ -0,0 +1,11 @@
import scala.reflect.runtime.universe
import scala.tools.reflect.ToolBox

object Test {
def main(args: Array[String]): Unit = {
val classloader = getClass.getClassLoader
val toolbox = universe.runtimeMirror(classloader).mkToolBox()
println(toolbox.compile(toolbox.parse("Array(1, 2, 3).toList")).apply())
}
}

0 comments on commit 777a0e5

Please sign in to comment.