Two fixes for overload resolution for PartialFunctions #5975

szeiger · 2017-07-05T19:40:16Z

The extension of shape-based overload resolutions for pattern-matching
anonymous functions in #5698 did not
work correctly in all cases:

Type unification for shapes needs to treat PartialFunction types
the same as FunctionN and SAM types. They were considered for
unification but the parameter type was not extracted correctly.
When the matched argument types are constructed after a successful
shape match, a pattern-matching anonymous function has to be typed as
PartialFunction instead of FunctionN.

The extension of shape-based overload resolutions for pattern-matching anonymous functions in scala#5698 did not work correctly in all cases: - Type unification for shapes needs to treat `PartialFunction` types the same as `FunctionN` and SAM types. They were considered for unification but the parameter type was not extracted correctly. - When the matched argument types are constructed after a successful shape match, a pattern-matching anonymous function has to be typed as `PartialFunction` instead of `FunctionN`.

lrytz

Looks good!

lrytz · 2017-07-06T12:51:44Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

-                    if (argPtAlts.nonEmpty && treeInfo.isFunctionMissingParamType(tree)) functionProto(argPtAlts)
+                    if (argPtAlts.isEmpty) WildcardType
+                    else if (treeInfo.isFunctionMissingParamType(tree)) functionProto(argPtAlts)
+                    else if (treeInfo.isPartialFunctionMissingParamType(tree)) partialFunctionProto(argPtAlts)


A difference is that now the compiler will generate a PartialFunction even if the selected alternative has a Function parameter type

object T { def m(f: Object => Int, s: String): Int = 1 def m(f: String => Int, s: Object): Int = 1 } T.m({ case x => 2 }, null)

generates

T.m(({ @SerialVersionUID(value = 0) final <synthetic> class $anonfun extends scala.runtime.AbstractPartialFunction[Object,Int] with Serializable { def <init>(): <$anon: Object => Int> = { $anonfun.super.<init>(); () }; final override def applyOrElse[A1 <: Object, B1 >: Int](x1: A1, default: A1 => B1): B1 = ((x1.asInstanceOf[Object]: Object): Object @unchecked) match { case (x @ _) => 2 case (defaultCase$ @ _) => default.apply(x1) }; final def isDefinedAt(x1: Object): Boolean = ((x1.asInstanceOf[Object]: Object): Object @unchecked) match { case (x @ _) => true case (defaultCase$ @ _) => false } }; new $anonfun() }: PartialFunction[Object,Int]), null);

In 2.13.0-M1

T.m(((x0$1: Object) => x0$1 match { case (x @ _) => 2 }), null);

My first impulse was to write "this works as intended and specified in #5698" but the spec change doesn't actually mandate it and it would be more efficient to create a Function1 in cases where that's sufficient.

Looks like a non-trivial problem. Perhaps extract the argument types from fun after inferMethodAlternative (is there a simple way to do that?) and then rebuild args1 with Function1 types in place of PartialFunction where it is sufficient?

lrytz · 2017-07-06T12:55:14Z

test/files/run/InferOverloadedPartialFunction.scala

+  def collectF[K2 : Ordering, V2](pf: Function1[(String, Int), (K2, V2)]): Int = 0
+  def foo(xs: SortedMap) = xs.collect { case (k, v) => 1 }
+  def fooF(xs: SortedMap) = xs.collectF { case (k, v) => 1 }
+}


Maybe add a (few) test(s) where the overloads mix Function and PartialFunction parameter types (and also assert the expected method is selected).

szeiger · 2017-07-07T20:07:12Z

I've added the changes necessary to type Match blocks as Function1 again after overload resolution unless the inferred alternative actually requires a PartialFunction.

lrytz · 2017-07-10T09:20:11Z

Hmm, that last commit adds quite a bit of complexity. How about using a PF expected type if some of the overloads takes a PF? Then we could still end up with a PF passed as F, but only when overloads mix F and PF parameter types. What do you think?

szeiger · 2017-07-10T11:27:00Z

Most of the complexity comes from retyping in the case of mixed overloads. This is the whole if(maybeRetypeArgs contains true) block. The rest looks bigger than it is because abstracting out typedSingleArg changed the indentation.

How about using a PF expected type if some of the overloads takes a PF? Then we could still end up with a PF passed as F, but only when overloads mix F and PF parameter types

This is already done (in the lines following https://github.com/scala/scala/pull/5975/files#diff-4eab1aad4533a31c10565971e90f73eaR3382) to avoid copying the tree in the non-mixed cases. If we remove the maybeRetypeArgs handling we end up with PartialFunction if any overload required it.

szeiger · 2017-07-17T13:56:23Z

How do we move this forward? Should I cut it back to a simpler impl that doesn't optimize the mixed overload case? I'd like to get this into M2 because we're running into these bugs in collections.

lrytz · 2017-07-18T13:05:47Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

+                  case TypeRef(_, _, tpes) => tpes.init
+                  case _ => Nil
+                }
+                val selectedParams = paramTypes(normalize(fun.tpe))


