[Experiment] Drop prioritization of givens over implicits

[EugeneFlesselle]

Should fix #21264 although that has not been tested yet

[EugeneFlesselle]

@WojciechMazur could you test if the changes here drop the warnings from #21264 (comment) ?
Note that even if it should solve those, it may likely also incur new warnings (including in other projects).

[EugeneFlesselle]

@odersky Unfortunately, there are already errors in the community-build C, e.g.:

[error] -- [E172] Type Error: /__w/scala3/scala3/community-build/community-projects/scalaz/core/src/main/scala/scalaz/Adjunction.scala:120:46 
[error] 120 |    new Adjunction[Writer[E, *], Reader[E, *]] {
[error]     |                                              ^
[error]     |Ambiguous given instances: both method writerTraverse in class WriterTInstances0 and method writerFunctor in class WriterTInstances14 match type scalaz.Functor[[_] =>> scalaz.Writer[E, _]] of parameter F of constructor Adjunction in class Adjunction

[error] -- [E007] Type Mismatch Error: /__w/scala3/scala3/community-build/community-projects/scala-collection-compat/compat/src/test/scala/test/scala/collection/BuildFromTest.scala:124:39 
[error] 124 |    val xs6: TreeMap[String, String] = xs5
[error]     |                                       ^^^
[error]     |          Found:    (xs5 : Map[String, String])
[error]     |          Required: scala.collection.immutable.TreeMap[String, String]
[error]     |
[error]     | longer explanation available when compiling with `-explain`

[error] -- [E172] Type Error: /__w/scala3/scala3/community-build/community-projects/parboiled2/parboiled-core/src/test/scala/org/parboiled2/ActionSpec.scala:40:32 
[error] 40 |      "`test`" - new TestParser0 {
[error]    |                                ^
[error]    |Ambiguous given instances: both method hnil in object Unpack and object HNilUnpack in object Unpack match type org.parboiled2.support.Unpack.Aux[org.parboiled2.support.hlist.HNil, Unit] of parameter unpack of constructor TestParser in class TestParser

[error] -- [E172] Type Error: /__w/scala3/scala3/community-build/community-projects/parboiled2/parboiled-core/src/test/scala/org/parboiled2/RunningSpec.scala:38:17 
[error] 38 |        p.A.run() ==> Success(())
[error]    |                 ^
[error]    |No best given instance of type org.parboiled2.Parser.DeliveryScheme[org.parboiled2.support.hlist.HNil] was found for parameter scheme of method run in trait RuleRunnable.
[error]    |I found:
[error]    |
[error]    |    org.parboiled2.Parser.DeliveryScheme.Try[org.parboiled2.support.hlist.HNil, Out]
[error]    |      (
[error]    |      /* ambiguous: both method hnil in object Unpack and object HNilUnpack in object Unpack match type org.parboiled2.support.Unpack.Aux[org.parboiled2.support.hlist.HNil, Out] */
[error]    |        summon[
[error]    |          org.parboiled2.support.Unpack.Aux[org.parboiled2.support.hlist.HNil, Out]]
[error]    |      )
[error]    |
[error]    |But both method hnil in object Unpack and object HNilUnpack in object Unpack match type org.parboiled2.support.Unpack.Aux[org.parboiled2.support.hlist.HNil, Out].

[error] -- [E007] Type Mismatch Error: /__w/scala3/scala3/community-build/community-projects/play-json/play-json/js/src/test/scala/JsonSpec.scala:38:26 
[error] 38 |        "key5" -> Json.obj(
[error]    |                  ^
[error]    |Found:    play.api.libs.json.JsObject
[error]    |Required: play.api.libs.json.Json.JsValueWrapper
[error]    |Note that implicit conversions cannot be applied because they are ambiguous;
[error]    |both method writes in object JsObject and method jsValueWrites in trait DefaultWrites match type play.api.libs.json.Writes[play.api.libs.json.JsObject]
[error] 39 |          "key6" -> "こんにちは",
[error] 40 |          "key7" -> BigDecimal("12345678901234567000")
[error] 41 |        )
[error]    |
[error]    | longer explanation available when compiling with `-explain`
[error] -- [E007] Type Mismatch Error: /__w/scala3/scala3/community-build/community-projects/play-json/play-json/js/src/test/scala/JsonSpec.scala:56:26 

[odersky]

@WojciechMazur Can we test this PR with the openCB? It is a modified implicit priority scheme.

On the one hand. we also reverse priority for implicit/implicit pairs, which was not the case before. The advantage is that we do the priority switch in one go. If a library switches from implicits to givens in the future there is no danger of an inadvertent change in resolution. There is one caveat: We still fall back to the old priority rules for implicit/implicit pairs if the new ones would give an ambiguity error. So changing these implicit/implicit pairs to givens risks causing ambiguity errors. But it does not risk a change in the resolved given.

On the other hand we add a tweak where in the case of ambiguities owner ranking trumps type ranking. This helps since there are libraries that explicitly prioritize their implicits by placing them into different classes of a linear hierarchy. If the type score now goes against that we honor the explicit prioritization according to owner instead of reporting an ambiguity.

[EugeneFlesselle]

@odersky I was in the process of writing a review, but ended up pushing two small changes instead:

• the first being a small optimization,
• the second (and more notable one) being that it seems we may not need the (2nd) owner score disambiguation scheme after all,

I hope that's okay.

The changes look good to me otherwise, hopefully we get positive results from the openCB

[WojciechMazur]

I've started the OpenCB based on last commit -e1cc065 I'll report results when it done

[odersky]

@EugeneFlesselle I don't think the optimization is worth it here (not that it matters much either way). It should be very rare to have an ambiguous implicit/implicit pair. On the other hand, a local lazy val requires an allocation even if it is never used (which will be the case for all new code past 3.6 which uses only gives). So since the benefit is dubious or even negative, I don't think it's worthwhile complicating the logic here.

EDIT: In fact looking at it more I find the revised logic clearer than the original one. So let's keep it, a single allocation should not matter anyway here.

[odersky]

I believe we still want the ownerscore disambiguation because this gives us a way to upgrade implicit hierarchies to given hierarchies. Right now, the ownerscre disambiguation was not needed since all owner hierarchies we have seen were over implicits so the second scheme for disambiguating implicit/implicit pairs worked in its place. But this will change when those implicits are changed to givens. Then the ownerscore disambiguation becomes essential.

[EugeneFlesselle]

I believe we still want the ownerscore disambiguation because this gives us a way to upgrade implicit hierarchies to given hierarchies. Right now, the ownerscre disambiguation was not needed since it all owner hierarchies we have seen were over implicits so the second scheme for disambiguating implicit/implicit pairs worked in its place. But this will change when those implicits are changed to givens. Then the ownerscore disambiguation becomes essential.

In that case, I don't think the previous version addressed that case as we were under the case matching ownerScore to 0.

EDIT: in fact, I think the ownerScore match may be inexhaustive

[EugeneFlesselle]

@EugeneFlesselle I don't think the optimization is worth it here (not that it matters much either way). It should be very rare to have an ambiguous implicit/implicit pair. On the other hand, a local lazy val requires an allocation even if it is never used (which will be the case for all new code past 3.6 which uses only gives). So since the benefit is dubious or even negative, I don't think it's worthwhile complicating the logic here.

EDIT: In fact looking at it more I find the revised logic clearer than the original one. So let's keep it, a single allocation should not matter anyway here.

Either way, dropping the commit is completely fine with me

[odersky]

@EugeneFlesselle @bracevac

Once we have the results from the OpenCB we might want to explore ways how to mitigate the breakage. Say we have a library with implicits whose priority changes. We find out in a test of the library or in an application using the library. We can fix this on ether the library side or the application side.

• On the library side: Prioritize explicitly using owner score.
• On the application side: Provide an explicit argument fixing the old choice.

One technique which might help would be to offer a rewrite rule in 3.5 or 3.6-migration that adds an explicit argument according to the old rules if a priority change is observed.

[odersky]

In that case, I don't think the previous version addressed that case as we were under the case matching ownerScore to 0.

Indeed. I now pushed a commit that should fix this problem.

[WojciechMazur]

I'm having a tough time comparing the results of the OpenCB due to the merge of #21257 which (probably) caused failures in ~90 projects in the last nightly. I might need to create a dedicated branch without this change and run build again so we would be able to gather any decent conclusions. We might try to run them with -source:3.5-migration to mitigate the problems with using clouses, but it would also affect results of given prioritization (especially the warnings instead of hard errors that are easy to miss)

[odersky]

I am shortly going to push a fix & analysis for the failing StringFormatterTest.

[odersky]

@WojciechMazur I see. Yes, probably it's best to temporarily revert #21257.

[WojciechMazur]

This PR causes at least 1 regression in lemonlabsuk/scala-uri
More issues will follow in the future as soon as I would be able to extract them from other regression caused by changes merged recently to main.

import scala.language.implicitConversions

trait QueryKey[A]
object QueryKey extends QueryKeyInstances
sealed trait QueryKeyInstances:
  implicit val stringQueryKey: QueryKey[String] = ???

trait QueryValue[-A]
object QueryValue extends QueryValueInstances 
sealed trait QueryValueInstances1:
  implicit final val stringQueryValue: QueryValue[String] = ???
  implicit final val noneQueryValue: QueryValue[None.type] = ???

sealed trait QueryValueInstances extends QueryValueInstances1:
  implicit final def optionQueryValue[A: QueryValue]: QueryValue[Option[A]] = ???

trait QueryKeyValue[A]
object QueryKeyValue:
  implicit def tuple2QueryKeyValue[K: QueryKey, V: QueryValue]: QueryKeyValue[(K, V)] = ???


@main def Test = summon[QueryKeyValue[(String, None.type)]] // error

yields

-- [E172] Type Error: /Users/wmazur/projects/sandbox/src/main/scala/usage.scala:22:59 
22 |@main def Test = summon[QueryKeyValue[(String, None.type)]]
   |                                                           ^
   |No best given instance of type QueryKeyValue[(String, None.type)] was found for parameter x of method summon in object Predef.
   |I found:
   |
   |    QueryKeyValue.tuple2QueryKeyValue[String, None.type](QueryKey.stringQueryKey,
   |      QueryValue.optionQueryValue[A](
   |        /* ambiguous: both value stringQueryValue in trait QueryValueInstances1 and value noneQueryValue in trait QueryValueInstances1 match type QueryValue[A] */
   |          summon[QueryValue[A]]
   |      )
   |    )
   |
   |But both value stringQueryValue in trait QueryValueInstances1 and value noneQueryValue in trait QueryValueInstances1 match type QueryValue[A].