Add bridges for overridden methods in lambda indy call #6087

hrhino · 2017-09-18T22:36:13Z

If a SAM trait's abstract method overrides a method in a supertrait while changing the return type, the generated invokedynamic instruction needs to pass the types of the overridden methods to LambdaMetaFactory so that bridge methods can be added to the generated lambda.

Java does this differently: it generates a default method in the subinterface overriding the superinterface method. Theoretically we could also generate the default bridges, but that is a binary-incompatible change, so I think it's safer to just add the bridges in the invokedynamic.

Honestly I'm surprised that this hasn't come up already.

Fixes scala/bug#10512.

retronym · 2017-09-19T03:57:30Z

src/reflect/scala/reflect/internal/Symbols.scala

+      * non-empty to empty following `erasure`; it seems that this is always
+      * initialized pre-erasure, so post-erasure this method is no longer
+      * equivalent to `allOverriddenSymbols.nonEmpty` like it claims.
+      */
    lazy val isOverridingSymbol = (
         canMatchInheritedSymbols
      && owner.ancestors.exists(base => overriddenSymbol(base) != NoSymbol)


Another drive-by warning: this cache can serve up outdated values in multi-Run compilation if the ancestors change! We should store either the Run or maybe the full Period alongside this boolean and recompute if needed, as is done for other caches in *Symbol

I'll put that in a separate commit once I deal with the ComputeBridges suggestions.

I wound up just caching allOverriddenSymbols...

It appears that isOverridingSymbol gets called (after typer) mainly in constructors and explicitouter, and probably at most once per symbol per each of those phases, so I don't feel that bad about moving the cache to allOverriddenSymbols.

retronym · 2017-09-19T04:03:05Z

src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala

@@ -1349,23 +1349,36 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder {
      val constrainedType = MethodBType(lambdaParams.map(p => typeToBType(p.tpe)), typeToBType(lambdaTarget.tpe.resultType)).toASMType
      val samMethodType = methodBTypeFromSymbol(sam).toASMType
      val markers = if (addScalaSerializableMarker) classBTypeFromSymbol(definitions.SerializableClass).toASMType :: Nil else Nil
-      visitInvokeDynamicInsnLMF(bc.jmethod, sam.name.toString, invokedType, samMethodType, implMethodHandle, constrainedType, isSerializable, markers)
+      val overriddenMethods = enteringErasure(sam.allOverriddenSymbols).map(o => methodBTypeFromSymbol(o).toASMType)


It would be ideal to unify the computation of the required bridges with the normal bridge logic in Erasure. We would need to change it to recognize that Function nodes are classes-in-waiting, and as such should have ComputeBridges applied. The listing of required bridges could be attached to the function symbol (probably inside the existing attachment, SAMFunction.)

The implementation of ComputeBridges as a SymbolPair-s walk has an efficiency argument over finding overriden members decl-wise. For SAM functions that won't make a difference, though, as we've only decl.

So the argument for unifying the logic would be more about keeping responsibilities clear, which might help curtail bugs down the track.

lrytz · 2017-09-19T08:14:54Z

LGTM

sjrd · 2017-09-19T08:18:34Z

Theoretically we could also generate the default bridges, but that is a binary-incompatible change

Why would it be binary incompatible to add a default method to an interface that already inherits a signature for that method (although abstract)?

sjrd · 2017-09-19T10:32:47Z

So I checked with Scala.js, and the test case also causes a linking error in Scala.js:

[error] Referring to non-existent method helloworld.JsonEncoderInstances$$$Lambda$1.encode(java.lang.Object)helloworld.JsonValue
[error]   called from helloworld.Test$.delayedEndpoint$helloworld$Test$1()scala.Unit
[error]   called from helloworld.Test$delayedInit$body.apply()java.lang.Object
[error]   called from scala.Function0.apply$mcZ$sp()scala.Boolean
[error]   called from scala.runtime.AbstractFunction0.apply$mcZ$sp()scala.Boolean
...

It seems that, with the fix in this PR, we'll have to duplicate this logic with overriddenSymbols in the Scala.js back-end. I would definitely be nice if the appropriate computation could be done earlier in a unified way.

hrhino · 2017-09-19T10:38:48Z

@sjrd thanks for the heads up. I guessed without checking, since this showed up with the 2.12 LMF-based lambda encoding, that it was JVM-specific. Now I guess not. The signature of computeBridges was a little awkward for me (IIRC it takes a symbol and a tree and mutates them? I'm on my phone...) but that's a good argument for a different fix.

sjrd · 2017-09-19T10:50:13Z

I guessed without checking, since this showed up with the 2.12 LMF-based lambda encoding, that it was JVM-specific. Now I guess not.

For all LMF-based lambdas, Scala.js has to replicate whatever the JVM back-end + LMF itself do in the Scala.js back-end. It's mostly around synthesizeSAMWrapper. It's possible that the solution is really back-end specific, but that we both have the same issue, which seems to be the case here. I'm not saying the back-end solution in this PR is not the correct one; but that if we can somehow compute the relevant information in a unified way, that would help keep the back-ends in sync and avoid duplicated efforts. It has also happened in the past the solution was lying entirely earlier in the pipeline (I can't find the PR/issue at the moment).

hrhino · 2017-09-19T15:55:52Z

@sjrd my bad w/r/t "binary compatibility", I think; it shouldn't matter with the 2.12 trait encoding.

However, there is another consideration here, which is that if the traits are defined in library A, and project B wants to use the lambda syntax, with this change the project B only needs to bump to a newer version of the compiler, whereas with the Java-style default method fix B would need to wait for A to recompile with the newer version and release.

Perhaps this could be revisited for 2.13, which would simplify things for SJS (it wouldn't be a backend change anymore) but it seems awkward for 2.12. If that sounds better I can slap a [nomerge] on this and make that change against 2.13.x.

sjrd · 2017-09-19T15:59:25Z

Ah, good point.

IMO your plan sounds good 👍

hrhino · 2017-09-22T00:20:59Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

+            /* Arguably I should do `fun.setSymbol(samCls)` rather than leaning
+             * on an attachment, but doing that confounds lambdalift's free var
+             * analysis in a way which does not seem to be trivially reparable. */
+            fun.updateAttachment(SAMFunction(samTp, sam, samCls))


@retronym it was plan "B", I'm afraid...

hrhino · 2017-09-23T03:54:30Z

FWIW: with a Statistics.incCounter in the non-cached branch of allOverriddenSymbols:

Before:

(library)
typer:   [info] #overridden symbol calculations: 3413
erasure: [info] #overridden symbol calculations: 20789

(reflect)
typer:   [info] #overridden symbol calculations: 22097
erasure: [info] #overridden symbol calculations: 43493

(compiler)
typer:   [info] #overridden symbol calculations: 45744
erasure: [info] #overridden symbol calculations: 77901

After:

(library)
typer:   [info] #overridden symbol calculations: 965
erasure: [info] #overridden symbol calculations: 18251

(reflect)
typer:   [info] #overridden symbol calculations: 18478
erasure: [info] #overridden symbol calculations: 39577

(compiler)
typer:   [info] #overridden symbol calculations: 40197
erasure: [info] #overridden symbol calculations: 72164

(cumulative stats, of course)

retronym · 2017-11-28T06:38:43Z

src/reflect/scala/reflect/internal/Symbols.scala

+        overridingPeriod = currentPeriod
+        overridingCache = result
+        result
+      }


I'm reluctant to introduce this cache. This method is called on each decl in each class symbol (source or classpath based) during specialization, and these caches will hang around until the end of the compiler. It appears as though the caches would also consume O(N^2) space wrt the length of the override chain (the N-th deepest symbol would have a cache of N-1 elements).

I forget the exact reason that the cache in isOverridingSymbol got in the way of this bugfix, could you refresh my memory? My changes to isOverridingSymbol in #6197 will conflict here.

Yeah, I saw those changes and they're probably better. I did this because I read the "we should" in your comment to mean "I should, and now". I'm just going to drop that commit from this PR.

retronym · 2017-11-28T06:41:10Z

src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala

@@ -1349,23 +1351,37 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder {
      val constrainedType = MethodBType(lambdaParams.map(p => typeToBType(p.tpe)), typeToBType(lambdaTarget.tpe.resultType)).toASMType
      val samMethodType = methodBTypeFromSymbol(sam).toASMType
      val markers = if (addScalaSerializableMarker) classBTypeFromSymbol(definitions.SerializableClass).toASMType :: Nil else Nil
-      visitInvokeDynamicInsnLMF(bc.jmethod, sam.name.toString, invokedType, samMethodType, implMethodHandle, constrainedType, isSerializable, markers)
+      val overriddenMethods =
+        bridges.map(b => methodBTypeFromSymbol(b).toASMType)


I'm pretty sure that methodBTypeFromSymbol is sufficient to ensure that the backend include types within to InnerClasses, @lrytz can you confirm this.

I'll come up with a test to be sure.

retronym · 2017-11-28T06:44:23Z

src/compiler/scala/tools/nsc/transform/Delambdafy.scala

+            case bridge: MethodSymbol if bridge hasFlag BRIDGE => bridge
+          }.toList
+        }
+      }


