Skip to content

Revert "Emit mixin forwarders as ordinary, non-bridge methods again" #23613

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

Merged
Merged
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
1 change: 0 additions & 1 deletion .gitmodules
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,6 @@
[submodule "community-build/community-projects/scala-parallel-collections"]
path = community-build/community-projects/scala-parallel-collections
url = https://github.com/dotty-staging/scala-parallel-collections.git
branch = serialisation-stability-fix
[submodule "community-build/community-projects/scala-collection-compat"]
path = community-build/community-projects/scala-collection-compat
url = https://github.com/dotty-staging/scala-collection-compat.git
Expand Down
18 changes: 6 additions & 12 deletions compiler/src/dotty/tools/backend/jvm/BCodeHelpers.scala
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ 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 @@ -396,20 +395,12 @@ trait BCodeHelpers extends BCodeIdiomatic {
*/
def getGenericSignature(sym: Symbol, owner: Symbol): String = {
atPhase(erasurePhase) {
def computeMemberTpe(): Type =
val memberTpe =
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 @@ -500,7 +491,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.isOneOf(Bridge | MixedIn)) m0.nextOverriddenSymbol else m0
val m = if (m0.is(Bridge)) 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 @@ -516,7 +507,10 @@ trait BCodeHelpers extends BCodeIdiomatic {
// we generate ACC_SYNTHETIC forwarders so Java compilers ignore them.
val isSynthetic =
m0.name.is(NameKinds.SyntheticSetterName) ||
m0.is(Bridge)
// 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
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)
// Bridges can be final, but final bridges confuse some frameworks
// Mixin forwarders are bridges and 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: 0 additions & 4 deletions compiler/src/dotty/tools/dotc/core/Phases.scala
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,6 @@ 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 @@ -267,7 +266,6 @@ 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 @@ -297,7 +295,6 @@ 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 @@ -553,7 +550,6 @@ 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: 1 addition & 28 deletions compiler/src/dotty/tools/dotc/transform/Mixin.scala
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ 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 @@ -116,15 +115,6 @@ 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 @@ -315,25 +305,8 @@ class Mixin extends MiniPhase with SymTransformer { thisPhase =>
for (meth <- mixin.info.decls.toList if needsMixinForwarder(meth))
yield {
util.Stats.record("mixin forwarders")
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))
transformFollowing(DefDef(mkForwarderSym(meth.asTerm, Bridge), forwarderRhsFn(meth)))
}
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: 2 additions & 0 deletions compiler/test/dotc/run-test-pickling.excludelist
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,13 @@ 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
40 changes: 1 addition & 39 deletions project/Scala2LibraryBootstrappedMiMaFilters.scala
Original file line number Diff line number Diff line change
Expand Up @@ -193,45 +193,7 @@ object Scala2LibraryBootstrappedMiMaFilters {
"scala.util.matching.Regex.Groups", "scala.util.matching.Regex.Match",
"scala.util.package.chaining",
"scala.util.Using.Manager", "scala.util.Using.Releasable", "scala.util.Using#Releasable.AutoCloseableIsReleasable",
).map(ProblemFilters.exclude[MissingFieldProblem]) ++ Seq(
// DirectMissingMethodProblem by flags being changed from ACC_PUBLIC, ACC_BRIDGE, ACC_SYNTHETIC to ACC_PUBLIC in mixin forwarders:
// * from java.util.Comparator:
Seq("reversed", "thenComparing", "thenComparingInt", "thenComparingLong", "thenComparingInt", "thenComparingDouble").flatMap { method =>
Seq(
ProblemFilters.exclude[DirectMissingMethodProblem](s"scala.Enumeration#ValueOrdering.$method"),
ProblemFilters.exclude[DirectMissingMethodProblem](s"scala.concurrent.duration.Deadline#DeadlineIsOrdered.$method"),
ProblemFilters.exclude[DirectMissingMethodProblem](s"scala.concurrent.duration.Duration#DurationIsOrdered.$method"),
ProblemFilters.exclude[DirectMissingMethodProblem](s"scala.concurrent.duration.FiniteDuration#FiniteDurationIsOrdered.$method"),
ProblemFilters.exclude[DirectMissingMethodProblem](s"scala.math.Numeric#*.$method"),
ProblemFilters.exclude[DirectMissingMethodProblem](s"scala.math.Ordering#*.$method"),
)
},
// * from java.util.Spliterator:
Seq("getExactSizeIfKnown", "hasCharacteristics", "getComparator").flatMap { method =>
Seq(
ProblemFilters.exclude[DirectMissingMethodProblem](s"scala.collection.DoubleStepper#DoubleStepperSpliterator.$method"),
ProblemFilters.exclude[DirectMissingMethodProblem](s"scala.collection.IntStepper#IntStepperSpliterator.$method"),
ProblemFilters.exclude[DirectMissingMethodProblem](s"scala.collection.LongStepper#LongStepperSpliterator.$method"),
)
},
// * from java.util.Enumeration:
Seq(
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.convert.JavaCollectionWrappers#IteratorWrapper.asIterator"),
),
// * from java.lang.CharSequence:
Seq(
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.Predef#ArrayCharSequence.isEmpty"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.Predef#SeqCharSequence.isEmpty"),
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.runtime.ArrayCharSequence.isEmpty")
),
// FinalMethodProblem - seems to be a bug in MiMa, as neither new or old version do not have final modifier.
// What did change is that sizeHint, and requireBounds had `final` added, but that does not get reported.
Seq(
ProblemFilters.exclude[FinalMethodProblem]("scala.**.sizeHint$default$2"),
ProblemFilters.exclude[FinalMethodProblem]("scala.collection.mutable.ArrayDeque.requireBounds$default$2")
)
).flatten

).map(ProblemFilters.exclude[MissingFieldProblem])
}
)
}
1 change: 0 additions & 1 deletion tests/pos/11484/A_2.java

