Skip to content
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

Support calling signature polymorphic methods (needed to use MethodHandle and VarHandle) #11332

Closed
smarter opened this issue Feb 6, 2021 · 37 comments · Fixed by #16225
Closed

Comments

@smarter
Copy link
Member

smarter commented Feb 6, 2021

See scala/scala#4139, scala/scala#5594, scala/scala#9530, scala/scala#9930

@smarter smarter changed the title Support calling Java 9+ @PolymorphicSignature methods (needed to use MethodHandle and VarHandle) Support calling @PolymorphicSignature methods (needed to use MethodHandle and VarHandle) Feb 6, 2021
@smarter

This comment has been minimized.

@ScalaWilliam
Copy link
Contributor

Hi @smarter is there a workaround / hackaround for this by any chance?

Eg some way to generate pure Java at compile-time or similar?

If it works, Scala 3 macros + VarHandles/MethodHandles = <3

@smarter
Copy link
Member Author

smarter commented Jul 11, 2021

I don't think there's any workaround besides writing the calls in java files

@ScalaWilliam
Copy link
Contributor

Thanks for getting back so quickly @smarter .

Just to add, I was doing simple calls like VarHandle#get and MethodHandle.invokeExact just fine in Scala 2.13.6 (compile & runtime), but it fails to compile in Scala 3.0.1, for example:

