Backport "Fix healAmbiguous to compareAlternatives with disambiguate = true" to 3.5.0 #21344
Conversation
Before the changes, if `isAsGoodValueType` was called with an extension and a given conversion, it would prefer the conversion over the extension, because only the former yielded true in `isGiven`. Which contradicted the logic from searchImplicit which preferred extension over conversions for member selection.
We now use a left-biased scheme, as follows. From 3.6 on: - A given x: X is better than a given or implicit y: Y if y can be instantiated/widened to X. - An implicit x: X is better than a given or implicit y: Y if y can be instantiated to a supertype of X. - Use owner score for givens as a tie breaker if after all other tests we still have an ambiguity. This is not transitive, so we need a separate scheme to work around that. Other change: - Drop special handling of NotGiven in prioritization. The previous logic pretended to do so, but was ineffective.
We only have transitivity between givens or between implicits. To cope with that - We tank first all implicits, giving a best implicit search result. - Then we rank all givens startign with the implicit result. If there is a given that is better than the best implicit, the best given will be chosen. Otherwise we will stick with the best implicit.
Warnings from 3.6, change in 3.7. This is one version later than originally planned.
Make the wording of a priority change warning message stable under different orders of eligibles. We now always report the previously chosen alternative first and the new one second. Note: We can still get ambiguities by fallging different pairs of alternatives depending on initial order.
On the final result, compared with all the ambiguous candidates we are trying to recover from. We should still use `disambiguate = false` when filtering the `pending` candidates for the purpose of warnings, as in the other cases. Before the changes, it was possible for an ambiguous SearchFailure to be healed by a candidate which was considered better (possibly only) under a prioritization scheme different from the current one. As an optimization, we can avoid redoing compareAlternatives in versions which could have only used the new prioritization scheme to begin with. Also restores behaviour avoiding false positive warnings. Specifically, in cases where we could report a change in prioritization, despite having not yet done `tryImplicit` on the alternative, i.e. it was only compared as part of an early filtering See scala#21045 for related changes
| @@ -0,0 +1,31 @@ | |||
| //> using options -source:3.6 | |||
There was a problem hiding this comment.
Backported version required -source:3.6 which is not needed in main
There was a problem hiding this comment.
main has the baseVersion set to 3.6 already, so we get the CompareScheme.New by default.
While in 3.5 we maintain the intermediate comparison scheme, in which the test case used to be un-ambiguous, so I think this is as expected.
|
This PR couses the same regressions as reported in #21328 (comment)
|
But only in |
|
It's happening in default, that is -source:3.5 compilation. I'm working on reproducers right now for missing implicits |
The only place emitting warnings with messages of the form "Given search preference for ... between alternatives ... none - it's ambiguous" is in val sv = Feature.sourceVersion
val isLastOldVersion = sv.stable == SourceVersion.`3.6`
val isWarnPriorityChangeVersion = isLastOldVersion || sv == SourceVersion.`3.7-migration`So I am very confused about how we can still be getting these warnings where the source version is |
|
Reproducer for object serializer:
trait Reader[T]
trait Writer[T]
// Needs to be implicit val
implicit val UnitReader: Reader[Unit] = ???
implicit val StringReader: Reader[String] = ???
// A way to derive instances needs to be available
inline given superTypeReader[T: scala.reflect.ClassTag]: Reader[T] = ???
import serializer.Reader
trait Codec[T]
trait Channel[F[_]]:
def notificationStub[In: Codec](): In => F[Unit]
trait Monadic[F[_]]
sealed abstract class LSPNotification():
type In
given inputReader: Reader[In]
class PreparedNotification[X <: LSPNotification](val x: X, val in: x.In):
type In = x.In
trait Communicate[F[_]]:
def notification[X <: LSPNotification](notif: X, in: notif.In): F[Unit]
object Communicate:
given codec[T: Reader]: Codec[T] = ???
def channel[F[_]: Monadic](channel: Channel[F]) =
new Communicate[F]:
override def notification[X <: LSPNotification](notif: X, in: notif.In): F[Unit] =
channel.notificationStub().apply(in) // error, does not infer notif.In type so cannot summon correct implicit |
The regressions are not about warnings. These are hard errors the first reproducer fails even with -source:3.4 |
I see, thank you for the reproducer This is caused by 8a41389, that commit is necessary only to maintain transitivity in the new comparison scheme. I will investigate what exactly is happing there. In the mean time, it should be fine to drop that commit for the purpose of the backport. We should at least relaunch the open CB on this PR - 8a41389 to verify there are no other issues. |
Minimized a little: trait Reader[T]
// Needs to be implicit val
implicit val UnitReader: Reader[Unit] = ???
implicit val StringReader: Reader[String] = ???
// A way to derive instances needs to be available
given superTypeReader[T: scala.reflect.ClassTag]: Reader[T] = ???
object Notif:
type In
given inputReader: Reader[In] = ???
object Test:
def notificationStub[T: Reader]: T => Unit = ???
val in: Notif.In = ???
notificationStub.apply(in) // error since 8a41389d
In 8a41389, we did not account for the fact that the result of case nil =>
if (rfailures.isEmpty) found
else found.recoverWith(_ => rfailures.reverse.maxBy(_.tree.treeSize))Having split the ranking of implicits and givens changes the accumulated |
|
Reproducer for trait Text[T]
trait Read[A]
object Read extends ReadImplicits:
implicit val unit: Read[Unit] = ???
trait ReadImplicits:
import scala.deriving.*
given roe: Read[Option[EmptyTuple]] = ???
given rou: Read[Option[Unit]] = ???
given cons1[H, T <: Tuple](using Read[Option[H]], Read[Option[T]]): Read[Option[H *: T]] = ???
trait Fragment:
def query[B: Read]: String = ???
@main def Test =
val f: Fragment = ???
f.query // errorThis snipet was compiled previously only thanks to a big amount of luck. Typical usage of -- [E172] Type Error: /Users/wmazur/projects/sandbox/src/test.scala:16:10 ------
16 | f.query // error
| ^
|No best given instance of type Read[B] was found for a context parameter of method query in trait Fragment.
|I found:
|
| Read.cons1[H, T](
| /* ambiguous: both given instance roe in trait ReadImplicits and given instance rou in trait ReadImplicits match type Read[Option[H]] */
| summon[Read[Option[H]]],
| Read.roe)
|
|But both given instance roe in trait ReadImplicits and given instance rou in trait ReadImplicits match type Read[Option[H]].
1 error found |
|
All 4 regressions are gone after reverting 8a41389, it does not cause any new problems. |
This reverts commit 8a41389.
OK great! I was thinking about how this could cause problems. The only thing that should be different is now we typecheck more implicits. Say we have a simple given and a complicated recursive implicit. Before 8a41389, we would typecheck the given first and then the implicit. Afterwards, it's the reverse. This could very well cause problems. One way to cause fewer problems is to start with the simplest term after sorting, then compare it with all terms of the same implicit-vs-given-class, then do the rest. But in general it seems impossible to guarantee no regressions when we change the order in which candidates are type checked. |
This reverts commit 7c4bd67 See scala#21344 (comment) Fixes scala#21320
* `neandertech/langoustine` - [scala#21344 (comment)] (scala#21344 (comment)) * `tpolcat/doobie` - [scala#21344 (comment)] (scala#21344 (comment)) Fixes scala#21352
This reverts commit 7c4bd67 See [#21344 comment] (#21344 (comment)) We will have to reconsider how to alleviate the transitory problem before releasing 3.6. Fixes #21320 Fixes #21352
This reverts commit 7c4bd67 See #21344 (comment) Fixes #21320 [Cherry-picked 58f3407][modified]
* `neandertech/langoustine` - [#21344 (comment)] (#21344 (comment)) * `tpolcat/doobie` - [#21344 (comment)] (#21344 (comment)) Fixes #21352 [Cherry-picked 5408a80]
Backports #21339 to Scala 3.5.0-RC7