From 4fc85641fd270aca6639b898ef453c664dfbe65c Mon Sep 17 00:00:00 2001 From: Dale Wijnand Date: Wed, 24 Jul 2024 12:01:02 +0100 Subject: [PATCH 1/2] Capture the kse3 issue in test cases The underlying type of an opaque type is only visible to anything within the scope that contains that opaque type. So, for instance, a Worm opaque type in a Worms object, anything within the Worms object sees the underlying type. So Worms.Worm is an opaque abstract type with bounds, while Worms.this.Worm is an opaque type with an underlying type. But you can also reference Worms.Worm while being inside of the Worms object. The change I made to opaque types allowed for member selection to see the underlying type when in a scope that can see that, which makes it consistent with how TypeComparer allows those two types to be seen as equivalent, when in the right scope. In kse3, it seems, the fact that this wasn't done was utilised by using an "external" reference to the opaque type, which avoided the underlying type's method being selected, and the extension method being selected instead. While my change broke kse3, I believe the change is good, as it brings consistency. And it is possible to fix the kse3 code, by using the "universal function call syntax" (to borrow from Rust nomenclature) for calling the extension method. Alternatively, the extension methods can be defined where they really don't see the underlying type, and then the companion object can be made to include the extension methods (to keep them in implicit scope). --- tests/neg/i21239.check | 7 +++++++ tests/neg/i21239.orig.check | 7 +++++++ tests/neg/i21239.orig.scala | 33 +++++++++++++++++++++++++++++++++ tests/neg/i21239.scala | 14 ++++++++++++++ tests/pos/i21239.alt.scala | 28 ++++++++++++++++++++++++++++ tests/pos/i21239.orig.scala | 34 ++++++++++++++++++++++++++++++++++ tests/pos/i21239.scala | 18 ++++++++++++++++++ 7 files changed, 141 insertions(+) create mode 100644 tests/neg/i21239.check create mode 100644 tests/neg/i21239.orig.check create mode 100644 tests/neg/i21239.orig.scala create mode 100644 tests/neg/i21239.scala create mode 100644 tests/pos/i21239.alt.scala create mode 100644 tests/pos/i21239.orig.scala create mode 100644 tests/pos/i21239.scala diff --git a/tests/neg/i21239.check b/tests/neg/i21239.check new file mode 100644 index 000000000000..5b6f2f8bcef5 --- /dev/null +++ b/tests/neg/i21239.check @@ -0,0 +1,7 @@ +-- [E007] Type Mismatch Error: tests/neg/i21239.scala:14:18 ------------------------------------------------------------ +14 | def get2: V = get // error + | ^^^ + | Found: AnyRef + | Required: V + | + | longer explanation available when compiling with `-explain` diff --git a/tests/neg/i21239.orig.check b/tests/neg/i21239.orig.check new file mode 100644 index 000000000000..26895bd50ed3 --- /dev/null +++ b/tests/neg/i21239.orig.check @@ -0,0 +1,7 @@ +-- [E007] Type Mismatch Error: tests/neg/i21239.orig.scala:32:8 -------------------------------------------------------- +32 | get // error + | ^^^ + | Found: AnyRef + | Required: V + | + | longer explanation available when compiling with `-explain` diff --git a/tests/neg/i21239.orig.scala b/tests/neg/i21239.orig.scala new file mode 100644 index 000000000000..3fb39d93446b --- /dev/null +++ b/tests/neg/i21239.orig.scala @@ -0,0 +1,33 @@ +// 1 +// A re-minimisated reproduction of the original issue in kse3 +// The one in the issue removes the usage of the package +// in the second extension bundle, which is crucial to +// why my change broke this code +package kse.flow + +import java.util.concurrent.atomic.AtomicReference + +opaque type Worm[V] = AtomicReference[AnyRef] +object Worm: + val notSetSentinel: AnyRef = new AnyRef {} + + extension [V](worm: Worm[V]) + inline def wormAsAtomic: AtomicReference[AnyRef] = worm + + extension [V](worm: kse.flow.Worm[V]) + + inline def setIfEmpty(v: => V): Boolean = + var old = worm.wormAsAtomic.get() + if old eq Worm.notSetSentinel then + worm.wormAsAtomic.compareAndSet(old, v.asInstanceOf[AnyRef]) + else false + + inline def get: V = worm.wormAsAtomic.get() match + case x if x eq Worm.notSetSentinel => throw new java.lang.IllegalStateException("Retrieved value before being set") + case x => x.asInstanceOf[V] + + inline def getOrSet(v: => V): V = worm.wormAsAtomic.get() match + case x if x eq Worm.notSetSentinel => + setIfEmpty(v) + get // error + case x => x.asInstanceOf[V] diff --git a/tests/neg/i21239.scala b/tests/neg/i21239.scala new file mode 100644 index 000000000000..4eb4d5808857 --- /dev/null +++ b/tests/neg/i21239.scala @@ -0,0 +1,14 @@ +// 2 +// A more minimised reproduction +package lib + +import java.util.concurrent.atomic.AtomicReference + +opaque type Worm[V] = AtomicReference[AnyRef] +object Worm: + extension [V](worm: Worm[V]) + inline def wormAsAtomic: AtomicReference[AnyRef] = worm + + extension [V](worm: lib.Worm[V]) + def get: V = worm.wormAsAtomic.get().asInstanceOf[V] + def get2: V = get // error diff --git a/tests/pos/i21239.alt.scala b/tests/pos/i21239.alt.scala new file mode 100644 index 000000000000..13a1647115f7 --- /dev/null +++ b/tests/pos/i21239.alt.scala @@ -0,0 +1,28 @@ +// 4 +// An alternative way to fix it, +// defining the extension method externally, +// in a scope that doesn't see through +// the opaque type definition. +// The setup here also makes sure those extension +// are on the opaque type's companion object +// (via class extension), meaning that they continue +// to be in implicit scope (as enforced by the usage test) +import java.util.concurrent.atomic.AtomicReference + +package lib: + object Worms: + opaque type Worm[V] = AtomicReference[AnyRef] + object Worm extends WormOps: + extension [V](worm: Worm[V]) + inline def wormAsAtomic: AtomicReference[AnyRef] = worm + + import Worms.Worm + trait WormOps: + extension [V](worm: Worm[V]) + def get: V = worm.wormAsAtomic.get().asInstanceOf[V] + def get2: V = get + +package test: + import lib.Worms.Worm + object Test: + def usage(worm: Worm[String]): String = worm.get2 diff --git a/tests/pos/i21239.orig.scala b/tests/pos/i21239.orig.scala new file mode 100644 index 000000000000..56666bab4b4d --- /dev/null +++ b/tests/pos/i21239.orig.scala @@ -0,0 +1,34 @@ +// 5 +// Finally, an alternative way to fix the original issue, +// by reimplementing `getOrSet` to not even need +// our `get` extension. +import java.util.concurrent.atomic.AtomicReference + +opaque type Worm[V] = AtomicReference[AnyRef] +object Worm: + val notSetSentinel: AnyRef = new AnyRef {} + + extension [V](worm: Worm[V]) + inline def wormAsAtomic: AtomicReference[AnyRef] = worm // deprecate? + + inline def setIfEmpty(v: => V): Boolean = + val x = worm.get() + if x eq notSetSentinel then + val value = v + worm.set(value.asInstanceOf[AnyRef]) + true + else false + + inline def get: V = + val x = worm.get() + if x eq notSetSentinel then + throw IllegalStateException("Retrieved value before being set") + else x.asInstanceOf[V] + + inline def getOrSet(v: => V): V = + val x = worm.get() + if x eq notSetSentinel then + val value = v + worm.set(value.asInstanceOf[AnyRef]) + value + else x.asInstanceOf[V] diff --git a/tests/pos/i21239.scala b/tests/pos/i21239.scala new file mode 100644 index 000000000000..950f90c233d8 --- /dev/null +++ b/tests/pos/i21239.scala @@ -0,0 +1,18 @@ +// 3 +// One way to fix the issue, using the +// "universal function call syntax" +// (to borrow from what Rust calls the syntax to +// disambiguate which trait's method is intended.) +import java.util.concurrent.atomic.AtomicReference + +package lib: + opaque type Worm[V] = AtomicReference[AnyRef] + object Worm: + extension [V](worm: Worm[V]) + def get: V = worm.get().asInstanceOf[V] + def get2: V = Worm.get(worm) + +package test: + import lib.Worm + object Test: + def usage(worm: Worm[String]): String = worm.get2 From 6c86910a155a82c9621a10883e9d75d8a2ae3890 Mon Sep 17 00:00:00 2001 From: Dale Wijnand Date: Wed, 7 Aug 2024 11:10:53 +0100 Subject: [PATCH 2/2] Add a -3.6-migration warning for opaque select changes --- compiler/src/dotty/tools/dotc/typer/Typer.scala | 17 ++++++++++++++++- tests/warn/i21239.Frac.check | 8 ++++++++ tests/warn/i21239.Frac.scala | 15 +++++++++++++++ 3 files changed, 39 insertions(+), 1 deletion(-) create mode 100644 tests/warn/i21239.Frac.check create mode 100644 tests/warn/i21239.Frac.scala diff --git a/compiler/src/dotty/tools/dotc/typer/Typer.scala b/compiler/src/dotty/tools/dotc/typer/Typer.scala index c518de7dbbfe..0102aea221fc 100644 --- a/compiler/src/dotty/tools/dotc/typer/Typer.scala +++ b/compiler/src/dotty/tools/dotc/typer/Typer.scala @@ -767,7 +767,22 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer val qual1 = qual.cast(liftedTp) val tree1 = cpy.Select(tree0)(qual1, selName) val rawType1 = selectionType(tree1, qual1) - tryType(tree1, qual1, rawType1) + val adapted = tryType(tree1, qual1, rawType1) + if !adapted.isEmpty && sourceVersion == `3.6-migration` then + val adaptedOld = tryExt(tree, qual) + if !adaptedOld.isEmpty then + val symOld = adaptedOld.symbol + val underlying = liftedTp match + case tp: TypeProxy => i" ${tp.translucentSuperType}" + case _ => "" + report.migrationWarning( + em"""Previously this selected the extension ${symOld}${symOld.showExtendedLocation} + |Now it selects $selName on the opaque type's underlying type$underlying + | + |You can change this back by selecting $adaptedOld + |Or by defining the extension method outside of the opaque type's scope. + |""", tree0) + adapted else EmptyTree // Otherwise, try to expand a named tuple selection diff --git a/tests/warn/i21239.Frac.check b/tests/warn/i21239.Frac.check new file mode 100644 index 000000000000..3c2868479f42 --- /dev/null +++ b/tests/warn/i21239.Frac.check @@ -0,0 +1,8 @@ +-- Migration Warning: tests/warn/i21239.Frac.scala:14:8 ---------------------------------------------------------------- +14 | f + Frac.wrap(((-g.numerator).toLong << 32) | (g.unwrap & 0xFFFFFFFFL)) // warn + | ^^^ + | Previously this selected the extension method + in object Frac + | Now it selects + on the opaque type's underlying type Long + | + | You can change this back by selecting kse.maths.Frac.+(f) + | Or by defining the extension method outside of the opaque type's scope. diff --git a/tests/warn/i21239.Frac.scala b/tests/warn/i21239.Frac.scala new file mode 100644 index 000000000000..b09dbfd6ecad --- /dev/null +++ b/tests/warn/i21239.Frac.scala @@ -0,0 +1,15 @@ +package kse.maths + +import scala.language.`3.6-migration` + +opaque type Frac = Long +object Frac { + inline def wrap(f: Long): kse.maths.Frac = f + extension (f: Frac) + inline def unwrap: Long = f + inline def numerator: Int = ((f: Long) >>> 32).toInt + extension (f: kse.maths.Frac) + def +(g: Frac): kse.maths.Frac = f // eliding domain-specific addition logic + def -(g: Frac): kse.maths.Frac = + f + Frac.wrap(((-g.numerator).toLong << 32) | (g.unwrap & 0xFFFFFFFFL)) // warn +}