Add safety checks to `samMethod` #6277

anatoliykmetyuk · 2019-04-10T15:12:50Z

sam in samMethod stands for "single abstract method". However,
presently, no check is performed to verify that the abstract method
it finds is indeed single. This commit adds such a check to this method.

As mentioned here, samMethod is unsafe. Since it is only used in genInvokeDynamicLambda, which is only used when generating the bytecode for Closures, this is no immediate problem. However, since this method is a part of a public API, not performing the safety checks can lead to runtime errors if it is used elsewhere incorrectly.

dottybot

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Have an awesome day! ☀️

smarter · 2019-04-11T18:22:48Z

compiler/src/dotty/tools/backend/jvm/DottyBackendInterface.scala

-      toDenot(sym).info.abstractTermMembers.headOption.getOrElse(toDenot(sym).info.member(nme.apply)).symbol
+    def samMethod(): Symbol = {
+      val abstractTermMembers = toDenot(sym).info.abstractTermMembers
+      if (abstractTermMembers.size > 1) abort(s"${sym.show} is not a functional interface. " +


I would pattern-match on abstractTermMembers and have cases for it being empty, one element and multiple elements.

smarter · 2019-04-15T09:43:29Z

There's a failure in the community build:

java.lang.RuntimeException: trait JProcedure1 is not a functional interface. It has the following abstract methods: apply, applyVoid

This happens because dotty-library and the 2.13 standard library are being jointly compiled, so we don't have any bridge for JProcedure1 since it's a .java source, this illustrates exactly the issue I predicted in #6266 (comment)

anatoliykmetyuk · 2019-04-15T11:09:00Z

Why does the community build fail but nothing else; what is special about the community build?

The two deferred methods are a problem though since JProcedure1 is no longer a functional interface.

smarter · 2019-04-15T11:11:59Z

In the community build, we compile the 2.13 standard library, but because we can't just mix a dotty-library compiled with the 2.12 standard library and the 2.13 standard library, we need to compile the dotty-library at the same time, so we end up jointly compiling the sources of the 2.13 standard library and the sources of dotty-library. Hence .java files go through the JavaParser then their compilation unit gets dropped after typer, so we don't see any bridge for them. The fix to samMethod I suggested in #6266 (comment) should fix this.

`sam` in `samMethod` stands for "single abstract method". However, presently, no check is performed to verify that the abstract method it finds is indeed single. This commit adds such a check to this method.

anatoliykmetyuk · 2019-04-15T11:49:32Z

Makes sense. Ok, let's try...

Btw, what's the correct procedure to run the community tests? Locally I am getting different errors but not the one as in the CI

smarter · 2019-04-15T11:51:39Z

community-build/test should work.

anatoliykmetyuk · 2019-04-15T11:58:21Z

Oh, this time it worked...

Hmm, now I'm getting the errors of the following kind:

[error] trait JFunction1$mcII$sp is not a functional interface. It has the following abstract methods: apply, apply$mcII$spjava.lang.RuntimeException: trait JFunction1$mcII$sp is not a functional interface. It has the following abstract methods: apply, apply$mcII$sp

	at dotty.tools.backend.jvm.DottyBackendInterface.abort(DottyBackendInterface.scala:390)
	at dotty.tools.backend.jvm.DottyBackendInterface$$anon$8.$anonfun$samMethod$1(DottyBackendInterface.scala:861)
	at dotty.tools.dotc.core.Periods.atPhase(Periods.scala:25)
	at dotty.tools.dotc.core.Phases.atPhase(Phases.scala:35)
	at dotty.tools.dotc.core.Phases.atPhase$(Phases.scala:34)
	at dotty.tools.dotc.core.Contexts$Context.atPhase(Contexts.scala:71)
	at dotty.tools.backend.jvm.DottyBackendInterface$$anon$8.samMethod(DottyBackendInterface.scala:856)
	at dotty.tools.backend.jvm.DottyBackendInterface$$anon$8.samMethod(DottyBackendInterface.scala:632)
	at dotty.tools.backend.jvm.BCodeBodyBuilder$PlainBodyBuilder.genInvokeDynamicLambda(BCodeBodyBuilder.scala:1472)
	at dotty.tools.backend.jvm.BCodeBodyBuilder$PlainBodyBuilder.genLoad(BCodeBodyBuilder.scala:335)
	at dotty.tools.backend.jvm.BCodeBodyBuilder$PlainBodyBuilder.genLoad(BCodeBodyBuilder.scala:408)
	at dotty.tools.backend.jvm.BCodeBodyBuilder$PlainBodyBuilder.$anonfun$genLoadArguments$1(BCodeBodyBuilder.scala:1051)
	at dotty.tools.backend.jvm.BCodeBodyBuilder$PlainBodyBuilder.$anonfun$genLoadArguments$1$adapted(BCodeBodyBuilder.scala:1051)
	at scala.collection.immutable.List.foreach(List.scala:392)

anatoliykmetyuk · 2019-04-15T12:07:11Z

It seems that we're indeed relying on the bridges in the joint compilation after all. And we're indeed having a bug there which was not previously exposed due to the samMethod not seeing the bridges, not performing safety checks and by coincidence picking up the right method. Each time, for some reason...

Since the last commit, `samMethod` looks for the single abstract method before erasure for correctness, and somehow this caused the implementation of `apply` in the various specialized JFunction classes to not be seen as overrides anymore. Replacing the raw type `JFunction1` by `JFunction1<Object, Object>` in the extends clauses of each interface fixes that (this is always the correct type to use because the generic signature we emit for `Function1[Int, Int]` is `Function1<Object, Object>` and not `Function1<Integer, Integer>`, the rationale for this is explained in scala/scala@e42733e).

smarter · 2019-04-15T17:49:21Z

trait JFunction1$mcII$sp is not a functional interface. It has the following abstract methods: apply, apply$mcII$sp

Just pushed a commit that fixes this.

anatoliykmetyuk · 2019-04-15T18:03:58Z

So no bridges are needed since the types are the same after erasure? Neat!

smarter · 2019-04-15T18:22:53Z

There's one remaining test failure but it's very weird and I don't have time to investigate right now, @nicolasstucki it involves quoting so I'm wondering if you've seen something like this before ?

Test tests/run-with-compiler/i3876-c.scala failed with output:                
6
Exception in thread "main" java.lang.reflect.InvocationTargetException
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:498)
        at dotty.tools.vulpix.ChildJVMMain.runMain(ChildJVMMain.java:40)
        at dotty.tools.vulpix.ChildJVMMain.main(ChildJVMMain.java:47)
Caused by: java.lang.ArrayIndexOutOfBoundsException: 36
        at dotty.tools.dotc.core.Periods.hasSameBaseTypesAs(Periods.scala:47)
        at dotty.tools.dotc.core.SymDenotations$ClassDenotation.baseTypeCache(SymDenotations.scala:1408)
        at dotty.tools.dotc.core.SymDenotations$ClassDenotation.baseTypeOf(SymDenotations.scala:1730)
        at dotty.tools.dotc.core.Types$Type.baseType(Types.scala:925)
        at dotty.tools.dotc.core.TypeOps$AsSeenFromMap.toPrefix$1(TypeOps.scala:46)
        at dotty.tools.dotc.core.TypeOps$AsSeenFromMap.apply(TypeOps.scala:65)
        at dotty.tools.dotc.core.TypeOps$AsSeenFromMap.apply(TypeOps.scala:63)
        at dotty.tools.dotc.core.TypeOps$AsSeenFromMap.apply(TypeOps.scala:35)
        at scala.collection.immutable.List.mapConserve(List.scala:179)
        at dotty.tools.dotc.core.Types$TypeMap.mapOverLambda$1(Types.scala:4437)
        at dotty.tools.dotc.core.Types$TypeMap.mapOver(Types.scala:4441)
        at dotty.tools.dotc.core.TypeOps$AsSeenFromMap.apply(TypeOps.scala:69)
        at dotty.tools.dotc.core.TypeOps.asSeenFrom(TypeOps.scala:30)
        at dotty.tools.dotc.core.Contexts$Context.asSeenFrom(Contexts.scala:71)
        at dotty.tools.dotc.core.Types$Type.asSeenFrom(Types.scala:832)
        at dotty.tools.dotc.core.Denotations$SingleDenotation.computeAsSeenFrom(Denotations.scala:1118)
        at dotty.tools.dotc.core.Denotations$SingleDenotation.computeAsSeenFrom(Denotations.scala:1111)
        at dotty.tools.dotc.core.Denotations$PreDenotation.asSeenFrom(Denotations.scala:128)
        at dotty.tools.dotc.core.SymDenotations$ClassDenotation.findMember(SymDenotations.scala:1725)
        at dotty.tools.dotc.core.Types$Type.go$1(Types.scala:562)
        at dotty.tools.dotc.core.Types$Type.goRefined$1(Types.scala:651)
        at dotty.tools.dotc.core.Types$Type.go$1(Types.scala:581)
        at dotty.tools.dotc.core.Types$Type.findMember(Types.scala:712)
        at dotty.tools.dotc.core.Types$Type.memberBasedOnFlags(Types.scala:545)
        at dotty.tools.dotc.core.Types$Type.member(Types.scala:530)
        at dotty.tools.dotc.typer.TypeAssigner.selectionType(TypeAssigner.scala:238)
        at dotty.tools.dotc.typer.Typer.selectionType(Typer.scala:87)
        at dotty.tools.dotc.typer.TypeAssigner.accessibleSelectionType(TypeAssigner.scala:268)
        at dotty.tools.dotc.typer.Typer.accessibleSelectionType(Typer.scala:87)
        at dotty.tools.dotc.typer.TypeAssigner.assignType(TypeAssigner.scala:299)
        at dotty.tools.dotc.typer.Typer.assignType(Typer.scala:87)
        at dotty.tools.dotc.ast.tpd$.Select(tpd.scala:30)
        at dotty.tools.dotc.ast.tpd$TreeOps$.select$extension3(tpd.scala:794)
        at dotty.tools.dotc.core.quoted.PickledQuotes$.rec$1(PickledQuotes.scala:148)
        at dotty.tools.dotc.core.quoted.PickledQuotes$.rec$1(PickledQuotes.scala:146)
        at dotty.tools.dotc.core.quoted.PickledQuotes$.rec$1(PickledQuotes.scala:136)
        at dotty.tools.dotc.core.quoted.PickledQuotes$.functionAppliedTo(PickledQuotes.scala:150)
        at dotty.tools.dotc.core.quoted.PickledQuotes$.quotedExprToTree(PickledQuotes.scala:51)
        at dotty.tools.dotc.quoted.QuoteCompiler$QuotedFrontend.runOn$$anonfun$1(QuoteCompiler.scala:53)
        at scala.collection.immutable.List.map(List.scala:286)
        at dotty.tools.dotc.quoted.QuoteCompiler$QuotedFrontend.runOn(QuoteCompiler.scala:61)
        at dotty.tools.dotc.Run.runPhases$4$$anonfun$4(Run.scala:158)
        at dotty.runtime.function.JProcedure1.apply(JProcedure1.java:15)
        at dotty.runtime.function.JProcedure1.apply(JProcedure1.java:10)
        at scala.collection.IndexedSeqOptimized.foreach(IndexedSeqOptimized.scala:36)
        at scala.collection.IndexedSeqOptimized.foreach$(IndexedSeqOptimized.scala:33)
        at scala.collection.mutable.ArrayOps$ofRef.foreach(ArrayOps.scala:198)
        at dotty.tools.dotc.Run.runPhases$5(Run.scala:170)
        at dotty.tools.dotc.Run.compileUnits$$anonfun$1(Run.scala:178)
        at dotty.runtime.function.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.java:12)
        at dotty.tools.dotc.util.Stats$.maybeMonitored(Stats.scala:102)
        at dotty.tools.dotc.Run.compileUnits(Run.scala:185)
        at dotty.tools.dotc.Run.compileUnits(Run.scala:125)
        at dotty.tools.dotc.quoted.QuoteCompiler$ExprRun.compileExpr(QuoteCompiler.scala:90)
        at dotty.tools.dotc.quoted.QuoteDriver.withTree(QuoteDriver.scala:73)
        at dotty.tools.dotc.quoted.QuoteDriver.show(QuoteDriver.scala:59)
        at dotty.tools.dotc.quoted.ToolboxImpl$$anon$1.show(ToolboxImpl.scala:31)
        at scala.quoted.Expr.show(Expr.scala:14)
        at Test$.main(i3876-c.scala:13)
        at Test.main(i3876-c.scala)
        ... 6 more

