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

linking error to java's Consumer.accept when using Scala.js #16065

Closed
jv3x opened this issue Sep 18, 2022 · 6 comments · Fixed by #18201
Closed

linking error to java's Consumer.accept when using Scala.js #16065

jv3x opened this issue Sep 18, 2022 · 6 comments · Fixed by #18201
Assignees
Milestone

Comments

@jv3x
Copy link

jv3x commented Sep 18, 2022

In scala3/js (scala 3.2.0, scalajs 1.11.0), but not in scala2/js, there is linking error "Referring to non-existent method ....accept(java.lang.Object)void" in the following:

  def foo(): Unit = {
    val set = new util.LinkedHashSet[String]
    set.forEach(bar)
  }

  def bar(v: String): Unit = ()

I can workaround it like this in foo:

  val barc: util.function.Consumer[String] = bar
  set.forEach(barc)

There is "(modName / Compile / fastLinkJS) There were linking errors". Couldn't reproduce on the JVM.

Sorry if reporting to wrong repo, scalajs says if it is only on 3.x, we should report here.

@jv3x jv3x added itype:bug stat:needs triage Every issue needs to have an "area" and "itype" label labels Sep 18, 2022
@smarter smarter changed the title linking error to java's Consumer.accept linking error to java's Consumer.accept when using Scala.js Sep 18, 2022
@smarter smarter added area:scala.js and removed stat:needs triage Every issue needs to have an "area" and "itype" label labels Sep 18, 2022
@sjrd
Copy link
Member

sjrd commented Sep 19, 2022

I can reproduce on the JVM with the following minimization:

trait MyConsumer[T] {
  var x: Int = 1
  def accept(x: T): Unit
}

object Test {
  def main(args: Array[String]): Unit = {
    val c: MyConsumer[_ >: String] = x => ()
    c.accept("foo")
  }
}

Mandatory ingredients:

  • A Scala-level SAM that is not a JVM SAM (or a platform SAM in the general case) -> removing the var x: Int = 1 definition makes the test pass.
  • The wildcard argument in MyConsumer[_ >: String] -> using MyConsumer[String] instead makes the test pass

The workaround from the original post works because it uses an expected type of Consumer[String], not Consumer[_ >: String].


Showing the trees before and after erasure definitely paints erasure as the culprit:

[[syntax trees at end of MegaPhase{pruneErasedDefs, uninitialized, inlinePatterns, vcInlineMethods, seqLiterals, intercepted, getters, specializeFunctions, specializeTuples, liftTry, collectNullableFields, elimOuterSelect, resolveSuper, functionXXLForwarders, paramForwarding, genericTuples, letOverApply, arrayConstructors}]] // tests\run\hello.scala
package <empty> {
  @SourceFile("tests/run/hello.scala") trait MyConsumer[T]() extends Object {
    private type T
    def x: Int = 1
    def x_=(x$1: Int): Unit = ()
    def accept(x: T): Unit
  }
  final lazy module val Test: Test = new Test()
  @SourceFile("tests/run/hello.scala") final module class Test() extends Object(
    )
   {
    private def writeReplace(): AnyRef =
      new scala.runtime.ModuleSerializationProxy(classOf[Test.type])
    def main(args: Array[String]): Unit =
      {
        val c: MyConsumer[? >: String] =
          {
            def $anonfun(x: String): Unit = ()
            {
              final class $anon() extends Object(), MyConsumer[? >: String] {
                final def accept(x: String): Unit = $anonfun(x)
              }
              new Object with MyConsumer[? >: String] {...}()
            }
          }
        c.accept("foo")
      }
  }
}

[[syntax trees at end of                   erasure]] // tests\run\hello.scala
package <empty> {
  @SourceFile("tests/run/hello.scala") trait MyConsumer() extends Object {
    def x(): Int = 1
    def x_=(x$1: Int): Unit = ()
    def accept(x: Object): Unit
  }
  final lazy module val Test: Test = new Test()
  @SourceFile("tests/run/hello.scala") final module class Test() extends Object(
    )
   {
    private def writeReplace(): Object =
      new scala.runtime.ModuleSerializationProxy(classOf[Test])
    def main(args: String[]): Unit =
      {
        val c: MyConsumer =
          {
            def $anonfun(x: String): Unit = ()
            {
              final class $anon() extends Object(), MyConsumer {
                final def accept(x: String): Unit = $anonfun(x)
              }
              new Object with MyConsumer {...}():MyConsumer
            }
          }
        c.accept("foo")
      }
  }
}

Removing the _ >: causes erasure to correctly add a bridge for accept(x: Object): Unit.


So this is not a Scala.js issue, but an erasure issue.

@sjrd
Copy link
Member

sjrd commented Sep 19, 2022

