Tweaks to given priority #21305
Tweaks to given priority #21305
Conversation
- Don't treat givens as higher priority than implicits. The previous logic was faulty anyway. - Drop special handling of NotGiven in prioritization. The previous logic pretended to do so, but was ineffective. - Don't use old priority in general for implicit/implicit pairs. This would make upgrading to givens a constant struggle. But do use it as a tie breaker in the case of ambiguities. - Also use owner score as a tie breaker if after all other tests we still have an ambiguity.
Previously warnings were produced but not shown since at the same position we already have an ambiguity error. We now add the note to the error message.
|
This PR reverts the changes to PPrint that turned a given back into an implicits. These are no longer needed. On the other hand, we do need a change to a scala-collection-compat test. |
|
I also added a commit that will show priority changes under 3.6-migration as additions to error messages. Previously these were usually swallowed since a warning was suppressed by the ambiguity error at the same position. |
There was a problem hiding this comment.
The logic behind the changes seems well-grounded to me.
We still need to see to what the extend the openCB is remains impacted by the changes in given prioritization (cc @WojciechMazur).
On a minor note, we should update the internal doc to reflect everything as you summarized:
- At
compareAlternatives: use old priority in the case of ambiguities in new priority - At
Applications#compare: use owner score as a tie breaker if after all other tests we still have an ambiguity- and update its' line still mentioning "A1's type is more specific than A2's type"
| result match | ||
| case result: SearchFailure => | ||
| result.reason match | ||
| case ambi: AmbiguousImplicits => ambi.priorityChangeWarning = msg |
There was a problem hiding this comment.
Good catch that these used to get swallowed up!
Couldn't it also be that there are multiple "critical" msgs found in result, in which case we way be overwriting only the last one into ambi.priorityChangeWarning.
There was a problem hiding this comment.
Good point. We should make a List.
| if result != 0 then result | ||
| else if strippedType1 eq fullType1 then | ||
| if strippedType2 eq fullType2 | ||
| then drawOrOwner // no implicits either side: its' a draw |
There was a problem hiding this comment.
Why do drawOrOwner here, as opposed to in compareWithTypes as you had in 39c49ad ?
We may also want to use the owner as the final tie breaker in the case where: we have an ambiguity with the strippedTypes, and that both alternatives have implicit parameters, and that the fullTypes are still ambiguous.
There was a problem hiding this comment.
It turns out there are counter-examples for both schemes. If we do drawOrOwner here, StringFormatterTest does the right thing, but slick-migrations does not work anymore. If we do drawOrOwner in compareTypes, slick-migratons works but StringFormatterTest fails. The question is what do we prioritize case of a draw? The owner or cases without additional implicit parameters? I'll try the other scheme as well. At least StringFormatterTest has an easy fix.
Co-authored-by: Eugene Flesselle <[email protected]>
|
Based on There's one more issue found in the OpenCB that's present in this PR but not in latest Nightly trait Migration
object Migration:
implicit class MigrationConcat[M <: Migration](m: M):
def &[N <: Migration, O](n: N)(implicit ccm: CanConcatMigrations[M, N, O]): O = ???
trait ReversibleMigration extends Migration
trait MigrationSeq extends Migration
trait ReversibleMigrationSeq extends MigrationSeq with ReversibleMigration
trait ToReversible[-A <: Migration]
object ToReversible:
implicit val reversible: ToReversible[ReversibleMigration] = ???
class CanConcatMigrations[-A, -B, +C]
trait CanConcatMigrationsLow:
implicit def default[A <: Migration, B <: Migration]: CanConcatMigrations[A, B, MigrationSeq] = ???
object CanConcatMigrations extends CanConcatMigrationsLow:
implicit def reversible[A <: Migration, B <: Migration](implicit reverseA: ToReversible[A],
reverseB: ToReversible[B]): CanConcatMigrations[A, B, ReversibleMigrationSeq] = ???
@main def Test =
val rm: ReversibleMigration = ???
val rms = rm & rm & rm
summon[rms.type <:< ReversibleMigrationSeq] // error Cannot prove that (rms : slick.migration.api.MigrationSeq) <:< slick.migration.api.ReversibleMigrationSeq. |
I'll try to minimize it @\odersky |
|
In summary there seems to be 4 failures found in OpenCB that are not present in Nightly. I'll try to provide reproducers for these
|
|
The issue based on trait Migration
trait Migration2 extends Migration
class Cov[+C]
object Cov:
implicit def default: Cov[Migration] = ???
implicit def reversible: Cov[Migration2] = ???
def foo[O](using Cov[O]): O = ???
@main def Test =
val f1 = foo
val f2: Migration2 = f1 // errorIt used to pick Modifying the last line of the reproducer to |
|
|
…ents This makes slick-migration-api-example work and makes the original scala-uri.scala fail. See the comment in neg/scala-uri.scala for why this is so. It also needs a change in the givens in dotty.tools.reporting.Formatting. The motivation for honoring owner score over the others is that it is often used for explicit prioritization, so we should take it into account more than other secondary criteria.
|
OK. So the last commit reverts to the previous disambiguation by owner before considering other criteria such as whether an alternative is conditional or unconditional. This fixes |
|
About the 4 reproducers: slick-migration-api should be fixed by latest commit, scala-collection-compat is a known case, and fixed in small CB. The others need more investigation. Not sure whether the latest commit would fix them, too. |
|
|
I've tried to test this PR on top of 3.5.0-RC6 to test potential RC7, there seems to be one unresolved issue that poped up in the previous PR #21273 (comment) related to |
|
Yes, the scala-uti problem is known (and fixable on their side). I mention it in the last commit message: Disambiguate by owner score before considering further implicit argum… This makes slick-migration-api-example work and makes the original scala-uri.scala fail. The motivation for honoring owner score over the others is that it is often used for explicit |
|
To my surprise this change is now leading to less ambiguous implicit, but in general there is ~1k more warnings in more projects, based on ~1570 projects I wonder if that's something we should investigate? Affected projects: Choice warnings in 3.5.0-RC6-bin-ef43053-SNAPSHOT: |
|
Great to have data! I have a theory which might explain it, but we need to do further investigations to be sure. We now have two new disambiguation measures:
Previously we always resolved implicit/implicit pairs with the old rules. But that would give a constant risk for priority changes when these implicits are rewritten to be givens. So this means:
I can do another experimental tweak that disables owner score disambiguation for implicit/implicit pairs so that then the old rules would kick in. Let's see how that performs. |
|
The last commit passes the tests, but I think we need to back out of this line of thought. The problem is that we can do only one of two mutually exclusive things:
If we do both, the compareAlternatives relation will no longer be a partial order since transitivity will fail. An example, which holds for the previous few commits:
Without transitivity, the priority computation for implicits becomes dependent on the order in which we compare different implicits. That we cannot have. I'd like to try a variant of this PR which does the new owner score disambiguation but keeps the "given beats implicit" rule. The original code in 3.4 should have prioritized givens over implicits but due to a one character typo failed to do so. |
I think we could address the transitivity issue, similarly to what we had discussed previously, by making Nevertheless, FWIW I think the second option alone (i.e. as in 3d9ce8e) would be best. Even if the first option (combined with the new owner score disambiguation) may give fewer changes in resolution, we would only be delaying those warnings to when old-style implicits are migrated to new-style givens. In addition, having different rules for implicits and givens would likely make predicting and/or debugging implicit resolution in large projects much trickier. In any case we can use the last commit 62acaf1 to validate the hypothesis of #21305 (comment). |
|
But the second option alone is supposed to be even worse than this: I am not sure this degree of breakage is acceptable. The alternative, ie. treating implicits and givens separately has two shortcomings:
I think the main difference is between library authors and library users. Library authors will have to be more careful if implicitis and givens are treated differently. But with work they can keep the old priorities. Library users will be safer since the many libraries that currently use implicits continue to be usable in the same way. All of these needs to be backed up by running OpenCB on the alternative and comparing with the results here. |
Right indeed. We should also keep as an option making mostGeneral resolution a |
|
Superseded by #21328 |
This PR is a consolidation of #21273, without the going back and forth.