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

Implicit argument list modifier affects overload resolution with explicit arguments #10526

Open
milessabin opened this issue Sep 27, 2017 · 4 comments
Open

Implicit argument list modifier affects overload resolution with explicit arguments #10526

milessabin opened this issue Sep 27, 2017 · 4 comments
Labels
fixed in Scala 3 This issue does not exist in the Scala 3 compiler (https://github.com/lampepfl/dotty/) implicit overloading typelevel
Milestone
Backlog

Comments

@milessabin
Copy link

milessabin commented Sep 27, 2017 •
edited
Loading

The following fails with an ambiguous overload,

object Test extends App {
  class Foo
  class Bar extends Foo

  def overload(implicit foo: Foo): Unit = {}
  def overload(implicit bar: Bar): Unit = {}

  overload(new Bar)
}
/*
implicit-overload.scala:8: error: ambiguous reference to overloaded definition,
both method overload in object Test of type (implicit bar: Test.Bar)Unit
and  method overload in object Test of type (implicit foo: Test.Foo)Unit
match argument types (Test.Bar)
  overload(new Bar)
  ^
one error found
*/

However, remove the implicit argument list modifier and it compiles as expected,

object Test extends App {
  class Foo
  class Bar extends Foo

  def overload(foo: Foo): Unit = {}
  def overload(bar: Bar): Unit = {}

  overload(new Bar)
}

This is because the logic in Inferencer#isAsSpecific only compares the result types of method types with an implicit argument list.

I think that at least part of this logic is intended to handle implicit method selection rather than overload resolution per se, but is misplaced and should actually be moved to ImplicitSearch#improves where, as well as fixing this bug, we have opportunities to improve implicit method selection based on implicit argument lists.

This bug is also present in Dotty.
@milessabin
Copy link
Author

I think that the logic is there in an attempt to bring the behaviour with implicit arguments into line with the behaviour with default arguments.

@smarter smarter mentioned this issue Sep 27, 2017
Closed
@odersky
Copy link

odersky commented Sep 27, 2017 •
edited
Loading

Note that this will be likely affected by the future requirement to pass implicit arguments using explicitly. I.e. in the future you have to write:

overload.explicitly(new Bar)

I believe that doing this change will pave the way to a natural fix: If f is of a type consisting of one or more alternatives with implicit method types, then f.explicitly is of the type where all implicit method alternatives are turned to explicit methods. Afterwards, normal overloading resolution will do its thing.

@milessabin milessabin mentioned this issue Oct 17, 2017
Closed
@adriaanm adriaanm mentioned this issue Oct 30, 2017
Open
milessabin added a commit to milessabin/scala that referenced this issue Mar 6, 2018
@milessabin
Make implicit selection consistent with normal overload resolution.
61ef9a3 
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 added a commit to milessabin/scala that referenced this issue Mar 22, 2018
@milessabin
Make implicit selection consistent with normal overload resolution.
5f45da6 
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 added a commit to milessabin/scala that referenced this issue Mar 22, 2018
@milessabin
Make implicit selection consistent with normal overload resolution.
64114ce 
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 mentioned this issue Nov 22, 2018
Closed
@milessabin milessabin mentioned this issue Feb 13, 2019
Closed
@milessabin
Copy link
Author

milessabin commented Mar 4, 2019 •
edited
Loading

This appears to be fixed. Needs a test case to close.

@milessabin
Copy link
Author

Whoops! The issue is fixed in Dotty, not here: scala/scala3#3190.

@jcracknell jcracknell mentioned this issue May 8, 2019
Closed
@SethTisue SethTisue added this to the Backlog milestone Mar 13, 2023
@SethTisue SethTisue added the fixed in Scala 3 This issue does not exist in the Scala 3 compiler (https://github.com/lampepfl/dotty/) label Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
fixed in Scala 3 This issue does not exist in the Scala 3 compiler (https://github.com/lampepfl/dotty/) implicit overloading typelevel
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

Disentangle implicit-selection logic from specificity rules and make … milessabin/scala
3 participants
@milessabin @SethTisue @odersky