Skip to content

Copy the type history of mixed in members from before uncurry.#7752

Closed
sjrd wants to merge 1 commit intoscala:2.13.xfrom
sjrd:retain-mixedin-info-before-uncurry
Closed

Copy the type history of mixed in members from before uncurry.#7752
sjrd wants to merge 1 commit intoscala:2.13.xfrom
sjrd:retain-mixedin-info-before-uncurry

Conversation

@sjrd
Copy link
Copy Markdown
Member

@sjrd sjrd commented Feb 14, 2019

Previously, the type history was only copied from before erasure. This is necessary for Java interop, to correctly emit generic Java signatures for the relevant members.

Scala.js wants more, because accurate JavaScript interop relies on the type history from before uncurry rather than before erasure. This commit therefore extends the type history of mixed in members all the way to uncurry.

Fixes scala-js/scala-js#3538.

I am not sure how to test this within this repository, as the change only affects Scala.js. I have tested it within Scala.js with:

  1. sbt publishLocal for this PR
  2. put this content in examples/helloworld/HelloWorld.scala in the Scala.js repo:
package helloworld

import scala.scalajs.js
import js.annotation._

trait Foo {
  @JSExport def broken(args: Int*): Int = args.sum
}

class Foo2 {
  @JSExport def notBroken1(args: Int*): Int = args.sum
}

object Bar extends Foo2 with Foo {
  @JSExport def notBroken2(args: Int*): Int = args.sum
}

object HelloWorld {
  def main(args: Array[String]): Unit = {
    val bar = Bar.asInstanceOf[js.Dynamic]

    println(bar.notBroken1(1, 2, 3))
    println(bar.notBroken2(4, 5, 6))
    println(bar.broken(7, 8, 9))
  }
}
  1. sbt ++2.13.0-pre-SNAPSHOT helloworld/run in the Scala.js repository

The third println crashes with any existing version, and correctly prints 24 with this branch.

If this PR is merged, I'll introduce a dedicated test in Scala.js, which will then be picked up by the community build.

In theory, I believe this commit could also be applied in a minor version of Scala, including 2.12.9. Hence it could also go to 2.13.1 and not in RC1. I'm happy to retarget the PR to any version that you deem appropriate.

@sjrd
Copy the type history of mixed in members from before uncurry.
bb3e93f 
Previously, the type history was only copied from before erasure.
This is necessary for Java interop, to correctly emit generic Java
signatures for the relevant members.

Scala.js wants more, because accurate JavaScript interop relies on
the type history from before *uncurry* rather than before erasure.
This commit therefore extends the type history of mixed in members
all the way to uncurry.

Fixes scala-js/scala-js#3538.
@sjrd sjrd requested review from adriaanm and retronym February 14, 2019 10:34
@scala-jenkins scala-jenkins added this to the 2.13.1 milestone Feb 14, 2019
retronym
retronym approved these changes Feb 15, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@retronym retronym left a comment

Choose a reason for hiding this comment

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

Seems fine to me. I'd be happy with this in 2.13.0 and even a backport to 2.12.x. But I'll let @adriaanm decide where/when to apply.

smarter added a commit to smarter/scala that referenced this pull request Mar 10, 2019
@smarter
Emit mixin forwarders as bridges to remove the need for generic signa…
cd7f12f 
…tures

The compiler spends a lot of effort trying to generate correct generic
signatures for mixin forwarders, but is still not correct as witnessed
by scala/bug#8905 among others. However, it seems that if we just emit
them with the BRIDGE flag set, both javac and ecj (the Eclipse Java
Compiler) will be smart enough to go look for the signature in the
overridden methods, which is exactly what we want.

This means that we should be able to get rid of `cloneBeforeErasure` and
related code, though that would conflict with scala#7752.

Because history always repeats itself, it turns out that this used to be
how mixin forwarders were emitted until
fe94bc7 changed this 7 years ago. The
commit only says that this "has deleterious effects since many tools
ignore bridge methods" which is too vague for me to do anything about.

Fixes scala/bug#8905 and probably countless others.
smarter added a commit to smarter/scala that referenced this pull request Mar 10, 2019
@smarter
Emit mixin forwarders as bridges to avoid needing generic signatures
46b143e 
The compiler spends a lot of effort trying to generate correct generic
signatures for mixin forwarders, but is still not correct as witnessed
by scala/bug#8905 among others. However, it seems that if we just emit
them with the BRIDGE flag set, both javac and ecj (the Eclipse Java
Compiler) will be smart enough to go look for the signature in the
overridden methods, which is exactly what we want.

This means that we should be able to get rid of `cloneBeforeErasure` and
related code, though that would conflict with scala#7752.

