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

Fix vararg overloading #9732

Merged
merged 1 commit into from
Sep 7, 2020
Merged

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Sep 5, 2020

Refine/clarify the rule when a method is applicable to a vararg parameter list.

@odersky
Copy link
Contributor Author

odersky commented Sep 5, 2020

Alternative to #9718. It solves the original issue in a less drastic way and keeps compatibility with Scala 2 for all tests we have.

@smarter
Copy link
Member

smarter commented Sep 5, 2020

I now realize I miswrote A_1.foo1, it was supposed to test the same thing as bar1:

class A_1 {
  public static int foo1(Object x) { return 1; }
  public static int foo1(String... x) { return 2; }
}

With thsi PR, this is now ambiguous:

-- [E051] Reference Error: tests/run/overload_repeated/B_2.scala:21:15 ---------
21 |    assert(A_1.foo1("") == 1) // Same as in Java and Scala 2
   |           ^^^^^^^^
   |Ambiguous overload. The overloaded alternatives of method foo1 in object A_1 with types
   | (x: String*): Int
   | (x: Object): Int
   |both match arguments (("" : String))

Scala 2 has the same issue, this can only be fixed by either going with #9718 or special-casing Java varargs.

@smarter
Copy link
Member

smarter commented Sep 5, 2020

If my understanding of the situation is correct, using "ASA" to stand for "as specific as"

So the only difference between the two is that in this PR we can be in a situation where we have a vararg and a non-vararg alternative, neither of which is ASA the other, resulting in ambiguous overloads, whereas in #9718 the non-vararg would be picked. Is this accurate or are there situation where this PR would pick a different overload than #9718?

@odersky
Copy link
Contributor Author

odersky commented Sep 7, 2020

I believe the biggest win of this over #9718 is that it does not create new special cases.

So the only difference between the two is that in this PR we can be in a situation where we have a vararg and a non-vararg alternative, neither of which is ASA the other, resulting in ambiguous overloads, whereas in #9718 the non-vararg would be picked. Is this accurate or are there situation where this PR would pick a different overload than #9718?

Plus all the interactions with other rules (e.g t8197.scala, which flipped in behavior with #9718).

Copy link
Contributor

@abgruszecki abgruszecki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't exactly what I intended when I proposed the rules, but after seeing the examples and thinking all the approaches through I'd say that this change is arguably the most intuitive. At least, it is more in line with the spirit of the spec than the other changes: an overload is selected as the most specific if all calls to it can also be calls to another overload, but not vice versa.

I'd like to point out (and be corrected if I'm wrong) that if we have two overloads, one of which has varargs, and neither is more specific than another then we should always be able to explicitly pick one of them, either by upcasting an argument or adding a repeated argument annotation. Specifically, in the foo(a: Any) / foo(s: String*) case, we can explicitly select either of the overloads.

Refine/clarify the rule when a method is applicable to a vararg parameter list.

Test cases adapted from scala#9718 which used a different approach.
@smarter
Copy link
Member

smarter commented Sep 7, 2020

I've updated the commit in this PR to fix my test to have the correct signature for foo1 and adapted the comments to match the behavior of this PR.

@som-snytt
Copy link
Contributor

Following up @abgruszecki this is different from 2.13.3:

Starting dotty REPL...
scala> object X { def f(x: Any) = 1 ; def f(x: String*) = 2 }
// defined object X

scala> X.f(List(""): _*)
1 |X.f(List(""): _*)
  |^^^
  |Ambiguous overload. The overloaded alternatives of method f in object X with types
  | (x: String*): Int
  | (x: Any): Int
  |both match arguments (String*)

scala>

It's not disallowing

scala> object Y { def f(x: Any) = 1 }
// defined object Y

scala> Y.f(List(""): _*)
val res0: Int = 1

@abgruszecki
Copy link
Contributor

@odersky @smarter the above is a separate bug, right?

@smarter
Copy link
Member

smarter commented Sep 8, 2020

Yeah we should open an issue for that.

eloots added a commit to lunatech-labs/lunatech-scala2-to-scala3-course that referenced this pull request Nov 9, 2020
…ions syntax

- Dotty 0.27.0-RC1 broke logging calls due to [Ambiguous overload between varargs and non-varargs #9688](scala/scala3#9688)
  which was fixed in [Fix vararg overloading #9732](scala/scala3#9732)
- Remove a few remaining extension methods that still used an 'older' extension
  mothod syntax
eloots added a commit to lunatech-labs/lunatech-scala2-to-scala3-course that referenced this pull request Nov 9, 2020
…ions syntax

- Dotty 0.27.0-RC1 broke logging calls due to [Ambiguous overload between varargs and non-varargs #9688](scala/scala3#9688)
  which was fixed in [Fix vararg overloading #9732](scala/scala3#9732)
- Remove a few remaining extension methods that still used an 'older' extension
  mothod syntax
eloots added a commit to lunatech-labs/lunatech-scala2-to-scala3-course that referenced this pull request Nov 9, 2020
* Bump Dotty version to Scala 3.0.0-M1/Remove workaround/Correct extensions syntax

- Dotty 0.27.0-RC1 broke logging calls due to [Ambiguous overload between varargs and non-varargs #9688](scala/scala3#9688)
  which was fixed in [Fix vararg overloading #9732](scala/scala3#9732)
- Remove a few remaining extension methods that still used an 'older' extension
  mothod syntax

* Update exercise instructions

- Exercise on extentions still used a syntax that has since then
  been changed
- Use indentation based syntax in instructions
@Kordyjan Kordyjan added this to the 3.0.0 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants