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

"Ambiguous reference to overloaded definition"... not ambiguous to Java, only to Scala #4728

Closed
scabug opened this issue Jun 22, 2011 · 15 comments

Comments

@scabug
Copy link

scabug commented Jun 22, 2011

Given

class X
class Y extends X
object Ambiguous {
  def f(x: X) = 1
  def f(ys: Y*) = 2
}
scala> Ambiguous.f(new X)
res2: Int = 1

scala> Ambiguous.f(new Y)
<console>:8: error: ambiguous reference to overloaded definition,
both method f in object Ambiguous of type (ys: Y*)Int
and  method f in object Ambiguous of type (x: X)Int
match argument types (Y)
       Ambiguous.f(new Y)

But the corresponding Java program works compiles:

edit: as pointed out by andrew (comment below), javac picks the first alternative in both cases.

public class AmbiguousJ {
    static int f(X x) { return 1; }
    static int f(Y... y) { return 2; }
    public static void main(String[] args) {
        System.out.println(f(new X()));
        System.out.println(f(new Y()));
    }
}

prints (when running):

1
1

(Assuming that X.class and Y.class already exist from compiling the Scala example.)

This is a PITA because X and Y, in my case, are JPA APIs, which I use a lot.

@scabug
Copy link
Author

scabug commented Jun 22, 2011

Imported From: https://issues.scala-lang.org/browse/SI-4728?orig=1
Reporter: Willis Blackburn (willisblackburn)
Affected Versions: 2.9.0, 2.10.0
Blocks #10219

@scabug
Copy link
Author

scabug commented Jul 5, 2011

@paulp said:
This is how I would have expected overloading to resolve anyway: is this a bug or a "specification enhancement" ?

@scabug
Copy link
Author

scabug commented Jul 5, 2011

@lrytz said:
Why do you expect it to be ambiguous? The way I read the spec, the second one should be used, it's more specific.

@scabug
Copy link
Author

scabug commented Jul 5, 2011

@paulp said:
Sorry, by "that" I meant what the reporter said. I don't expect it to be ambiguous.

@scabug
Copy link
Author

scabug commented May 12, 2012

@som-snytt said:
I thought I understood until I heard the sound of cosmic snickering.

The reporter says it works in Java, but does that mean 1 or 2? I can't tell from Paul's comment or the bug report. The answer is 1 for both calls. The reason is that, for backward compatibility, Java does fixed-arity first, and vari-arity third (JLS 15.12.2.4). It never gets around to looking at specificity.

Here is the sample. JPA is already a PITA without the extra ambiguity.

/*
 * AbstractQuery<T> where(Expression<Boolean> restriction)
 * AbstractQuery<T> where(Predicate... restrictions)
 *
def f(x: X) = ???
def f(ys: Y*) = ???
 */
object Test {
  type Result = String
  def main(args: Array[String]) {
    import javax.persistence.criteria.{AbstractQuery, Expression, Predicate}
    val p: Predicate = null
    val q: AbstractQuery[Result] = null
    q.where(p)
  }
}

The Java spec takes 25 pages to cover what the Scala spec does in a page or two, less if you leave out the example. It really is the phone book versus T. S. Eliot.

Here is an interpretation: picture the domains of the two functions as a Venn diagram (if you will). f1 is as specific as f2 means that domain of f1 is a subset of domain of f2, i.e., f2 can be applied to anything f1 can. For B <: A, it's easy to see that f1(B) isAsSpecific as f2(A), but not conversely.

What about f1(B*)? Although you can apply f2(b: B), you can't f2(b, b) [etc]. For specificity, it doesn't matter that all we're asking to do is f(b: B). You should be able to tell which of two methods is more specific just by looking at them. We can do that, right?

The subtyping in the example shows why this is simple and good, and that the Book is rich with wisdom. It's easy to think that f1(B*) is the intended target; but in the JPA API, they intend to distinguish a single restriction from multiple. This is more exactly expressed as is required in Scala:

def f1(b1: B, b2: B, bs: B*)  // supply two or more bees

// is there any momentum to introduce syntax?
def f1(bs: B{2,})  // two or more!  enuf said

The temptation is to say: if the applicable alternatives are f1(B*) and f2(A), then the fixed-arity of f2 suggests we should compare f1(B) with f2(A). "Come on, I'm passing you a B! One B!" But it's actually simpler just to remember that vari-arity is never as specific as fixed-arity.

Duh, says the universe.

An alternative solution, of course, is to reject programs which are remotely confusing.

$ scalac -Yask-why t4728.scala
t4728.scala:16: error: why would you even do that?

@scabug
Copy link
Author

scabug commented May 13, 2012

