Skip to content

Generate anonymous class for SAMs defining narrowed overrides #15461

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Generate anonymous class for SAMs defining narrowed overrides #15461

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 7 additions & 1 deletion compiler/src/dotty/tools/dotc/transform/ExpandSAMs.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class ExpandSAMs extends MiniPhase:
case tpe @ SAMType(_) if tpe.isRef(defn.PartialFunctionClass) =>
val tpe1 = checkRefinements(tpe, fn)
toPartialFunction(tree, tpe1)
case tpe @ SAMType(_) if ExpandSAMs.isPlatformSam(tpe.classSymbol.asClass) =>
case tpe @ SAMType(_) if ExpandSAMs.isPlatformSam(tpe.classSymbol.asClass) && !definesNarrowedOverrides(tpe) =>
checkRefinements(tpe, fn)
tree
case tpe =>
Expand All @@ -66,6 +66,12 @@ class ExpandSAMs extends MiniPhase:
tree
}

private def definesNarrowedOverrides(tpe: Type)(using Context): Boolean =
tpe.decls.exists { sym =>
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that's enough, the decls of a types don't include inherited member, so what happens in the test case if I have trait Foo2 extends Foo and I try to make a SAM of Foo2? At the same time, the check seems too broad: we only need to worry about the type of the single abstract method we're implementing, for all other methods in the trait, it's fine if they're covariantly overridden.
Maybe try to have a look at what logic Scala 2 uses (probably in Uncurry?) and try to match that?

By the way, since Jave also has covariant overrides, the JVM does have some support for generating bridges in lambdas for this usecase (grep for bridge in https://docs.oracle.com/javase/8/docs/api/java/lang/invoke/MethodHandles.html), and both Scala 2 and Scala 3 actually have code to deal with that, so I'm not sure why it's not being used in this example for Scala 2:

Copy link
Contributor Author

@WojciechMazur WojciechMazur Jun 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@smarter from what I've observed the diverged behavior starts when checking if the SAM can be compiled to pure interface. In Scala 2 it involves checking if SAM has no primary constructor https://github.com/scala/scala/blob/92f1948cb746b1afbf71c90bde0f20e577b8a9fd/src/compiler/scala/tools/nsc/transform/Erasure.scala#L1418-L1421

In Scala 3 however, we use NoInits for that kind of check. The problem is that the primary constructor always exits in both (Foo and Foo2) and are marked as no inits.
https://github.com/WojciechMazur/dotty/blob/f3b66e148efeed4c11958c35974e52dffbf93744/compiler/src/dotty/tools/dotc/config/JavaPlatform.scala#L41-L46

I've seen a 1 major difference between Scala 2 and 3 in this logic. If we would try to use other SAMs, in a similar way eg:

trait SimpleNamed:
  def foo(x: String): String

trait Foo3 extends SimpleNamed

then both of them would have flags set to PureInterface in Dotty and no primary constructor in Scala 2 , which would allow using it in LambdaMetaFactory. Primary constructor is always present in Dotty
On the other hand, Foo from the original snippet would have a primary constructor in Scala 2 (because of abstract def me: Named) which forces expanding closure to an anonymous class, while in Scala 3 it is a NoInitsTrait (not a PureInterface) which leads to the evaluation of isSam test to true.

Do you think isSam test should be more strict and allow only for NoInitsInterfaces instead of NoInitsTrait for SAM class symbol (it fixes Foo case) and also require NoInitsInterface or recursive isSam test in all directly inherited traits instead of NoInits (to fix Foo2 case)?

I'm not sure how would such a change be binary compatible with previous Scala version and is it not too strict, however it looks like Scala 2 generate anonymous class wrapper for all traits and uses LambdaFactory only for interfaces.

val resultType = sym.info.resultType
sym.allOverriddenSymbols.exists(resultType <:< _.info.resultType)
}

/** A partial function literal:
*
* ```
Expand Down
15 changes: 15 additions & 0 deletions tests/run/i15402.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
trait Named:
def me: Named

trait Foo extends Named:
def me: Foo = this
def foo(x: String): String

class Names(xs: List[Named]):
def mkString = xs.map(_.me).mkString(",")

object Names:
def single[T <: Named](t: T): Names = Names(List(t))

@main def Test() =
Names.single[Foo](x => x).mkString