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

Code not compiling when Unit-returning SAM and overloaded methods involved #13549

Closed
jlprat opened this issue Sep 17, 2021 · 7 comments
Closed

Comments

@jlprat
Copy link

jlprat commented Sep 17, 2021

I think there seems to be a problem with SAM conversions and choosing the right overloaded method. Sorry if the bug already exists I couldn't find one.

Compiler version

Scala 3.0.2 and 3.1.0-RC2

Minimized code

You can find a reproducer here: https://github.com/jlprat/overload-sam-reproducer/

import java.lang.Throwable
import java.util.function.Supplier

object Reproducer {
  
  //assertThrows has 3 overloads, 2 with 3 parameters and 1 with only 2 parameters.

  //This overload is taking a subclasss of `Throwable`, and an `Executable` which is a parameterless SAM returning void
  assertThrows(classOf[IllegalArgumentException], () => 3)
  
  //This overload is taking a subclasss of `Throwable`, an `Executable` which is a parameterless SAM returning void, and a `String`
  assertThrows(classOf[IllegalArgumentException], () => 3, "This is a message")

  //This overload is taking a subclasss of `Throwable`, an `Executable` which is a parameterless SAM returning void, and a `Supplier` returning `String`
  assertThrows(classOf[IllegalArgumentException], () => 3, () => "This is a message")

  def assertThrows[T <: Throwable](clazz: Class[T], executable: Executable): Unit = ???

  def assertThrows[T <: Throwable](clazz: Class[T], executable: Executable, message: String): Unit = ???

  def assertThrows[T <: Throwable](clazz: Class[T], executable: Executable, supplier: Supplier[String]): Unit = ???

  @FunctionalInterface
  trait Executable {
    @throws[Throwable]
    def execute(): Unit
  }

}

Output

[warn] -- [E129] Potential Issue Warning: /home/josep.prat/playground/overload-sam-reproducer/src/main/scala/example/Reproducer.scala:11:56
[warn] 11 |  assertThrows(classOf[IllegalArgumentException], () => 3)
[warn]    |                                                        ^
[warn]    |A pure expression does nothing in statement position; you may be omitting necessary parentheses
[error] -- [E134] Type Error: /home/josep.prat/playground/overload-sam-reproducer/src/main/scala/example/Reproducer.scala:14:2
[error] 14 |  assertThrows(classOf[IllegalArgumentException], () => 3, "This is a message")
[error]    |  ^^^^^^^^^^^^
[error]    |None of the overloaded alternatives of method assertThrows in object Reproducer with types
[error]    | [T <: Throwable]
[error]    |  (clazz: Class[T], executable: example.Reproducer.Executable, supplier:
[error]    |    java.util.function.Supplier[String]
[error]    |  ): Unit
[error]    | [T <: Throwable]
[error]    |  (clazz: Class[T], executable: example.Reproducer.Executable, message: String
[error]    |    ):
[error]    |  Unit
[error]    | [T <: Throwable]
[error]    |  (clazz: Class[T], executable: example.Reproducer.Executable): Unit
[error]    |match arguments ((classOf[IllegalArgumentException] : Class[IllegalArgumentException]), () => Int, ("This is a message" : String))
[error] -- [E134] Type Error: /home/josep.prat/playground/overload-sam-reproducer/src/main/scala/example/Reproducer.scala:17:2
[error] 17 |  assertThrows(classOf[IllegalArgumentException], () => 3, () => "This is a message")
[error]    |  ^^^^^^^^^^^^
[error]    |None of the overloaded alternatives of method assertThrows in object Reproducer with types
[error]    | [T <: Throwable]
[error]    |  (clazz: Class[T], executable: example.Reproducer.Executable, supplier:
[error]    |    java.util.function.Supplier[String]
[error]    |  ): Unit
[error]    | [T <: Throwable]
[error]    |  (clazz: Class[T], executable: example.Reproducer.Executable, message: String
[error]    |    ):
[error]    |  Unit
[error]    | [T <: Throwable]
[error]    |  (clazz: Class[T], executable: example.Reproducer.Executable): Unit
[error]    |match arguments ((classOf[IllegalArgumentException] : Class[IllegalArgumentException]), () => Int, () => String)

Expectation

Code should compile

@smarter smarter added area:typer compat:java stat:needs minimization Needs a self contained minimization labels Sep 17, 2021
@smarter
Copy link
Member

smarter commented Sep 17, 2021

