10000 SI-10026 Fix endless cycle in runtime reflection by retronym · Pull Request #5659 · scala/scala · GitHub
[go: up one dir, main page]

Skip to content

SI-10026 Fix endless cycle in runtime reflection #5659

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 20, 2017

Conversation

retronym
Copy link
Member
@retronym retronym commented Jan 24, 2017

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.

@retronym retronym added the WIP label Jan 24, 2017
@scala-jenkins scala-jenkins added this to the 2.12.2 milestone Jan 24, 2017
@retronym
Copy link
Member Author
retronym commented Jan 25, 2017

/cc @xeno-by for any insights

Note @paulp's analysis in the ticket:

MappedBaseTypeSeq and SynchronizedBaseTypeSeq both extend BaseTypeSeq, both override lateMap, and neither override ever calls super.lateMap so there's no scenario where new MappedBaseTypeSeq(this, f) with SynchronizedBaseTypeSeq will do the right thing.

@jvican
Copy link
Member
jvican commented Jan 26, 2017

Thanks for this fix.

@retronym retronym force-pushed the ticket/10026 branch 2 times, most recently from bbc7b2a to 397f8b2 Compare February 17, 2017 05:46
@retronym retronym requested review from adriaanm and xeno-by February 17, 2017 05:46
@retronym
Copy link
Member Author
retronym commented Feb 17, 2017

Here is the part of the compiler that uses lateMap:

if (tpe.parents exists typeContainsTypeVar) {
// rename type vars to fresh type params, take base type sequence of
// resulting type, and rename back all the entries in that sequence
var tvs = Set[TypeVar]()
for (p <- tpe.parents)
for (t <- p) t match {
case tv: TypeVar => tvs += tv
case _ =>
}
val varToParamMap: Map[Type, Symbol] =
mapFrom[TypeVar, Type, Symbol](tvs.toList)(_.origin.typeSymbol.cloneSymbol)
val paramToVarMap = varToParamMap map (_.swap)
val varToParam = new TypeMap {
def apply(tp: Type) = varToParamMap get tp match {
case Some(sym) => sym.tpe_*
case _ => mapOver(tp)
}
}
val paramToVar = new TypeMap {
def apply(tp: Type) = tp match {
case TypeRef(_, tsym, _) if paramToVarMap.isDefinedAt(tsym) => paramToVarMap(tsym)
case _ => mapOver(tp)
}
}
val bts = copyRefinedType(tpe.asInstanceOf[RefinedType], tpe.parents map varToParam, varToParam mapOver tpe.decls).baseTypeSeq
tpe.baseTypeSeqCache = bts lateMap paramToVar
} else {

Added in: 49bfcbe

To fix: https://issues.scala-lang.org/browse/SI-2311

Running the test case for that commit through 2.12.1 fails with the same cycle described above:

Welcome to Scala 2.12.1 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_112).
scala> tb.typecheck(tb.parse("""import scala.collection.JavaConversions._; val jmap = new java.util.HashMap[String,Int](); jmap("one") = 1; jmap("one")"""))
java.lang.StackOverflowError
  at scala.reflect.runtime.SynchronizedOps$SynchronizedBaseTypeSeq$$anon$2.scala$reflect$runtime$SynchronizedOps$SynchronizedBaseTypeSeq$$$outer(SynchronizedOps.scala:21)
  at scala.reflect.runtime.SynchronizedOps$SynchronizedBaseTypeSeq.map(SynchronizedOps.scala:27)
  at scala.reflect.runtime.SynchronizedOps$SynchronizedBaseTypeSeq.map$(SynchronizedOps.scala:27)

With this patch, it typechecks:

Welcome to Scala 2.12.2-20170217-154635-397f8b2 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_112).
scala> tb.typecheck(tb.parse("""import scala.collection.JavaConversions._; val jmap = new java.util.HashMap[String,Int](); jmap("one") = 1; jmap("one")"""))
res1: tb.u.Tree =
{
  import scala.collection.JavaConversions._;
  val jmap: java.util.HashMap[String,Int] = new java.util.HashMap[String,Int]();
  scala.collection.JavaConversions.deprecated mapAsScalaMap[String, Int](jmap).update("one", 1);
  scala.collection.JavaConversions.deprecated mapAsScalaMap[String, Int](jmap).apply("one")
}

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.
@adriaanm adriaanm merged commit e87a436 into scala:2.12.x Feb 20, 2017
@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Feb 23, 2017
SethTisue added a commit to SethTisue/scala that referenced this pull request Mar 6, 2017
only trivial merge conflict involving a method renaming