|    var memoryAddress: MemoryAddress = ref.invokeExact(
[error]     |                                       ^
[error]     |                         Found:    Object
[error]     |                         Required: jdk.incubator.foreign.MemoryAddress

and:

[error] -- [E007] Type Mismatch Error: 
[error] 198 |    val rootVal: MemoryAddress = addrHandle.get(theDoc)
[error]     |                                 ^^^^^^^^^^^^^^^^^^^^^^
[error]     |                         Found:    Object
[error]     |                         Required: jdk.incubator.foreign.MemoryAddress

Changing the code to have type-casting with asInstanceOf compiles, although fails at runtime:

java.lang.invoke.WrongMethodTypeException: expected (MemoryAddress,int,int,MemoryAddress,MemoryAddress)MemoryAddress but found (Object[])Object

I do appreciate it's a completely new compiler - because it looks like a regression (or I missed some sort of migration document), would there be a chance to prioritize this? I imagine there might be a few Scala codebases that might not be able to take the leap because of this.

@smarter
Copy link
Member Author

smarter commented Jul 11, 2021

I'd be happy to help along anyone who wants to port the scala 2 implementation of this feature but I don't have the time to deal with it myself, and I don't know if anyone else does.

@vasilmkd
Copy link
Contributor

vasilmkd commented Oct 2, 2021

I would like to work on this issue, but I have not yet contributed to the Scala 3 compiler and I'm honestly not sure where to begin. It seems to me that the Scala 2 code cannot be easily ported to the new compiler. I'd appreciate any pointers.

@markehammons
Copy link

Seconding the above. I looked, and I don't know how the scala 2 code corresponds to the scala 3 code.

@smarter
Copy link
Member Author

smarter commented Oct 4, 2021

Welcome! Some pointers:

I think the Scala 2 approach could work in Scala 3 too, but we also store our typed trees into tasty and can deserialize them at any point (for example, when calling an inline def), so the deserializer would likely need to use the same symbol creation logic (https://github.com/lampepfl/dotty/blob/6e680453fc24cc9e9d2bb873638d76fec3883375/compiler/src/dotty/tools/dotc/core/tasty/TreeUnpickler.scala#L1138-L1140).


It might be worth exploring alternative approaches which don't involve creating fake symbols, but as mentioned in scala/scala#4139 this is likely to touch many parts of the compiler. I had a quick look just now and it seems pretty daunting indeed.

@vasilmkd
Copy link
Contributor

vasilmkd commented Oct 4, 2021

Thank you for the great list of resources @smarter. I will take some time to internalize everything and hopefully come up with a PR. Thank you again.

@vasilmkd
Copy link
Contributor

vasilmkd commented Oct 6, 2021

I've been working on this issue for 2 days now, but I am unable to make any progress. The code in the type checker, while I mostly know where to put it and what to aim for, I am unable to translate the Scala 2 compiler code to the Scala 3 compiler code. I'm especially having difficulty trying to copy symbols and modify them, as well as the purpose of the FunProto class and its relation to type checking a method/function invocation.

I would appreciate any help that anyone could provide, because for the moment I'm stuck, with no clear way to proceed.

@smarter
Copy link
Member Author

smarter commented Oct 6, 2021

trying to copy symbols and modify them

I'd expect you'd need something like val s2 = sym.copy(info = theNewTypeOfTheSymbol), then you can create a type that points to this symbol using val tp = TermRef(s2.owner.namedType, s2), then you can create a tree with this type using val tree2 = tree.withType(tp)

the purpose of the FunProto class

Type-checking proceeds top-down: at every level we have a tree we want to give a type to and an "expected type" aka "prototype", for example in 1: Long, the expected type of 1 is Long, in a.foo, the expected type of a is "something with a member named foo", and for fun(1, 2) the expected type of fun is a FunProto which contains the arguments 1 and 2. FunProto is pretty complicated because it caches the type of its arguments to speed-up overload resolution.

@vasilmkd
Copy link
Contributor

vasilmkd commented Oct 6, 2021

I found the copy extension method, but I was unable to call it on a symbol, for some reason Scala was unable to offer it.

@smarter
Copy link
Member Author

smarter commented Oct 6, 2021

Indeed, looks like you need to do sym.asTerm.copy(...) for the extension to be applicable.

@SethTisue
Copy link
Member

SethTisue commented Dec 21, 2021

Guillaume walked Dale through how to add the needed code to typer, inspired by Jason's patch but with the right Dotty details. We did that and have a basic test case passing. (Dale will push a WIP branch soon, I expect.)

Remember for later: We need to be mindful of forward compatibility under the forthcoming -scala-release flag. (that flag is no longer planned)

Minor hurdle: -Ycheck sensibly complained about what looks like mismatched types. (Did we fix that properly yet, or did we temporarily kludge it?)

(Aside: It might be nice if we didn't always make a fresh symbol at every call site, even if we already saw a call site with the same types. But probably it isn't worth doing the extra work to add a cache.)

Bigger hurdle: TASTy support. If we do nothing, the TASTy unpickler blows up because the type of the method we're calling doesn't match the types at the call site anymore.

Two possible solution paths:

  • ("Goofus") consider it an edge case and kludge it by adding an annotation on the call site, and then have the unpickler notice and handle that annotation
  • ("Gallant") admit this is an actual language feature, despite being obscure/rarely-used, and add a new TASTy node type to support it

@dwijnand
Copy link
Member

@vasilmkd
Copy link
Contributor

Glad to see competent people on this issue and not me. 😄

@som-snytt
Copy link
Contributor

I think we need a name for the competent team. (Besides Goofus & Gallant.) I would propose "Chip 'n' Dale", but then we'd have to start calling Seth "Chip".

@SethTisue
Copy link
Member

SethTisue commented Dec 22, 2021

It seems to Dale and me that we are required here to be Gallant and do it it the right way. Forward compatibility isn't possible here, so we need a bumped TASTy version to signal that, and if we're bumping the TASTy version anyway, we might as well add the new node type.

@SethTisue
Copy link
Member

SethTisue commented Dec 22, 2021

A small wrinkle here is that the definition of polymorphic signature methods in the Java Language Specification broadened in JDK 17 11, compared to the JDK 8 version which is what Jason implemented in Scala 2.

JLS 8 wording at https://docs.oracle.com/javase/specs/jls/se8/html/jls-15.html is:

A method is signature polymorphic if all of the following are true:

  • It is declared in the java.lang.invoke.MethodHandle class.
  • It takes a single variable arity parameter (§8.4.1) whose declared type is Object[].
  • It has a return type of Object.
  • It is native.

The JLS 17 11 wording at https://docs.oracle.com/javase/specs/jls/se11/html/jls-15.html#jls-15.12.3 is:

A method is signature polymorphic if all of the following are true:

  • It is declared in the java.lang.invoke.MethodHandle class or the java.lang.invoke.VarHandle class.
  • It has a single variable arity parameter (§8.4.1) whose declared type is Object[].
  • It is native.

Compared to JLS 8, the mention of VarHandle is new and the condition on the return type was dropped. So if we're going to hardcode the class and method names we need a longer list, but perhaps we should be Gallant again here and actually implement the stated checks in the spec (UPDATE: as Jason did in scala/scala#9930).

@smarter
Copy link
Member Author

smarter commented Dec 22, 2021

I wouldn't hard code the class or method names on the unpickler side, just trust the pickler to do something sensible

@SethTisue
Copy link
Member

SethTisue commented Dec 22, 2021

Dale pushed new code with the TASTy support.

We're about 95% of the way there, I think. I will do the generalization to the additional JDK 17 11 methods, think about whether any more tests are needed, and maybe comment the code a bit better.

(Also, this interacts with #14156, can't forget that.) (PR reverted)

@vasilmkd
Copy link
Contributor

Fantastic work. I'm looking forward to this feature. Thank you for the updates. Very much appreciated.

@smarter
Copy link
Member Author

smarter commented Feb 6, 2022

@SethTisue @dwijnand do you think you'll manage to finish this for 3.2 (which is scheduled to be the next release) or should we defer this to 3.3?

@SethTisue
Copy link
Member

SethTisue commented Feb 7, 2022

thanks for the reminder — tbh, I'd forgotten (but self-assigning should prevent that from happening again).

this week is pretty tight for me, timewise. how much more time do I have, when is 3.2 coming?

@vasilmkd
Copy link
Contributor

vasilmkd commented Feb 7, 2022

Please don't rush with the implementation. Personally, I'm fine if it lands in 3.3. I will also help you test it. Cats Effect is a bit stuck on 3.0 for now in any case, there's really no pressure here.

@smarter
Copy link
Member Author

smarter commented Feb 7, 2022

You can see the tentative release date of 3.2.0-RC1 on https://github.com/lampepfl/dotty/milestones (currently march 9)

@SethTisue
Copy link
Member

It seems plausible we could make that. I'll try. (And I'll keep Vasil's remarks in mind.)

@smarter
Copy link
Member Author

smarter commented Mar 10, 2022

3.2.0-RC1 has been delayed until April 27 (https://github.com/lampepfl/dotty/milestones) if you want another chance at this :).

@smarter smarter added this to the 3.2.0-RC1 milestone Mar 10, 2022
@smarter
Copy link
Member Author

smarter commented Mar 13, 2022

Recent related PR on the Scala 2 side: scala/scala#9930

@odersky
Copy link
Contributor

odersky commented May 23, 2022

ping to get the status for this. If it should be in 3.2 we need to merge it this week, or next at the latest

@oscar-broman
Copy link

A very simple workaround for those who need this now.

import java.lang.invoke.MethodHandle;

public class InvokePolySig {
    public static <R, A> R invoke(MethodHandle methodHandle, A a) throws Throwable {
        return (R)methodHandle.invoke(a);
    }

    public static <R, A, B> R invoke(MethodHandle methodHandle, A a, B b) throws Throwable {
        return (R)methodHandle.invoke(a, b);
    }

    // etc.
}

Instead of fn.invoke(123) do InvokePolySig.invoke(fn, 123).

@markehammons
Copy link

Please note that while the above workaround does work, it's still not great.

I've restarted development of my slinc library, and it uses methodhandles. The implementation using this approach runs at 93 ops/us, while a version using a hardcoded invokeExact runs at 147 ops/us. I cannot use inokeExact right now because Scala 3 trips over it when trying to compile it. If it weren't for that, I could write the optimal form of any method binding using invokeExact and macros. Boxing is really costing a lot here, so I hope that support for PolyMorphicSignature methods comes soon and frees me from it.

@SethTisue
Copy link
Member

SethTisue commented Oct 20, 2022

Guillaume, Dale, and I will revisit this today (for 3.3.0-RC1)

Dale's WIP branch is https://github.com/dwijnand/scala3/tree/support-poly-sigs

  • rebase WIP branch
  • in the Scala 2 repo, on 2.12.x branch, add tests for VarHandle
  • stop hardcoding method names and implement the spec
  • look in VarHandle too, not just MethodHandle
  • make sure we can use MethodHandle to call a generic method such as def id[T](x: T) = x
  • handle signature polymorphic methods whose return type isn'tObject
    • JVM spec: "The compile-time result is determined as follows ..."
  • exempt signature polymorphic methods from Recheck after unpickling
  • does the null special case in the spec need special support?
    • "An argument expression which is the null literal null (§3.10.7) is treated as having the type Void"
    • also see if this was handled in Scala 2
  • do we need additional testing to cover the TASTy changes?
    • TestPickler should give us some confidence
    • we also tried inserting a bug into the unpickling code and making sure tests failed
    • plus we did a run with -Ythrough-tasty
  • is the TASTy support done?
  • port MethodHandle test from Scala 2 (test/files/run/t7965.scala) we already surpassed it
  • port VarHandle JDK11+ test from Scala 2 (test/files/run/t12348.scala) ditto
  • get it working with explicit nulls (as caught by CI)
  • fix test failures related to implementation of wasPolymorphicSignature
  • implement fallback to scala.AnyRef as result type when other conditions don't apply
  • consider whether Scala should also have (both spec and implement) part of JLS spec that selects void result type for so-called "expression statements"
  • ask Seb to review the TASTy changes
  • revive Test can be skipped #15463 so we can do test: -jvm 11+ for VarHandle tests
  • backport any tests and/or fixes to Scala 2? (and spec changes related to void handling and Object fallback?)
    Backport Scala 3 signature polymorphic method tests bug#12678

@SethTisue SethTisue changed the title Support calling @PolymorphicSignature methods (needed to use MethodHandle and VarHandle) Support calling signature polymorphic methods (needed to use MethodHandle and VarHandle) Oct 20, 2022
@SethTisue SethTisue changed the title Support calling signature polymorphic methods (needed to use MethodHandle and VarHandle) Support calling signature polymorphic methods (needed to use MethodHandle and VarHandle) Oct 20, 2022
@SethTisue

This comment was marked as outdated.

@SethTisue
Copy link
Member

We're about 95% of the way there, I think

LOL, we were maybe 25% of the way there

@ScalaWilliam
Copy link
Contributor

We're about 95% of the way there, I think

LOL, we were maybe 25% of the way there

ach, the painful reality:-)

@SethTisue
Copy link
Member

SethTisue commented Nov 29, 2022

blog post, for publication once this makes it into an RC: scala/scala-lang#1428

UPDATE: now published at https://scala-lang.org/blog-detail/2023/07/17/signature-polymorphic-methods.html

@Kordyjan Kordyjan modified the milestones: 3.3.0-RC1, 3.3.0 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.