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 overload between varargs and non-varargs #9688

Closed
eloots opened this issue Sep 1, 2020 · 10 comments
Closed

Ambiguous overload between varargs and non-varargs #9688

eloots opened this issue Sep 1, 2020 · 10 comments

Comments

@eloots
Copy link
Contributor

eloots commented Sep 1, 2020

Minimized code

Calling a method on the Java Log4J library with 2 arguments (the last one in a position where a varargs polymorphic version exists) results in the compiler reporting an error.

  context.log.error("Received unexpected message in state 'trackProgress': {}", msg)

Compiling the above line results in the following error message:

Output

[error] -- [E051] Reference Error: /Users/ericloots/Trainingen/LBT/lunatech-scala-2-to-scala3-course/exercises/exercise_000_sudoku_solver_initial_state/src/main/scala/org/lunatechlabs/dotty/sudoku/SudokuProgressTracker.scala:43:20
[error] 43 |        context.log.error("Received unexpected message in state 'trackProgress': {}", msg)
[error]    |        ^^^^^^^^^^^^^^^^^
[error]    |Ambiguous overload. The overloaded alternatives of method error in trait Logger with types
[error]    | (x$0: String, x$1: Object*): Unit
[error]    | (x$0: String, x$1: Object): Unit
[error]    |both match arguments (("Received unexpected message in state \'trackProgress\': {}" : String), (msg : org.lunatechlabs.dotty.sudoku.SudokuProgressTracker.SudokuDetailState))

Expectation

The sample line of code given above should compile (as it does in pre-0.27.0 and of course Scala 2.x)

@smarter smarter changed the title Incorrect flagging of ambi Ambiguous overload between Java varargs and non-varargs Sep 1, 2020
@eloots
Copy link
Contributor Author

eloots commented Sep 1, 2020

@smarter
Created a ready to run example to reproduce the issue.

eloots added a commit to lunatech-labs/lunatech-scala2-to-scala3-course that referenced this issue Sep 1, 2020
- The biggest change in 0.27.0-RC1 is the change in the extension methods
  syntax
- Because of [#9688](scala/scala3#9688) a temporary
  fix was applied in calls to `log.xxx` that pass in a single object to log
@smarter smarter changed the title Ambiguous overload between Java varargs and non-varargs Ambiguous overload between varargs and non-varargs Sep 2, 2020
@smarter
Copy link
Member

smarter commented Sep 2, 2020

Turns out this isn't even specific to Java varargs:

object Test {
  def foo(x: Any*): Unit = {}
  def foo(x: Any): Unit = {}

  foo("") // error: ambiguous overloads
}

@smarter
Copy link
Member

smarter commented Sep 2, 2020

@odersky Any idea what difference there might be between Scala 2 and Dotty's overloading resolution that leads to the previous example being ambiguous?

@smarter
Copy link
Member

smarter commented Sep 2, 2020

It's also worth noting that the following is not ambiguous in Scala 2 nor Dotty:

object Test {
  def foo(x: String*): Unit = {}
  def foo(x: String): Unit = {}

  foo("")
}

I don't know why using String instead of Any makes a difference.

@dwijnand
Copy link
Member

dwijnand commented Sep 2, 2020

Is it because Seq[Any] <: Any?

@smarter
Copy link
Member

smarter commented Sep 2, 2020

That should be fine, but maybe what's going on is that Any* <: Any in Dotty but not in Scala 2. In Dotty, Any* is encoded using a synthetic class: <repeated>[Any], I don't know how it's represented in Scala 2 but there's a comment hinting that * is magic somehow (https://github.com/scala/scala/blob/074dae29c9a6bd3a22c4c6289f224ec28110fdfd/src/compiler/scala/tools/nsc/typechecker/Infer.scala#L339) so it might not be a subtype of any regular type.

@som-snytt
Copy link
Contributor

som-snytt commented Sep 2, 2020

A couple of links via gitter:

#6230 still open from a pre-pandemic year, with a long conversation that ends with Martin: "The rabbit hole just got deeper."

The canonical scala 2 ticket scala/bug#8342 doesn't actually link to all the issues, we need paulp-level linking.

Sample subtlety scala/bug#4728.

I think scala/bug#8344 was resolved incorrectly [but what do I know].

Gitter converts links to text so they are no longer cut/pasteable. [Edit: github converts them back. Synergy!]

@smarter smarter mentioned this issue Sep 2, 2020
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.
@He-Pin
Copy link

He-Pin commented Sep 4, 2020

It's anoying in Java too

foo(Object ... values) //1
foo(List<Object> values) //2
List<String> values = ...
foo(values) //link to 1
List values2 = values;
foo(values2) //link to 2

@smarter
Copy link
Member

smarter commented Sep 7, 2020

Fixed in #9732

@smarter smarter closed this as completed Sep 7, 2020
eloots added a commit to lunatech-labs/lunatech-scala2-to-scala3-course that referenced this issue Sep 22, 2020
* Bump Dotty version

* Bump Dotty version to 0.27.0-RC1

- The biggest change in 0.27.0-RC1 is the change in the extension methods
  syntax
- Because of [#9688](scala/scala3#9688) a temporary
  fix was applied in calls to `log.xxx` that pass in a single object to log

* Organise code snippets

* Fix extension method syntax/Fix indentation

* Switch to Significant Indentation Based Syntax

- Switch to SIB syntax for exercise 3 and later

* Remove option for .sbtopts that will be deprecated in the future
eloots added a commit to lunatech-labs/lunatech-scala2-to-scala3-course that referenced this issue 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 issue 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 issue 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants