Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refine implicit priority change warnings #21045

Merged
merged 5 commits into from
Jul 8, 2024
Merged

Refine implicit priority change warnings #21045

merged 5 commits into from
Jul 8, 2024

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Jul 5, 2024

Fixes #21036
Fixes #20572

@EugeneFlesselle EugeneFlesselle mentioned this pull request Jul 5, 2024
Closed
EugeneFlesselle
EugeneFlesselle requested changes Jul 5, 2024
View reviewed changes
Copy link
Contributor

@EugeneFlesselle EugeneFlesselle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise looks right to me 👍

compiler/src/dotty/tools/dotc/typer/Implicits.scala Show resolved Hide resolved
@EugeneFlesselle
Copy link
Contributor

We now miss the following change:

trait A[T]
trait B[T] extends A[T]

given b[T]: B[T] = ???
given a[T]: A[T] = ???

val y = summon[A[String]] // changes from `b` in 3.5 to `a` 3.6 as expected but without warning in either

@EugeneFlesselle
Copy link
Contributor

It seems we really do need to know whether the cand we are filtering out early would have been a SearchSuccess to decide whether or not to report a warning.

I guess an alternative solution would be to just disable the filtering of candidates for which compareAlternatives changes between the two modes.

EugeneFlesselle
EugeneFlesselle approved these changes Jul 8, 2024
View reviewed changes
Copy link
Contributor

@EugeneFlesselle EugeneFlesselle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was tricky to do efficiently, but we got a good solution after all!

compiler/src/dotty/tools/dotc/typer/Implicits.scala
if alt1.ref eq alt2.ref then 0
else if alt1.level != alt2.level then alt1.level - alt2.level
else
var cmp = comp(using searchContext())
val sv = Feature.sourceVersion
if sv.stable == SourceVersion.`3.5` || sv == SourceVersion.`3.6-migration` then
if isWarnPriorityChangeVersion(sv) then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose sv is constant and isWarnPriorityChangeVersion could then be a val

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I use it below as well in the assert that we originally commented out.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I meant even isWarnPriorityChangeVersion(Feature.sourceVersion) would be constant between calls of compareAlternatives. But no big deal either way, this is still ready to merge for me

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but be need sourceVersion as sv separately in compareAlternatives.

@odersky odersky merged commit 0e36424 into scala:main Jul 8, 2024
24 checks passed
@odersky odersky deleted the fix-21036 branch July 8, 2024 20:31
@odersky odersky added this to the 3.5.0 milestone Jul 8, 2024
@odersky odersky added the backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. label Jul 8, 2024
@odersky
Copy link
Contributor Author

odersky commented Jul 8, 2024

This should be part of 3.5.0 RC4.

@WojciechMazur WojciechMazur added backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" and removed backport:nominated If we agree to backport this PR, replace this tag with "backport:accepted", otherwise delete it. labels Jul 10, 2024
@WojciechMazur WojciechMazur mentioned this pull request Jul 10, 2024
Merged
WojciechMazur added a commit that referenced this pull request Jul 11, 2024
@WojciechMazur
Backport "Refine implicit priority change warnings" to 3.5.0 (#21171)
744b1b7 
Backports #21045 to 3.5.0-RC4
@WojciechMazur WojciechMazur added backport:done This PR was successfully backported. backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" and removed backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" backport:done This PR was successfully backported. labels Jul 11, 2024
@WojciechMazur WojciechMazur modified the milestones: 3.5.0, 3.5.1 Jul 12, 2024
@WojciechMazur WojciechMazur mentioned this pull request Jul 12, 2024
Merged
WojciechMazur added a commit that referenced this pull request Jul 14, 2024
@WojciechMazur
Backport "Refine implicit priority change warnings" to 3.5.1 (#21182)
db4a1a1 
Backports #21045 to 3.5.1-RC1
@WojciechMazur WojciechMazur added backport:done This PR was successfully backported. and removed backport:accepted This PR needs to be backported, once it's been backported replace this tag by "backport:done" labels Jul 14, 2024
EugeneFlesselle added a commit to dotty-staging/dotty that referenced this pull request Aug 6, 2024
@EugeneFlesselle
Fix healAmbiguous to compareAlternatives with disambiguate = true
0de4a2f 
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, we could heal an ambiguous SearchFailure with a candidate which was
considered better only under the alternative given prioritization scheme,
see scala#21045 for details.
EugeneFlesselle added a commit to dotty-staging/dotty that referenced this pull request Aug 6, 2024
@EugeneFlesselle
Fix healAmbiguous to compareAlternatives with disambiguate = true
057e7a9 
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, we could heal an ambiguous SearchFailure with a candidate which was
considered better only under the alternative given prioritization scheme,
see scala#21045 for details.
EugeneFlesselle added a commit to dotty-staging/dotty that referenced this pull request Aug 6, 2024
@EugeneFlesselle
Fix healAmbiguous to compareAlternatives with disambiguate = true
1e93f22 
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, we could heal an ambiguous SearchFailure with a candidate which was
considered better only under the alternative given prioritization scheme,
see scala#21045 for details.
EugeneFlesselle added a commit to dotty-staging/dotty that referenced this pull request Aug 6, 2024
@EugeneFlesselle
Fix healAmbiguous to compareAlternatives with disambiguate = true
dcf90e1 
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
EugeneFlesselle added a commit to dotty-staging/dotty that referenced this pull request Aug 7, 2024
@EugeneFlesselle
Fix healAmbiguous to compareAlternatives with disambiguate = true
0361991 
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
EugeneFlesselle added a commit that referenced this pull request Aug 7, 2024
@EugeneFlesselle
Fixes and tweaks to implicit priority change scheme (#21339)
590b496 
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 pushed a commit to WojciechMazur/dotty that referenced this pull request Aug 7, 2024
@EugeneFlesselle @WojciechMazur
Fix healAmbiguous to compareAlternatives with disambiguate = true
d439b58 
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
WojciechMazur pushed a commit to WojciechMazur/dotty that referenced this pull request Aug 12, 2024
@EugeneFlesselle @WojciechMazur
Fix healAmbiguous to compareAlternatives with disambiguate = true
127d64b 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EugeneFlesselle EugeneFlesselle EugeneFlesselle approved these changes

Assignees

@EugeneFlesselle EugeneFlesselle

Labels
backport:done This PR was successfully backported.
Projects
None yet
Milestone
3.5.1
3 participants
@odersky @EugeneFlesselle @WojciechMazur