Change rules for given prioritization

[odersky]

Consider the following program:

class A
class B extends A
class C extends A

given A = A()
given B = B()
given C = C()

def f(using a: A, b: B, c: C) =
  println(a.getClass)
  println(b.getClass)
  println(c.getClass)

@main def Test = f

With the current rules, this would fail with an ambiguity error between B and C when trying to synthesize the A parameter. This is a problem without an easy remedy.

We can fix this problem by flipping the priority for implicit arguments. Instead of requiring an argument to be most specific, we now require it to be most general while still conforming to the formal parameter.

There are three justifications for this change, which at first glance seems quite drastic:

• It gives us a natural way to deal with inheritance triangles like the one in the code above. Such triangles are quite common.
• Intuitively, we want to get the closest possible match between required formal parameter type and synthetisized argument. The "most general" rule provides that.
• We already do a crucial part of this. Namely, with current rules we interpolate all type variables in an implicit argument downwards, no matter what their variance is. This makes no sense in theory, but solves hairy problems with contravariant typeclasses like Comparable. Instead of this hack, we now do something more principled, by flipping the direction everywhere, preferring general over specific, instead of just flipping contravariant type parameters.

[odersky]

This is a test PR, to see what breaks in the CB.

[odersky]

It turns out nothing broke. I only had to change

• some tests that explicitly tested the old behavior
• a typo in a shapeless 3 test which was not caught because of the old behavior

So I think we can go ahead with this. What's currently implemented:

• Change to the new scheme in 3.5
• Diagnose with a warning where the precedence will change in 3.5-migration

In 3.4 we only revert back overloading resolution to be always specializing without flipping contravariant arguments. I believe that was a mistake to do it in the first place. The only use cases for the strange flip came from implicit arguments, there was never a question to extend it to overloading resolutioin.

[smarter]

The implementation looks good to me, but I believe this will need to be experimental until we go through the SIP process since it's a spec change.

• Concerning the change for 3.4:

  In 3.4 we only revert back overloading resolution to be always specializing without flipping contravariant arguments. I believe that was a mistake to do it in the first place. The only use cases for the strange flip came from implicit arguments, there was never a question to extend it to overloading resolutioin.

  The spec says (/cc @sjrd):

  If there are several eligible arguments which match the implicit parameter's type, a most specific one will be chosen using the rules of static overloading resolution.

  This is nice since it's really parsimonious, if implicit specificity and overloading resolution diverge, we'll pay a price in term of spec complexity.

• Concerning the main change:

  It gives us a natural way to deal with inheritance triangles like the one in the code above. Such triangles are quite common.

  This is true, but I'm not sure if the change as-is fixes the kind of issues people encounter in practice with triangles. The typical issue seems to be something like:

  trait Functor[F[_]]:
    extension [A, B](x: F[A]) def map(f: A => B): F[B]
  trait Monad[F[_]] extends Functor[F] { ... }
  trait Traverse[F[_]] extends Functor[F] { ... }
  
  given Monad[List] with Traverse[List] with { ... }
  
  def test[F[_]: Monad: Traverse](x: F[A]) =
    x.map(...) // error: map is ambiguous

  I believe the recommended workaround in Scala 3 so far was:

  def test[F[_]: Monad: Traverse](x: F[A]) =
    given Functor[F] = summon[Monad[F]]
    x.map(...)

  and it seems that this change enables a slightly different workaround:

  def test[F[_]: Functor: Monad: Traverse](x: F[A]) =
    x.map(...)

  But it's not clear to me that this is worth changing the rules over, at least compared to other rule changes (like prioritizing implicit parameters based on their position in the parameter list).

[odersky]

That's the problem: The example we got (and handled) was already very specialized. Who wants a monad and a traversable in the same function? But what's much more common is that I have the three classes as defined and I want a Functor! Seems obvious, but so far I could not do it. This PR solves the problem.

About going through the SIP process. In theory, yes. But I must admit to myself that I simply don't have the bandwidth to do this. So unless we get a jump in resources and other people going ahead with this, we have to conclude that this will likely not get fixed.