nicolasstucki · 2019-04-17T16:00:20Z

It looks like another instance of #5161

Fix #6277: Add test

dottybot reviewed Apr 10, 2019

View reviewed changes

anatoliykmetyuk self-assigned this Apr 11, 2019

smarter reviewed Apr 11, 2019

View reviewed changes

Add safety checks to samMethod

b1971b4

`sam` in `samMethod` stands for "single abstract method". However, presently, no check is performed to verify that the abstract method it finds is indeed single. This commit adds such a check to this method.

anatoliykmetyuk force-pushed the samMethod-safety branch from 80cf18f to b1971b4 Compare April 15, 2019 11:48

Workaround issue scala#5161

833a71e

anatoliykmetyuk merged commit 2eebb70 into scala:master Apr 18, 2019

anatoliykmetyuk deleted the samMethod-safety branch April 18, 2019 09:53

odersky added a commit to dotty-staging/dotty that referenced this pull request Jul 28, 2019

Fix scala#6277: Add test

91703ae

nicolasstucki added a commit that referenced this pull request Jul 28, 2019

Merge pull request #6956 from dotty-staging/fix-#6227

51ccfb3

Fix #6277: Add test

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add safety checks to `samMethod` #6277

Add safety checks to `samMethod` #6277

Uh oh!

