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

Add support for Java records in patterns. #19577

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

yilinwei
Copy link
Contributor

@yilinwei yilinwei commented Jan 31, 2024

For each Java record, we create a synthetic unapply method returning a Tuple on the companion object.

I briefly toyed with the idea of adding inline methods for Product to reduce the allocation, but I don't think that would be safe. In particular, I expect the following block to be unsound if that was the case:

val x: Product = R1()
x.productArity

I would like to test this further; hopefully after #19578 is fixed.

Many thanks to @som-snytt, @SethTisue and @sjrd for the help.

There was also a suggestion from the Discord chat of a different desugaring involving extension methods and by-name pattern matching with an opaque type alias to reduce the allocations.

However we currently only check for the by name extension methods on the actual extractor objects (see Applications#extractorMember) and looking for extension methods in this case looks as though it might open a whole can of worms.

Using an AnyVal works - but I would like to verify that the byte code generated is correct.

EDIT:

The byte code looks fine in that there is no allocation of an intermediate object.

However the isEmpty isn't omitted even if the type is narrowed to false and the accessors, _1 and _2 aren't inlined either.

I'm also worried about records compiled via javac for this encoding since the method descriptors stick around in the compiled code so presumably they would need to be bundled somewhere otherwise it would fail at runtime? It's unclear to me how methods which aren't inlined on the companion module of the record would work due to my rather limited understanding of the nuances of the mixed compilation processes.

Roughly speaking, here is the encoding which I experimented with:

record Moo(x: Int, y: Int) {}
object Moo {
  inline def unapply(moo: Moo): Forward = Forward(moo)
}

final class Matcher(val moo: Moo) extends AnyVal {
  inline def _1: Int = moo.x
  inline def _2: Int = moo.y
}

final class Forward(val moo: Moo) extends AnyVal {
  def get: Matcher = Matcher(moo)
  def isEmpty: false = false
}

EDIT:

As per the discussions in #19631, we now deal with the problems in the PatternMatcher and make Java records a first class citizen of the pattern matcher.

I have a few points here and there I'd like to check, but after that I will start writing the SIP.

EDIT:

Pre-SIP is here.

@yilinwei yilinwei marked this pull request as draft January 31, 2024 16:34
@He-Pin
Copy link

He-Pin commented Jan 31, 2024

This should be epic, does it support compiled records, eg the records from a jar?

@joroKr21
Copy link
Member

joroKr21 commented Feb 1, 2024

Perhaps related - I wonder if we can support mirrors for records (and sealed classes)

@yilinwei
Copy link
Contributor Author

yilinwei commented Feb 1, 2024

@He-Pin It should do - I'm not entirely clear about the way Scala works with raw jars, but you can certainly call the synthesized apply methods on Java classes when they are from a jar rather than source.

@joroKr21 That is indeed the intention; there are a few other issues that I want to fix to help the record support along; like #19386 for example.

@nicolasstucki
Copy link
Contributor

nicolasstucki commented Feb 7, 2024

What is the point in inlining inline def _1: Int = moo.x? If it is for an optimization, then its better to not mark it as inline and let the JVM, Scala.js, or Native backend inline it.

@yilinwei
Copy link
Contributor Author

yilinwei commented Feb 7, 2024

@nicolasstucki

It is because I am unsure whether it is OK to let the final compiled code refer to the functions in the synthesized companion module. I do not understand where/how that code would be bundled, especially for a jar not compiled with scalac.

@sjrd
Copy link
Member

sjrd commented Feb 7, 2024

I strongly suggest you use run tests for these things. You want to make sure that your code can actually be executed by the JVM.

bishabosha added a commit that referenced this pull request Feb 9, 2024
This is a dependency of #19577 and should be reviewed and merged first.

fixes #18639
fixes #19386
fixes #19578
This adds Java records as a first-class citizen within Scala's
pattern matching.

Broadly, we synthesize an `inline unapply` on the companion module
which we later use to correlate with the bound variables within
the pattern matcher.

val params = tp.typeSymbol.javaRecordComponents.map(_.info.resultType)

tp match
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is surely too standard to need to do it by hand...

import scala.reflect.ClassTag

// TODO: Rename to JavaRecordReflectMirror
object JavaRecordMirror:
Copy link
Contributor Author

@yilinwei yilinwei Feb 10, 2024

Choose a reason for hiding this comment

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

Is this going to be a big deal? It means that compiling the scala library would need JDK 16+ but could target older versions, but having this on the classpath and not loaded should not be an issue? Maybe I could exclude the source, if records are not supported but would that cause issues with the current publishing process?

@@ -407,6 +407,12 @@ class Synthesizer(typer: Typer)(using @constructorOnly c: Context):
def newTupleMirror(arity: Int): Tree =
New(defn.RuntimeTupleMirrorTypeRef, Literal(Constant(arity)) :: Nil)

def newJavaRecordReflectMirror(tpe: Type) =
Copy link
Contributor Author

@yilinwei yilinwei Feb 10, 2024

Choose a reason for hiding this comment

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

This mirror is of limited use and requires the user to check the type at the derivation site for the mirrored mono type. This isn't the end of the world - we could add an extension method which uses the RecordComponent to invoke the accessor, but can we add a method to the mirror like productElement(a: A)(i: Int) which allows users which derive with the mirror to hide the implementation details?

Maybe this doesn't matter because actual practitioners would use the dotty reflect to select the field labels.

@@ -88,6 +93,11 @@ class Inlining extends MacroTransform, IdentityDenotTransformer {

flatTree(trees2)
else super.transform(tree)
case Apply(fun, args) if fun.symbol.name == nme.unapply && fun.symbol.is(ConstructorProxy) =>
Copy link
Contributor Author

@yilinwei yilinwei Feb 16, 2024

Choose a reason for hiding this comment

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

Should this transformation happen in the Typer just after we've created the unapply object instead - this would mean the pickler wouldn't see the proxy; would that be better?

def checkNoConstructorProxy(tree: Tree)(using Context): Unit =
if tree.symbol.is(ConstructorProxy) then
if tree.symbol.is(ConstructorProxy) && tree.symbol.name != nme.unapply then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can transform the unapply into an identity function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants