Fixes and tweaks to implicit priority change scheme #21328

odersky · 2024-08-05T09:40:38Z

We now use a left-biased scheme, as follows.

From 3.7 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, but the PR implements a scheme to work around that.

Other change:

Drop special handling of NotGiven in prioritization. The previous logic pretended to do so, but was ineffective.

odersky · 2024-08-05T11:51:22Z

@WojciechMazur Can we get OpenCB results also for the two commits in this PR. Ideally first ffff15c and then 37dd612. My theory is that they would solve the uplicke problem and hopefully some other problems as well.

WojciechMazur · 2024-08-05T11:55:56Z

Sure, I'll start the OpenCB

WojciechMazur · 2024-08-05T13:34:42Z

Another comparison based on 3.5.0-RC6 + this PR

New regressions:

Project	Version	Build URL	Notes
neandertech/langoustine	0.0.22	Open CB logs	missing implicit
softwaremill/tapir	1.11.0	Open CB logs	crash
tpolecat/doobie	1.0.0-RC5	Open CB logs	missing implicit
virtuslab/scala-cli	1.4.1	Open CB logs	crash

Givens changes warnings:

Given alternatives choice: Some(the second alternative) - 503
Given alternatives choice: Some(none - it's ambiguous) - 753
Given alternatives choice: Some(the first alternative) - 397
Projects with changed alternatives warnings - 26

When compared with other alternatives this solution seems to keep the most ambitious implicits and overall seems to give the most similar when comparing stats with 3.5.0-RC6

odersky · 2024-08-05T14:14:32Z

The crash in Tapir seems to be unrelated to implicits, it's probably just because some different code is generated now.

Do you have the results from 3.5.0-RC6 for comparison?

odersky · 2024-08-05T14:18:38Z

When compared with #21325 (comment) this solution seems to keep the most ambitious implicits and overall seems to give the most similar when comparing stats with 3.5.0-RC6

Is upickle compiling now or still failing?

Most ambiguous is explainable since we now don't pick a winner in a given/implicit pair. These are treated as ambiguous, unless there is another overriding factor.

odersky · 2024-08-06T11:15:23Z

@Kordyjan This is the PR we will go with for 3.5. It implements the delay of the switchover that we discussed.

EugeneFlesselle · 2024-08-06T13:30:56Z

compiler/src/dotty/tools/dotc/typer/Applications.scala

-          // 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,
-          // and in 3.5 and 3.6-migration when we compare with previous rules.
+        if scheme == CompareScheme.Intermediate || alt1IsImplicit then


Suggested change

if scheme == CompareScheme.Intermediate || alt1IsImplicit then

if scheme == CompareScheme.Intermediate then

Is it intentional that we still do the CompareScheme.Intermediate when specifically alt1 is an implicit ?

Yes! The idea is that for implicit/givens pairs we do the rules according to the left operand. Say an implicit I is more specialized than a given G. Then I wins by type against G but G also wins by type against I. So it's a draw in the end.
But in more complex situations it could be (for instance) that G doe not win against I because there are some variable's to instantiate on I but not on G. So I wins. (or G, vice versa). The PR comment mentions this.

compiler/src/dotty/tools/dotc/typer/Implicits.scala

tests/neg/i21212.scala

odersky · 2024-08-06T18:00:36Z

I am testing a fix to disambiguate as we discussed. Let's try that and if that fails the CI we can fallback to this PR.

…

On Tue, Aug 6, 2024 at 7:54 PM Eugene Flesselle ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In tests/neg/i21212.scala <#21328 (comment)>: > @@ -0,0 +1,11 @@ +//> using options -source 3.7 +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] // error: ambiguous Oh my bad, they in fact do *not*, I had simply forgotten to use -source:3.7, sorry about that I will add the test back — Reply to this email directly, view it on GitHub <#21328 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGCKVRBIYETFG7E3D5K3VLZQEEUNAVCNFSM6AAAAABL75WTICVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEMRRHEZDKNZRHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

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

EugeneFlesselle · 2024-08-06T18:06:20Z

I am testing a fix to disambiguate as we discussed. Let's try that and if that fails the CI we can fallback to this PR.

@odersky I initially implemented what we had proposed for disambiguate, i.e. if the alt1 is a failure, only return alt2 if it is strictly better than alt1 w.r.t compareAlternatives(disambiguate = true). This logic however should only apply if we are trying to recover from an ambiguous result, i.e. coming from healAmbigous (otherwise we do still want to keep alt2).
We can either thread another boolean parameter indicating whether or we are in that case, or move this logic into healAmbigous. I opted for the latter approach in #21339.

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.

EugeneFlesselle · 2024-08-06T19:55:42Z

compiler/src/dotty/tools/dotc/typer/Implicits.scala

+        case fail: SearchFailure =>
+          fail.reason match
+            case ambi: AmbiguousImplicits =>
+              if compareAlternatives(ambi.alt1, alt2, disambiguate = true) < 0


I think this would be the expected behavior for disambiguate

when coming from healAmbiguous from a standard ambiguity case, that is, for the current search;

but not when coming from healAmbiguous from an ambiguity which happened in a nested search.

In the latter case, we would then be comparing alt2 to alternatives for an unrelated expected type.

How about we make use disambiguate = true for compareAlternatives when healing nested implicits?

As far as I can tell, that would mean potentially faulty as in false positive (but never missing) warnings for a change in resolution in the nested search

Perhaps we could do the check twice only in the 3.7-migration version, and keep the single check in 3.6 for better performance

Indeed. In light of this, I think we should go with #49. Can you rebase it on the current state?

Can you rebase it on the current state?

Yes, will do that right now + plus first checking if we are in a migration version before the second comparison

I've pushed the changes (on #49) after the rebase, the optimization for versions staring from 3.7, and some documentation.

odersky · 2024-08-07T09:52:14Z

Superseded by #21339.

Based on #21328: We now use a left-biased scheme, as follows. From 3.7 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, but the PR implements a scheme to work around that. Other changes: - Drop special handling of NotGiven in prioritization. The previous logic pretended to do so, but was ineffective. - Fix healAmbiguous to compareAlternatives with disambiguate = true, to account for changes made in #21045

WojciechMazur mentioned this pull request Aug 5, 2024

Another given priority scheme to ensure transitivity #21325

Closed

odersky force-pushed the given-priority-mixed branch from 5e14513 to ff0b6b9 Compare August 6, 2024 08:02

odersky requested a review from EugeneFlesselle August 6, 2024 11:14

odersky assigned EugeneFlesselle Aug 6, 2024

odersky added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Aug 6, 2024

odersky mentioned this pull request Aug 6, 2024

Tweaks to given priority #21305

Closed

odersky changed the title ~~A left-biased variant for implicit/given pairs~~ Fixes and tweaks to implicit priority change scheme Aug 6, 2024

EugeneFlesselle reviewed Aug 6, 2024

View reviewed changes

tests/neg/i21212.scala Show resolved Hide resolved

EugeneFlesselle mentioned this pull request Aug 6, 2024

Fix healAmbiguous to compareAlternatives with disambiguate = true #21339

Merged

odersky added 4 commits August 6, 2024 20:52

Delay priority change until 3.7

f62e141

Warnings from 3.6, change in 3.7. This is one version later than originally planned.

Fix ranking logic

87f5ca0

odersky force-pushed the given-priority-mixed branch from c94ffac to 58545eb Compare August 6, 2024 19:20

odersky force-pushed the given-priority-mixed branch from 58545eb to f5f390e Compare August 6, 2024 19:43

EugeneFlesselle reviewed Aug 6, 2024

View reviewed changes

EugeneFlesselle force-pushed the given-priority-mixed branch from 0361991 to f5f390e Compare August 7, 2024 07:22

odersky closed this Aug 7, 2024

WojciechMazur mentioned this pull request Aug 7, 2024

Backport "Fix healAmbiguous to compareAlternatives with disambiguate = true" to 3.5.0 #21344

Merged

WojciechMazur removed the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Nov 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes and tweaks to implicit priority change scheme #21328

Fixes and tweaks to implicit priority change scheme #21328

odersky commented Aug 5, 2024 •

edited

Loading

odersky commented Aug 5, 2024

WojciechMazur commented Aug 5, 2024

WojciechMazur commented Aug 5, 2024 •

edited

Loading

odersky commented Aug 5, 2024

odersky commented Aug 5, 2024

odersky commented Aug 6, 2024

EugeneFlesselle Aug 6, 2024

odersky Aug 6, 2024

odersky commented Aug 6, 2024 via email

EugeneFlesselle commented Aug 6, 2024

EugeneFlesselle Aug 6, 2024 •

edited

Loading

odersky Aug 6, 2024

EugeneFlesselle Aug 6, 2024

EugeneFlesselle Aug 6, 2024 •

edited

Loading

odersky Aug 6, 2024

EugeneFlesselle Aug 6, 2024

EugeneFlesselle Aug 6, 2024

odersky commented Aug 7, 2024

	if scheme == CompareScheme.Intermediate \|\| alt1IsImplicit then
	if scheme == CompareScheme.Intermediate then

Fixes and tweaks to implicit priority change scheme #21328

Fixes and tweaks to implicit priority change scheme #21328

Conversation

odersky commented Aug 5, 2024 • edited Loading

odersky commented Aug 5, 2024

WojciechMazur commented Aug 5, 2024

WojciechMazur commented Aug 5, 2024 • edited Loading

odersky commented Aug 5, 2024

odersky commented Aug 5, 2024

odersky commented Aug 6, 2024

EugeneFlesselle Aug 6, 2024

Choose a reason for hiding this comment

odersky Aug 6, 2024

Choose a reason for hiding this comment

odersky commented Aug 6, 2024 via email

EugeneFlesselle commented Aug 6, 2024

EugeneFlesselle Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

odersky Aug 6, 2024

Choose a reason for hiding this comment

EugeneFlesselle Aug 6, 2024

Choose a reason for hiding this comment

EugeneFlesselle Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

odersky Aug 6, 2024

Choose a reason for hiding this comment

EugeneFlesselle Aug 6, 2024

Choose a reason for hiding this comment

EugeneFlesselle Aug 6, 2024

Choose a reason for hiding this comment

odersky commented Aug 7, 2024

odersky commented Aug 5, 2024 •

edited

Loading

WojciechMazur commented Aug 5, 2024 •

edited

Loading

EugeneFlesselle Aug 6, 2024 •

edited

Loading

EugeneFlesselle Aug 6, 2024 •

edited

Loading