anatoliykmetyuk commented Apr 10, 2019

Uh oh!

dottybot left a comment

Uh oh!

smarter Apr 11, 2019

Uh oh!

smarter commented Apr 15, 2019

Uh oh!

anatoliykmetyuk commented Apr 15, 2019

Uh oh!

smarter commented Apr 15, 2019

Uh oh!

anatoliykmetyuk commented Apr 15, 2019

Uh oh!

smarter commented Apr 15, 2019

Uh oh!

anatoliykmetyuk commented Apr 15, 2019

Uh oh!

anatoliykmetyuk commented Apr 15, 2019 •

edited

Loading

Uh oh!

smarter commented Apr 15, 2019 •

edited

Loading

Uh oh!

anatoliykmetyuk commented Apr 15, 2019

Uh oh!

smarter commented Apr 15, 2019 •

edited

Loading

Uh oh!

nicolasstucki commented Apr 17, 2019

Uh oh!

Uh oh!

Add safety checks to samMethod #6277

Add safety checks to samMethod #6277

Uh oh!

Conversation

anatoliykmetyuk commented Apr 10, 2019

Uh oh!

dottybot left a comment

Choose a reason for hiding this comment

Uh oh!

smarter Apr 11, 2019

Choose a reason for hiding this comment

Uh oh!

smarter commented Apr 15, 2019

Uh oh!

anatoliykmetyuk commented Apr 15, 2019

Uh oh!

smarter commented Apr 15, 2019

Uh oh!

anatoliykmetyuk commented Apr 15, 2019

Uh oh!

smarter commented Apr 15, 2019

Uh oh!

anatoliykmetyuk commented Apr 15, 2019

Uh oh!

anatoliykmetyuk commented Apr 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smarter commented Apr 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anatoliykmetyuk commented Apr 15, 2019

Uh oh!

smarter commented Apr 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolasstucki commented Apr 17, 2019

Uh oh!

Uh oh!

Add safety checks to `samMethod` #6277

Add safety checks to `samMethod` #6277

anatoliykmetyuk commented Apr 15, 2019 •

edited

Loading

smarter commented Apr 15, 2019 •

edited

Loading

smarter commented Apr 15, 2019 •

edited

Loading