I am thinking of creating a separate branch where I do these improvements. Then the SIP committee or anybody else can pick what they think is worthwhile and turn it into a SIP.

[odersky]

I think this needs to be merged, and I will not have the bandwidth to make a SIP for this.

Are there other opinions how to proceed here?

[odersky]

@smarter It turns out that Dimi's attempted hylolib port had lots of problems with ambiguities, and they all went away with
#19395, which includes this PR as the only change to given resolution. So I think the triangle problem is quite common in "natural" code, and this is a simple fix, which so far broke nothing.

I suggest we roll this in for 3.5 and run the open community build to see whether there are any breakages that can't be fixed.

[smarter]

It turns out that Dimi's attempted hylolib port had lots of problems with ambiguities, and they all went away with
#19395, which includes this PR as the only change to given resolution.

I assume #19395 will have a SIP? If the change to given resolution passes the open community build then it's probably uncontroversial enough that it could be a line item in the larger SIP of #19395. Alternatively, maybe @kyouko-taiga would be interested in making a SIP just for the given resolution changes motivated by examples from hylolib?

[odersky]

Sorry, I misread. Yes, of course #19395 will be a SIP. I'd be OK with this fix being a line item, except that I think it would be important to roll it in quickly before there is more code that could break. Type inference changes are always tricky, as we know. So I think it's better to treat it as an independent first step.

I believe #19395 would take a lot more time to be properly discussed.

[smarter]

I'm not opposed to this being merged now, but I do hope we find the time to discuss it in a wider venue nevertheless.

[kyouko-taiga]

I'm happy to work on a SIP draft for #19395 but it should be clear that I am not the designer of the proposed changes. I merely presented Martin with a list of complaints and let him do the hard work.

Maybe something that could make more sense is for me to write a sort of blog post justifying why one would like to write code in the style of hylo-lib.

[bishabosha]

I'm happy to work on a SIP draft for #19395 but it should be clear that I am not the designer of the proposed changes. I merely presented Martin with a list of complaints and let him do the hard work.

You can co-sign with Martin - as you say you have a lot to say in the motivation section

[odersky]

@WojciechMazur We are considering merging this change for 3.5. Can we run an open CB to see what the damage would be? In the small CB it only caused one regression in a Shapeless 3 test, which arguably detected an error in the test.

[WojciechMazur]

I've run the Open CB for this project 2 weeks ago and there were some failures:

Project	Version	Build URL	Notes
7mind/izumi	1.2.7	Open CB logs	No given instance of
apache/pekko-connectors	1.0.2	Open CB logs	No given instance of
typelevel/kittens	3.3.0	Open CB logs	No given instance of
j5ik2o/akka-persistence-s3	1.2.177	Open CB logs	no member new scala.beans.BeanProperty
kalin-rudnicki/harness	4.1.4 -> 4.1.7	Open CB logs	Recursion limit exceeded.
dfianthdl/dfiant	0.3.7	Open CB logs	Recursion limit exceeded
rssh/dotty-cps-async	0.9.19 -> 0.9.21	Open CB logs

I've started a new build using the rebased version of this PR to confirm the results

[odersky]

Ah yes, thanks for reminding me. I think the results will be similar now.

…

