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

Change implicit resolution rule wrt implicit parameters #6071

Merged
merged 13 commits into from Mar 30, 2019

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Mar 11, 2019

The method with fewer implicit parameters now wins over methods with more parameters.
The reason for the change is i3430.scala, with

Nil.sum

Here, orderings for any type are eligible implied instances for sum's implicit Ordering
parameter. If we prioritize arguments with implicit parameters, we have to try all
tuple orderings which themselves take as many implicit parameters as the tuple has elements.
This leads to an infinite recursion with very broad fanout, so the observed effect is that
the compiler hangs. What we would expect instead is an implicit ambiguity error.

By prioritizing shorter implicit parameter lists, we avoid this problem. A previous PR #6034 used a
different strategy, taking implicits parameters only into account if the result types of implied instances
were unifiable. But that seems equally ad hoc, and more fragile than the current rule

To summarize: With the currently implemented rule we can add weaker alternatives to a set of implied instances by adding implicit parameters. Previously adding implicit parameters gave stronger alternatives. We can do that now as well, but using a different trick based on intersecting the implicit result type with some opaque type that erases to Any. This is shown in run/implicit-specificity.scala.

Between a set of alternatives with the same number of implicit parameters nothing has changed: We still prefer the alternative with the most specific parameters.

@odersky
Copy link
Contributor Author

odersky commented Mar 17, 2019

@milessabin We discussed at the SIP meeting whether we should drop number of implicit parameters as a disambiguation criterion. But after thinking about it, I believe there are two reasons to keep it like this.

  1. It corresponds to normal overloading rules. Here's something I added to the tests:
  def f: Int = 1
  def f(x: Int): Int = x

  val x = f
  val x1: Int = x

This shows that values take precedence over methods. By analogy methods without implicit parameters should take precedence over methods with implicit parameters since that would be the case anyway in the next step where we compare as if implicit parameters were normal parameters. Normal overloading rules do not have an analogue that compares number of parameters, but if they had one, preferring methods with fewer parameters would be consistent with the rule above.

  1. It provides a useful way to install a fallback with lower priority than a number of existing alternatives: Just give it enough implicit parameters. We already have a way to install an alternative with a higher priority than a set of existing ones: Let it return a more specific type. This can always be achieved by &-ing with an opaque type alias of Any.

@odersky
Copy link
Contributor Author

odersky commented Mar 26, 2019

After thinking even more about it, I now believe we should revise the rule. The idea of using implicits as a priority mechanism is so that we can write something like this:

  class LowPrio
  class MidPrio extends LowPrio
  class HighPrio extends MidPrio
  implied for LowPrio
  implied for MidPrio
  implied for HighPrio

  implied fallback given LowPrio for A
  implied standard given MidPrio for A
  implied specialized given HighPrio for A

(usually in different modules, of course). This is a good alternative to the previous priorization by owner subclassing since it places no demands on where implied instances are put and is stable under aliasing/forwarding.

But to do this reliably it should also work if some of the alternatives take additional implicit parameters. E.g.

  implied standard given MidPrio given B for A

The "more implicit parameters is better" rule would make standard of higher priority than specialized, which defeats the purpose. So I propose to revise as follows:

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 no implicit parameters but B does, or

  • the relative weights are the same, both A and B take implicit parameters, and
    A is more specific than B if all implicit parameters in either alternative are
    replaced by regular parameters.

Besides being more useful, this new version is also in better alignment to what happens in the normal overloading case.

@odersky
Copy link
Contributor Author

odersky commented Mar 26, 2019

@milessabin Latest comment is implemented in 9881529.

Copy link
Contributor

@milessabin milessabin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@odersky Yes, I think this works.

@milessabin
Copy link
Contributor

On reflection, perhaps not ... counterexample on its way ...

@milessabin milessabin self-requested a review March 27, 2019 15:23
@odersky
Copy link
Contributor Author

odersky commented Mar 28, 2019

counterexamples seems to have got lost on its way 😉?

@milessabin
Copy link
Contributor

Sorry, still brewing. A ton of stuff seems to have broken for me recently and I'm trying to tease out exactly what and why.

@milessabin
Copy link
Contributor

I will say that I'm finding the new rules extremely confusing.

