-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add support for java records in pattern matching #24140
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -900,8 +900,31 @@ object JavaParsers { | |
needsDummyConstr = true | ||
) | ||
).withMods(mods.withFlags(Flags.JavaDefined | Flags.Final)) | ||
.withAddedAnnotation( | ||
New( | ||
ref(defn.JavaRecordFieldsAnnot), | ||
header.map(field => Literal(Constant(field.name.toString))) :: Nil, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See, we have the names here, so we will have them in line 923. Same for the |
||
).withSpan(Span(start)) | ||
) | ||
} | ||
addCompanionObject(statics, recordTypeDef) | ||
|
||
val unapplyDef = { | ||
val tparams2 = tparams.map(td => TypeDef(td.name, td.rhs).withMods(Modifiers(Flags.Param))) | ||
|
||
val selfTpt = if tparams2.isEmpty then Ident(name) else | ||
AppliedTypeTree(Ident(name), tparams2.map(tp => Ident(tp.name))) | ||
val param = ValDef(nme.x_0, selfTpt, EmptyTree) | ||
.withMods(Modifiers(Flags.JavaDefined | Flags.SyntheticParam)) | ||
|
||
DefDef( | ||
nme.unapply, | ||
joinParams(tparams2, List(List(param))), | ||
selfTpt, | ||
Ident(nme.x_0) | ||
).withMods(Modifiers(Flags.JavaDefined | Flags.SyntheticMethod | Flags.Inline)) | ||
} | ||
|
||
addCompanionObject(unapplyDef :: statics, recordTypeDef) | ||
end recordDecl | ||
|
||
def interfaceDecl(start: Offset, mods: Modifiers): List[Tree] = { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,7 +30,7 @@ import util.chaining.tap | |
import collection.mutable | ||
import config.Printers.{overload, typr, unapp} | ||
import TypeApplications.* | ||
import Annotations.Annotation | ||
import Annotations.{Annotation, JavaRecordFieldsAnnotation} | ||
|
||
import Constants.{Constant, IntTag} | ||
import Denotations.SingleDenotation | ||
|
@@ -185,6 +185,11 @@ object Applications { | |
(0 until argsNum).map(i => if (i < arity - 1) selectorTypes(i) else elemTp).toList | ||
end seqSelectors | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't matter, but a little cleaner to just return |
||
|
||
/** A utility class that matches results of unapplys with patterns. Two queriable members: | ||
* val argTypes: List[Type] | ||
* def typedPatterns(qual: untpd.Tree, typer: Typer): List[Tree] | ||
|
@@ -263,6 +268,10 @@ object Applications { | |
case _ => None | ||
case _ => None | ||
|
||
private def javaRecordTypes(tp: Type): List[Type] = | ||
javaRecordFields(tp).map: name => | ||
tp.member(name).suchThat(_.paramSymss == List(Nil)).info.resultType | ||
|
||
/** The computed argument types which will be the scutinees of the sub-patterns. */ | ||
val argTypes: List[Type] = | ||
if unapplyName == nme.unapplySeq then | ||
|
@@ -282,6 +291,8 @@ object Applications { | |
productSelectorTypes(unapplyResult, pos) | ||
// this will cause a "wrong number of arguments in pattern" error later on, | ||
// which is better than the message in `fail`. | ||
else if unapplyResult.classSymbol.isJavaRecord then | ||
javaRecordTypes(unapplyResult) | ||
else fail | ||
|
||
/** The typed pattens of this unapply */ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -169,13 +169,23 @@ class CompilationTests { | |
|
||
@Test def runAll: Unit = { | ||
implicit val testGroup: TestGroup = TestGroup("runAll") | ||
aggregateTests( | ||
var tests = List( | ||
compileFilesInDir("tests/run", defaultOptions.and("-Wsafe-init")), | ||
compileFilesInDir("tests/run-deep-subtype", allowDeepSubtypes), | ||
compileFilesInDir("tests/run-custom-args/captures", allowDeepSubtypes.and("-language:experimental.captureChecking", "-language:experimental.separationChecking", "-source", "3.8")), | ||
// Run tests for legacy lazy vals. | ||
compileFilesInDir("tests/run", defaultOptions.and("-Wsafe-init", "-Ylegacy-lazy-vals", "-Ycheck-constraint-deps"), FileFilter.include(TestSources.runLazyValsAllowlist)), | ||
).checkRuns() | ||
) | ||
|
||
if scala.util.Properties.isJavaAtLeast("16") then | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need, we run everything with Java 17 now. |
||
tests ++= List( | ||
// for separate compilation | ||
compileFilesInDir("tests/run-java16+", defaultOptions), | ||
// for joint compilation | ||
compileDir("tests/run-java16+/java-records-match", defaultOptions), | ||
Florian3k marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) | ||
|
||
aggregateTests(tests*).checkRuns() | ||
} | ||
|
||
// Generic java signatures tests --------------------------------------------- | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @hamzaremmal Do you have any ideas how to implement this without adding annotation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But we know the field names when we build the body of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this PR, it is marked as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why can't we? The method is completely built by hand anyways. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
package scala.annotation.internal | ||
|
||
import scala.annotation.StaticAnnotation | ||
|
||
/** An annotation attached by JavaParsers/ClassfileParser to Java record class | ||
* with a list of that record's fields. Used in pattern matching on records. | ||
*/ | ||
final class JavaRecordFields(args: String*) extends StaticAnnotation |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
empty | ||
hello | ||
hahaha | ||
hehehe | ||
21 | ||
hihihi | ||
hohoho | ||
unapply | ||
hejhejhejhej |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
public record Rec0_1() {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
public record Rec1_1(String s) {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
public record Rec2_1(int x, String y) {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
public record Rec3_1<T>(int x, T y) {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
public record Rec4_1<T extends Comparable<Integer>>(int x, T y) {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
public record Rec5_1<T, U extends Comparable<T>, W extends Comparable<W>>(T t, U u, W w) {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
public record RecUnapply_1(int i, String s) { | ||
public static boolean unapply(RecUnapply_1 r) { return true; } | ||
} |
Florian3k marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
case class Foo(val value: String) extends Comparable[Integer]: | ||
override def compareTo(other: Integer) = 0 | ||
|
||
case class Bar(val value: String) extends Comparable[Bar]: | ||
override def compareTo(other: Bar) = 0 | ||
|
||
case class Baz(val s: String, val i: Int) | ||
|
||
object Baz: | ||
def unapply(b: Baz): Rec2_1 = Rec2_1(b.i + 1, b.s + "j") | ||
|
||
@main def Test = | ||
val r0 = Rec0_1() | ||
r0 match { case Rec0_1() => println("empty") } | ||
|
||
val r1 = Rec1_1("hello") | ||
r1 match { case Rec1_1(s) => println(s) } | ||
|
||
val r2 = Rec2_1(3, "ha") | ||
r2 match { case Rec2_1(i, s) => println(s * i) } | ||
|
||
// type param (no bounds) | ||
val r3a = Rec3_1(3, "he") | ||
r3a match { case Rec3_1(i, s) => println(s * i) } | ||
val r3b = Rec3_1(3, 7) | ||
r3b match { case Rec3_1(i, j) => println(i * j) } | ||
|
||
// type param with simple bounds | ||
val r4 = Rec4_1(3, Foo("hi")) | ||
r4 match { case Rec4_1(i, f) => println(f.value * i) } | ||
|
||
// type params with recursion / mutual reference | ||
val r5 = Rec5_1(3 : Integer, Foo("h"), Bar("o")) | ||
r5 match { case Rec5_1(i, f, b) => println((f.value + b.value) * i) } | ||
|
||
// custom unapply | ||
val r6 = RecUnapply_1(3, "x") | ||
r6 match { case RecUnapply_1() => println("unapply") } | ||
|
||
// scala class returning record from unapply | ||
val r7 = Baz("he", 3) | ||
r7 match { case Baz(i, s) => println(s * i) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please. I've been arguing this behind the doors but this is very much a SIP change because TASTy is affected.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure TASTy being affected is SIP material by default, per se.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be used unless we special case it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I mean using a special name and double checking it will not leak into regular code at the end.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)