From d86372f4ca72d64dad750b2d22b40a2c08e82218 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Mon, 4 May 2015 21:10:07 +0200 Subject: [PATCH 1/5] Make computeDenot take overrides into account If previous denotation was a sym denotation, it might be overridden by a different symbol in a new phase or run. So it is not correct to simply return the current version of the symbol, as was done before. We now recompute the member if there is a chance that the symbol could be overridden. --- src/dotty/tools/dotc/core/Types.scala | 48 +++++++++++++++++---------- 1 file changed, 30 insertions(+), 18 deletions(-) diff --git a/src/dotty/tools/dotc/core/Types.scala b/src/dotty/tools/dotc/core/Types.scala index 595732b377ff..60255bd72f97 100644 --- a/src/dotty/tools/dotc/core/Types.scala +++ b/src/dotty/tools/dotc/core/Types.scala @@ -126,6 +126,17 @@ object Types { false } + /** Same as `isRef`, but follows more types: all type proxies as well as and- and or-types */ + private[Types] def isWeakRef(sym: Symbol)(implicit ctx: Context): Boolean = stripTypeVar match { + case tp: NamedType => tp.info.isWeakRef(sym) + case tp: ClassInfo => tp.cls eq sym + case tp: Types.ThisType => tp.cls eq sym + case tp: TypeProxy => tp.underlying.isWeakRef(sym) + case tp: AndType => tp.tp1.isWeakRef(sym) && tp.tp2.isWeakRef(sym) + case tp: OrType => tp.tp1.isWeakRef(sym) || tp.tp2.isWeakRef(sym) + case _ => false + } + /** Is this type an instance of a non-bottom subclass of the given class `cls`? */ final def derivesFrom(cls: Symbol)(implicit ctx: Context): Boolean = this match { case tp: TypeRef => @@ -1216,27 +1227,17 @@ object Types { val sym = lastSymbol if (sym == null) loadDenot else denotOfSym(sym) case d: SymDenotation => - if ( d.validFor.runId == ctx.runId - || ctx.stillValid(d) - || this.isInstanceOf[WithFixedSym]) d.current + if (this.isInstanceOf[WithFixedSym]) d.current + else if (d.validFor.runId == ctx.runId || ctx.stillValid(d)) + if (prefix.isWeakRef(d.owner) || d.isConstructor) d.current + else recomputeMember(d) // symbol could have been overridden, recompute membership else { val newd = loadDenot if (newd.exists) newd else d.staleSymbolError } case d => - if (d.validFor.runId != ctx.period.runId) - loadDenot - // The following branch was used to avoid an assertErased error. - // It's idea was to void keeping non-sym denotations after erasure - // since they violate the assertErased contract. But the problem is - // that when seen again in an earlier phase the denotation is - // still seen as a SymDenotation, whereas it should be a SingleDenotation. - // That's why the branch is disabled. - // - // else if (ctx.erasedTypes && lastSymbol != null) - // denotOfSym(lastSymbol) - else - d.current + if (d.validFor.runId != ctx.period.runId) loadDenot + else d.current } if (ctx.typerState.ephemeral) record("ephemeral cache miss: loadDenot") else if (d.exists) { @@ -1245,7 +1246,10 @@ object Types { // phase but a defined denotation earlier (e.g. a TypeRef to an abstract type // is undefined after erasure.) We need to be able to do time travel back and // forth also in these cases. - setDenot(d) + + // Don't use setDenot here; double binding checks can give spurious failures after erasure + lastDenotation = d + lastSymbol = d.symbol checkedPeriod = ctx.period } d @@ -1253,6 +1257,14 @@ object Types { finally ctx.typerState.ephemeral |= savedEphemeral } + /** A member of `prefix` (disambiguated by `d.signature`) or, if none was found, `d.current`. */ + private def recomputeMember(d: SymDenotation)(implicit ctx: Context): Denotation = + asMemberOf(prefix) match { + case NoDenotation => d.current + case newd: SingleDenotation => newd + case newd => newd.atSignature(d.signature).orElse(d.current) + } + private def denotOfSym(sym: Symbol)(implicit ctx: Context): Denotation = { val d = sym.denot val owner = d.owner @@ -1275,7 +1287,7 @@ object Types { // for overriddenBySynthetic symbols a TermRef such as SomeCaseClass.this.hashCode // might be rewritten from Object#hashCode to the hashCode generated at SyntheticMethods ), - s"data race? overwriting symbol of ${this.show} / $this / ${this.getClass} / ${lastSymbol.id} / ${sym.id}") + s"data race? overwriting symbol of ${this.show} / $this / ${this.getClass} / ${lastSymbol.id} / ${sym.id} / ${sym.owner} / ${lastSymbol.owner} / ${ctx.phase}") protected def sig: Signature = Signature.NotAMethod From 0c637364a92536a968e4f3c88fce805b6eb95aec Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Mon, 4 May 2015 21:13:31 +0200 Subject: [PATCH 2/5] Fix infinite recursion when creating extension methods Phase ExtensionMethods creates new symbols for extension methods and then installs these symbols into the companion object of a value class. It's important that the creation of these symbols is done in the phase ExtensionMethods itself, and not in the next phase, as was done before. If we do it in the next phase, we need the owner at the next phase and with the new scheme of computeDenot that owner might be forced, leading to an infinite cycle. --- src/dotty/tools/dotc/transform/ExtensionMethods.scala | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/dotty/tools/dotc/transform/ExtensionMethods.scala b/src/dotty/tools/dotc/transform/ExtensionMethods.scala index b2f402bc5c05..009257dc06ee 100644 --- a/src/dotty/tools/dotc/transform/ExtensionMethods.scala +++ b/src/dotty/tools/dotc/transform/ExtensionMethods.scala @@ -54,7 +54,7 @@ class ExtensionMethods extends MiniPhaseTransform with DenotTransformer with Ful ctx.atPhase(thisTransformer.next) { implicit ctx => // In Scala 2, extension methods are added before pickling so we should // not generate them again. - if (!(origClass is Scala2x)) { + if (!(origClass is Scala2x)) ctx.atPhase(thisTransformer) { implicit ctx => for (decl <- origClass.classInfo.decls) { if (isMethodWithExtension(decl)) decls1.enter(createExtensionMethod(decl, ref.symbol)) @@ -91,7 +91,6 @@ class ExtensionMethods extends MiniPhaseTransform with DenotTransformer with Ful else NoSymbol private def createExtensionMethod(imeth: Symbol, staticClass: Symbol)(implicit ctx: Context): TermSymbol = { - assert(ctx.phase == thisTransformer.next) val extensionName = extensionNames(imeth).head.toTermName val extensionMeth = ctx.newSymbol(staticClass, extensionName, imeth.flags | Final &~ (Override | Protected | AbsOverride), From cfa2ba030becec1f0e12994f120b65ca5e99badb Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Mon, 4 May 2015 21:15:46 +0200 Subject: [PATCH 3/5] Add test case Tests #518. --- tests/pos/i518.scala | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 tests/pos/i518.scala diff --git a/tests/pos/i518.scala b/tests/pos/i518.scala new file mode 100644 index 000000000000..390437dae137 --- /dev/null +++ b/tests/pos/i518.scala @@ -0,0 +1,6 @@ +class Meter(val underlying: Int) extends AnyVal + +class Test { + val x: Int = new Meter(3).hashCode() + // After phase VCInline the rhs should be expanded to Meter.hashCode$extension(3) +} From 6780dbd0a66fdaec6a7be2faac78620e8922cbd4 Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Mon, 4 May 2015 21:20:38 +0200 Subject: [PATCH 4/5] Make data race detection more liberal. Allow to rebind a NmedType to refer to a symbol in a subclass of where the previous symbol was defined. This generalizes the previous rule that we allow to rebind from a root method to its synthetic implementation. The change is not necessary to make the new scheme of computeDenot pass the tests, but it seems useful to avoid spurious errors elsehere. --- src/dotty/tools/dotc/core/Definitions.scala | 1 - src/dotty/tools/dotc/core/Types.scala | 4 +--- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/dotty/tools/dotc/core/Definitions.scala b/src/dotty/tools/dotc/core/Definitions.scala index 784773fd3fb1..c21620048ca3 100644 --- a/src/dotty/tools/dotc/core/Definitions.scala +++ b/src/dotty/tools/dotc/core/Definitions.scala @@ -451,7 +451,6 @@ class Definitions { lazy val RootImports = List[Symbol](JavaLangPackageVal, ScalaPackageVal, ScalaPredefModule, DottyPredefModule) - lazy val overriddenBySynthetic = Set[Symbol](Any_equals, Any_hashCode, Any_toString, Product_canEqual) def isTupleType(tp: Type)(implicit ctx: Context) = { val arity = tp.dealias.argInfos.length arity <= MaxTupleArity && (tp isRef TupleClass(arity)) diff --git a/src/dotty/tools/dotc/core/Types.scala b/src/dotty/tools/dotc/core/Types.scala index 60255bd72f97..97e3f0390663 100644 --- a/src/dotty/tools/dotc/core/Types.scala +++ b/src/dotty/tools/dotc/core/Types.scala @@ -1283,9 +1283,7 @@ object Types { (lastDefRunId == NoRunId) } || (lastSymbol.infoOrCompleter == ErrorType || - defn.overriddenBySynthetic.contains(lastSymbol) - // for overriddenBySynthetic symbols a TermRef such as SomeCaseClass.this.hashCode - // might be rewritten from Object#hashCode to the hashCode generated at SyntheticMethods + sym.owner.derivesFrom(lastSymbol.owner) && sym.owner != lastSymbol.owner ), s"data race? overwriting symbol of ${this.show} / $this / ${this.getClass} / ${lastSymbol.id} / ${sym.id} / ${sym.owner} / ${lastSymbol.owner} / ${ctx.phase}") From 3edf285a48f4208b209571580f8aa418ca8a794f Mon Sep 17 00:00:00 2001 From: Martin Odersky Date: Tue, 5 May 2015 10:49:56 +0200 Subject: [PATCH 5/5] Rename isWeakRef -> isTightPrefix isWeakRef was confusing because this has nothing to do with weak pointers. --- src/dotty/tools/dotc/core/Types.scala | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/dotty/tools/dotc/core/Types.scala b/src/dotty/tools/dotc/core/Types.scala index 97e3f0390663..f7abbf14ce07 100644 --- a/src/dotty/tools/dotc/core/Types.scala +++ b/src/dotty/tools/dotc/core/Types.scala @@ -126,14 +126,16 @@ object Types { false } - /** Same as `isRef`, but follows more types: all type proxies as well as and- and or-types */ - private[Types] def isWeakRef(sym: Symbol)(implicit ctx: Context): Boolean = stripTypeVar match { - case tp: NamedType => tp.info.isWeakRef(sym) + /** Does this type refer exactly to class symbol `sym`, instead of to a subclass of `sym`? + * Implemented like `isRef`, but follows more types: all type proxies as well as and- and or-types + */ + private[Types] def isTightPrefix(sym: Symbol)(implicit ctx: Context): Boolean = stripTypeVar match { + case tp: NamedType => tp.info.isTightPrefix(sym) case tp: ClassInfo => tp.cls eq sym case tp: Types.ThisType => tp.cls eq sym - case tp: TypeProxy => tp.underlying.isWeakRef(sym) - case tp: AndType => tp.tp1.isWeakRef(sym) && tp.tp2.isWeakRef(sym) - case tp: OrType => tp.tp1.isWeakRef(sym) || tp.tp2.isWeakRef(sym) + case tp: TypeProxy => tp.underlying.isTightPrefix(sym) + case tp: AndType => tp.tp1.isTightPrefix(sym) && tp.tp2.isTightPrefix(sym) + case tp: OrType => tp.tp1.isTightPrefix(sym) || tp.tp2.isTightPrefix(sym) case _ => false } @@ -1229,7 +1231,7 @@ object Types { case d: SymDenotation => if (this.isInstanceOf[WithFixedSym]) d.current else if (d.validFor.runId == ctx.runId || ctx.stillValid(d)) - if (prefix.isWeakRef(d.owner) || d.isConstructor) d.current + if (prefix.isTightPrefix(d.owner) || d.isConstructor) d.current else recomputeMember(d) // symbol could have been overridden, recompute membership else { val newd = loadDenot