Add support for java records in pattern matching - #24140
Conversation
jchyb
left a comment
jchyb
left a comment
There was a problem hiding this comment.
Really great work!
I have some small nitpicks/suggestions, mostly connected to the tests.
I have also left one more general comment about the JavaRecordFields addition that isn't really actionable, but I still wanted to point this out in case you or anyone else from the compiler team have any better suggestions there.
| def javaRecordFields(tp: Type)(using Context): List[Name] = | ||
| tp.typeSymbol.getAnnotation(defn.JavaRecordFieldsAnnot) match | ||
| case Some(JavaRecordFieldsAnnotation(fields)) => fields.map(termName) | ||
| case _ => assert(false) |
There was a problem hiding this comment.
Doesn't matter, but a little cleaner to just return Nil here in my opinion
There was a problem hiding this comment.
Ideally we would not need this, since we don't need that information pickled and it's only relevant for the current compilation run. We could add a property key for a tree, but that would only work for the situations where we use the JavaOutlineParser, when we read the classfiles we would not have access to a tree on which a property could be used. I see we can really only get this information out when parsing the tree, and we don't save it anywhere else, which I guess justifies the annotation here.
There was a problem hiding this comment.
I'm 99.9% sure we can do without and I think it's best if we don't introduce this.
There was a problem hiding this comment.
I would prefer to do this without adding new annotation, but I couldn't find another solution. If you have any suggestions, I'm happy to explore them.
There was a problem hiding this comment.
@hamzaremmal Do you have any ideas how to implement this without adding annotation?
There was a problem hiding this comment.
I just realized. We are only holding Strings inside of the annotation (we don't need any of the actual trees), so perhaps it would be ok to have a Map[Symbol, Seq[String]] held in the ContextBase (basically, a global value)? We could even change it to Map[Int, Seq[String]] with symbolIDs if comparing Symbols will prove to be problematic
There was a problem hiding this comment.
But we know the field names when we build the body of unapply. If we know the field names to attach an annotation to the record, we surely know them to synthesis a method.
There was a problem hiding this comment.
You can't synthesize an actual method. It's a Java-compiled class. It's all fake. It must be handled at call site.
There was a problem hiding this comment.
In this PR, it is marked as inline, I'm suggesting this change based on that assumption. If we remove inline, this all breaks down.
There was a problem hiding this comment.
Even inline is fake. It has no @retainedBody annotation. And we can't really make up one when reading from .class files.
There was a problem hiding this comment.
Why can't we? The method is completely built by hand anyways.
There was a problem hiding this comment.
I'm 99.9% sure we can do without and I think it's best if we don't introduce this.
|
Can we have Scala's case class be a record too? seems we can't. |
Nope, we can't. |
| ).checkRuns() | ||
| ) | ||
|
|
||
| if scala.util.Properties.isJavaAtLeast("16") then |
There was a problem hiding this comment.
No need, we run everything with Java 17 now.
There was a problem hiding this comment.
We do the Scala 2 schema instead. For record Foo(int x, String y), we would generate def unapply(x$0: Foo): Option[(Int, String)] = ... instead of def unapply(x$0: Foo): Foo = x$0.
@sjrd What do you think about this?
| .withAddedAnnotation( | ||
| New( | ||
| ref(defn.JavaRecordFieldsAnnot), | ||
| header.map(field => Literal(Constant(field.name.toString))) :: Nil, |
There was a problem hiding this comment.
See, we have the names here, so we will have them in line 923. Same for the ClassfileParser.
| val completer = RecordUnapplyCompleter() | ||
| val member = newSymbol( | ||
| moduleRoot.symbol, | ||
| nme.unapply, |
There was a problem hiding this comment.
In fact I'm not a big fan of this fake synthetic method. It could be used in regular calls that are not pattern matching. That means TASTy effectively embeds knowledge of these synthetic, fake methods.
Could we directly adapt pattern matching instead?
There was a problem hiding this comment.
That means TASTy effectively embeds knowledge of these synthetic, fake methods.
Yes please. I've been arguing this behind the doors but this is very much a SIP change because TASTy is affected.
There was a problem hiding this comment.
I'm not sure TASTy being affected is SIP material by default, per se.
There was a problem hiding this comment.
Yes, because TASTy will contain a synthetic method that will have to be in the spec. It will be saying Java records generate an unapply of this form...
There was a problem hiding this comment.
If we can generate a fake function only for unapply during typing, then it will not be used by regular calls. I don't think this will affect tasty?
There was a problem hiding this comment.
If we can generate a fake function only for unapply during typing, then it will not be used by regular calls. I don't think this will affect tasty?
It can be used unless we special case it.
There was a problem hiding this comment.
yes, I mean using a special name and double checking it will not leak into regular code at the end.
There was a problem hiding this comment.
Anyways, we will explore a different way of doing things; special case the pattern matching rules for Java Records (i.e. changing this part of the spec)
odersky
left a comment
odersky
left a comment
There was a problem hiding this comment.
I think whatever we do, this needs a spec change.
We should avoid a situation where we add something to the implementation and leave the spec change for later. So, ideally, this PR is accompanied by a PR against the Scala spec. And the changes are either summarized or linked to here.
I completely agree. |
@sjrd would you be able to help with that? |
Gedochao
added
the
stat:tbc
|
Superseded by #26497 |
8 participants
Closes #20561