@lrytz said:
The way I read the spec (this corrects my previous comment):

  • (1) f(x: X) is not as specific as (2) f(ys: Y*) because (2) cannot be applied to arguments (p) of type (X).
  • (2) is not as specific as (1), because (1) cannot be applied to arguments (p) of type (Y*). (*)

(*) There are no values of type T* in Scala. The implementation of isAsSpecific in Infer.scala uses the underlying type T, but only if both method types that are being compared have a vararg-parameter.

  • the relative weight of (1) over (2) is 0

  • the relative weight of (2) over (1) is 0

  • therefore there is no most specific, the call is ambiguous.

So I vote for closing this as "Not a Bug".

@scabug
Copy link
Author

scabug commented May 13, 2012

@paulp said (edited on May 13, 2012 8:32:41 AM UTC):
Section 4.6.2:

Methods with repeated parameters T * take a variable number of arguments of type T.
That is, if a method m with type (p1:T1,...,pn:Tn,ps:S*)U isappliedtoarguments(e1,...,ek)wherek≥n,
then m is taken in that application to have type (p1 : T1,...,pn : Tn,ps : S,...,ps′S)U,
with k − n occurrences of type S where any parameter names beyond ps are fresh.

To my eye that excerpt definitively marks this as a bug. "m is taken in that application to have type" (Y)Int. (X)Int vs. (Y)Int, being applied to Y, where Y extends X.

@scabug
Copy link
Author

scabug commented May 13, 2012

@lrytz said (edited on May 13, 2012 9:02:02 AM UTC):
yes, but this part only applies to wether a method is applicable or not. afterwards, overloading resolution is applied to all applicable alternatives. overloading resolution is independent of the concrete arguments, it is based exclusively on the methods (definition-site) types.

@scabug
Copy link
Author

scabug commented May 13, 2012

@paulp said:
You're right. I'm not sure it lends itself to predictability to treat the method as having one type for applicability testing and another for overloading resolution. Maybe we could get some of this clearer language (like your comment to which I am responding) somewhere which people might see it before they open more tickets.

@scabug
Copy link
Author

scabug commented May 13, 2012

@som-snytt said:
The spec is a bit confusing precisely because, to expand what Lukas said, there is no type "T*"; the language is clearly that the parameter is marked T* and what you get are "arguments of type T." The type of the parameter marked T* is Seq[T] inside the method; there is no type T* even as an internal fiction. (The analogy would be =>T cbn params, where =>T is such a fiction.)

(Referring to the implementation doesn't necessarily clarify, since the other real bug is #3761 where the nifty impl breaks down. BTW, on that other bug, I won't submit the cbn tie-breaker, which I feel is misguided after all.)

This is certainly not a bug -- but a natural-language expression of the intuition would help us hoi poloi.

@scabug
Copy link
Author

scabug commented May 13, 2012

@lrytz said:
bq. there is no type T* even as an internal fiction

Not sure what you mean by that, but the compiler represents T* internally as [T], i.e. as an AppliedType.

Of course the implementation is not part of the specification, but it's certainly one reference point in the cases where the specification is incomplete.

@scabug scabug closed this as completed May 13, 2012
@scabug
Copy link
Author

scabug commented May 13, 2012

@som-snytt said:
Yes; I meant that the spec doesn't call T* a type, as opposed to =>A, where it does call it a type (albeit not the type of any value).

It's a bit hard to reason about =>A, witness recent scala-user discussion about whether there is a subtype relation between A and =>A (based on subsitutability). Let alone: isn't =>A just Function0[A]?

So, it's at least as hard for T*. "T* isAsHardToReasonAbout =>A". Isn't T* just Seq[T]? Really? You're right, maybe the spec should use the language of the compiler and call it a type.

For the record, my fix for this bug (which I also felt was definitely a bug) was just as Paul said: in this application, the effective type of m is to have k params. In the impl, if both fixed- and vari-arity apply, you know what k is. That is so obvious, it still bugs me.

Yet it also seems proper that specificity is independent of a particular application. But that may be wrong, because what we're really doing is selecting a member; maybe it's crazy that selection becomes detached from the call used to make the selection. This was also my question around using cbn as a tie-breaker.

I was making a joke about the syntax f1(bs: B{2,}), but the disambiguated syntax means that I can't treat my args as a Seq[B] anymore. That just seems wrong. Is the only lesson to be drawn that overloading is evil?

I'm not going to sleep well for a while.

@scabug
Copy link
Author

scabug commented May 13, 2012

@lrytz said:
I agree with all you're saying.

Yes, it feels unnatural not to fix this bug. Making overloading resolution depend on a particular application is a language change that needs deep discussion and evaluation, so it's out of the scope of this ticket.

If there are ideas on how to simplify the spec with respect to T* and =>T, I and others would be happy to discuss.

@scabug
Copy link
Author

scabug commented Feb 20, 2014