On Wed, Apr 17, 2024 at 2:39 PM Wojciech Mazur ***@***.***> wrote: I've run the Open CB for this project 2 weeks ago and there were some failures: Project Version Build URL Notes 7mind/izumi 1.2.7 Open CB logs <https://github.com/VirtusLab/community-build3/actions/runs/8546672779/job/23422651877> No given instance of apache/pekko-connectors 1.0.2 Open CB logs <https://github.com/VirtusLab/community-build3/actions/runs/8546672779/job/23417510904> No given instance of typelevel/kittens 3.3.0 Open CB logs <https://github.com/VirtusLab/community-build3/actions/runs/8546672779/job/23418739535> No given instance of j5ik2o/akka-persistence-s3 1.2.177 Open CB logs <https://github.com/VirtusLab/community-build3/actions/runs/8546672779/job/23417522349> no member new scala.beans.BeanProperty kalin-rudnicki/harness 4.1.4 -> 4.1.7 Open CB logs <https://github.com/VirtusLab/community-build3/actions/runs/8552762608/job/23434682090> Recursion limit exceeded. dfianthdl/dfiant 0.3.7 Open CB logs <https://github.com/VirtusLab/community-build3/actions/runs/8546672779/job/23418552357> Recursion limit exceeded rssh/dotty-cps-async 0.9.19 -> 0.9.21 Open CB logs <https://github.com/VirtusLab/community-build3/actions/runs/8546672779/job/23417531690> I've started a new build using the rebased version of this PR to confirm the results — Reply to this email directly, view it on GitHub <#19300 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGCKVROGJXZ3ZAPQ7GRJC3Y5ZUQNAVCNFSM6AAAAABA33VSW2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANRRGE3DMMJRHE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

-- Martin Odersky Professor, Programming Methods Group (LAMP) Faculty IC, EPFL Station 14, Lausanne, Switzerland

[lenguyenthanh]

👍 for RC3

[WojciechMazur]

@odersky Question about this snippet:

trait Eq[A] 
trait Order[A] extends Eq[A]:
  def toOrdering: Ordering[A]

def Test[Element: Eq: Order] = summon[Eq[Element]].toOrdering

It compiles fine (in both 3.5 and 3.nightly) unless when using -source:future. Bisect points out that it started to fail with value toOrdering is not a member of Eq[Element] since this change, but there seems to be nothing specific introduced that might change behaviour under -source:future.
In general, it seems to be more correct behaviour, although I cannot understand why it changed and why it behaves like that only in the future source version.

[odersky]

The difference is that under -source:future, context bounds become using clauses, so the new prioritization applies. That's another (hopefully the last) thing we need to change. Maybe we can change it for 3.6 already, so that the priority change all happens at once?

…

On Tue, Jul 23, 2024 at 11:29 PM Wojciech Mazur ***@***.***> wrote: @odersky <https://github.com/odersky> Question about this snippet: trait Eq[A] trait Order[A] extends Eq[A]: def toOrdering: Ordering[A] def Test[Element: Eq: Order] = summon[Eq[Element]].toOrdering It compiles fine (in both 3.5 and 3.nightly) unless when using -source:future. Bisect points out that it started to fail with value toOrdering is not a member of Eq[Element] since this change, but there seems to be nothing specific introduced that might change behaviour under -source:future. In general, it seems to be more correct behaviour, although I cannot understand why it changed and why it behaves like that only in the future source version. — Reply to this email directly, view it on GitHub <#19300 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGCKVSKYJUT4NHJKFDRNJTZN3DLTAVCNFSM6AAAAABA33VSW2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBWGM2TAOJSGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Martin Odersky Professor, Programming Methods Group (LAMP) Faculty IC, EPFL Station 14, Lausanne, Switzerland

[odersky]

In fact, I think we should do the switch to using clauses also in 3.6. Migration should not be a problem since we warn about the change since 3.4. For instance

class Eq[A]

val eq: Eq[Int] = Eq()

def foo[T: Eq]: Eq[T] = ???
val x = foo(eq)

gives:

-- Warning: test.scala:6:8 -----------------------------------------------------
6 |val x = foo(eq)
  |        ^^^
  |Context bounds will map to context parameters.
  |A `using` clause is needed to pass explicit arguments to them.
  |This code can be rewritten automatically under -rewrite -source 3.4-migration.

We could make the warning an error in 3.6-migration and switch to the new behavior in 3.6. That would make the implicit priority change more coherent, and avoid a surprising exception.

[odersky]

Correction: The warning is already an error in 3.5. So we are already covered and all that's needed is to do the switch.

[SethTisue]

blog post about this: scala/scala-lang#1675 (still a draft, at the moment)