*   d1d700e - (origin/HEAD, origin/2.12.x) Merge pull request scala#5754 from Philippus/issue/html-tag-in-hover (2 days ago) <Lukas Rytz>
|\
| * cf64718 - pattern for entitylink was too narrow, cleaned up the tests (3 days ago) <Philippus Baalman>
* |   f771395 - Merge pull request scala#5671 from retronym/topic/stubby-2 (3 days ago) <Lukas Rytz>
|\ \
| * | ad13063 - Remove non-essential fix for stub symbol failure (4 days ago) <Jason Zaugg>
| * | 7f2d1fa - Avoid forcing info transforms of primitive methods (2 weeks ago) <Jason Zaugg>
| * | 37a0eb7 - Avoid stub symbol related crash in backend (2 weeks ago) <Jason Zaugg>
* | |   96a7617 - Merge pull request scala#5622 from edmundnoble/extra-errs (4 days ago) <Adriaan Moors>
|\ \ \
| * | | 466e52b - Match error lengths (4 weeks ago) <Edmund Noble>
| * | | d1fc983 - Improved error messages for identically named, differently prefixed types (9 weeks ago) <Edmund Noble>
|  / /
* | |   f2e05c2 - Merge pull request scala#5728 from Philippus/issue/html-tag-in-hover (4 days ago) <Lukas Rytz>
|\ \ \
| | |/
| |/|
| * | e3c5df8 - added missing Inline matches to inlineToStr so it is now exhaustive scala.xml.XML.loadString(tag).text will remove all html tags inside the HtmlTag (9 days ago) <Philippus Baalman>
* | |   920bc4e - Merge pull request scala#5743 from som-snytt/issue/10207-bad-update (7 days ago) <Lukas Rytz>
|\ \ \
| * | | 094f7f9 - SI-10207 Error before update conversion (8 days ago) <Som Snytt>
* | | |   1b4d36f - Merge pull request scala#5746 from paulp/pr/partest (7 days ago) <Lukas Rytz>
|\ \ \ \
| |/ / /
|/| | |
| * | | eac00e1 - Add partest paths to the list of watched sources. (8 days ago) <Paul Phillips>
|/ / /
* | |   5f1a638 - Merge pull request scala#5732 from retronym/topic/build-info-malarkey (10 days ago) <Adriaan Moors>
|\ \ \
| * | | 5e9acac - More predictable performance of SBT build startup, reload (11 days ago) <Jason Zaugg>
|  / /
* | |   759a7b7 - Merge pull request scala#5735 from SethTisue/sd-313 (10 days ago) <Adriaan Moors>
|\ \ \
| * | | ed4ddf5 - increase timeouts on some sys.process tests (11 days ago) <Seth Tisue>
* | | |   e2aaf2c - Merge pull request scala#5723 from dragos/issue/regression-assert-ide (11 days ago) <Lukas Rytz>
|\ \ \ \
| |/ / /
|/| | |
| * | | e5c957e - Fix regression in 5751763 (12 days ago) <Iulian Dragos>
* | | |   f174bfb - Merge pull request scala#5731 from janekdb/issue/scalaGH-644/fix-spec-latex-rendering (11 days ago) <Seth Tisue>
|\ \ \ \
| * | | | ba4c6d4 - scalaGH-644: Remove static html styling of spec code blocks (11 days ago) <Janek Bogucki>
|/ / / /
* | | |   8e40bef - Merge pull request scala#5729 from scala/revert-5658-topic/hashhash (12 days ago) <Adriaan Moors>
|\ \ \ \
| |_|/ /
|/| | |
| * | | 86cd70f - (origin/revert-5658-topic/hashhash) Revert "Fix erasure of the qualifier of ##" (12 days ago) <Adriaan Moors>
|/ / /
* | |   cbf7daa - Merge pull request scala#5681 from Philippus/issue/9704 (13 days ago) <Lukas Rytz>
|\ \ \
| * | | b8a8ac1 - moved Pattern and TagsNotToClose to a HtmlTag companion object (13 days ago) <Philippus Baalman>
| * | | a019082 - SI-9704 don't add a closed HtmlTag if it is already closed (4 weeks ago) <Philippus Baalman>
|  / /
* | |   effde0c - Merge pull request scala#5726 from scala/revert-5629-issue/10120-quote-err (13 days ago) <Adriaan Moors>
|\ \ \
| * | | d60f6e3 - (origin/revert-5629-issue/10120-quote-err) Revert "SI-10133 Require escaped single quote char lit" (13 days ago) <Adriaan Moors>
|/ / /
* | |   a8c4a54 - Merge pull request scala#5663 from gourlaysama/ticket/sd-256-enable-repl-colors-unix-2 (13 days ago) <Adriaan Moors>
|\ \ \
| * | | 6411170 - SD-256 enable colored output by default on unix (13 days ago) <Antoine Gourlay>
* | | |   c96a977 - Merge pull request scala#5658 from retronym/topic/hashhash (13 days ago) <Lukas Rytz>
|\ \ \ \
| |/ / /
|/| | |
| * | | f85c62e - Fix erasure of the qualifier of ## (6 weeks ago) <Jason Zaugg>
|  / /
* | |   76bfb9e - Merge pull request scala#5708 from szeiger/issue/si10194 (13 days ago) <Lukas Rytz>
|\ \ \
| * | | 1d22ee4 - SI-10194: Fix abstract type resolution for overloaded HOFs (13 days ago) <Stefan Zeiger>
|  / /
* | |   dabec1a - Merge pull request scala#5700 from retronym/ticket/10154-refactor (13 days ago) <Lukas Rytz>
|\ \ \
| * | | 06eee79 - Refactor implementation of lookupCompanion (2 weeks ago) <Jason Zaugg>
|  / /
* | |   2f1e0c2 - Merge pull request scala#5704 from som-snytt/issue/10190-elide-string (13 days ago) <Lukas Rytz>
|\ \ \
| * | | 6fb3825 - SI-10190 Elide string to empty instead of null (3 weeks ago) <Som Snytt>
|  / /
* | |   13f7b2a - Merge pull request scala#5640 from optimizely/repl-import-handler (2 weeks ago) <Adriaan Moors>
|\ \ \
| * | | aa7e335 - Fix ImportHandler's reporting of importedNames and importedSymbols (8 weeks ago) <Hao Xia>
| * | | c89d821 - Fix SIOOBE in Name#pos for substrings of length 1 (8 weeks ago) <Jason Zaugg>
|  / /
* | |   023a96a - Merge pull request scala#5629 from som-snytt/issue/10120-quote-err (2 weeks ago) <Adriaan Moors>
|\ \ \
| * | | 05cc3e2 - SI-10120 ReplReporter handles message indent (7 weeks ago) <Som Snytt>
| * | | 939abf1 - SI-10120 Extra advice on unclosed char literal (8 weeks ago) <Som Snytt>
| * | | 855492c - SI-10133 Require escaped single quote char lit (8 weeks ago) <Som Snytt>
|  / /
* | |   e21ab42 - Merge pull request scala#5660 from som-snytt/issue/9464-spec (2 weeks ago) <Adriaan Moors>
|\ \ \
| * | | a6dcceb - SI-9464 Clarify spec on no final trait (6 weeks ago) <Som Snytt>
|  / /
* | |   e87a436 - Merge pull request scala#5659 from retronym/ticket/10026 (2 weeks ago) <Adriaan Moors>
|\ \ \
| |/ /
|/| |
| * | 777a0e5 - SI-10026 Fix endless cycle in runtime reflection (2 weeks ago) <Jason Zaugg>
| |/
* |   23e5ed9 - Merge pull request scala#5707 from retronym/topic/java9-prepare (2 weeks ago) <Lukas Rytz>
|\ \
| * | 96e8e97 - Workaround bug in Scala runtime reflection on JDK 9 (3 weeks ago) <Jason Zaugg>
| * | fe2d9a4 - Avoid ambiguous overload on JDK 9 (3 weeks ago) <Jason Zaugg>
| * | 6bba8f7 - Adapt to change in ClassLoader in JDK 9 (3 weeks ago) <Jason Zaugg>
| * | 8136057 - Bump scala-asm version (3 weeks ago) <Jason Zaugg>
|  /
* |   cad3c3d - Merge pull request scala#5709 from adriaanm/sam_wild_bound (2 weeks ago) <Lukas Rytz>
|\ \
| * | c396e96 - Ignore BoundedWildcardType in erasure type map (2 weeks ago) <Adriaan Moors>
* | |   3e9df41 - Merge pull request scala#5711 from retronym/ticket/jrt (2 weeks ago) <Lukas Rytz>
|\ \ \
| * | | 09c7edc - Faster and simpler Java 9 classpath implementation (2 weeks ago) <Jason Zaugg>
|  / /
* | |   7b9d3cc - Merge pull request scala#5713 from janekdb/issue/scalaGH-644/sync-jekyll-README-to-Gemfile (2 weeks ago) <Lukas Rytz>
|\ \ \
| * | | 5e5ec9a - scalaGH-644: Expand note regarding Jekyll versions (2 weeks ago) <Janek Bogucki>
|  / /
* | |   144f7e0 - Merge pull request scala#5714 from dragos/issue/usage-sterr-SI-10178 (2 weeks ago) <Lukas Rytz>
|\ \ \
| |/ /
|/| |
| * | 640c85e - SI-10178 Route reporter.echo to stdout (2 weeks ago) <Iulian Dragos>
|  /
* |   2fec08b - Merge pull request scala#5717 from som-snytt/issue/10148-followup (2 weeks ago) <Adriaan Moors>
|\
8340
 \
| |/
|/|
| * f3d271b - SI-10148 Accept verbose zero (2 weeks ago) <Som Snytt>
* 147e5dd - Merge pull request scala#5716 from adriaanm/i296 (2 weeks ago) <Jason Zaugg>
* 12437a0 - Ensure ordering for args to `choose` in DurationTest (2 weeks ago) <Adriaan Moors>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes worth highlighting in next release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
0