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

Take implicit parameters into account for is-as-specific computations #5925

Merged
merged 5 commits into from Feb 22, 2019

Conversation

Projects
None yet
2 participants
@odersky
Copy link
Contributor

odersky commented Feb 14, 2019

Number of implicit parameters is applied as a final tie break, if
two alternatives would be otherwise equally specific.

I tried to roll them into type specificity first, but this fails
the dotc bootstrap where we have:

 def name(implicit ctx: Context): Name  in class Denotation

 val name: Name                         in subclass SymDenotation

The val name wins, since it is defined in a subclass and the result types
are the same. If we let the type (implicit ctx: Context): Name win over
Name this would be ambiguous instead.

@odersky

This comment has been minimized.

Copy link
Contributor Author

odersky commented Feb 14, 2019

Based on #5887. First new commit is 4d6b82e.

@odersky odersky force-pushed the dotty-staging:change-implicit-specifity branch from c3a7b23 to b1a0234 Feb 14, 2019

@milessabin

This comment has been minimized.

Copy link
Contributor

milessabin commented Feb 14, 2019

@odersky take a look at the logic I used here to deal with the specificity problem you mention in the description above ... search for nullaryImplicitArgs.

@odersky

This comment has been minimized.

Copy link
Contributor Author

odersky commented Feb 15, 2019

@milessabin Yes, that's what I implemented first. But it runs afoul of the problem I mentioned. The difference between the approaches is that I now use implicit parameters only as a final tie breaker, after both type specificity and location are taken into account.

@milessabin

This comment has been minimized.

Copy link
Contributor

milessabin commented Feb 15, 2019

@odersky If I can make it work for that case would you be open to going in that direction?

@odersky odersky force-pushed the dotty-staging:change-implicit-specifity branch from 6264a67 to b17435f Feb 15, 2019

@odersky

This comment has been minimized.

Copy link
Contributor Author

odersky commented Feb 15, 2019

@odersky If I can make it work for that case would you be open to going in that direction?

Possibly, but I quite like the simplicity of the current solution (both implementation and spec-wise). What would we gain by changing it?

@odersky

This comment has been minimized.

Copy link
Contributor Author

odersky commented Feb 15, 2019

Here's the current text:

  1. The rule for picking a most specific alternative among a set of overloaded or implicit
    alternatives is refined to take the number of inferable parameters into account. All else
    being equal, an alternative that takes more inferable parameters is taken to be more specific
    than an alternative that takes fewer. The following paragraph in the SLS is affected by this change:

    Original version:

    An alternative A is more specific than an alternative B if the relative weight of A over B is greater than the relative weight of B over A.

    Modified version:

    An alternative A is more specific than an alternative B if

    • the relative weight of A over B is greater than the relative weight of B over A, or
    • the relative weights are the same and A takes more inferable parameters than B.
@milessabin

This comment has been minimized.

Copy link
Contributor

milessabin commented Feb 17, 2019

What would we gain by changing it?

Without it we miss out on things like this (from the Scala 2 PR),

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)
}
@odersky

This comment has been minimized.

Copy link
Contributor Author

odersky commented Feb 18, 2019

OK, I see now. We need not just the number but also the types of the implicit parameters to influence priority. We could probably do this also as a final tie break, however.

I like to push these tie breaks back in the sequence of decisions not just for backwards compatibility but also for complexity. Overloading comparisons can be a significant part of compile-time (I believe for ScalaTest ~20%) and they can explode combinatorically. The current implementation tries very hard to minimize the number of specificity comparisons for types. If we do more complex checks only as a last resort when we would report an ambiguity otherwise, we know that they will be called only rarely.

@odersky

This comment has been minimized.

Copy link
Contributor Author

odersky commented Feb 18, 2019

A modified^2 version could be:

  1. An alternative A is more specific than an alternative B if

    • the relative weight of A over B is greater than the relative weight of B over A, or
    • the relative weights are the same and A takes more inferable parameters than B, or
    • the relative weights and the number of inferable parameters are the same and
      A is more specific than B if all inferable parameters in either alternative are replaced by regular parameters.

Would that work?

@milessabin

This comment has been minimized.

Copy link
Contributor

milessabin commented Feb 18, 2019

Where "relative weight" is the specificity of the result type? Yes, I think that works :-)

@odersky

This comment has been minimized.

Copy link
Contributor Author

odersky commented Feb 18, 2019

Where "relative weight" is the specificity of the result type? Yes, I think that works :-)

Relative weight is the outcome of the combined specificity / ownership test. You get one weight point for "as specific as" and one weight point for "owned by a subclass (or its companion object)".

@odersky odersky force-pushed the dotty-staging:change-implicit-specifity branch from b17435f to 2234c2b Feb 18, 2019

odersky added some commits Feb 14, 2019

Take implicit parameters into account for is-as-specific computations
Number of implicit parameters is applied as a final tie break, if
two alternatives would be otherwise equally specific.

I tried to roll them into type specificity first, but this fails
the dotc bootstrap where we have:

     def name(implicit ctx: Context): Name  in class Denotation

     val name: Name                         in subclass SymDenotation

The `val name` wins, since it is defined in a subclass and the result types
are the same. If we let the type `(implicit ctx: Context): Name` win over
`Name` this would be ambiguous instead.
Update doc on implicit resolution changes
Update doc on implicit resolution changes to take the previous changes
in #5887 into account.
Widen result types of implied methods for specificity testing
When testing specificity of methods, widen result type of
synthetic implied methods.

I tried widening the inferred result type of these methods
already in Namer instead, but this causes problems since
it can hide extension methods.
Refine disambiguation rules
If both alternatives have same weights and same number of implicit parameters,
treat them as normal (non-implicit) methods and try again.

@odersky odersky force-pushed the dotty-staging:change-implicit-specifity branch from 2234c2b to 2f00820 Feb 18, 2019

@odersky

This comment has been minimized.

Copy link
Contributor Author

odersky commented Feb 21, 2019

@milessabin The changed spec is in
docs/docs/reference/changed-features/implicit-resolution.md

Can you take a look whether we can leave it like this?

@odersky odersky requested a review from milessabin Feb 21, 2019

@milessabin
Copy link
Contributor

milessabin left a comment

This is absolutely spot on ... it's a huge improvement over Scala 2 and addresses pretty much all of my issues :-)

@odersky odersky merged commit 019e45d into lampepfl:master Feb 22, 2019

2 checks passed

CLA User signed CLA
Details
continuous-integration/drone/pr the build was successful
Details

@allanrenucci allanrenucci deleted the dotty-staging:change-implicit-specifity branch Feb 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.