userSamCls.findMembers(0L, BRIDGE) might also work here (we know the only base class is Object without bridges).

Seems to work, and you're right that our base types are only the SAM trait and Object. I'll change it.

retronym · 2017-11-28T06:46:10Z

src/reflect/scala/reflect/internal/StdAttachments.scala

    *
    * @since 2.12.0-M4
    */
-  case class SAMFunction(samTp: Type, sam: Symbol) extends PlainAttachment
+  case class SAMFunction(samTp: Type, sam: Symbol, samCls: Symbol) extends PlainAttachment


Perhaps we should call this syntheticSamClass here and in vals the reference it (e.g. userSamCls below) to make it really clear that it isn't the functional interface.

retronym · 2017-11-28T06:48:11Z

src/compiler/scala/tools/nsc/typechecker/Typers.scala

+             * to go somewhere. Erasure knows to compute bridges for these classes
+             * just as if they were real templates extending the SAM type. */
+            val samCls = fun.symbol.owner.newClassWithInfo(
+              name = TypeName(s"${samTp.typeSymbol.name}$$${sam.nameString}$$samImpl"),


Do we need a name for this? I'd probably just use tpnme.ANON_FUN_NAME.

We don't. That's better.

retronym · 2017-11-28T06:57:51Z

src/compiler/scala/tools/nsc/transform/Erasure.scala

+      assert(phase == currentRun.erasurePhase, phase)
+      assert(lambdaClass.isClass, lambdaClass)
+      val eb = new EnterBridges(unit, lambdaClass)
+      eb.computeAndEnter(includeDeferred = true)


Could you add a comment on the reason for includeDeferred = true? It isn't clear to me.

Ugh. I recall needing this, but I didn't comment it and now I can't remember. I'll poke at it until I find out why.

Ah. I think it is that at one point I was cloning the SAM's MethodSymbol and not changing its flags, so it would show up deferred here. Good catch.

If a SAM trait's abstract method overrides a method in a supertrait while changing the return type, the generated invokedynamic instruction needs to pass the types of the overridden methods to `LambdaMetaFactory` so that bridge methods can be added to the generated lambda. SAM `Function` trees now have a synthetic `ClassSymbol`, into which we enter an overriding concrete method symbol that represents the runtime LMF-generated implementation of that function. `erasure` then computes bridge methods for that class, which are added to the `LMF.metafactory` call in `jvm`. Java does this instead by generating a default method in the subinterface overriding the superinterface's method. Theoretically, we could generate the bridges in the same way, but that has the downside that the project with the interfaces would need recompiled, which might not be the project that creates the lambdas. Generating bridges the LMF way means that only the classes affected by this bug need to be recompiled to get the fix. Honestly I'm surprised that this hasn't come up already. Fixes scala/bug#10512.

hrhino · 2017-11-30T19:07:47Z

/rebuild

retronym

The end result LGTM!

@adriaanm Could you please review as well, in particular, the changes around inferSamType?

@sjrd I believe this version should be relatively straight forward for you to support in ScalaJS. Out of curiousity, would your life be easier if, under ScalaJS mode, we just desugared user defined SAMs to anonymous classes after typechecking? Or do you have a runtime component analagous to LambdaMetafactory?

hrhino · 2017-12-01T00:49:32Z

@retronym thanks :D Sorry for biting off a bit much.

I'm willing to jump into the scala.js code this weekend to update it for this, assuming that this PR is in vaguely final shape.

sjrd · 2017-12-01T09:41:47Z

Out of curiousity, would your life be easier if, under ScalaJS mode, we just desugared user defined SAMs to anonymous classes after typechecking?

It would, but only ever so slightly. It would allow us to remove synthesizeSAMWrapper, which I mentioned above. But it's a pretty small thing, and pretty clean too. I suspect the degradation in terms of compile time performance (more trees through the pipeline) is not worth it.

What is more annoying for us is actually the early desugaring of SAMs for js.FunctionNs and js.ThisFunctionNs. Those are not JVM SAMs because they inherit from classes, so scalac desugars them. But then we have to reverse-engineer them in the back-end as they must become JavaScript functions. That's done in tryGenAnonFunctionClassGeneric, which is really ugly and fragile, as it can be broken by small encoding changes that would be done in the earlier phases of scalac.

Or do you have a runtime component analagous to LambdaMetafactory?

We might eventually have a link-time component for that, but I'm not sure. For 0.6.x and even until 1.0.0, that is very unlikely to happen.

I'm willing to jump into the scala.js code this weekend to update it for this, assuming that this PR is in vaguely final shape.

Cool :) Make sure to work on the 0.6.x branch if you do. We regularly forward merge 0.6.x into master.

