SI-8197 clarify overloading resolution with default args #3583

Merged
merged 1 commit into from Feb 25, 2014

Conversation

Projects
None yet
4 participants
Owner

adriaanm commented Feb 25, 2014

If there are multiple applicable alternatives, drop those
that use default arguments. This is done indirectly by
checking applicability based on arity
(disallowing tupling to ensure exact arity matching!).
If defaults are required in the application, the arities
won't match up exactly.

TODO this namesMatch business is not spec'ed, and is
the wrong fix for SI-4592. We should instead clarify what
the spec means by "typing each argument with an undefined expected type".

What does typing a named argument entail when we don't know what
the valid parameter names are? (Since we're doing overload resolution,
there are multiple alternatives that can define different names.)

Luckily, the next step checks applicability to the individual alternatives,
so it knows whether an assignment is:

  • a valid named argument
  • a well-typed assignment
  • an error (e.g., rhs does not refer to a variable)

I propose the following solution (as a TODO):
check whether a named argument (when typing it in doTypedApply)
could be interpreted as an assign; infer.checkNames must not use
UnitType for its type unless it's a legal assignment

Owner

adriaanm commented Feb 25, 2014

Review by @lrytz. This is a minimal fix. TODOs added for consideration in 2.11.1/2.12. It's too late for an overhaul now.

Owner

retronym commented Feb 25, 2014

I propose the following solution (as a TODO): check whether a named argument (when typing it in doTypedApply) could be interpreted as an assign; infer.checkNames must not use UnitType for its type unless it's a legal assignment

One (minor?) wrinkle: assignments need not be of type Unit; they can desugar to setter_= call to a user-defined setter.

adriaanm added the tested label Feb 25, 2014

Owner

lrytz commented Feb 25, 2014

Oh, I didn't know you were also working on it.. my version is here: lrytz/scala@5ab7a7b

Owner

lrytz commented Feb 25, 2014

One (minor?) wrinkle: assignments need not be of type Unit; they can desugar to setter_= call to a user-defined setter.

Indeed, I never thought of this case. The following (wrongly) prints f-unit:

object t {
  def x: Int = 33
  def x_=(a: Int): String = "hui"
  def f(a: String) = "f-string"
  def f(u: Unit) = "f-unit"
  def g() = f(x = 100)
}
t.g()
Owner

adriaanm commented Feb 25, 2014

I pushed a commit that includes @lrytz's analysis and @retronym's wrinkle.

@adriaanm adriaanm added tested and removed tested labels Feb 25, 2014

Owner

lrytz commented Feb 25, 2014

thanks; I have to say I'm not sure about the implications of setting tuplingAllowed = false. Note that all invocations of isApplicableBasedOnArity in the compiler now have tuplingAllowed = false. So far I failed to come up with an example for which that change matters..

Owner

adriaanm commented Feb 25, 2014

Good point. I'll think about this some more, but my intention was to implement "Otherwise, let C be the set of applicable alternatives which don’t employ any default argument in the application to e1, ..., em.". To rule out "employment" of default arguments, my interpretation is that SI-8197 shows that you can't allow tupling.

Owner

adriaanm commented Feb 25, 2014

It seems to me tupling should not be part of applicability, as it's a fall-back after trying everything else.

Owner

lrytz commented Feb 25, 2014

I'd prefer to fix spec and implementation, but I guess the time for that has run out.. I'm just a bit worried that the changes in this PR and/or d5bb19f introduce more issues that we didn't see so far. But since I could not come up with counter-examples, LGTM.

As a way forward, I see two options:

  1. What you propose, only treat an x = e expression as assignment if type checks as an assignment.
  2. Simplify the rules and disallow assignments in parameter position. This is actually what dotty does: In f(x = e), the expression is parsed as named argument, it can never be an assignment. If you want an assignment you have to write f({x = e}) or f { x = e } or f((x = e)).

I think it would be good to migrate Scala to 2.

Owner

lrytz commented Feb 25, 2014

It seems to me tupling should not be part of applicability, as it's a fall-back after trying everything else.

The first problem is that auto-tupling is not in the spec (as far as i can see).. But I think it needs to be checked in applicability, otherwise a valid invocation would not be applicable (?). Btw, are there plans to deprecate auto-tupling?

my interpretation is that SI-8197 shows that you can't allow tupling

I don't see how SI-8197 is related to auto-tupling..

Owner

adriaanm commented Feb 25, 2014

This little excursion has convinced me to start with method/function application as my first exercise in aligning the spec & implementation. I agree we should try to simplify.

In the end, the only change by the SI-8197 PRs has been to remove a no-op (checking the default param flag) and disallowing tuple conversion during this part of overload resolution (so that the original check actually excluded default arguments by comparing arities more strictly).

Owner

adriaanm commented Feb 25, 2014

my interpretation is that SI-8197 shows that you can't allow tupling

I don't see how SI-8197 is related to auto-tupling..

my reconstruction is that the original arity check was indirectly checking that no default arguments were used, which was then weakened to allow tupling, and thus no longer excluded defaults when tupling could resolve the arity mismatch

Owner

adriaanm commented Feb 25, 2014

This theory comes from flipping the tuplingAllowed bit and watching t8197 flip between compiling and not compiling.

Owner

lrytz commented Feb 25, 2014

disallowing tuple conversion during this part of overload resolution (so that the original check actually excluded default arguments by comparing arities more strictly).

in my patch (lrytz/scala@5ab7a7b), tupling conversion and default arguments are both checked.

my reconstruction is that the original arity check was indirectly checking that no default arguments were used, which was then weakened to allow tupling, and thus no longer excluded defaults when tupling could resolve the arity mismatch

I think this is not what happend; IRC we initially (before pauls commit) checked for both tupling and the use of defaults

Owner

adriaanm commented Feb 25, 2014

You're right. I was just looking at that:

 -              mtypes exists (t =>
 -                compareLengths(t.params, argtpes) < 0 ||  // tupling (*)
 -                hasExactlyNumParams(t, argtpes.length)    // same nb or vararg
 -              )
 -              // (*) more arguments than parameters, but still applicable: tuplig conversion works.
 -              //     todo: should not return "false" when paramTypes = (Unit) no argument is given
 -              //     (tupling would work)
Owner

adriaanm commented Feb 25, 2014

Ok, to avoid regressions at this late stage, I'll incorporate your approach.

Owner

lrytz commented Feb 25, 2014

OK, thanks. I'll create a ticket summarizing the current situation and the plan for the future. I also found a number of other bugs while looking at this thing. Here's one that crashes the compiler

def foo(a: Int*) = 0; foo(??? : _*)

@adriaanm adriaanm SI-8197 clarify overloading resolution with default args
This commit was co-authored with Lukas. His analysis is below.

If there are multiple applicable alternatives, drop those
that use default arguments. This is done indirectly by
checking applicability based on arity.

TODO: this `namesMatch` business is not spec'ed, and is
the wrong fix for SI-4592. We should instead clarify what
the spec means by "typing each argument with an undefined expected type".

What does typing a named argument entail when we don't know what
the valid parameter names are? (Since we're doing overload resolution,
there are multiple alternatives that can define different names.)

Luckily, the next step checks applicability to the individual alternatives,
so it knows whether an assignment is:
 - a valid named argument
 - a well-typed assignment (which doesn't necessarily have type `Unit`!)
 - an error (e.g., rhs does not refer to a variable)

I propose the following solution (as a TODO):
check whether a named argument (when typing it in `doTypedApply`)
could be interpreted as an assign; `infer.checkNames` should use
the type of the well-typed assignment instead of `Unit`.

Lukas's analysis:

990fa04 misunderstood the spec of overloading resolution with
defaults. it should not discard applicable alternatives that define
defaults, but only those that use defaults in the given invocation.

bugs were shadowed because the refactoring used `alt.hasDefault` to
check whether the alternative has some parameters with defaults, but
this never worked.

d5bb19f fixed that part by checking the flags of parameters, which
fixed some but but un-shadowed others:

```
object t { def f(x: String = "") = 1; def f(x: Object) = 2 }
scala> t.f("") // should return 1, but returns 2 after d5bb19f
```

The commit message of d5bb19f also mentions that its test "should
fail to compile according to SI-4728", which is another
misunderstanding.

```
class A; class B extends A
object t { def f(x: A = null) = 1; def f(x: B*) = 2 }
t.f()
```

This should return `2`: both alternatives are applicable, so the one
that uses a default is eliminated, and we're left with the vararg one.
SI-4728 is different in that it uses explicit parameters,
`t.f(new B)` is ambiguous.
a5cd601
Owner

adriaanm commented Feb 25, 2014

Diff with previous commit:

--- c/src/compiler/scala/tools/nsc/typechecker/Infer.scala
+++ w/src/compiler/scala/tools/nsc/typechecker/Infer.scala
@@ -582,13 +582,17 @@ trait Infer extends Checkable {
         alts exists (alt => isApplicableBasedOnArity(pre memberType alt, argsCount, varargsStar, tuplingAllowed))
       case _ =>
         val paramsCount   = tpe.params.length
+        // simpleMatch implies we're not using defaults
         val simpleMatch   = paramsCount == argsCount
         val varargsTarget = isVarArgsList(tpe.params)

-        // vararg parameter lists cannot have default arguments, so we know that a mismatch
-        // of paramsCount and argsCount cannot be due to defaults.
+        // varargsMatch implies we're not using defaults, as varargs and defaults are mutually exclusive
         def varargsMatch  = varargsTarget && (paramsCount - 1) <= argsCount
+        // another reason why auto-tupling is a bad idea: it can hide the use of defaults, so must rule those out explicitly
         def tuplingMatch  = tuplingAllowed && eligibleForTupleConversion(paramsCount, argsCount, varargsTarget)
+        // varargs and defaults are mutually exclusive, so not using defaults if `varargsTarget`
+        // we're not using defaults if there are (at least as many) arguments as parameters (not using exact match to allow for tupling)
+        def notUsingDefaults = varargsTarget || paramsCount <= argsCount

         // A varargs star call, e.g. (x, y:_*) can only match a varargs method
         // with the same number of parameters.  See SI-5859 for an example of what
@@ -596,7 +600,7 @@ trait Infer extends Checkable {
         if (varargsStar)
           varargsTarget && simpleMatch
         else
-          simpleMatch || varargsMatch || tuplingMatch
+          simpleMatch || varargsMatch || (tuplingMatch && notUsingDefaults)
     }

     private[typechecker] def followApply(tp: Type): Type = tp match {
@@ -1342,9 +1346,10 @@ trait Infer extends Checkable {
           case namesMatch if namesMatch.nonEmpty => namesMatch // TODO: this has no basis in the spec, remove!
           case _ =>
             // If there are multiple applicable alternatives, drop those using default arguments.
-            // This is done indirectly by checking applicability based on arity (disallowing tupling to ensure exact arity matching!).
+            // This is done indirectly by checking applicability based on arity in `isApplicableBasedOnArity`.
             // If defaults are required in the application, the arities won't match up exactly.
-            eligible filter (alt => isApplicableBasedOnArity(alt.tpe, argtpes.length, varargsStar, tuplingAllowed = false))
+            // TODO: should we really allow tupling here?? (If we don't, this is the only call-site with `tuplingAllowed = true`)
+            eligible filter (alt => isApplicableBasedOnArity(alt.tpe, argtpes.length, varargsStar, tuplingAllowed = true))
         }
     }
Owner

adriaanm commented Feb 25, 2014

Since the !usingDefaults && from your patch is a no-op when distributed over the || -- except for the tuplingMatch -- I decided to make that explicit (documented). Also reverted my tuplingAllowed = false hack.

adriaanm removed the tested label Feb 25, 2014

Owner

lrytz commented Feb 25, 2014

LGTM!

adriaanm added the reviewed label Feb 25, 2014

Owner

adriaanm commented Feb 25, 2014

Thanks for all the clarifications! I feel much better about this now. (We have a ways to go, obviously, in aligning spec & implementation, but I'm making that my priority once RC1 is done.)

adriaanm added the tested label Feb 25, 2014

Owner

adriaanm commented Feb 25, 2014

Woo-hoo! This will get us to inbox-zero!! 🍻
Won't last long, unfortunately -- have one more PR coming up regarding a regression in ScalaTest's test suite.

adriaanm merged commit b4ff1e9 into scala:master Feb 25, 2014

1 check passed

default pr-scala Took 68 min.
Details

adriaanm deleted the adriaanm:t8197 branch Feb 25, 2014

Contributor

som-snytt commented Feb 26, 2014

For some reason 🍻 shows two full steins, but mine is already drained.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment