Skip to content
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
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/backend/jvm/BCodeHelpers.scala
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ trait BCodeHelpers(val backendUtils: BackendUtils)(using ctx: Context) extends B
// Mixins are resolved _after_ erasure, so we cannot simply ask for "the information before erasure" for these,
// since that information never existed.
// Thus, we first check if the symbol was specifically marked as having generic information,
if sym.is(MixedIn) then mixinPhase.asInstanceOf[Mixin].mixinGenericInfos.get(sym) match
mixinPhase.asInstanceOf[Mixin].mixinGenericInfos.get(sym) match
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Adding MixedIn to everything that's mixed in causes so many tests to fail... and is not necessary here anyway

// and if so, we use it.
case Some(genericInfo) => return genericInfo
case _ => ()
Expand Down
20 changes: 14 additions & 6 deletions compiler/src/dotty/tools/dotc/transform/Mixin.scala
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import core.*
import MegaPhase.*
import Contexts.*
import Flags.*

import Symbols.*
import SymDenotations.*
import Types.*
Expand All @@ -18,7 +17,6 @@ import NameKinds.*
import NameOps.*
import Phases.erasurePhase
import ast.Trees.*

import dotty.tools.dotc.transform.sjs.JSSymUtils.isJSType

object Mixin {
Expand Down Expand Up @@ -166,6 +164,8 @@ class Mixin extends MiniPhase with SymTransformer { thisPhase =>
val setter = makeTraitSetter(decl.asTerm)
setter.validFor = thisPhase.validFor // validity of setter = next phase up to next transformer afterwards
decls1.enter(setter)
// Re-create the setter from the unerased getter so we can have its unerased form for generic signatures
mixinGenericInfos(setter) = atPhase(erasurePhase) { makeTraitSetter(decl.asTerm).info }
modified = true
if modified then
sym.copySymDenotation(
Expand Down Expand Up @@ -297,8 +297,11 @@ class Mixin extends MiniPhase with SymTransformer { thisPhase =>
Underscore(getter.info.resultType)
// transformFollowing call is needed to make memoize & lazy vals run
val forwarder = mkForwarderSym(getter.asTerm)
val erased = atPhase(erasurePhase) { cls.thisType.memberInfo(getter) }
mixinGenericInfos(forwarder) = erased
// Store the unerased form for generic signature use later,
// but only if it's not private (which we must check at erasure time, as here we've removed that flag already),
// since otherwise it might refer to private classes
if atPhase(erasurePhase) { !getter.is(Private) } then
mixinGenericInfos(forwarder) = atPhase(erasurePhase) { cls.thisType.memberInfo(getter) }
transformFollowing(DefDef(forwarder, rhs))
}
else if wasOneOf(getter, ParamAccessor) then
Expand All @@ -311,8 +314,13 @@ class Mixin extends MiniPhase with SymTransformer { thisPhase =>
val mixinSetters = mixin.info.decls.filter { sym =>
sym.isSetter && (!wasOneOf(sym, Deferred) || sym.name.is(TraitSetterName))
}
for (setter <- mixinSetters)
yield transformFollowing(DefDef(mkForwarderSym(setter.asTerm), unitLiteral.withSpan(cls.span)))
mixinSetters.map(setter => {
val copied = transformFollowing(DefDef(mkForwarderSym(setter.asTerm), unitLiteral.withSpan(cls.span)))
mixinGenericInfos.get(setter) match
case Some(gi) => mixinGenericInfos(copied.symbol) = gi
case None => ()
copied
})

def mixinForwarders(mixin: ClassSymbol): List[Tree] =
for meth <- mixin.info.decls.filter(d => needsMixinForwarder(mixin, d))
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
public void Bar.Foo$_setter_$foo_$eq(scala.Option)
public void Bar.Foo$_setter_$foo_$eq(scala.Option<java.lang.String>)
public scala.Option Bar.foo()
public scala.Option<java.lang.String> Bar.foo()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

❤️

Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
trait Foo {
val foo: Option[String] = ???
}
class Bar extends Foo

object Test:
def main(args: Array[String]): Unit =
classOf[Bar].getMethods.sortBy(_.getName).filter(_.getName.contains("foo")).foreach(m => {
println(m)
println(m.toGenericString)
})
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
Foo:
public abstract void Foo.Foo$_setter_$foo_$eq(scala.Option)
public abstract void Foo.Foo$_setter_$foo_$eq(scala.Option<java.lang.String>)
public abstract scala.Option Foo.foo()
public abstract scala.Option<java.lang.String> Foo.foo()
bar:
public void bar$.Foo$_setter_$foo_$eq(scala.Option)
public void bar$.Foo$_setter_$foo_$eq(scala.Option<java.lang.String>)
public scala.Option bar$.foo()
public scala.Option<java.lang.String> bar$.foo()
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
trait Foo {
val foo: Option[String] = ???
}
object bar extends Foo

object Test:
def main(args: Array[String]): Unit =
println("Foo:")
classOf[Foo].getMethods.sortBy(_.getName).filter(_.getName.contains("foo")).foreach(m => {
println(m)
println(m.toGenericString)
})
println("bar:")
classOf[bar.type].getMethods.sortBy(_.getName).filter(_.getName.contains("foo")).foreach(m => {
println(m)
println(m.toGenericString)
})
Comment on lines +14 to +17
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How about Class.forName("bar") ? The generic signature for bar.foo() seems still a raw type based on javap (above is testing for bar$).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Indeed... There's a whole separate way to create generic signatures for mirror classes. I've spent ~10h trying to fix this and gone nowhere because the interaction of erasure and mixins means we fundamentally have incorrect information once we reach the backend, so we end up in cases where descriptors and generic signatures diverge in ways that aren't correct, e.g., the gensig has String where the descriptor has Object.

I suggest we open an issue and accept this for now.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, for Java interop, the relevant entry point is bar.foo() static forwarder, not bar$.foo(). If the generic signature for bar.foo() remains raw type, I think we should keep #24275 open for tracking the issue.
On the other hand, this PR seems fixing #6350 ? We might wanna add a test for #6350, merge this PR and close it, and keep #24275 open?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

(I've changed the description to say "part of #24275" instead of "fixes")


12 changes: 12 additions & 0 deletions tests/pos/private-class-in-trait.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
trait T {
private val classValue = new PrivateClass
private val objectValue = PrivateObject

private val classOption: Option[PrivateClass] = Some(new PrivateClass)
private val objectOption: Option[PrivateObject.type] = Some(PrivateObject)

private class PrivateClass
private object PrivateObject
}

object Test extends T
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

regression test for some crashes while developing this

Loading