@adriaanm said:
For the record, this now compiles... Both in 2.10.4-RC1 and (almost) 2.11.0-RC1. See also: #8197.

@scabug
Copy link
Author

scabug commented Feb 26, 2014

@lrytz said:
I just tested 2.10.4-RC2, there it doesn't compile (as expected). Also 2.11 is OK. Test case in scala/scala#3585

@scabug scabug added this to the 2.10.0-M3 milestone Apr 7, 2017
smarter added a commit to dotty-staging/dotty that referenced this issue Sep 3, 2020
We recntly merged scala#9601 which unified our handling of `Object` coming
from Java methods, an unintended consequence of that change is that some
existing Java APIs can no longer be called without running into
ambiguity errors, for example log4j defines two overloads for
`Logger.error`:

    (x: String, y: Object): Unit
    (x: String, y: Object*): Unit

previously we translated `Object` to `Any` but left `Object*` alone, now
they're both treated the same way (translated to a special alias of
`Object`) and so neither method ends up being more specific than the
other, so `error("foo: {}, 1)` is now ambiguous.

Clearly the problem lies with how we handle varargs in overloading
resolution, but this has been a source of issues for years with no clear
resolution:
- scala/bug#8342
- scala/bug#4728
- scala/bug#8344
- scala#6230

This PR cuts the Gordian knot by simply declaring that non-varargs
methods are always more specific than varargs. This has several
advantages:
- It's an easy rule to remember
- It matches what Java does (see
  https://docs.oracle.com/javase/specs/jls/se8/html/jls-15.html#jls-15.12.2)
- It avoids unnecessary wrapping of arguments

The downside is that it doesn't match what Scala 2 does, but our current
behavior isn't a perfect match either (also it seems that Scala 2
handles Java varargs and Scala varargs differently in overloading
resolution which is another source of complexity best avoided, see
`tests/run/overload_repeated`).
smarter added a commit to dotty-staging/dotty that referenced this issue Sep 3, 2020
We recently merged scala#9601 which unified our handling of `Object` coming
from Java methods, an unintended consequence of that change is that some
existing Java APIs can no longer be called without running into
ambiguity errors, for example log4j defines two overloads for
`Logger.error`:

    (x: String, y: Object): Unit
    (x: String, y: Object*): Unit

previously we translated `Object` to `Any` but left `Object*` alone, now
they're both treated the same way (translated to a special alias of
`Object`) and so neither method ends up being more specific than the
other, so `error("foo: {}, 1)` is now ambiguous.

Clearly the problem lies with how we handle varargs in overloading
resolution, but this has been a source of issues for years with no clear
resolution:
- scala/bug#8342
- scala/bug#4728
- scala/bug#8344
- scala#6230

This PR cuts the Gordian knot by simply declaring that non-varargs
methods are always more specific than varargs. This has several
advantages:
- It's an easy rule to remember
- It matches what Java does (see
  https://docs.oracle.com/javase/specs/jls/se8/html/jls-15.html#jls-15.12.2)
- It avoids unnecessary wrapping of arguments

The downside is that it doesn't match what Scala 2 does, but our current
behavior isn't a perfect match either (also it seems that Scala 2
handles Java varargs and Scala varargs differently in overloading
resolution which is another source of complexity best avoided, see
`tests/run/overload_repeated`).
smarter added a commit to dotty-staging/dotty that referenced this issue Sep 3, 2020
We recently merged scala#9601 which unified our handling of `Object` coming
from Java methods, an unintended consequence of that change is that some
existing Java APIs can no longer be called without running into
ambiguity errors, for example log4j defines two overloads for
`Logger.error`:

    (x: String, y: Object): Unit
    (x: String, y: Object*): Unit

previously we translated `Object` to `Any` but left `Object*` alone, now
they're both treated the same way (translated to a special alias of
`Object`) and so neither method ends up being more specific than the
other, so `error("foo: {}, 1)` is now ambiguous.

Clearly the problem lies with how we handle varargs in overloading
resolution, but this has been a source of issues for years with no clear
resolution:
- scala/bug#8342
- scala/bug#4728
- scala/bug#8344
- scala#6230

This PR cuts the Gordian knot by simply declaring that non-varargs
methods are always more specific than varargs. This has several
advantages:
- It's an easy rule to remember
- It matches what Java does (see
  https://docs.oracle.com/javase/specs/jls/se8/html/jls-15.html#jls-15.12.2)
- It avoids unnecessary wrapping of arguments

The downside is that it doesn't match what Scala 2 does, but our current
behavior isn't a perfect match either (also it seems that Scala 2
handles Java varargs and Scala varargs differently in overloading
resolution which is another source of complexity best avoided, see
`tests/run/overload_repeated`).

Fixes scala#9688, supercedes scala#6230.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants