Eliminate non-matching overloads early for partial function literals

[szeiger]

This was already done for function literals. It is important in cases
where only one overload of a method takes a Function1 or
PartialFunction, for example:

def map[U](f: T => U): R[U]
def map[U](f: (A, B) => U): R[C]

By recognizing that a partial function literal always conforms to
PartialFunction[Any, Nothing], in a call such as

m.map { case (a, b) => a }

the non-matching overload (which takes a Function2) can be eliminated
early, thus allowing the known types A and B to be used during
type-checking of the partial function (which would otherwise fail).

[julienrf]

By recognizing that a partial function literal always conforms to
PartialFunction[Any, Nothing], in a call such as

m.map { case (a, b) => a }

the non-matching overload (which takes a Function2) can be eliminated
early

However it is currently possible to write xs.foldRight[List[Int]](Nil) { case (x, xs) => x :: xs }. The important part is that foldRight accepts a Function2 but get passed a partial function that extracts two arguments.

Will it be still possible to write the above code with your change?

[julienrf]

Can you check that the return type is effectively MyMap?

[szeiger]

It's not (because the definitions of map are wrong). I'll change the example to make this work properly.

[julienrf]

Oh, I see that you address the use case of my first comment here: https://github.com/scala/scala/pull/5698/files#diff-183572032af7d0a0ae543630826723b8R18

[szeiger]

I didn't realize there was any auto-tupling for PartialFunctions. I'll have to check how it's done but I suspect it may be the killing blow to this change (ruling out the Function2 case was the whole point, after all). The map2 call you pointed to doesn't apply because 1) it takes a Function1 and 2) it's not overloaded.

[szeiger]

@adriaanm BTW, I was at first surprised that overloads with matching parameter types already work for partial function literals, but they do because your changes there apply to parameter types, not argument types. So it's only corner cases like def f[R](f: PartialFunction[T, R]); def f(f: PartialFunction[T, Int]) that aren't already covered.

Update: I may have been wrong here because the current test doesn't cover this case and when I add it I run into type inference problems. Investigating.

[szeiger]

The relevant parts of the spec are pattern-matching anonymous functions and the shape-based applicability in overloading resolution.

A potential problem arises from the auto-untupling of pattern-matching anonymous functions, i.e. they can be type-checked as PartialFunction or FunctionN for N > 1. The change in this PR means that for the purpose of overloading resolution we disallow the FunctionN case. A pattern-matching anonymous function is always treated as having a PartialFunction shape. In practice though the only consequence of this is that it makes some code compile that would have previously failed (which is the point), or make it not compile in a different. I was unable to come up with a case where it would lead to a different interpretation of a valid program. Here's the worst case scenario:

  def g(x: Int): Unit = ()
  def g[R](pf: Function2[Double, Double, R]): Unit = ()

  g { case (a: Double, b: Double) => 42: Int }

This is as unambiguous and fully typed as it gets but it still leads to:

InferOverloadedPartialFunction.scala:40: error: missing parameter type for expanded function
The argument types of an anonymous function must be fully known. (SLS 8.5)
Expected type was: ?
  g { case (a: Int, b: String) => 42: Int }
    ^

As far as I can tell there is no way to make the argument types fully known short of a full type ascription, i.e.:

  g({ case (a: Double, b: Double) => 42: Int }: Function2[Double, Double, Int])

This, of course, resolves the ambiguity because you have to decide whether it's a Function2 or a PartialFunction.

Here's an example of code that did not compile before but now does:

  def h[R](pf: Function2[Int, String, R]): Unit = ()
  def h[R](pf: PartialFunction[(Double, Double), R]): Unit = ()

  h { case (a: Double, b: Double) => 42: Int }

Concerning the miserable state of type inference for pattern-matching anonymous functions in overloaded method calls my only hesitation to implementing this change would be a conflict with Dotty. I was unable to find a complete spec for Dotty but maybe @odersky can enlighten us because scala/scala3#897 was the most relevant bit I could find. Dotty adds auto-tupling for function types, which already seems to be a bit at odds with the auto-untupling of pattern-matching anonymous functions. I suppose overload resolution will have to favor the non-tupled function types but could still favor PartialFunction for pattern-matching anonymous functions.

[szeiger]

Added spec update and an extension of #5307 to partial function literals (which is also needed to make our use case in the collections strawman work).

[julienrf]

Which one is selected? The one defined at line 8 or 9?

[szeiger]

Line 9 because overloading resolution now demands that the pattern-matching anonymous function is a PartialFunction. If line 8 was the only definition (not overloaded), it would still be applicable.

[julienrf]

This makes me think of another use case (possibly unrelated with your PR): is it still possible to explicitly select an overload based on the return type? In your example that does not really change anything, but in the case of TreeSet we really want to be able to differentiate between the overload that returns a TreeSet or the one that returns an unordered Set.

[szeiger]

It's not quite unrelated because getting this part right is what led to the 2nd change. Look the definitions of map1 in the latest version:

class MySeq[T] {
  def map1[U](f: T => U): MySeq[U] = new MySeq[U] // 1
}

class MyMap[A, B] extends MySeq[(A, B)] {
  def map1[C](f: (A, B) => C): MySeq[C] = new MySeq[C] // 2
  def map1[C, D](f: (A, B) => (C, D)): MyMap[C, D] = new MyMap[C, D] // 3
  def map1[C, D](f: ((A, B)) => (C, D)): MyMap[C, D] = new MyMap[C, D] // 4

1 is the original Function1-based version, 2 and 3 are the Function2-based versions that we want for MyMap. The only reason for having 4 in addition to 3 (which should be more efficient because it doesn't have to tuple the arguments) is to support pattern-matching anonymous functions (for which Function2 is no longer applicable in overload resolution), otherwise you would always get 1 (with the wrong return type) in this case.

[julienrf]

So, in your example, is it possible to force the resolution of version 1 over 4?

[szeiger]

You'd have to use (m: MySeq[(Int, String)]).map1 { ... }. In the case of TreeSet, 4 would take an additional implicit Ordering. Since implicits are not taken into account for overload resolution, you can't select an overload based on the presence or absence of an implicit:

Welcome to Scala 2.12.1 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_102).
Type in expressions for evaluation. Or try :help.

scala> class MySet[T] { def map[R](f: T => R): MySet[R] = new MySet[R] }
defined class MySet

scala> class MyTreeSet[T] extends MySet[T] { def map[R : Ordering](f: T => R): MyTreeSet[R] = new MyTreeSet[R] }
defined class MyTreeSet

scala> (new MyTreeSet[Int]).map(x => x)
res0: MyTreeSet[Int] = MyTreeSet@46e69805

scala> (new MyTreeSet[AnyRef]).map(x => x)
<console>:13: error: No implicit Ordering defined for AnyRef.
       (new MyTreeSet[AnyRef]).map(x => x)
                                  ^

If we want to support this we need a more elaborate scheme where the implicit parameter determines the return type (so it couldn't be a simple Ordering), in other words: CanBuildFrom. I assume that's what @Ichoran had in mind when he suggested that CBF would still be useful for some operations that need additional flexibility, even if the base methods don't use it.

[julienrf]

For reference, here is a scastie showing that their is no way to force the resolution of a lower precedence overload: http://scastie.org/27343

[Ichoran]

@szeiger - Precisely.

[szeiger]

This was merged into Dotty as scala/scala3#2015 which resolves my concerns for possible incompatibilities in case Dotty would take a different direction.

[lrytz]

I guess the controversial part is that it rules out a method that is applicable.

This compiles:

object t {
  def m(f: (Int, Int) => Int) = 0
}
t.m({ case (x, y) => 0 }) // 0

Then I add a second applicable method to t but don't get an ambiguity error, instead it selects the other method

object t {
  def m(f: (Int, Int) => Int) = 0
  def m(f: PartialFunction[(Int, Int), Int]) = 1
}
t.m({ case (x, y) => 0 }) // 1

Do we currently have existing similar cases?

[szeiger]

That's pretty much the same as adding a more specific overload in any other case. Since PartialFunction extends Function1 you could argue that the same rule should apply.

[lrytz]

The difference is that the two types are unrelated, it's Function2 and PartialFunction. If the compiler was to select the most specific alternative among the two, it would be ambiguous.

scala> :power
scala> object t {
     |   def m(f: (Int, Int) => Int) = 0
     |   def m(f: PartialFunction[(Int, Int), Int]) = 1
     | }
scala> val List(m1, m2) = typeOf[t.type].member(newTermName("m")).alternatives

scala> global.typer.infer.isAsSpecific(m1.tpe, m2.tpe)
res8: Boolean = false

scala> global.typer.infer.isAsSpecific(m2.tpe, m1.tpe)
res9: Boolean = false

[lrytz]

That said, I'm not against this change, just discussing it in detail. Could you add some minimal test cases that show what changed, and what didn't? Also, make them run tests (not pos) to ensure the expected method is selected.

[szeiger]

Sure, it's a new case (otherwise it wouldn't require a spec change) but it's conceptually similar. I'll update the test cases.

[lrytz]

You somehow included (an old version of) #5708 in the commit list - that fix will get to 2.13.x anyway by merging in 2.12.x. Or did I misunderstand something?

[szeiger]

Hm, not sure why that commit was there. It's not necessary to make the tests work. I removed it and squashed and rebased the rest.

[lrytz]

This would be good to get in M1. @adriaanm do you want to take a final look?

[adriaanm]

The change looks logical to me, and I think it matches programmer intuition. I'm glad you pointed out the controversial part, @lrytz. I say, let's go ahead and make the change anyway, but let's keep an eye out for potential interactions of auto-tupled functions and partial functions.