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.

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

Another given priority scheme to ensure transitivity #21325

Closed
wants to merge 2 commits into from

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Aug 3, 2024

It applies the following tweaks:

  • Do treat givens as higher priority than implicits. The previous logic was faulty, but is corrected now.
  • Drop special handling of NotGiven in prioritization. The previous logic pretended to do so, but was ineffective.
  • Do use the old priority scheme for implicit/implicit pairs.
  • Do use owner score for givens as a tie breaker if after all other tests we still have an ambiguity.

It applies the following tweaks:

 - Do treat givens as higher priority than implicits. The previous logic was faulty, but is corrected now.
 - Drop special handling of NotGiven in prioritization. The previous logic pretended to do so,
   but was ineffective.
 - Do use the old priority scheme for implicit/implicit pairs.
 - Do owner score for givens as a tie breaker if after all other tests we still have an ambiguity.
@odersky
Copy link
Contributor Author

odersky commented Aug 3, 2024

Alternative to #21305

@odersky
Copy link
Contributor Author

odersky commented Aug 3, 2024

@WojciechMazur Can we test this one as well with the OpenCB?

@odersky odersky marked this pull request as draft August 3, 2024 16:51
@WojciechMazur
Copy link
Contributor

I've started the OpenCB for 3.5.0-RC6 + this change so we can compare it directly with the results from alternative PR and don't need to care of other issues present on main. I should have results in ~3h

@WojciechMazur
Copy link
Contributor

WojciechMazur commented Aug 3, 2024

There seems to be less arnings in general when compared to previous solution

Given stats for 3.5.0-RC6 (baseline)
Given alternatives choice: Some(the second alternative) - 483
Given alternatives choice: Some(none - it's ambiguous) - 762
Given alternatives choice: Some(the first alternative) - 264
Projects with changed alternatives warnings - 23

Given stats for 3.5.0-RC6 + #21305
Given alternatives choice: Some(the second alternative) - 1868
Given alternatives choice: Some(none - it's ambiguous) - 21
Given alternatives choice: Some(the first alternative) - 831
Projects with changed alternatives warnings - 88

Given stats for 3.5.0-RC6 + this PR 
Given alternatives choice: Some(the second alternative) - 1437
Given alternatives choice: Some(none - it's ambiguous) - 22
Given alternatives choice: Some(the first alternative) - 215
Projects with changed alternatives warnings - 28

There is 1 new ambiguous choice in com-lihaoyi/upickle (it was not ambiguous in neither of other revisions)

Search [1]: ReadersVersionSpecific_this.Reader[upickle.Gadt2.IsDir[V]] : choice=none
    -  (upickle.Gadt2.IsDir.rw : [V²] (implicit evidence$1: upickle.default.ReadWriter[V²]): upickle.default.ReadWriter[upickle.Gadt2.IsDir[V²]] )
    -  (upickle.default.superTypeReader : [T, V² >: T] (implicit evidence$1: scala.deriving.Mirror.ProductOf[T], evidence$2: upickle.default.Reader[V²], evidence$3: scala.deriving.Mirror.SumOf[V²], x$1: scala.util.NotGiven[upickle.core.CurrentlyDeriving[V²]]): upickle.default.Reader[T] )

Here're links to each of the types:
ReadWriter
default.superTypeReader
Gadt2.IsDir and it's rw implicit instance

No new compilation failures in OpenCB, lemonlabsuk/scala-uri is now building successfully (failed previously)

Since we specialize implicits and generalize givens, it's logical that we can't
compare givens and implicits.
@odersky
Copy link
Contributor Author

odersky commented Aug 4, 2024

There is 1 new ambiguous choice in com-lihaoyi/upickle (it was not ambiguous in neither of other revisions)

OK, here's what goes on here.

  • superTypeReader is a given and isDir.rw is an implicit.
  • Both superTypeReaderandisDir.rw` are polymorphic
  • superTypeReader's type variables can be instantiated to match a supertype of isDir.rw's type. This would normally
    make isDir.rw chosen over superTypeReader except that isDir.rw is now an implicit and therefore loses the comparison. The result is that isDir.rw is not better than superTypeReader in the "wins type" contest.
  • In the other direction we alsp get a "not better" since isDir.rw's type variable cannot be instantiated to match superTypeReade's result at all.

@WojciechMazur
Copy link
Contributor

Based on the results in #21328 should I run additional OpenCB build with additional 5d58407 change?

@odersky
Copy link
Contributor Author

odersky commented Aug 5, 2024

No I think we should hold off with l 5d58407 unless the other PR #21328 is disappointing. I think the other PR is both nicer and might solve more problems.

@odersky
Copy link
Contributor Author

odersky commented Aug 6, 2024

Superseded by #21328

@odersky odersky closed this Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants