SI-8197 Overload resolution should not consider default arguments

[adriaanm]

The spec says

Let B be the set of alternatives in A that are applicable (§6.6)
[...] It is an error if none of the members in B is applicable. If
there is one single applicable alternative, that alternative is
chosen. Otherwise, let C be the set of applicable alternatives which
don’t employ any default argument in the application to e1, ..., em.
It is again an error if C is empty. Otherwise, one chooses the most
specific alternative among the alternatives in C [...].

There are many ways to interpret this, but none of them involves
excluding default getters, which is what the old code was doing.
We now exclude all alternatives that define any default arguments.

NOTE: according to SI-4728, this should fail to compile with an
ambiguity error. The compiler has been accepting this program
for all of 2.10.x, as far as I can tell, so let's not change that
for 2.11.0-RC1...

[adriaanm]

Review by @retronym. Unclear topic, but clear fix, IMO.

[adriaanm]

It seems to me that

alternatives which don’t employ any default argument in the application to e1, ..., em.

should be checked before we even consider specificity, as it involves comparing the method's signature to the actual arguments (to determine whether the application to e1...em has to employ one or more default arguments), rather than considering an ordering of the method signatures (without considering the actual application)

[adriaanm]

However, if you add a comma to that sentence, it becomes equivalent to

Otherwise, let C be the set of applicable alternatives (which
don’t employ any default argument) in the application to e1, ..., em.

which is what's implemented by this commit

[adriaanm]

/cc @lrytz

[adriaanm]

Spec clarified at scala/scala-dist#132

[som-snytt]

I haven't followed the conversation, but the change is not obvious to me. OTOH, the notion of an application that didn't require the use of defaults to get applied is pretty clear. Where "clear" means "internalized." Please don't laugh if I said it wrong. Every time overloading comes up, I say I won't get sucked in again. I don't know why the puzzle page in the newspaper doesn't offer a crossword, a word scramble, a sudoku and a Scala overload. I'll read this one later in the evening, so I can update my resume.

[adriaanm]

hasDefault is a getter intended for symbols of method arguments, not of methods themselves. To be complete, it's also used to detect the getters that are synthesized to provide the defaults for said arguments.

def hasDefault         = hasFlag(DEFAULTPARAM) && hasFlag(METHOD | PARAM) // Second condition disambiguates with TRAIT

The name is unfortunate, def isDefaultParam would've been a more obvious choice, but I'd rather not make sweeping changes like that right before an RC

[xeno-by]

In reflection API it's called isParamWithDefault

[xeno-by]

Just to clarify for those who aren't up to speed. This doesn't actually change overloading behavior wrt 2.10.x, does it? Or there is some code that used to compile in 2.10.x and does not now?

[retronym]

LGTM

[adriaanm]

For some weird reason, the hasDefault check used to work in 2.10.x, so this PR brings us back to the same behavior as in 2.10.x.

[retronym]

Really?

Welcome to Scala version 2.10.3-20130923-060037-e2fec6b28d (Java HotSpot(TM) 64-Bit Server VM, Java 1.6.0_37).
Type in expressions to have them evaluated.
Type :help for more information.

scala> :power
** Power User mode enabled - BEEP WHIR GYVE **
** :phase has been set to 'typer'.          **
** scala.tools.nsc._ has been imported      **
** global._, definitions._ also imported    **
** Try  :help, :vals, power.<tab>           **

scala> class C { def foo(a: Int = 0) = 0 }
defined class C
scala> val foo = typeOf[C].decl(newTermName("foo"))
foo: $r.intp.global.Symbol = method foo

scala> foo.hasDefault
res0: Boolean = false

[adriaanm]

Hmm, then I have no explanation why this used to work... The test case compiles on 2.10.x but not on 2.11.0-SNAPSHOT right now

[retronym]

This area could do with a few more tests as a followup: methods (rather than just constructors), applications that need to followApply, polymorphic methods.

[lrytz]

Sorry I'm late to the party, I was on vacation last week.

I think overloading resolution with defaults is misunderstood. B is the set of all applicable alternatives. If there's more than one, we throw out those alternatives who can only be applied using defaults. However, we should not throw out methods that define unused defaults. This leads to the following regression.

On 2.10 (correct)

scala> lucmac:~/scala/scala-2.10.3/bin/scala
Welcome to Scala version 2.10.3 (Java HotSpot(TM) 64-Bit Server VM, Java 1.6.0_65).

scala> object t { def f(x: String = "") = "String param"; def f(x: Object) = "Object param" }
defined module t

scala> t.f("")
res0: String = String param

On current master (718a897)

lucmac:scala luc$ build/pack/bin/scala
Welcome to Scala version 2.11.0-20140223-182524-718a8974f4 (Java HotSpot(TM) 64-Bit Server VM, Java 1.6.0_65).

scala> object t { def f(x: String = "") = "String param"; def f(x: Object) = "Object param" }
defined object t

scala> t.f("")
res2: String = Object param

I'm looking at where this was introduced. I kind of suspected 990fa04, but this commit is in 2.10, which behaves correctly as you see above. Maybe it does so only because hasDefaultFlag does not work as expected? I continue looking at this.

[lrytz]

I can confirm that this PR introduced the issue in my example above.

[lrytz]

Below is one bug that got introduced in 2.10 (probably by the mentioned commit 990fa04).

before (2.9):

Welcome to Scala version 2.9.3 (Java HotSpot(TM) 64-Bit Server VM, Java 1.6.0_65).
scala> object t { def f(x: Int) = 0; def f(y: Unit) = 1 }
defined module t

scala> var x = 1
x: Int = 1

scala> t.f(x = 1)
<console>:10: error: ambiguous reference to overloaded definition,
both method f in object t of type (y: Unit)Int
and  method f in object t of type (x: Int)Int
match argument types (x: Int)
              t.f(x = 1)
                ^

after (2.10): The compiler now assumes that the argument expression x = 1 can only be a named argument. By spec, it can also be an assignment (and therefore of type Unit).

scala> object t { def f(x: Int) = 0; def f(y: Unit) = 1 }
defined module t

scala> var x = 1
x: Int = 1

scala> t.f(x = 1)
res1: Int = 0

[adriaanm]

Drad. We did consider both interpretations (see above), but the one that excluded all alternatives with defaults seemed what was implemented. This is illustrated by the included test case test/files/run/t8197.scala, which shouldn't compile according to your explanation, but did compile in 2.10 (scalafx exploited it pretty intensely), and didn't compile in 2.11 until this PR.

[som-snytt]

That's what I meant by "OTOH, the notion of an application that didn't require the use of defaults to get applied is pretty clear." But apparently I lied about taking another look.

The incorrect fix for SI-4592 is echoed in the confusion over at SI-4728. (I didn't see the latest comment on SI-4728 because I had unwatched it, as a way of mentally putting it to rest.)

I don't understand why the test "shouldn't compile according to your explanation." [Ed. Sorry, you didn't mean that it ought not compile.] Both ctors are candidates but the primary is discarded because a default arg was used in the application. (That represents scalac trying to respect what we actually wrote, hurrah.) (I think the comments on SI-4728 say, Why doesn't it do the same when a vararg was used; a different discussion.)

The test case comment is incorrect. The old issue shows this not working because B is a subclass.

class A ; class B extends A
class C(val a: A) {
  def this(bs: B*) = this(new A)
}
new C(new B)  // ambiguous application

[adriaanm]

Below is one bug that got introduced in 2.10 (probably by the mentioned commit 990fa04).

yep, it's the namesMatch bit, which has no basis in the spec

My confusion stemmed from the t.typeSymbol.hasDefaultFlag that seems to have been a random mutation introduced in the same commit. Removing those two offending pieces is probably the way to go. I'll investigate further, but test suite's looking good.

[som-snytt]

FWIW, I did try deleting the namesMatch clause and ran the test sample. I guess it's worth about two cents.

[adriaanm]

Two metric cents, even!

It's really unfortunate that namesMatch business snuck into overloading resolution. I also figured out why the arity check wasn't excluding default params (which presumably triggered the addition of the hasDefaultFlag condition): it needed to disallow tupling. As you pointed out, the fix for SI-4592 is incorrect. Let's hope not too many people were relying on it.

The proper fix for SI-4592, I hope, is to clarity what it means to type check a named argument. If the name exists among the parameter names (or the deprecated ones), we type check the RHS of the assignment, otherwise, we type check the assignment as-is. If the variable that's assigned to exists, the result type will be Unit, if not, it's an error. This means SI-4592 will still compile, as there is no count variable.

Hrm, it's not simple as that because before doing the actual resolution, you don't know what the valid parameter names are. Still, an invalid assignment should not be assumed to have type Unit, which is the root cause for the failure to compile the code in SI-4592.

[lrytz]

i have a patch ready (will upload soon, on the train right now). however it re-breaks SI-4592 because its initial fix (990fa04) was wrong. i'm looking at this.

[adriaanm]

sorry I missed this -- best to @-mention me on closed PRs as otherwise those comments don't end up in my inbox (they won't be lost forever, but I only clear that queue sporadically)