Compendium of several PRs for testing with the new collections before M4#6378
Closed
milessabin wants to merge 5 commits intoscala:2.13.xfrom
Closed
Compendium of several PRs for testing with the new collections before M4#6378milessabin wants to merge 5 commits intoscala:2.13.xfrom
milessabin wants to merge 5 commits intoscala:2.13.xfrom
Conversation
This is a backport of the following Dotty change to scalac, scala/scala3@8954026 As in the Dotty implementation the specificity comparison which is used for overload resolution and implicit selection is now performed as-if all contravariant positions in top level type constructors were covariant. Currently this breaks test/files/run/t2030.scala in exactly the same way as in Dotty as reported here, scala/scala3#1246 (comment) and in this PR it has been changed to a neg test. However, test/files/run/t2030.scala passes when this PR is combined with, scala#6139 so if/when that is merged it can be switched back to a pos test. Fixes scala/bug#2509. Fixes scala/bug#7768.
Partial unification is now enabled unless -Xsource:2.12 is specified. The -Ypartial-unification flag has been removed and the -Xexperimental option, which is now redundant, has been deprecated.
This is an attempt to make implicit selection rules align more closely with people's intuitions and the spec's aspirations that it be consistent with normal overload resolution rules. It also fixes scala/bug#10526. Candidate implicits are ordered via Infer#isStrictlyMoreSpecific which delegates to Infer#isAsSpecific. Prior to this commit the latter treats all methods with an implicit argument list as if they were nullary methods, leading to scala/bug#10526 ... I believe that the motivation for this behaviour was to allow methods which are intended to appear to users to be nullary to have implicit arguments without affecting their overload resolution behaviour wrt same-named methods which are actually nullary. Whilst this does the right thing in some circumstances, it leads to oddities such as scala/bug#10526 and also means that the specificity test used to order and select implicits is very crude, in effect only considering the result type of an implicit method, not its implicit arguments. This commit addresses these issues by making the "treat implicit methods as nullary" logic explicitly flagged and enabling it only at the relevant call site in Inferencer#inferExprAlternative. Normal overloading rules are unchanged, and the implicit selection logic is extracted out to ImplicitSearch#Improves. There the relative ordering of a pair of candidate implicits is computed as follows, 1. The candidates are compared by their result type, the most specific by normal overload rules being selected (this is consistent with the behaviour prior to this commit). 2. If (1) is a tie then we compare the lengths of the implicit argument lists of the candidates (if any). The candidate with the longest argument list wins (this matches users intuitions that methods with more implicit arguments are "more demanding" hence more specific). 3. If (2) is a tie then we have a pair of candidates with implicit argument lists of equal length and which are both equally specific wrt their result type/the expected type. We now apply the full normal overloading resolution rules and choose the most specific. This change makes combining implicits from multiple implicit scopes a great deal more straightforward, since we can now rely on more than just the result type of the implicit definition to guide implicit selection. With this commit the following works as expected, class Show[T](val i: Int) object Show { def apply[T](implicit st: Show[T]): Int = st.i implicit val showInt: Show[Int] = new Show[Int](0) implicit def fallback[T]: Show[T] = new Show[T](1) } class Generic object Generic { implicit val gen: Generic = new Generic implicit def showGen[T](implicit gen: Generic): Show[T] = new Show[T](2) } object Test extends App { assert(Show[Int] == 0) assert(Show[String] == 1) assert(Show[Generic] == 2) // showGen beats fallback due to longer argument list } Thanks to the use of normal overload resolution rules we also have the ability to use a "phantom" implicit parameter with arguments ordered by subtyping to explicitly prioritize implicit definitions, something for which language extensions of one form or another have occasionally been proposed, class Low object Low { implicit val low: Low = new Low } class Medium extends Low object Medium { implicit val medium: Medium = new Medium } class High extends Medium object High { implicit val high: High = new High } class Foo[T](val i: Int) object Foo { def apply[T](implicit fooT: Foo[T]): Int = fooT.i implicit def foo[T](implicit priority: Low): Foo[T] = new Foo[T](0) implicit def foobar[T](implicit priority: Low): Foo[Bar[T]] = new Foo[Bar[T]](1) implicit def foobarbaz(implicit priority: Low): Foo[Bar[Baz]] = new Foo[Bar[Baz]](2) } class Bar[T] object Bar { implicit def foobar[T](implicit priority: Medium): Foo[Bar[T]] = new Foo[Bar[T]](3) implicit def foobarbaz(implicit priority: Medium): Foo[Bar[Baz]] = new Foo[Bar[Baz]](4) } class Baz object Baz { implicit def baz(implicit priority: High): Foo[Bar[Baz]] = new Foo[Bar[Baz]](5) } object Test extends App { assert(Foo[Int] == 0) assert(Foo[Bar[Int]] == 3) assert(Foo[Bar[Baz]] == 5) } This commit changes the outcome of only one partest (test/files/neg/t7289_status_quo.scala) which is intended to exactly capture the current failure behaviour in a specific implicit resolution scenario ... I believe the new (successful) behaviour is an improvement on the old.
fb578a8 to
fd80ed6
Compare
Contributor
Author
|
Rebased. |
Contributor
|
Does this PR still serve any purpose now that the new collections have been merged into 2.13.x or should it be closed? |
Contributor
Author
|
No. Closing now. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is single PR amalgamating,
Stabilizing definitions fixes #6312mergedTo allow them to be tested jointly with the new collections prior to 2.13.0-M4.