Skip to content

Alternative fix for #10044 #15129

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 4 commits into from
May 18, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions compiler/src/dotty/tools/dotc/Compiler.scala
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,8 @@ class Compiler {
* all refs to it would become outdated - they could not be dereferenced in the
* new phase.
*
* After erasure, signature changing denot-transformers are OK because erasure
* will make sure that only term refs with fixed SymDenotations survive beyond it. This
* is possible because:
*
* - splitter has run, so every ident or select refers to a unique symbol
* - after erasure, asSeenFrom is the identity, so every reference has a
* plain SymDenotation, as opposed to a UniqueRefDenotation.
* After erasure, signature changing denot-transformers are OK because signatures
* are never recomputed later than erasure.
*/
def phases: List[List[Phase]] =
frontendPhases ::: picklerPhases ::: transformPhases ::: backendPhases
Expand Down
35 changes: 26 additions & 9 deletions compiler/src/dotty/tools/dotc/core/Types.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1079,17 +1079,13 @@ object Types {
* @param checkClassInfo if true we check that ClassInfos are within bounds of abstract types
*/
final def overrides(that: Type, relaxedCheck: Boolean, matchLoosely: => Boolean, checkClassInfo: Boolean = true)(using Context): Boolean = {
def widenNullary(tp: Type) = tp match {
case tp @ MethodType(Nil) => tp.resultType
case _ => tp
}
val overrideCtx = if relaxedCheck then ctx.relaxedOverrideContext else ctx
inContext(overrideCtx) {
!checkClassInfo && this.isInstanceOf[ClassInfo]
|| (this.widenExpr frozen_<:< that.widenExpr)
|| matchLoosely && {
val this1 = widenNullary(this)
val that1 = widenNullary(that)
val this1 = this.widenNullaryMethod
val that1 = that.widenNullaryMethod
((this1 `ne` this) || (that1 `ne` that)) && this1.overrides(that1, relaxedCheck, false, checkClassInfo)
}
}
Expand Down Expand Up @@ -1326,6 +1322,11 @@ object Types {
this
}

/** If this is a nullary method type, its result type */
def widenNullaryMethod(using Context): Type = this match
case tp @ MethodType(Nil) => tp.resType
case _ => this

/** The singleton types that must or may be in this type. @see Atoms.
* Overridden and cached in OrType.
*/
Expand Down Expand Up @@ -2561,7 +2562,7 @@ object Types {
* A test case is neg/opaque-self-encoding.scala.
*/
final def withDenot(denot: Denotation)(using Context): ThisType =
if (denot.exists) {
if denot.exists then
val adapted = withSym(denot.symbol)
val result =
if (adapted.eq(this)
Expand All @@ -2570,9 +2571,25 @@ object Types {
|| adapted.info.eq(denot.info))
adapted
else this
result.setDenot(denot)
val lastDenot = result.lastDenotation
denot match
case denot: SymDenotation
if denot.validFor.firstPhaseId < ctx.phase.id
&& lastDenot != null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not completely sure that we won't run into issues where lastDenot didn't exist, but we need to get the denotation at an earlier phase in a subsequent run or via time-travelling. How expensive would it be to always create UniqueRefDenotation after erasure?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer caution here. I don't have the time to look into a change like this, which looks risky to me.

&& lastDenot.validFor.lastPhaseId > denot.validFor.firstPhaseId
&& !lastDenot.isInstanceOf[SymDenotation] =>
// In this case the new SymDenotation might be valid for all phases, which means
// we would not recompute the denotation when travelling to an earlier phase, maybe
// in the next run. We fix that problem by creating a UniqueRefDenotation instead.
core.println(i"overwrite ${result.toString} / ${result.lastDenotation}, ${result.lastDenotation.getClass} with $denot at ${ctx.phaseId}")
result.setDenot(
UniqueRefDenotation(
denot.symbol, denot.info,
Period(ctx.runId, ctx.phaseId, denot.validFor.lastPhaseId),
this.prefix))
case _ =>
result.setDenot(denot)
result.asInstanceOf[ThisType]
}
else // don't assign NoDenotation, we might need to recover later. Test case is pos/avoid.scala.
this

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,9 @@ object GenericSignatures {
fullNameInSig(tp.typeSymbol)
builder.append(';')
case _ =>
boxedSig(tp)
boxedSig(tp.widenDealias.widenNullaryMethod)
// `tp` might be a singleton type referring to a getter.
// Hence the widenNullaryMethod.
}

if (pre.exists) {
Expand Down
12 changes: 12 additions & 0 deletions compiler/test-resources/repl/i10044
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
scala> object Foo { opaque type Bar = Int; object Bar { extension (b: Bar) def flip: Bar = -b; def apply(x: Int): Bar = x }}
// defined object Foo
scala> val a = Foo.Bar(42)
val a: Foo.Bar = 42
scala> val b = a.flip
val b: Foo.Bar = -42
scala> val c = b.flip
val c: Foo.Bar = 42
scala> val d = c.flip
val d: Foo.Bar = -42
scala> val e = d.flip
val e: Foo.Bar = 42