Skip to content

Compendium of several PRs for testing with the new collections before M4#6378

Closed
milessabin wants to merge 5 commits intoscala:2.13.xfrom
milessabin:topic/collections-compendium
Closed

Compendium of several PRs for testing with the new collections before M4#6378
milessabin wants to merge 5 commits intoscala:2.13.xfrom
milessabin:topic/collections-compendium

Conversation

@milessabin
Copy link
Contributor

@milessabin milessabin commented Mar 5, 2018

@scala-jenkins scala-jenkins added this to the 2.13.0-M4 milestone Mar 5, 2018
@milessabin milessabin added the WIP label Mar 5, 2018
@milessabin milessabin self-assigned this Mar 5, 2018
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.
@milessabin milessabin force-pushed the topic/collections-compendium branch from fb578a8 to fd80ed6 Compare March 22, 2018 22:33
@milessabin
Copy link
Contributor Author

Rebased.

@szeiger
Copy link
Contributor

szeiger commented May 3, 2018

Does this PR still serve any purpose now that the new collections have been merged into 2.13.x or should it be closed?

@milessabin
Copy link
Contributor Author

No. Closing now.

@milessabin milessabin closed this May 3, 2018
@SethTisue SethTisue removed this from the 2.13.0-M4 milestone May 4, 2018
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.

4 participants