I've not been able to construct a useful example which uses the class Low; class Mid extends Low; class High extends Mid pattern or which reduces priority by adding an additional implicit argument, at least not in cases which the (intended to be) lowest priority implicit has no essential arguments (other than ones used to adjust priority) and the (intended to be) higher priority implicit has one or more essential arguments. I noticed that all your tests have implicits with no non-priority essential arguments.

Can we consider back-tracking a bit? I think we might have given up on the "more arguments is more specific" rule too quickly. In the Nil.min case we need an Ordering[Nothing] ... why are orderings for tuples even considered in this case, given that Ordering is invariant? Is Dotty missing some of the early pruning I added to scalac a while back (motivated by performance, but I think it should deal with this case too)?

@odersky
Copy link
Contributor Author

odersky commented Mar 28, 2019

in cases which the (intended to be) lowest priority implicit has no essential arguments (other than ones used to adjust priority) and the (intended to be) higher priority implicit has one or more essential arguments.

With essential arguments I assume you mean implicit ones? In that case the essential arguments must come in an implicit parameter section following the priority arguments. E.g.

implied lowest given Priority.Low for T
implied highest given Priority.High given EssentialArgument for T

Nil.min case we need an Ordering[Nothing]

No, we need an ordering for an undetermined type parameter T. T is not instantiated to Nothing at that point.

The method with fewer parameters wins now over methods with more parameters.
The reason for the change is i3430.scala, with

    Nil.sum

Here, orderings for any type are eligible implied instances for `sum`'s implicit `Ordering`
parameter. If we prioritize arguments with implicit parameters, we have to try all
tuple orderings which themselves take as many implicit parameters as the tuple has elements.
This leads to an infinite recursion with very broad fanout, so the observed effect is that
the compiler hangs.

By prioritizing shorter implicit parameter lists, we avoid this problem.
After thinking even more about it, I now believe we should revise the rule. The idea of using implicits as a priority mechanism is so that we can write something like this:
```
  class LowPrio
  class MidPrio extends LowPrio
  class HighPrio extends MidPrio
  implied for LowPrio
  implied for MidPrio
  implied for HighPrio

  implied fallback given LowPrio for A
  implied standard given MidPrio for A
  implied specialized given HighPrio for A
```
(usually in different modules, of course). This is a good alternative to the previous priorization by owner subclassing since it places no demands on where implied instances are put and is stable under aliasing/forwarding.

But to do this reliably it should also work if some of the alternatives take additional implicit parameters. E.g.
```
  implied standard given MidPrio given B for A
```
The "more implicit parameters is better" rule would make standard of higher priority than specialized, which defeats the purpose.
@milessabin
Copy link
Contributor

milessabin commented Mar 28, 2019

In that case the essential arguments must come in an implicit parameter section following the priority arguments. E.g.

Ahh, right. So this won't be available for implicit methods until multiple implicit parameter lists are supported?

T is not instantiated to Nothing at that point.