retronym · 2018-01-12T08:22:35Z

@sjrd Just mentioning you to highlight that this is (finally) merged and you can plan the downstream changes.

sjrd · 2018-01-12T08:29:27Z

Thanks. I was keeping an eye on this one ;)

This is current as of scala/scala@09ebc826968b.

sjrd · 2018-01-17T15:57:00Z

FTR, here is the Scala.js PR: scala-js/scala-js#3260

Adapt scalameta custom analyzer to breaking changes in scala/scala#6087.

adriaanm · 2019-01-21T12:28:29Z

I haven't fully gotten to the bottom of this, but is this missing a check that the bridges are indeed for the SAM method (i.e. find member should also pass in sam.name)? This looks to have regressed when bridges for other methods are seen as needed, causing scala/bug#11373

smarter · 2020-10-24T20:29:49Z

Java does this differently: it generates a default method in the subinterface overriding the superinterface method.
...
Perhaps this could be revisited for 2.13, which would simplify things for SJS

Looks like it wasn't as 2.13 contains code from this PR, was this explored at all? Also what happens if I write a lambda in Java that calls a Scala trait with such an overridden method, does Java emits code that crashes at runtime? Asking since this is affecting Dotty too: scala/scala3#10068

lrytz · 2020-10-26T11:07:25Z

was this explored at all?

I don't think so...

what happens if I write a lambda in Java that calls a Scala trait with such an overridden method

It seems javac knows how to handle this. Quick test (lmk if I got it wrong):

$> cat A.scala
trait A {
  def f(): Object
}
trait B extends A {
  def f(): String
}
$> cat Test.java
public class Test {
  public interface JA {
    Object foo();
  }
  public interface JB extends JA {
    String foo();
  }

  static B  b  = () -> "";
  static JB jb = () -> "";

  public static void main(String[] args) {
    System.out.println(Test.b);
    System.out.println(Test.jb);
  }
}
$> sc A.scala
$> javac -cp . Test.java
$> java Test
Test$$Lambda$1/321001045@1b28cdfa
Test$$Lambda$2/1705736037@eed1f14
$> asmc Test.class

...

    INVOKEDYNAMIC f()LB; [
      // handle kind 0x6 : INVOKESTATIC
      java/lang/invoke/LambdaMetafactory.altMetafactory(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;[Ljava/lang/Object;)Ljava/lang/invoke/CallSite;
      // arguments:
      ()Ljava/lang/String;, 
      // handle kind 0x6 : INVOKESTATIC
      Test.lambda$static$0()Ljava/lang/String;, 
      ()Ljava/lang/String;, 
      4, 
      1, 
      ()Ljava/lang/Object;
    ]

...

    INVOKEDYNAMIC foo()LTest$JB; [
      // handle kind 0x6 : INVOKESTATIC
      java/lang/invoke/LambdaMetafactory.metafactory(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/CallSite;
      // arguments:
      ()Ljava/lang/String;, 
      // handle kind 0x6 : INVOKESTATIC
      Test.lambda$static$1()Ljava/lang/String;, 
      ()Ljava/lang/String;
    ]

...

The indy-lambda for B asks for a bridge method, the one for JB not.

smarter · 2020-10-26T12:33:43Z

Interesting, so it seems that no matter what we do we still need to be able to generate indy-lambdas that ask for bridges in the backend. I assume javac does that for backward-compatibility with pre-java8 interfaces?

smarter · 2020-10-26T13:44:14Z

Thinking about this more, it seems that we cannot assume that a sub-interface coming from Java implements the necessary bridge(s) and so we always need to do the altMetafactory dance: the problem is that under separate compilation we can easily check if the subinterface contains bridges, but under joint compilation of java/scala sources we don't have this information, and we cannot assume that the bridge is always there since it's going to depend on the version of javac (and presumably isn't something mandated by the spec in any version although I haven't checked), to keep the compilation output stable we cannot do something different under joint or separate compilation, therefore we have to be pessimistic and never assume that the bridge is present.

Partially based on scala/scala#6087 Fixes scala#10068 Fixes scala#11676

scala-jenkins added this to the 2.12.5 milestone Sep 18, 2017

retronym reviewed Sep 19, 2017

View reviewed changes

scala-jenkins added the reviewed label Sep 19, 2017

smarter mentioned this pull request Sep 19, 2017

Meta-issue: Port Backend improvements scala/scala3#3113

Closed

10 tasks

hrhino force-pushed the bugfix/t10512 branch 2 times, most recently from 4c9aae9 to 4a03e6d Compare September 22, 2017 00:20

hrhino commented Sep 22, 2017

View reviewed changes

retronym reviewed Nov 28, 2017

View reviewed changes

hrhino force-pushed the bugfix/t10512 branch from 4a03e6d to 9812c7d Compare November 29, 2017 21:56

hrhino removed the reviewed label Nov 30, 2017

retronym approved these changes Dec 1, 2017

View reviewed changes

retronym requested a review from adriaanm December 1, 2017 00:43

retronym merged commit 9b4554d into scala:2.12.x Jan 12, 2018

SethTisue added the release-notes worth highlighting in next release notes label Jan 16, 2018

SethTisue mentioned this pull request Jan 17, 2018

regression: scalameta(-2) started failing scala/community-build#663

Closed

hrhino added a commit to hrhino/scalameta that referenced this pull request Jan 17, 2018

Adapt scalameta custom analyzer to breaking changes in scala/scala#6087.

f0935ff

This is current as of scala/scala@09ebc826968b.

sjrd mentioned this pull request Jan 17, 2018

SAM and subtyping causes linking errors scala-js/scala-js#3259

Closed

hrhino deleted the bugfix/t10512 branch January 17, 2018 16:02

SethTisue added a commit to scalacommunitybuild/scalameta that referenced this pull request Jan 17, 2018

Merge pull request #3 from hrhino/cb663

8ee19ff

Adapt scalameta custom analyzer to breaking changes in scala/scala#6087.

lrytz added 2.12 and removed 2.12 labels Mar 14, 2018

SethTisue removed the release-notes worth highlighting in next release notes label Mar 15, 2018

adriaanm mentioned this pull request Jan 21, 2019

LMF only builds bridges for SAMmy #7668

Merged

griggt mentioned this pull request Oct 24, 2020

Lambda syntax for SAM crashes with AbstractMethodError scala/scala3#10068

Closed

prolativ added a commit to dotty-staging/dotty that referenced this pull request Mar 22, 2021

Fix bytecode generation for Single Abstract Method lambdas

7622781

Partially based on scala/scala#6087 Fixes scala#10068 Fixes scala#11676

prolativ added a commit to dotty-staging/dotty that referenced this pull request Mar 22, 2021

Fix bytecode generation for Single Abstract Method lambdas

bb77a29

Partially based on scala/scala#6087 Fixes scala#10068 Fixes scala#11676

prolativ mentioned this pull request Mar 22, 2021

Fix bytecode generation for Single Abstract Method lambdas scala/scala3#11839

Merged

Add bridges for overridden methods in lambda indy call #6087

Add bridges for overridden methods in lambda indy call #6087

Conversation

hrhino commented Sep 18, 2017

retronym Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

retronym Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrytz commented Sep 19, 2017

sjrd commented Sep 19, 2017

sjrd commented Sep 19, 2017

hrhino commented Sep 19, 2017

sjrd commented Sep 19, 2017

hrhino commented Sep 19, 2017

sjrd commented Sep 19, 2017

Choose a reason for hiding this comment

hrhino commented Sep 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

retronym Nov 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hrhino commented Nov 30, 2017

retronym left a comment

Choose a reason for hiding this comment

hrhino commented Dec 1, 2017

sjrd commented Dec 1, 2017

retronym commented Jan 12, 2018

sjrd commented Jan 12, 2018

sjrd commented Jan 17, 2018

adriaanm commented Jan 21, 2019

smarter commented Oct 24, 2020

lrytz commented Oct 26, 2020

smarter commented Oct 26, 2020

smarter commented Oct 26, 2020

retronym Sep 19, 2017 •

edited

Loading

retronym Sep 19, 2017 •

edited

Loading

retronym Nov 28, 2017 •

edited

Loading