Could you simplify the example to not rely on an external library? It should only require defining the assertThrows overload manually with a ??? right hand side

@jlprat
Copy link
Author

jlprat commented Sep 17, 2021

@smarter Shall I still write the assertThrows in a Java file to be closer to the original, or should I try to use only Scala files?

@jlprat
Copy link
Author

jlprat commented Sep 17, 2021

OK, I managed to reproduce it using only Scala, @smarter let me know if you'd like more info and/or if the reproducer is good enough.

@jlprat
Copy link
Author

jlprat commented Sep 17, 2021

I also tried Scala 3.1.0-RC2 and it fails with the same errors

@smarter smarter removed compat:java stat:needs minimization Needs a self contained minimization labels Sep 17, 2021
@smarter
Copy link
Member

smarter commented Sep 17, 2021

Thanks, this is great!

jlprat added a commit to jlprat/kafka that referenced this issue Sep 21, 2021
@smarter smarter changed the title Code not compiling when SAM and overloaded methods involved Code not compiling when Unit-returning SAM and overloaded methods involved Sep 22, 2021
smarter added a commit to dotty-staging/dotty that referenced this issue Sep 22, 2021
Without overloading, a function literal `() => 3` whose expected type is
a Unit-returning SAM gets typed as `() => { 3; () }` as expected, but
with overloading we first type the function literal without an expected
type and then compare it against the formal parameter type.

This commit makes this check succeed by replacing a result type of
`Unit` by `WildcardType` so we end up comparing
`() => Int <:< () => ?` instead of `() => Int <:< () => Unit`.

Fixes scala#13549.
smarter added a commit to dotty-staging/dotty that referenced this issue Sep 22, 2021
Without overloading, a function literal `() => 3` whose expected type is
a Unit-returning SAM gets typed as `() => { 3; () }` as expected, but
with overloading we first type the function literal without an expected
type and then compare it against the formal parameter type.

This commit makes this check succeed by replacing a result type of
`Unit` by `WildcardType` so we end up comparing
`() => Int <:< () => ?` instead of `() => Int <:< () => Unit`.

Fixes scala#13549.
@smarter
Copy link
Member

smarter commented Sep 23, 2021

I gave this an attempt in #13584 which also explains what the issue is, but it broke one project in our community build because overloading resolution in one situation became ambiguous. It might be possible to fix this by tweaking overloading resolution some more but our resolution rules are already very complex so I'm hesitant to pile on more things on top.

I also discovered that Java never does this adaptation of the result type of a lambda:

Assertions.assertThrows(IllegalArgumentException.class, () -> 3);
A.java:5: error: no suitable method found for assertThrows(Class<IllegalArgumentException>,()->3)
    Assertions.assertThrows(IllegalArgumentException.class, () -> 3);
              ^
    method Assertions.<T#1>assertThrows(Class<T#1>,Executable) is not applicable
      (cannot infer type-variable(s) T#1
        (argument mismatch; bad return type in lambda expression
          int cannot be converted to void))
    method Assertions.<T#2>assertThrows(Class<T#2>,Executable,String) is not applicable
      (cannot infer type-variable(s) T#2
        (actual and formal argument lists differ in length))
    method Assertions.<T#3>assertThrows(Class<T#3>,Executable,Supplier<String>) is not applicable
      (cannot infer type-variable(s) T#3
        (actual and formal argument lists differ in length))
  where T#1,T#2,T#3 are type-variables:
    T#1 extends Throwable declared in method <T#1>assertThrows(Class<T#1>,Executable)
    T#2 extends Throwable declared in method <T#2>assertThrows(Class<T#2>,Executable,String)
    T#3 extends Throwable declared in method <T#3>assertThrows(Class<T#3>,Executable,Supplier<String>)

To me this is a huge point in favor of staying with the current behavior because having overloading resolution behaves like Java ensures that APIs designed for Java will work the same in Scala, whereas if we stray from that we can easily break them as evidenced by the failure in #13584.

Therefore I'm closing this issue as "won't fix", although you're welcome to contest that and keep discussing here or on https://contributors.scala-lang.org/

@jlprat
Copy link
Author

jlprat commented Sep 23, 2021

jlprat added a commit to jlprat/kafka that referenced this issue Apr 6, 2022
This is a reported bug that unfortunately can't be fixed easily without breakage on Scala's side. For further information check scala/scala3#13549
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.

2 participants