Skip to content

Emit mixin forwarders as ordinary, non-bridge methods again #21890

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions compiler/src/dotty/tools/backend/jvm/BCodeHelpers.scala
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import dotty.tools.dotc.core.Types.*
import dotty.tools.dotc.core.TypeErasure
import dotty.tools.dotc.transform.GenericSignatures
import dotty.tools.dotc.transform.ElimErasedValueType
import dotty.tools.dotc.transform.Mixin
import dotty.tools.io.AbstractFile
import dotty.tools.dotc.report

Expand Down Expand Up @@ -395,12 +396,20 @@ trait BCodeHelpers extends BCodeIdiomatic {
*/
def getGenericSignature(sym: Symbol, owner: Symbol): String = {
atPhase(erasurePhase) {
val memberTpe =
def computeMemberTpe(): Type =
if (sym.is(Method)) sym.denot.info
else if sym.denot.validFor.phaseId > erasurePhase.id && sym.isField && sym.getter.exists then
// Memoization field of getter entered after erasure, see run/i17069 for an example
sym.getter.denot.info.resultType
else owner.denot.thisType.memberInfo(sym)

val memberTpe = if sym.is(MixedIn) then
mixinPhase.asInstanceOf[Mixin].mixinForwarderGenericInfos.get(sym) match
case Some(genericInfo) => genericInfo
case none => computeMemberTpe()
else
computeMemberTpe()

getGenericSignatureHelper(sym, owner, memberTpe).orNull
}
}
Expand Down Expand Up @@ -491,7 +500,7 @@ trait BCodeHelpers extends BCodeIdiomatic {
report.debuglog(s"Potentially conflicting names for forwarders: $conflictingNames")

for (m0 <- sortedMembersBasedOnFlags(moduleClass.info, required = Method, excluded = ExcludedForwarder)) {
val m = if (m0.is(Bridge)) m0.nextOverriddenSymbol else m0
val m = if (m0.isOneOf(Bridge | MixedIn)) m0.nextOverriddenSymbol else m0
if (m == NoSymbol)
report.log(s"$m0 is a bridge method that overrides nothing, something went wrong in a previous phase.")
else if (m.isType || m.is(Deferred) || (m.owner eq defn.ObjectClass) || m.isConstructor || m.name.is(ExpandedName))
Expand All @@ -507,10 +516,7 @@ trait BCodeHelpers extends BCodeIdiomatic {
// we generate ACC_SYNTHETIC forwarders so Java compilers ignore them.
val isSynthetic =
m0.name.is(NameKinds.SyntheticSetterName) ||
// Only hide bridges generated at Erasure, mixin forwarders are also
// marked as bridge but shouldn't be hidden since they don't have a
// non-bridge overload.
m0.is(Bridge) && m0.initial.validFor.firstPhaseId == erasurePhase.next.id
m0.is(Bridge)
addForwarder(jclass, moduleClass, m, isSynthetic)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,7 @@ class BTypesFromSymbols[I <: DottyBackendInterface](val int: I, val frontendAcce
// illegal combination of modifiers at the bytecode level so
// suppress final if abstract if present.
&& !sym.isOneOf(AbstractOrTrait)
// Mixin forwarders are bridges and can be final, but final bridges confuse some frameworks
// Bridges can be final, but final bridges confuse some frameworks
&& !sym.is(Bridge), ACC_FINAL)
.addFlagIf(sym.isStaticMember, ACC_STATIC)
.addFlagIf(sym.is(Bridge), ACC_BRIDGE | ACC_SYNTHETIC)
Expand Down
4 changes: 4 additions & 0 deletions compiler/src/dotty/tools/dotc/core/Phases.scala
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,7 @@ object Phases {
private var myErasurePhase: Phase = uninitialized
private var myElimErasedValueTypePhase: Phase = uninitialized
private var myLambdaLiftPhase: Phase = uninitialized
private var myMixinPhase: Phase = uninitialized
private var myCountOuterAccessesPhase: Phase = uninitialized
private var myFlattenPhase: Phase = uninitialized
private var myGenBCodePhase: Phase = uninitialized
Expand Down Expand Up @@ -266,6 +267,7 @@ object Phases {
final def gettersPhase: Phase = myGettersPhase
final def erasurePhase: Phase = myErasurePhase
final def elimErasedValueTypePhase: Phase = myElimErasedValueTypePhase
final def mixinPhase: Phase = myMixinPhase
final def lambdaLiftPhase: Phase = myLambdaLiftPhase
final def countOuterAccessesPhase = myCountOuterAccessesPhase
final def flattenPhase: Phase = myFlattenPhase
Expand Down Expand Up @@ -295,6 +297,7 @@ object Phases {
myErasurePhase = phaseOfClass(classOf[Erasure])
myElimErasedValueTypePhase = phaseOfClass(classOf[ElimErasedValueType])
myPatmatPhase = phaseOfClass(classOf[PatternMatcher])
myMixinPhase = phaseOfClass(classOf[Mixin])
myLambdaLiftPhase = phaseOfClass(classOf[LambdaLift])
myCountOuterAccessesPhase = phaseOfClass(classOf[CountOuterAccesses])
myFlattenPhase = phaseOfClass(classOf[Flatten])
Expand Down Expand Up @@ -551,6 +554,7 @@ object Phases {
def gettersPhase(using Context): Phase = ctx.base.gettersPhase
def erasurePhase(using Context): Phase = ctx.base.erasurePhase
def elimErasedValueTypePhase(using Context): Phase = ctx.base.elimErasedValueTypePhase
def mixinPhase(using Context): Phase = ctx.base.mixinPhase
def lambdaLiftPhase(using Context): Phase = ctx.base.lambdaLiftPhase
def flattenPhase(using Context): Phase = ctx.base.flattenPhase
def genBCodePhase(using Context): Phase = ctx.base.genBCodePhase
Expand Down
29 changes: 28 additions & 1 deletion compiler/src/dotty/tools/dotc/transform/Mixin.scala
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import StdNames.*
import Names.*
import NameKinds.*
import NameOps.*
import Phases.erasurePhase
import ast.Trees.*

import dotty.tools.dotc.transform.sjs.JSSymUtils.isJSType
Expand Down Expand Up @@ -115,6 +116,15 @@ object Mixin {
class Mixin extends MiniPhase with SymTransformer { thisPhase =>
import ast.tpd.*

/** Infos before erasure of the generated mixin forwarders.
*
* These will be used to generate Java generic signatures of the mixin
* forwarders. Normally we use the types before erasure; we cannot do that
* for mixin forwarders since they are created after erasure, and therefore
* their type history does not have anything recorded for before erasure.
*/
val mixinForwarderGenericInfos = MutableSymbolMap[Type]()

override def phaseName: String = Mixin.name

override def description: String = Mixin.description
Expand Down Expand Up @@ -306,8 +316,25 @@ class Mixin extends MiniPhase with SymTransformer { thisPhase =>
for (meth <- mixin.info.decls.toList if needsMixinForwarder(meth))
yield {
util.Stats.record("mixin forwarders")
transformFollowing(DefDef(mkForwarderSym(meth.asTerm, Bridge), forwarderRhsFn(meth)))
transformFollowing(DefDef(mkMixinForwarderSym(meth.asTerm), forwarderRhsFn(meth)))
}

def mkMixinForwarderSym(target: TermSymbol): TermSymbol =
val sym = mkForwarderSym(target, extraFlags = MixedIn)
val (infoBeforeErasure, isDifferentThanInfoNow) = atPhase(erasurePhase) {
val beforeErasure = cls.thisType.memberInfo(target)
(beforeErasure, !(beforeErasure =:= sym.info))
}
if isDifferentThanInfoNow then
// The info before erasure would not have been the same as the info now.
// We want to store it for the backend to compute the generic Java signature.
// However, we must still avoid doing that if erasing that signature would
// not give the same erased type. If it doesn't, we'll just give a completely
// incorrect Java signature. (This could be improved by generating dedicated
// bridges, but we don't go that far; scalac doesn't either.)
if TypeErasure.transformInfo(target, infoBeforeErasure) =:= sym.info then
mixinForwarderGenericInfos(sym) = infoBeforeErasure
sym

cpy.Template(impl)(
constr =
Expand Down
2 changes: 0 additions & 2 deletions compiler/test/dotc/run-test-pickling.excludelist
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,11 @@ i9473.scala
i13433.scala
i13433b.scala
macros-in-same-project1
mixin-forwarder-overload
t10889
t3452d
t3452e
t3452g
t7374
t8905
tuple-drop.scala
tuple-ops.scala
tuple-ops.scala
Expand Down
1 change: 1 addition & 0 deletions tests/pos/11484/A_2.java
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
public class A_2 extends C<String> { }
6 changes: 6 additions & 0 deletions tests/pos/11484/C_1.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
class B[A]
sealed trait T[A] {
def overloaded(that: List[T[A]]): T[A] = that.head
def overloaded(that: List[B[A]]): B[A] = that.head
}
abstract class C[A] extends T[A]
1 change: 1 addition & 0 deletions tests/pos/11512/A_2.java
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
public class A_2 extends C { }
7 changes: 7 additions & 0 deletions tests/pos/11512/C_1.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
trait T { this: U =>
def m: Int
}
trait U {
def m: Int = ???
}
abstract class C extends U with T
18 changes: 18 additions & 0 deletions tests/pos/mixin-generic-extended-by-java/ExtensionId_1.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
trait Extension

class ClassicActorSystemProvider

/**
* Identifies an Extension
* Lookup of Extensions is done by object identity, so the Id must be the same wherever it's used,
* otherwise you'll get the same extension loaded multiple times.
*/
trait ExtensionId[T <: Extension] {

def get(system: ClassicActorSystemProvider): T = ???
}

/**
* Java API for ExtensionId
*/
abstract class AbstractExtensionId[T <: Extension] extends ExtensionId[T]
8 changes: 8 additions & 0 deletions tests/pos/mixin-generic-extended-by-java/JavaExtension_2.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@

public class JavaExtension_2 {
static class TestExtensionId extends AbstractExtensionId<TestExtension> {
}

static class TestExtension implements Extension {
}
}
17 changes: 17 additions & 0 deletions tests/run/i19270.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// scalajs: --skip

trait T {
def foo(x: Int): Int = x + 1
}

class C extends T

object Test {
def main(args: Array[String]): Unit = {
println("i19270")
val m = classOf[C].getDeclaredMethod("foo", classOf[Int])
assert(m.getDeclaringClass() == classOf[C], m.getDeclaringClass())
assert(!m.isBridge(), "foo should not have the ACC_BRIDGE flag")
assert(!m.isSynthetic(), "foo should not have the ACC_SYNTHETIC flag")
}
}
16 changes: 16 additions & 0 deletions tests/run/mixin-bridge-methods.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// scalajs: --skip

trait Foo {
def getFoo() = "foo"
}

class Sub extends Foo {
def getBar() = "bar"
}

object Test {
def main(args: Array[String]): Unit = {
val ms = classOf[Sub].getDeclaredMethods
assert(ms forall (x => !x.isBridge), ms mkString " ")
}
}
19 changes: 19 additions & 0 deletions tests/run/mixin-final-def-object-lucre.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
trait EventLike

trait GrandParent:
def changed: EventLike

trait HasChanged extends GrandParent:
override def changed: EventLike

abstract class Parent extends GrandParent:
object changed extends EventLike

class Child extends Parent with HasChanged

object Test:
def main(args: Array[String]): Unit =
val child = Child()
println(child.changed)
println((child: HasChanged).changed)
end Test
9 changes: 0 additions & 9 deletions tests/run/mixin-forwarder-overload/A.scala

This file was deleted.

9 changes: 0 additions & 9 deletions tests/run/mixin-forwarder-overload/Test.java

This file was deleted.

14 changes: 7 additions & 7 deletions tests/run/mixin-signatures.check
Original file line number Diff line number Diff line change
@@ -1,19 +1,19 @@
class Test$bar1$ {
public java.lang.String Test$bar1$.f(java.lang.Object)
public java.lang.Object Test$bar1$.f(java.lang.Object) <bridge> <synthetic>
public java.lang.String Test$bar1$.f(java.lang.Object) <bridge> <synthetic>
public java.lang.String Test$bar1$.g(java.lang.String)
public java.lang.Object Test$bar1$.g(java.lang.Object) <bridge> <synthetic>
public java.lang.String Test$bar1$.g(java.lang.Object) <bridge> <synthetic>
public java.lang.Object Test$bar1$.h(java.lang.Object) <bridge> <synthetic>
public java.lang.Object Test$bar1$.h(java.lang.Object)
}

class Test$bar2$ {
public java.lang.Object Test$bar2$.f(java.lang.String)
public java.lang.Object Test$bar2$.f(java.lang.Object) <bridge> <synthetic>
public java.lang.Object Test$bar2$.f(java.lang.String) <bridge> <synthetic>
public java.lang.String Test$bar2$.g(java.lang.String)
public java.lang.Object Test$bar2$.g(java.lang.Object) <bridge> <synthetic>
public java.lang.Object Test$bar2$.g(java.lang.String) <bridge> <synthetic>
public java.lang.Object Test$bar2$.h(java.lang.Object) <bridge> <synthetic>
public java.lang.Object Test$bar2$.h(java.lang.Object)
}

class Test$bar3$ {
Expand All @@ -23,7 +23,7 @@ class Test$bar3$ {
public java.lang.String Test$bar3$.g(java.lang.String)
public java.lang.Object Test$bar3$.g(java.lang.Object) <bridge> <synthetic>
public java.lang.String Test$bar3$.g(java.lang.Object) <bridge> <synthetic>
public java.lang.Object Foo3.h(java.lang.Object) <bridge> <synthetic>
public java.lang.Object Foo3.h(java.lang.Object)
}

class Test$bar4$ {
Expand All @@ -33,7 +33,7 @@ class Test$bar4$ {
public java.lang.String Test$bar4$.g(java.lang.String)
public java.lang.Object Test$bar4$.g(java.lang.Object) <bridge> <synthetic>
public java.lang.Object Test$bar4$.g(java.lang.String) <bridge> <synthetic>
public java.lang.Object Foo4.h(java.lang.Object) <bridge> <synthetic>
public java.lang.Object Foo4.h(java.lang.Object)
}

class Test$bar5$ {
Expand All @@ -45,7 +45,7 @@ class Test$bar5$ {
public java.lang.Object Test$bar5$.g(java.lang.Object) <bridge> <synthetic>
public java.lang.Object Test$bar5$.g(java.lang.String) <bridge> <synthetic>
public java.lang.String Test$bar5$.g(java.lang.Object) <bridge> <synthetic>
public java.lang.Object Test$bar5$.h(java.lang.Object) <bridge> <synthetic>
public java.lang.Object Test$bar5$.h(java.lang.Object)
}

interface Foo1 {
Expand Down
18 changes: 0 additions & 18 deletions tests/run/t11485.scala

This file was deleted.

6 changes: 6 additions & 0 deletions tests/run/t3452b-bcode/J_2.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
public class J_2 {
public static void j() {
StringSearch.search("test");
StringSearch.searchC("test");
}
}
17 changes: 17 additions & 0 deletions tests/run/t3452b-bcode/S_1.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
trait Search[M] {
def search(input: M): C[Int] = {
println("Search received: " + input)
null
}
}

class SearchC[M] {
def searchC(input: M): C[Int] = {
println("SearchC received: " + input)
null
}
}

object StringSearch extends SearchC[String] with Search[String]

trait C[T]
7 changes: 7 additions & 0 deletions tests/run/t3452b-bcode/S_3.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// scalajs: --skip

object Test {
def main(args: Array[String]): Unit = {
J_2.j()
}
}
2 changes: 1 addition & 1 deletion tests/run/t3452d/A.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@ trait TraversableLike[A, Repr] {
def tail: Repr = null.asInstanceOf[Repr]
}

abstract class AbstractTrav[A] extends TraversableLike[A, Iterable[A]]
abstract class AbstractTrav[A] extends TraversableLike[A, Traversable[A]]

class C[A] extends AbstractTrav[A]
4 changes: 3 additions & 1 deletion tests/run/t3452d/Test.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
public class Test {
public static void main(String[] args) {
C<String> c = new C<String>();
scala.collection.Iterable<String> ls = c.tail();
// TODO add a bridge during mixin so we can expose
// sharper generic signature for `tail`.
/*Traversable<String>*/ Object ls = c.tail();
}
}
Loading
Loading