Because history always repeats itself, it turns out that this used to be
how mixin forwarders were emitted until
fe94bc7 changed this 7 years ago. The
commit only says that this "has deleterious effects since many tools
ignore bridge methods" which is too vague for me to do anything about.

Fixes scala/bug#8905 and probably countless others.
@smarter smarter mentioned this pull request Mar 10, 2019
Merged
smarter added a commit to smarter/scala that referenced this pull request Mar 10, 2019
@smarter
Emit mixin forwarders as bridges to avoid needing generic signatures
06aee8c 
The compiler spends a lot of effort trying to generate correct generic
signatures for mixin forwarders, but is still not correct as witnessed
by scala/bug#8905 among others. However, it seems that if we just emit
them with the BRIDGE flag set, both javac and ecj (the Eclipse Java
Compiler) will be smart enough to go look for the signature in the
overridden methods, which is exactly what we want.

This means that we should be able to get rid of `cloneBeforeErasure` and
related code, though that would conflict with scala#7752.

Because history always repeats itself, it turns out that this used to be
how mixin forwarders were emitted until
fe94bc7 changed this 7 years ago. The
commit only says that this "has deleterious effects since many tools
ignore bridge methods" which is too vague for me to do anything about.

Fixes scala/bug#8905 and probably countless others.
smarter added a commit to smarter/scala that referenced this pull request Mar 10, 2019
@smarter
Emit mixin forwarders as bridges to avoid needing generic signatures
c86005a 
The compiler spends a lot of effort trying to generate correct generic
signatures for mixin forwarders, but is still not correct as witnessed
by scala/bug#8905 among others. However, it seems that if we just emit
them with the BRIDGE flag set, both javac and ecj (the Eclipse Java
Compiler) will be smart enough to go look for the signature in the
overridden methods, which is exactly what we want.

This means that we should be able to get rid of `cloneBeforeErasure` and
related code, though that would conflict with scala#7752.

Because history always repeats itself, it turns out that this used to be
how mixin forwarders were emitted until
fe94bc7 changed this 7 years ago. The
commit only says that this "has deleterious effects since many tools
ignore bridge methods" which is too vague for me to do anything about.

Fixes scala/bug#8905 and probably countless others.
smarter added a commit to smarter/scala that referenced this pull request Mar 11, 2019
@smarter
Emit mixin forwarders as bridges to avoid needing generic signatures
7699dc5 
The compiler spends a lot of effort trying to generate correct generic
signatures for mixin forwarders, but is still not correct as witnessed
by scala/bug#8905 among others. However, it seems that if we just emit
them with the BRIDGE flag set, both javac and ecj (the Eclipse Java
Compiler) will be smart enough to go look for the signature in the
overridden methods, which is exactly what we want.

This means that we should be able to get rid of `cloneBeforeErasure` and
related code, though that would conflict with scala#7752.

Because history always repeats itself, it turns out that this used to be
how mixin forwarders were emitted until
fe94bc7 changed this 7 years ago. The
commit only says that this "has deleterious effects since many tools
ignore bridge methods" which is too vague for me to do anything about.

Fixes scala/bug#8905 and probably countless others.
smarter added a commit to smarter/scala that referenced this pull request Mar 12, 2019
@smarter
Emit mixin forwarders as bridges to avoid needing generic signatures
3e1f47d 
The compiler spends a lot of effort trying to generate correct generic
signatures for mixin forwarders, but is still not correct as witnessed
by scala/bug#8905 among others. However, it seems that if we just emit
them with the BRIDGE flag set, both javac and ecj (the Eclipse Java
Compiler) will be smart enough to go look for the signature in the
overridden methods, which is exactly what we want.

This means that we should be able to get rid of `cloneBeforeErasure` and
related code, though that would conflict with scala#7752.

Because history always repeats itself, it turns out that this used to be
how mixin forwarders were emitted until
fe94bc7 changed this 7 years ago. The
commit only says that this "has deleterious effects since many tools
ignore bridge methods" which is too vague for me to do anything about.

Fixes scala/bug#8905 and probably countless others.
@sjrd sjrd mentioned this pull request Mar 12, 2019
Merged
@sjrd
Copy link
Copy Markdown
Member Author

sjrd commented Mar 12, 2019

Inspired by the discussion that happened at #7843, I managed to find a complete solution on Scala.js' side. So this PR is superseded by scala-js/scala-js#3584.

Sorry for the time you spent reviewing this PR 😖

@sjrd sjrd closed this Mar 12, 2019
@sjrd sjrd deleted the retain-mixedin-info-before-uncurry branch March 12, 2019 16:15
@SethTisue SethTisue removed this from the 2.13.1 milestone Mar 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@retronym retronym retronym approved these changes

@adriaanm adriaanm Awaiting requested review from adriaanm

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sjrd @retronym @SethTisue @scala-jenkins