Right, but it could be instantiated earlier (that's what my scalac change does). If we can consider literally any Ordering then it's not just things like tuples which can blow up.

@odersky
Copy link
Contributor Author

odersky commented Mar 28, 2019

Ahh, right. So this won't be available for _implicit _ methods until multiple implicit parameter lists are supported?

Exactly. And now I understand why you were confused. Multiple implicit parameter lists were not handled correctly in overloading resolution but I just fixed that. implicied-specifity.scala contains a test
for what I menioned:

implied for Arg

class Bam(val str: String)
implied lo given Low for Bam("lo")
implied hi given High given Arg for Bam("hi")

assert(the[Bam].str == "hi")

@milessabin
Copy link
Contributor

OK, that looks a lot better.

That gives us explicit prioritization. You also suggested that it would be possible to reduce the priority of an implicit by adding additional dummy arguments. Again I couldn't make that work with this PR ... is it still supposed to be possible?

The test previously compiled by accident due to the "more implicit parameters win" rule.
Once that rule got changed it should have failed, but it seems the error (illegal
forward reference) was not detected. Rebasing this commit on latest master detects
the error:
```
  implicit val co_i: Conversion[Char, Position[CharSequence]] = the[Conversion[Char, Position[CharSequence]]]
                                                                                                             ^
                                             co_i is a forward reference extending over the definition of co_i
```
The fix to the test avoids the error again.
@odersky
Copy link
Contributor Author

odersky commented Mar 28, 2019

That gives us explicit prioritization. You also suggested that it would be possible to reduce the priority of an implicit by adding additional dummy arguments. Again I couldn't make that work with this PR ... is it still supposed to be possible?

If you add implicit parameters to something that did not have them before that works. Example:

class Bam2(val str: String)
implied lo2 given Low for Bam2("lo")
implied mid2 given High given Arg for Bam2("mid")
implied hi2 for Bam2("hi")
assert(the[Bam2].str == "hi")

But it currently does not work if the normal implicit already takes implicit parameters and we want to add implicit with reduced priority. We could probably make that work in the same way by just adding arbitrary implicit parameters. I can give it a try.

@milessabin
Copy link
Contributor

The main case for this would be for a fallback, which typically wouldn't have implicit arguments itself, eg.,

implicit def high[T](implicit arg: Arg[T]): Foo[T]
implicit def low[T]: Foo[T]

We want high selected if an Arg[T] is available, otherwise low. As is, low will be selected. If we add a dummy to low it creates an ambiguity, and when I was experimenting with it yesterday adding a second dummy argument to low didn't help either (ambiguity again).

@odersky
Copy link
Contributor Author

odersky commented Mar 28, 2019

We want high selected if an Arg[T] is available, otherwise low.

I fear that's beyond all our approaches. We always determine applicability without knowing whether implicit arguments can be fulfilled or not. I believe the only exception to this rule is the current scheme where we first do a contextual search and only if that fails we do a type based search. So if low is in the type scope and high in the contextual scope, that works, currently. Except we wanted to drop that rule... Also, I think that's not what you want, because low typically comes from an import, right?

It seems implied match is the only way to solve this.

@milessabin
Copy link
Contributor

The "more implicit arguments wins" rule does solve this problem though. That's why I'm suggesting we revisit the Nil.min issue. The additional pruning that's done in scalac would rule out the exploding Ordering case.

@odersky
Copy link
Contributor Author

odersky commented Mar 28, 2019

The "more implicit arguments wins" rule does solve this problem though.

I don't think so. It would pick high but then fail in implicit search if no Arg was found. That would be a hard failure; there is no backtracking to low at this point.

No, scratch that. That's only for overload resolution. Implicits prune differently. Have to think some more about it.

@milessabin
Copy link
Contributor

milessabin commented Mar 29, 2019

@odersky I tried a few variations on the "disjunctive implicit argument to reduce priority" idea. It sort of works in simple cases like the one you've covered in the test. But it's really awkward if the argument we're disjoining with is more complex, ie. has type parameters, or has a type member which is then depended on by a subsequent implicit argument.

One related alternative would be to have the implicit argument (ie. Arg2 in this case) extend the priority traits then parameterize the fallback in terms of the priority trait only. That gets around the type parameters issue, but clearly it depends on cooperation from the author of the type that has to extend the priority type.

I think we have to find a solution for fallbacks which doesn't depend on cooperative/closed world assumptions.

Inline traces are bulky, it's best not to show them by default
Default arguments must be passed as `given` arguments to match.
Without this change we get a stackoverflow in implied-priority.scala.
@odersky
Copy link
Contributor Author

odersky commented Mar 29, 2019

I think we have to find a solution for fallbacks which doesn't depend on cooperative/closed world assumptions.

What about the technique shown in the latest commit on this PR? Direct link to test source:

https://github.com/lampepfl/dotty/blob/0d1d9637f981f6358e7e69e8b670ad458cf94aa5/tests/run/implied-priority.scala

I tried to elaborate there all techniques I know for prioritising implicits, both cooperative and after the fact.

You'll need this branch to run it, as I had to fix a bug related to the interaction of given parameters and default values.

@odersky
Copy link
Contributor Author

odersky commented Mar 30, 2019

I'm going to merge this now since I'd like to continue with another PR on top of it: replace implicit match expressions. It turns out we can simulate them using default arguments to implicit parameters.

@odersky odersky merged commit e4c4e0a into scala:master Mar 30, 2019
@allanrenucci allanrenucci deleted the fix-impl-res2 branch March 30, 2019 12:39
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.

None yet

2 participants