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.

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

Jump to bottom

Support signature polymorphic methods (MethodHandle and VarHandle) #16225

Merged
merged 10 commits into from
Nov 22, 2022
Merged

Support signature polymorphic methods (MethodHandle and VarHandle) #16225

merged 10 commits into from
Nov 22, 2022

Conversation

dwijnand
Copy link
Member

@dwijnand dwijnand commented Oct 20, 2022 •
edited by SethTisue
Loading

fixes #11332

@bishabosha bishabosha added the needs-minor-release This PR cannot be merged until the next minor release label Oct 20, 2022
@dwijnand dwijnand linked an issue Oct 21, 2022 that may be closed by this pull request
Support calling signature polymorphic methods (needed to use MethodHandle and VarHandle) #11332
Closed
@dwijnand dwijnand self-assigned this Oct 24, 2022
@dwijnand dwijnand force-pushed the support-poly-sigs branch 4 times, most recently from 63acd79 to 7f9a0a7 Compare October 25, 2022 16:31
@dwijnand dwijnand marked this pull request as ready for review October 25, 2022 22:27
@dwijnand dwijnand assigned smarter and unassigned dwijnand Oct 25, 2022
@SethTisue
Copy link
Member

@sjrd we were hoping you could review the TASTy changes

@SethTisue SethTisue mentioned this pull request Oct 26, 2022
Closed
@SethTisue SethTisue changed the title Support polymorphic signatures Support signature polymorphic methods (MethodHandle and VarHandle) Oct 27, 2022
@SethTisue SethTisue requested a review from sjrd October 27, 2022 13:40
@dwijnand dwijnand removed the needs-minor-release This PR cannot be merged until the next minor release label Nov 2, 2022
@smarter smarter added this to the 3.3.0-RC1 milestone Nov 2, 2022
@smarter
Copy link
Member

smarter commented Nov 2, 2022

@dwijnand removed the needs-minor-release label

If you have to tweak the unpickler to get the newly pickled stuff to work, then it's still a breaking tasty change and will still require a minor release. Also I don't understand what the changes you're proposing to the unpickler do, so it's not clear to me if it's a better idea than adding a new tag?

@dwijnand
Copy link
Member Author

dwijnand commented Nov 3, 2022

It's peaking through a Typed we added to get the result type, which together with the types of the arguments we can use to do the same symbol copying we do during typing, where we used the args and the prototype.

At the meeting on Monday we discussed whether it was better to special case with a tag or special-case with existing constructs, and decided it might be good to do it without a new tag.

@smarter smarter added the needs-minor-release This PR cannot be merged until the next minor release label Nov 3, 2022
@sjrd
Copy link
Member

sjrd commented Nov 3, 2022

Whether or not it's a new tag, it is new semantics in tasty that an existing reader (including earlier versions of the compiler) won't be able to read. So it must be a new minor version of tasty and of the compiler.

Given that, you have a choice between:

  • a tag and straightforward code, or
  • no tag and complicated logic to read that actually interprets a combo of tags as semantically another tag.

I think it's pretty clear that the new tag is preferable.

Moreover, I've been thinking about how this should be handled in tasty-query. I really don't want to create fake symbols, since that doesn't match a user's intuition about the language. Because of that, I think in tasty it would make more sense to have a dedicated APPLY-like node. One that carries a regular TermRef and args, plus a specific MethodType. That wouldn't prevent from reading that into a magical symbol in the compiler, of course.

@dwijnand dwijnand assigned sjrd and unassigned smarter Nov 7, 2022
@dwijnand dwijnand requested a review from sjrd November 7, 2022 16:06
sjrd
sjrd reviewed Nov 7, 2022
View reviewed changes
Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Very nice. IMO it's much better now :)

I have two comments which are more questions, really.

compiler/src/dotty/tools/dotc/transform/Recheck.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/dotc/core/tasty/TreePickler.scala
else if fun.symbol.originalSignaturePolymorphic.exists then
writeByte(APPLYsigpoly)
withLength {
pickleTree(fun)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't originalSignaturePolymorphic be involved here, somehow?

Copy link
Member Author

Choose a reason for hiding this comment

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

Like how? The function Select will be pickled as a SELECTin with the name "invokeExact" and the "in" as MethodHandle, which during unpickling will pick the original method and that's why we have to fix it up.

@dwijnand dwijnand requested a review from sjrd November 8, 2022 08:35
@dwijnand
Copy link
Member Author

@sjrd Had to rebase this. Any chance you can give it a last look please?

@sjrd
Copy link
Member

sjrd commented Nov 15, 2022

Yes, sorry. I will do that tomorrow afternoon.

@timothyklim
Copy link

Yes, sorry. I will do that tomorrow afternoon.

Will it get into 3.3.0-RC1? 👀

@dwijnand dwijnand merged commit 439a17d into scala:main Nov 22, 2022
@dwijnand dwijnand deleted the support-poly-sigs branch November 22, 2022 16:03
smarter
smarter reviewed Nov 22, 2022
View reviewed changes
tests/run/i11332.scala
val mhOverI = l.findVirtual(classOf[Foo], "over", methodType(classOf[String], classOf[Int]))
val mhUnit = l.findVirtual(classOf[Foo], "unit", methodType(classOf[Unit], classOf[String]))
val mhObj = l.findVirtual(classOf[Foo], "obj", methodType(classOf[Any], classOf[String]))
val mhCL = l.findStatic(classOf[ClassLoader], "getPlatformClassLoader", methodType(classOf[ClassLoader]))
Copy link
Member

Choose a reason for hiding this comment

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

This broke the CI because some of our jobs are running on Java 8 which does not define this method:
https://github.com/lampepfl/dotty/actions/runs/3524847656/jobs/5910798965

Copy link
Member Author

Choose a reason for hiding this comment

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

#16394

@SethTisue SethTisue mentioned this pull request Nov 29, 2022
Merged
@SethTisue
Copy link
Member

blog post on this work: 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
Reviewers

@smarter smarter smarter left review comments

@sjrd sjrd sjrd approved these changes

Assignees

@sjrd sjrd

Labels
needs-minor-release This PR cannot be merged until the next minor release
Projects
None yet
Milestone
3.3.0
Development

Successfully merging this pull request may close these issues.

Support calling signature polymorphic methods (needed to use MethodHandle and VarHandle)
7 participants
@dwijnand @SethTisue @smarter @sjrd @timothyklim @Kordyjan @bishabosha