This file was deleted.

6 changes: 0 additions & 6 deletions tests/pos/11484/C_1.scala

This file was deleted.

1 change: 0 additions & 1 deletion tests/pos/11512/A_2.java

This file was deleted.

7 changes: 0 additions & 7 deletions tests/pos/11512/C_1.scala

This file was deleted.

18 changes: 0 additions & 18 deletions tests/pos/mixin-generic-extended-by-java/ExtensionId_1.scala

This file was deleted.

8 changes: 0 additions & 8 deletions tests/pos/mixin-generic-extended-by-java/JavaExtension_2.java

This file was deleted.

17 changes: 0 additions & 17 deletions tests/run/i19270.scala

This file was deleted.

16 changes: 0 additions & 16 deletions tests/run/mixin-bridge-methods.scala

This file was deleted.

19 changes: 0 additions & 19 deletions tests/run/mixin-final-def-object-lucre.scala

This file was deleted.

9 changes: 9 additions & 0 deletions tests/run/mixin-forwarder-overload/A.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
case class Foo(x: Int)

trait A[X] {
def concat[Dummy](suffix: Int): Dummy = ???
}

class Bar extends A[Foo] {
def concat(suffix: Int): Foo = Foo(0)
}
9 changes: 9 additions & 0 deletions tests/run/mixin-forwarder-overload/Test.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// scalajs: --skip

public class Test {
public static void main(String[] args) {
Bar bar = new Bar();
Foo x = bar.concat(0);
System.out.println(x);
}
}
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)
public java.lang.Object Test$bar1$.h(java.lang.Object) <bridge> <synthetic>
}

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)
public java.lang.Object Test$bar2$.h(java.lang.Object) <bridge> <synthetic>
}

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)
public java.lang.Object Foo3.h(java.lang.Object) <bridge> <synthetic>
}

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)
public java.lang.Object Foo4.h(java.lang.Object) <bridge> <synthetic>
}

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)
public java.lang.Object Test$bar5$.h(java.lang.Object) <bridge> <synthetic>
}

interface Foo1 {
Expand Down
Loading
Loading