Skip to content

Commit

Permalink
Fix prioritization of givens over implicits (#21226)
Browse files Browse the repository at this point in the history
- Fix a typo in `Applications#compare#isAsGood#isGiven` which always 
used `alt1`, to determine if the alternatives passed to `isAsGoodValueType`
 were givens.

- Update `isAsGoodValueType` to not prefer givens over extensions, by
negating the `isGiven` parameter, letting extensions and givens now have
the same priority level as far as that rule is concerned.

- Modify `given`/`implicit` definitions from the `Namer` and the
`PPrint` community-project to resolve ambiguity errors introduced by the changes.
  • Loading branch information
EugeneFlesselle authored Jul 23, 2024
2 parents fb983db + 8f490e1 commit 46ff151
Show file tree
Hide file tree
Showing 6 changed files with 45 additions and 13 deletions.
2 changes: 1 addition & 1 deletion community-build/community-projects/PPrint
16 changes: 7 additions & 9 deletions compiler/src/dotty/tools/dotc/typer/Applications.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1830,10 +1830,8 @@ trait Applications extends Compatibility {
isAsGood(alt1, tp1.instantiate(tparams.map(_.typeRef)), alt2, tp2)
}
case _ => // (3)
def isGiven(alt: TermRef) =
alt1.symbol.is(Given) && alt.symbol != defn.NotGivenClass
def compareValues(tp1: Type, tp2: Type)(using Context) =
isAsGoodValueType(tp1, tp2, isGiven(alt1), isGiven(alt2))
isAsGoodValueType(tp1, tp2, alt1.symbol.is(Implicit), alt2.symbol.is(Implicit))
tp2 match
case tp2: MethodType => true // (3a)
case tp2: PolyType if tp2.resultType.isInstanceOf[MethodType] => true // (3a)
Expand All @@ -1851,7 +1849,7 @@ trait Applications extends Compatibility {
* available in 3.0-migration if mode `Mode.OldImplicitResolution` is turned on as well.
* It is used to highlight differences between Scala 2 and 3 behavior.
*
* - In Scala 3.0-3.5, the behavior is as follows: `T <:p U` iff there is an impliit conversion
* - In Scala 3.0-3.5, the behavior is as follows: `T <:p U` iff there is an implicit conversion
* from `T` to `U`, or
*
* flip(T) <: flip(U)
Expand All @@ -1870,15 +1868,15 @@ trait Applications extends Compatibility {
* for overloading resolution (when `preferGeneral is false), and the opposite relation
* `U <: T` or `U convertible to `T` for implicit disambiguation between givens
* (when `preferGeneral` is true). For old-style implicit values, the 3.4 behavior is kept.
* If one of the alternatives is a given and the other is an implicit, the given wins.
* If one of the alternatives is an implicit and the other is a given (or an extension), the implicit loses.
*
* - In Scala 3.5 and Scala 3.6-migration, we issue a warning if the result under
* Scala 3.6 differ wrt to the old behavior up to 3.5.
*
* Also and only for given resolution: If a compared type refers to a given or its module class, use
* the intersection of its parent classes instead.
*/
def isAsGoodValueType(tp1: Type, tp2: Type, alt1isGiven: Boolean, alt2isGiven: Boolean)(using Context): Boolean =
def isAsGoodValueType(tp1: Type, tp2: Type, alt1IsImplicit: Boolean, alt2IsImplicit: Boolean)(using Context): Boolean =
val oldResolution = ctx.mode.is(Mode.OldImplicitResolution)
if !preferGeneral || Feature.migrateTo3 && oldResolution then
// Normal specificity test for overloading resolution (where `preferGeneral` is false)
Expand All @@ -1896,7 +1894,7 @@ trait Applications extends Compatibility {

if Feature.sourceVersion.isAtMost(SourceVersion.`3.4`)
|| oldResolution
|| !alt1isGiven && !alt2isGiven
|| alt1IsImplicit && alt2IsImplicit
then
// Intermediate rules: better means specialize, but map all type arguments downwards
// These are enabled for 3.0-3.5, and for all comparisons between old-style implicits,
Expand All @@ -1911,8 +1909,8 @@ trait Applications extends Compatibility {
case _ => mapOver(t)
(flip(tp1p) relaxed_<:< flip(tp2p)) || viewExists(tp1, tp2)
else
// New rules: better means generalize, givens always beat implicits
if alt1isGiven != alt2isGiven then alt1isGiven
// New rules: better means generalize, givens (and extensions) always beat implicits
if alt1IsImplicit != alt2IsImplicit then alt2IsImplicit
else (tp2p relaxed_<:< tp1p) || viewExists(tp2, tp1)
end isAsGoodValueType

Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/typer/Namer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1106,7 +1106,7 @@ class Namer { typer: Typer =>
class ClassCompleter(cls: ClassSymbol, original: TypeDef)(ictx: Context) extends Completer(original)(ictx) {
withDecls(newScope(using ictx))

protected implicit val completerCtx: Context = localContext(cls)
protected given completerCtx: Context = localContext(cls)

private var localCtx: Context = uninitialized

Expand Down
2 changes: 1 addition & 1 deletion tests/pos/i13044.scala
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//> using options -Xmax-inlines:33
//> using options -Xmax-inlines:35

import scala.deriving.Mirror
import scala.compiletime._
Expand Down
3 changes: 2 additions & 1 deletion tests/pos/i19715.scala
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ class NT(t: Tup):
object NT:
extension (x: NT)
def app(n: Int): Boolean = true
given Conversion[NT, Tup] = _.toTup
given c1: Conversion[NT, Tup] = _.toTup
implicit def c2(t: NT): Tup = c1(t)

def test =
val nt = new NT(Tup())
Expand Down
33 changes: 33 additions & 0 deletions tests/pos/i21212.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@

trait Functor[F[_]]:
def map[A, B](fa: F[A])(f: A => B): F[B] = ???
trait Monad[F[_]] extends Functor[F]
trait MonadError[F[_], E] extends Monad[F]:
def raiseError[A](e: E): F[A]
trait Temporal[F[_]] extends MonadError[F, Throwable]

trait FunctorOps[F[_], A]:
def map[B](f: A => B): F[B] = ???
implicit def toFunctorOps[F[_], A](target: F[A])(implicit tc: Functor[F]): FunctorOps[F, A] = ???

class ContextBounds[F[_]: Temporal](using err: MonadError[F, Throwable]):
def useCase = err.raiseError(new RuntimeException())
val bool: F[Boolean] = ???
def fails = toFunctorOps(bool).map(_ => ()) // warns under -source:3.5, // error under -source:3.6

class UsingArguments[F[_]](using Temporal[F])(using err: MonadError[F, Throwable]):
def useCase = err.raiseError(new RuntimeException())
val bool: F[Boolean] = ???
def works = toFunctorOps(bool).map(_ => ()) // warns under -source:3.5


object Minimization:

trait A
trait B extends A

def test1(using a1: A)(using b1: B) = summon[A] // picks (most general) a1
def test2(using a2: A)(implicit b2: B) = summon[A] // picks (most general) a2, was ambiguous
def test3(implicit a3: A, b3: B) = summon[A] // picks (most specific) b3

end Minimization

0 comments on commit 46ff151

Please sign in to comment.