Investigating further, it's not Erasure's fault either. It's either Typer or ExpandSAMs, depending on what are the invariants we want. After ExpandSAMs, the tree for the closure is:

        val c: MyConsumer[? >: String] =
          {
            def $anonfun(x: String): Unit = println(x)
            {
              final class $anon() extends Object(), MyConsumer[? >: String] {
                final def accept(x: String): Unit = $anonfun(x)
              }
              new Object with MyConsumer[? >: String] {...}()
            }
          }

which is wrong: it has MyConsumer[? >: String] in the extends clause, but accept takes a String. In fact, if we write this by hand, RefChecks complains:

-- Error: tests\run\i16065.scala:10:37 -----------------------------------------
10 |    val c: MyConsumer[_ >: String] = new MyConsumer[_ >: String] {
   |                                     ^
   |object creation impossible, since def accept(x: T): Unit in trait MyConsumer is not defined
   |(Note that
   | parameter T in def accept(x: T): Unit in trait MyConsumer does not match
   | parameter String in def accept(x: String): Unit in anonymous class Object with MyConsumer[? >: String] {...}
   | )

So why does ExpandSAMs generate that? Well because already after Typer, the tree is dubious:

        val c: MyConsumer[? >: String <: Any] =
          {
            def $anonfun(x: String): Unit = println(x)
            closure($anonfun:MyConsumer[? >: String])
          }

The closure node already mentions MyConsumer[? >: String], but points to $anonfun(String).

I suspect this portion of the typer:
https://github.com/lampepfl/dotty/blob/c3ba2f4bf137e6834f081aaf9927e1ffe9a48e13/compiler/src/dotty/tools/dotc/typer/Typer.scala#L1517-L1531
In particular, note that with isWildcardClassSAM it rejects a ? if MyConsumer is a class, but explicitly allows a trait. Sure enough, if we define MyConsumer as a class, we get:

-- Error: tests\run\i16065.scala:8:52 ------------------------------------------
8 |    val c: MyConsumer[_ >: String] = x => println(x)
  |                                                    ^
  |result type of lambda is an underspecified SAM type MyConsumer[? >: String]

Allowing wildcards for traits was most likely done to support Java-style closures, which use use-site variance everywhere. And rejecting classes was most likely done as a proxy to detecting things that would not be LambdaMetaFactory-capable. For LMF stuff, it "works out", because bridges are taken care of by the backend, who doesn't care about wildcards. But clearly, that proxy is not good enough, since it allows the MyConsumer in my repro.

We might think that we should reject non-LMF-capable things there instead of just classes, but that would mean that what SAMs are actually language-level SAMs becomes platform-dependent, and that's very, very bad.

So the solution is not to reject classes, and instead correctly get rid of such wildcards by choosing the appropriate bound. Upper bound if it is used in covariant position in the SAM type, or Lower bound in contravariant. If used in both, the code must be rejected.


At this point, this issue exceeds my area of expertise, so I will throw it back to someone typer-savvy. To spare you the trouble, here are test cases:

// i16065.scala

abstract class MyClassConsumer[T] {
  def accept(x: T): Unit
}

trait MyTraitConsumer[T] {
  var x: Int = 1
  def accept(x: T): Unit
}

abstract class MyClassProducer[T] {
  def produce(): T
}

trait MyTraitProducer[T] {
  var x: Int = 1
  def produce(): T
}

object Test {
  def main(args: Array[String]): Unit = {
    val c1: MyClassConsumer[_ >: String] = x => println(x)
    c1.accept("MyClassConsumer")

    val c2: MyTraitConsumer[_ >: String] = x => println(x)
    c2.accept("MyTraitConsumer")

    val p1: MyClassProducer[_ <: String] = () => "MyClassProducer"
    println(p1.produce())

    val p2: MyTraitProducer[_ <: String] = () => "MyTraitProducer"
    println(p2.produce())
  }
}
// i16065.check
MyClassConsumer
MyTraitConsumer
MyClassProducer
MyTraitProducer

@sjrd sjrd assigned odersky and unassigned sjrd Sep 19, 2022
@smarter
Copy link
Member

smarter commented Sep 19, 2022

Thanks for the minimization and investigation, another piece of the puzzle here is the SAMType extractor which is where we strip the wildcards from the method type we use to represent the closure:
https://github.com/lampepfl/dotty/blob/c3ba2f4bf137e6834f081aaf9927e1ffe9a48e13/compiler/src/dotty/tools/dotc/core/Types.scala#L5414-L5430
Either we should instead strip wildcards from Closure#tpt (so generate closure($anonfun:MyConsumer[String]) in your example) or ExpandSAM should strip wildcards itself (because wildcards are not supposed to be allowed in the extends clause)

@odersky
Copy link
Contributor

odersky commented Sep 19, 2022

Is it OK to assign it to you @smarter?

@smarter
Copy link
Member

smarter commented Sep 19, 2022

Sure.

@sjrd
Copy link
Member

sjrd commented Feb 2, 2023

This came back again as #16388.

smarter added a commit to dotty-staging/dotty that referenced this issue Jul 13, 2023
When typing a closure with an expected type containing a wildcard, the closure
type itself should not contain wildcards, because it might be expanded to an
anonymous class extending the closure type (this happens on non-JVM backends as
well as on the JVM itself in situations where a SAM trait does not compile down
to a SAM interface).

We were already approximating wildcards in the method type returned by the
SAMType extractor, but to fix this issue we had to change the extractor to
perform the approximation on the expected type itself to generate a valid
parent type. The SAMType extractor now returns both the approximated parent
type and the type of the method itself.

The wildcard approximation analysis relies on a new `VarianceMap` opaque type
extracted from Inferencing#variances.

Fixes scala#16065.
Fixes scala#18096.
smarter added a commit to dotty-staging/dotty that referenced this issue Jul 13, 2023
When typing a closure with an expected type containing a wildcard, the closure
type itself should not contain wildcards, because it might be expanded to an
anonymous class extending the closure type (this happens on non-JVM backends as
well as on the JVM itself in situations where a SAM trait does not compile down
to a SAM interface).

We were already approximating wildcards in the method type returned by the
SAMType extractor, but to fix this issue we had to change the extractor to
perform the approximation on the expected type itself to generate a valid
parent type. The SAMType extractor now returns both the approximated parent
type and the type of the method itself.

The wildcard approximation analysis relies on a new `VarianceMap` opaque type
extracted from Inferencing#variances.

Fixes scala#16065.
Fixes scala#18096.
smarter added a commit to dotty-staging/dotty that referenced this issue Jul 14, 2023
When typing a closure with an expected type containing a wildcard, the closure
type itself should not contain wildcards, because it might be expanded to an
anonymous class extending the closure type (this happens on non-JVM backends as
well as on the JVM itself in situations where a SAM trait does not compile down
to a SAM interface).

We were already approximating wildcards in the method type returned by the
SAMType extractor, but to fix this issue we had to change the extractor to
perform the approximation on the expected type itself to generate a valid
parent type. The SAMType extractor now returns both the approximated parent
type and the type of the method itself.

The wildcard approximation analysis relies on a new `VarianceMap` opaque type
extracted from Inferencing#variances.

Fixes scala#16065.
Fixes scala#18096.
smarter added a commit to dotty-staging/dotty that referenced this issue Jul 15, 2023
When typing a closure with an expected type containing a wildcard, the closure
type itself should not contain wildcards, because it might be expanded to an
anonymous class extending the closure type (this happens on non-JVM backends as
well as on the JVM itself in situations where a SAM trait does not compile down
to a SAM interface).

We were already approximating wildcards in the method type returned by the
SAMType extractor, but to fix this issue we had to change the extractor to
perform the approximation on the expected type itself to generate a valid
parent type. The SAMType extractor now returns both the approximated parent
type and the type of the method itself.

The wildcard approximation analysis relies on a new `VarianceMap` opaque type
extracted from Inferencing#variances.

Fixes scala#16065.
Fixes scala#18096.
smarter added a commit that referenced this issue Jul 15, 2023
When typing a closure with an expected type containing a wildcard, the
closure
type itself should not contain wildcards, because it might be expanded
to an
anonymous class extending the closure type (this happens on non-JVM
backends as
well as on the JVM itself in situations where a SAM trait does not
compile down
to a SAM interface).

We were already approximating wildcards in the method type returned by
the
SAMType extractor, but to fix this issue we had to change the extractor
to
perform the approximation on the expected type itself to generate a
valid
parent type. The SAMType extractor now returns both the approximated
parent
type and the type of the method itself.

The wildcard approximation analysis relies on a new `VarianceMap` opaque
type
extracted from Inferencing#variances.

Fixes #16065.
Fixes #18096.
@Kordyjan Kordyjan added this to the 3.4.0 milestone Aug 1, 2023
Kordyjan pushed a commit that referenced this issue Nov 30, 2023
[Cherry-picked 89735d0][modified]

When typing a closure with an expected type containing a wildcard, the closure
type itself should not contain wildcards, because it might be expanded to an
anonymous class extending the closure type (this happens on non-JVM backends as
well as on the JVM itself in situations where a SAM trait does not compile down
to a SAM interface).

We were already approximating wildcards in the method type returned by the
SAMType extractor, but to fix this issue we had to change the extractor to
perform the approximation on the expected type itself to generate a valid
parent type. The SAMType extractor now returns both the approximated parent
type and the type of the method itself.

The wildcard approximation analysis relies on a new `VarianceMap` opaque type
extracted from Inferencing#variances.

Fixes #16065.
Fixes #18096.
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