It's surprising me a bit that fun.tpe can be ExistentialType or TypeRef, what are examples? How about using fun.symbol.tpe instead, would that always be a MethodType?

I expected MethodType but TypeRef was the most common case. MethodType and ExistentialType came up in some partest cases but I didn't check for more details. I'll give fun.symbol.tpe a try.

ClassArgsTypeRef shows up in the last two test cases (passing a case literal to a method with mixed overloads) in run/InferOverloadedPartialFunction.scala, both for fun.tpe and fun.symbol.tpe:

assert(ys.collect { case (k, v) => 1 } == 0) assert(ys.collect { case (k, v) => (1, 1) } == 2)

I wasn't able to reproduce the ExistentialType anymore with partest pos neg run. Maybe it would show up in a full test run.

Ah, the source of the non-method types is the call to normalize - why is it there? Otherwise you should get a PolyType or MethodType

Ah, right, normalize converts the method type to a function type. Just cargo-cult programming (copied from some place that did something similar). I've pushed a change that doesn't normalize anymore, now I get the expected MethodType or PolyType.

lrytz · 2017-07-18T13:13:24Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

+                    }
+                    map3(args, altArgPts, keepTypes) {
+                      case (_, _, Some(keep)) => keep
+                      case (arg, argPtAlts, None) => typedSingleArg(arg, argPtAlts, amode, matchAsPartial = false)._1


@retronym wdyt about throwing away a type checked tree and typing it again? Are you aware of other places where we do this? Any potential problems that come to mind with side-effects of type checking, for examples related to definitions within that tree? I cannot think of any from the top of my head.

The alternative would be to generate a PartialFunction instance in case an overloaded method has alternatives with Function1 and PF parameter types, and the Function1 alternative is chosen.

lrytz · 2017-07-18T13:23:41Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

+                    }
+                    map3(args, altArgPts, keepTypes) {
+                      case (_, _, Some(keep)) => keep
+                      case (arg, argPtAlts, None) => typedSingleArg(arg, argPtAlts, amode, matchAsPartial = false)._1


You could probably use the selected function's parameter type here as expected type to type check the argument, so you wouldn't need to go through typedSingleArg / functionProto again. Just a thought, maybe it's better to keep the two type checking invocations as similar as possible.

szeiger · 2017-07-19T18:15:00Z

This is the simpler version that doesn't retype arguments: 2.13.x...szeiger:fix/pf-overloads2

lrytz · 2017-07-20T14:29:27Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

                  case MethodType(syms, _) => syms.map(_.tpe)
-                  case TypeRef(_, _, tpes) => tpes.init
-                  case _ => Nil
+                  case ErrorType => Nil


I'd go for the more cautious case _

lrytz · 2017-07-21T07:41:10Z

Let's merge your "simpler version that doesn't retype arguments" (2.13.x...szeiger:fix/pf-overloads2) for M2. We can re-visit the other version for M3, but I think it's not worth the additional complexity.

SethTisue · 2018-02-24T02:48:17Z

@szeiger should this remain in the PR queue...?

szeiger · 2018-03-09T16:04:23Z

The mixed overloads case is probably not important enough to warrant further optimization.

scala-jenkins added this to the 2.13.0-M2 milestone Jul 5, 2017

szeiger mentioned this pull request Jul 5, 2017

Add collect scala/collection-strawman#117

Merged

lrytz reviewed Jul 6, 2017

View reviewed changes

Don’t create PartialFunction unless necessary

3c95dac

szeiger mentioned this pull request Jul 13, 2017

Failure in shape-based overload resolution for PartialFunction scala/scala3#2866

Closed

lrytz reviewed Jul 18, 2017

View reviewed changes

lrytz reviewed Jul 20, 2017

View reviewed changes

Don’t normalize the method type

bb91a8e

szeiger force-pushed the fix/pf-overloads branch from ddb549a to bb91a8e Compare July 20, 2017 15:40

szeiger mentioned this pull request Jul 21, 2017

Two fixes for overload resolution for PartialFunctions #6003

Merged

szeiger modified the milestones: 2.13.0-M3, 2.13.0-M2 Jul 21, 2017

julienrf mentioned this pull request Jul 26, 2017

Drop PartialFunction type annotations. scala/collection-strawman#172

Closed

szeiger removed this from the 2.13.0-M3 milestone Nov 28, 2017

SethTisue added this to the WIP milestone Dec 12, 2017

szeiger closed this Mar 9, 2018

Two fixes for overload resolution for PartialFunctions #5975

Two fixes for overload resolution for PartialFunctions #5975

Uh oh!

Conversation

szeiger commented Jul 5, 2017

Uh oh!

lrytz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szeiger commented Jul 7, 2017

Uh oh!

lrytz commented Jul 10, 2017

Uh oh!

szeiger commented Jul 10, 2017

Uh oh!

szeiger commented Jul 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szeiger commented Jul 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lrytz commented Jul 21, 2017

Uh oh!

SethTisue commented Feb 24, 2018

Uh oh!

szeiger commented Mar 9, 2018

Uh oh!

Uh oh!