Re: RFR: 8262889: Compiler implementation for Record Patterns [v7]
On Wed, 25 May 2022 04:20:35 GMT, Jan Lahoda wrote: >> 8262889: Compiler implementation for Record Patterns >> >> A first version of a patch that introduces record patterns into javac as a >> preview feature. For the specification, please see: >> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html >> >> There are two notable tricky parts: >> -in the parser, it was necessary to improve the `analyzePattern` method to >> handle nested/record patterns, while still keeping error recovery reasonable >> -in the `TransPatterns`, the desugaring of the record patterns is very >> straightforward - effectivelly the record patterns are desugared into >> guards/conditions. This will likely be improved in some future >> version/preview >> >> `MatchException` has been extended to cover additional cases related to >> record patterns. > > Jan Lahoda has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 34 commits: > > - Merge branch 'master' into patterns-record-deconstruction3 > - Post merge fix. > - Merge branch 'master' into patterns-record-deconstruction3 > - Fixing (non-)exhaustiveness for enum. > - Merge branch 'type-pattern-third' into patterns-record-deconstruction3 > - Merge branch 'master' into type-patterns-third > - Using Type.isRaw instead of checking the AST structure. > - Exhaustiveness should accept supertypes of the specified type. > - Renaming the features from deconstruction pattern to record pattern. > - Fixing guards after record patterns. > - ... and 24 more: > https://git.openjdk.java.net/jdk/compare/742644e2...f49d3f0a Yai ! champagne (at least virtual). - PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns [v7]
> 8262889: Compiler implementation for Record Patterns > > A first version of a patch that introduces record patterns into javac as a > preview feature. For the specification, please see: > http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html > > There are two notable tricky parts: > -in the parser, it was necessary to improve the `analyzePattern` method to > handle nested/record patterns, while still keeping error recovery reasonable > -in the `TransPatterns`, the desugaring of the record patterns is very > straightforward - effectivelly the record patterns are desugared into > guards/conditions. This will likely be improved in some future version/preview > > `MatchException` has been extended to cover additional cases related to > record patterns. Jan Lahoda has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 34 commits: - Merge branch 'master' into patterns-record-deconstruction3 - Post merge fix. - Merge branch 'master' into patterns-record-deconstruction3 - Fixing (non-)exhaustiveness for enum. - Merge branch 'type-pattern-third' into patterns-record-deconstruction3 - Merge branch 'master' into type-patterns-third - Using Type.isRaw instead of checking the AST structure. - Exhaustiveness should accept supertypes of the specified type. - Renaming the features from deconstruction pattern to record pattern. - Fixing guards after record patterns. - ... and 24 more: https://git.openjdk.java.net/jdk/compare/742644e2...f49d3f0a - Changes: https://git.openjdk.java.net/jdk/pull/8516/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8516=06 Stats: 2255 lines in 50 files changed: 2169 ins; 15 del; 71 mod Patch: https://git.openjdk.java.net/jdk/pull/8516.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8516/head:pull/8516 PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns [v6]
> 8262889: Compiler implementation for Record Patterns > > A first version of a patch that introduces record patterns into javac as a > preview feature. For the specification, please see: > http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html > > There are two notable tricky parts: > -in the parser, it was necessary to improve the `analyzePattern` method to > handle nested/record patterns, while still keeping error recovery reasonable > -in the `TransPatterns`, the desugaring of the record patterns is very > straightforward - effectivelly the record patterns are desugared into > guards/conditions. This will likely be improved in some future version/preview > > `MatchException` has been extended to cover additional cases related to > record patterns. Jan Lahoda has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 33 commits: - Post merge fix. - Merge branch 'master' into patterns-record-deconstruction3 - Fixing (non-)exhaustiveness for enum. - Merge branch 'type-pattern-third' into patterns-record-deconstruction3 - Merge branch 'master' into type-patterns-third - Using Type.isRaw instead of checking the AST structure. - Exhaustiveness should accept supertypes of the specified type. - Renaming the features from deconstruction pattern to record pattern. - Fixing guards after record patterns. - Raw types are not allowed in record patterns. - ... and 23 more: https://git.openjdk.java.net/jdk/compare/0155e4b7...924daa0c - Changes: https://git.openjdk.java.net/jdk/pull/8516/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8516=05 Stats: 2255 lines in 50 files changed: 2169 ins; 15 del; 71 mod Patch: https://git.openjdk.java.net/jdk/pull/8516.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8516/head:pull/8516 PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns [v5]
On Tue, 10 May 2022 09:57:48 GMT, Jan Lahoda wrote: >> 8262889: Compiler implementation for Record Patterns >> >> A first version of a patch that introduces record patterns into javac as a >> preview feature. For the specification, please see: >> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html >> >> There are two notable tricky parts: >> -in the parser, it was necessary to improve the `analyzePattern` method to >> handle nested/record patterns, while still keeping error recovery reasonable >> -in the `TransPatterns`, the desugaring of the record patterns is very >> straightforward - effectivelly the record patterns are desugared into >> guards/conditions. This will likely be improved in some future >> version/preview >> >> `MatchException` has been extended to cover additional cases related to >> record patterns. > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Fixing (non-)exhaustiveness for enum. The dependent pull request has now been integrated, and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork: git checkout patterns-record-deconstruction3 git fetch https://git.openjdk.java.net/jdk master git merge FETCH_HEAD # if there are conflicts, follow the instructions given by git merge git commit -m "Merge master" git push - PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns [v5]
On Tue, 10 May 2022 09:57:48 GMT, Jan Lahoda wrote: >> 8262889: Compiler implementation for Record Patterns >> >> A first version of a patch that introduces record patterns into javac as a >> preview feature. For the specification, please see: >> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html >> >> There are two notable tricky parts: >> -in the parser, it was necessary to improve the `analyzePattern` method to >> handle nested/record patterns, while still keeping error recovery reasonable >> -in the `TransPatterns`, the desugaring of the record patterns is very >> straightforward - effectivelly the record patterns are desugared into >> guards/conditions. This will likely be improved in some future >> version/preview >> >> `MatchException` has been extended to cover additional cases related to >> record patterns. > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Fixing (non-)exhaustiveness for enum. looks good to me, great job! - Marked as reviewed by vromero (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns [v5]
> 8262889: Compiler implementation for Record Patterns > > A first version of a patch that introduces record patterns into javac as a > preview feature. For the specification, please see: > http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html > > There are two notable tricky parts: > -in the parser, it was necessary to improve the `analyzePattern` method to > handle nested/record patterns, while still keeping error recovery reasonable > -in the `TransPatterns`, the desugaring of the record patterns is very > straightforward - effectivelly the record patterns are desugared into > guards/conditions. This will likely be improved in some future version/preview > > `MatchException` has been extended to cover additional cases related to > record patterns. Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Fixing (non-)exhaustiveness for enum. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8516/files - new: https://git.openjdk.java.net/jdk/pull/8516/files/10cd9d0c..a0d0c78b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8516=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8516=03-04 Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8516.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8516/head:pull/8516 PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns [v4]
On Mon, 9 May 2022 20:52:15 GMT, Vicente Romero wrote:
> I've noticed that this code:
>
> ```
> class Test {
> String e(E e) {
> return switch (e) {
> case A -> "42";
> };
> }
>
> enum E {
> A, B;
> }
> }
> ```
>
> fails with:
>
> ```
> Test.java:3: error: the switch expression does not cover all possible input
> values
> return switch (e) {
>^
> 1 error
> ```
>
> before this change but gets accepted with no errors after the change in this
> PR
Oops, sorry, should be fixed now
([a0d0c78](https://github.com/openjdk/jdk/pull/8516/commits/a0d0c78bcbb5ecb010c2e9bd235b3ae89eb00823)).
-
PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns [v4]
On Mon, 9 May 2022 14:37:35 GMT, Jan Lahoda wrote:
>> 8262889: Compiler implementation for Record Patterns
>>
>> A first version of a patch that introduces record patterns into javac as a
>> preview feature. For the specification, please see:
>> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html
>>
>> There are two notable tricky parts:
>> -in the parser, it was necessary to improve the `analyzePattern` method to
>> handle nested/record patterns, while still keeping error recovery reasonable
>> -in the `TransPatterns`, the desugaring of the record patterns is very
>> straightforward - effectivelly the record patterns are desugared into
>> guards/conditions. This will likely be improved in some future
>> version/preview
>>
>> `MatchException` has been extended to cover additional cases related to
>> record patterns.
>
> Jan Lahoda has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains eight commits:
>
> - Merge branch 'type-pattern-third' into patterns-record-deconstruction3
> - Using Type.isRaw instead of checking the AST structure.
> - Exhaustiveness should accept supertypes of the specified type.
> - Renaming the features from deconstruction pattern to record pattern.
> - Fixing guards after record patterns.
> - Raw types are not allowed in record patterns.
> - Reflecting review feedback.
> - 8262889: Compiler implementation for Record Patterns
I've noticed that this code:
class Test {
String e(E e) {
return switch (e) {
case A -> "42";
};
}
enum E {
A, B;
}
}
fails with:
Test.java:3: error: the switch expression does not cover all possible input
values
return switch (e) {
^
1 error
before this change but gets accepted with no error message after it
-
PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns [v4]
On Mon, 9 May 2022 14:37:35 GMT, Jan Lahoda wrote: >> 8262889: Compiler implementation for Record Patterns >> >> A first version of a patch that introduces record patterns into javac as a >> preview feature. For the specification, please see: >> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html >> >> There are two notable tricky parts: >> -in the parser, it was necessary to improve the `analyzePattern` method to >> handle nested/record patterns, while still keeping error recovery reasonable >> -in the `TransPatterns`, the desugaring of the record patterns is very >> straightforward - effectivelly the record patterns are desugared into >> guards/conditions. This will likely be improved in some future >> version/preview >> >> `MatchException` has been extended to cover additional cases related to >> record patterns. > > Jan Lahoda has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains eight commits: > > - Merge branch 'type-pattern-third' into patterns-record-deconstruction3 > - Using Type.isRaw instead of checking the AST structure. > - Exhaustiveness should accept supertypes of the specified type. > - Renaming the features from deconstruction pattern to record pattern. > - Fixing guards after record patterns. > - Raw types are not allowed in record patterns. > - Reflecting review feedback. > - 8262889: Compiler implementation for Record Patterns src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 822: > 820: > nestedComponentPatterns); > 821: > 822: //for each of the symbols covered by the starting component, > find all deconstruction patterns this comment should probably read: find all `record` patterns? - PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns [v4]
On Fri, 6 May 2022 17:40:25 GMT, Jan Lahoda wrote: >> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 4217: >> >>> 4215: } >>> 4216: ListBuffer outBindings = new ListBuffer<>(); >>> 4217: List recordTypes = expectedRecordTypes; >> >> nit: probably a matter of style but why not reusing the already declared >> `expectedRecordTypes` declaring a new variable seems unnecessary > > Please note the full `expectedRecordTypes` are used for error reporting, but > the reference to `List` in `recordTypes` is overwritten in the loop (at the > time of an error report, it may not longer point to the full expected types, > and hence cannot be used for error reporting). Ok I see, thanks - PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns [v4]
> 8262889: Compiler implementation for Record Patterns > > A first version of a patch that introduces record patterns into javac as a > preview feature. For the specification, please see: > http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html > > There are two notable tricky parts: > -in the parser, it was necessary to improve the `analyzePattern` method to > handle nested/record patterns, while still keeping error recovery reasonable > -in the `TransPatterns`, the desugaring of the record patterns is very > straightforward - effectivelly the record patterns are desugared into > guards/conditions. This will likely be improved in some future version/preview > > `MatchException` has been extended to cover additional cases related to > record patterns. Jan Lahoda has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits: - Merge branch 'type-pattern-third' into patterns-record-deconstruction3 - Using Type.isRaw instead of checking the AST structure. - Exhaustiveness should accept supertypes of the specified type. - Renaming the features from deconstruction pattern to record pattern. - Fixing guards after record patterns. - Raw types are not allowed in record patterns. - Reflecting review feedback. - 8262889: Compiler implementation for Record Patterns - Changes: https://git.openjdk.java.net/jdk/pull/8516/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8516=03 Stats: 2255 lines in 50 files changed: 2169 ins; 15 del; 71 mod Patch: https://git.openjdk.java.net/jdk/pull/8516.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8516/head:pull/8516 PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns [v3]
On Sat, 7 May 2022 12:03:04 GMT, Jan Lahoda wrote:
>> 8262889: Compiler implementation for Record Patterns
>>
>> A first version of a patch that introduces record patterns into javac as a
>> preview feature. For the specification, please see:
>> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html
>>
>> There are two notable tricky parts:
>> -in the parser, it was necessary to improve the `analyzePattern` method to
>> handle nested/record patterns, while still keeping error recovery reasonable
>> -in the `TransPatterns`, the desugaring of the record patterns is very
>> straightforward - effectivelly the record patterns are desugared into
>> guards/conditions. This will likely be improved in some future
>> version/preview
>>
>> `MatchException` has been extended to cover additional cases related to
>> record patterns.
>
> Jan Lahoda has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Fixing guards after record patterns.
> - Raw types are not allowed in record patterns.
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 4208:
> 4206: if (site.tsym.kind == Kind.TYP && ((ClassSymbol)
> site.tsym).isRecord()) {
> 4207: ClassSymbol record = (ClassSymbol) site.tsym;
> 4208: if (record.type.getTypeArguments().nonEmpty()) {
There is a `Type::isRaw()` - I supposed you tried that one? Doesn't it do what
you want?
test/langtools/tools/javac/patterns/DeconstructionPatternErrors.out line 3:
> 1: DeconstructionPatternErrors.java:15:28:
> compiler.err.underscore.as.identifier
> 2: DeconstructionPatternErrors.java:15:29: compiler.err.expected:
> token.identifier
> 3: DeconstructionPatternErrors.java:43:37: compiler.err.illegal.start.of.type
should error be more specific here? E.g. diamond not supported with type test
pattern?
-
PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns [v3]
> 8262889: Compiler implementation for Record Patterns > > A first version of a patch that introduces record patterns into javac as a > preview feature. For the specification, please see: > http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html > > There are two notable tricky parts: > -in the parser, it was necessary to improve the `analyzePattern` method to > handle nested/record patterns, while still keeping error recovery reasonable > -in the `TransPatterns`, the desugaring of the record patterns is very > straightforward - effectivelly the record patterns are desugared into > guards/conditions. This will likely be improved in some future version/preview > > `MatchException` has been extended to cover additional cases related to > record patterns. Jan Lahoda has updated the pull request incrementally with two additional commits since the last revision: - Fixing guards after record patterns. - Raw types are not allowed in record patterns. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8516/files - new: https://git.openjdk.java.net/jdk/pull/8516/files/90b37c3a..0e384fb3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8516=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8516=01-02 Stats: 191 lines in 11 files changed: 157 ins; 22 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/8516.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8516/head:pull/8516 PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns [v2]
On Fri, 6 May 2022 14:30:10 GMT, Maurizio Cimadamore
wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Reflecting review feedback.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 752:
>
>> 750:Iterable> JCCaseLabel> labels) {
>> 751: Set coveredSymbols = new HashSet<>();
>> 752: Map>
>> deconstructionPatternsBySymbol = new HashMap<>();
>
> since you seem to have settled on "recordPattern" for implementation names -
> you can probably revisit some of these names to say "record" instead of
> "deconstruction".
Right. Will do.
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 801:
>
>> 799: //i.e. represent all possible combinations.
>> 800: //This is done by categorizing the patterns based on the
>> type covered by the given
>> 801: //starting component.
>
> Example needed here. For instance (I discussed this with @biboudis):
>
>
> record Outer(R r) { };
> sealed interface I { };
> class A implements I { };
> class B implements I { };
> sealed interface R { };
> record Foo(I i) implements R { }
> record Bar(I i) implements R { }
>
> switch (o) {
>case Outer(Foo(A), Foo(A)):
>case Outer(Foo(B), Foo(B)):
>case Outer(Foo(A), Foo(B)):
>case Outer(Foo(B), Foo(A)):
>case Outer(Bar(A), Bar(A)):
>case Outer(Bar(B), Bar(B)):
>case Outer(Bar(A), Bar(B)):
>case Outer(Bar(B), Bar(A)):
> }
>
>
> Which generates two sets:
>
>
>case Outer(Foo(A), Foo(A)):
>case Outer(Foo(B), Foo(B)):
>case Outer(Foo(A), Foo(B)):
>case Outer(Foo(B), Foo(A)):
>
>
> And
>
>
>case Outer(Bar(A), Bar(A)):
>case Outer(Bar(B), Bar(B)):
>case Outer(Bar(A), Bar(B)):
>case Outer(Bar(B), Bar(A)):
>
>
> Sorry for being pedantic - this code is tricky and I'm worried we'll all
> forget exactly how it works in 2 months :-)
Sure. Will try to improve.
-
PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns [v2]
On Thu, 5 May 2022 18:11:54 GMT, Vicente Romero wrote: >> Jan Lahoda has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Reflecting review feedback. > > src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 4217: > >> 4215: } >> 4216: ListBuffer outBindings = new ListBuffer<>(); >> 4217: List recordTypes = expectedRecordTypes; > > nit: probably a matter of style but why not reusing the already declared > `expectedRecordTypes` declaring a new variable seems unnecessary Please note the full `expectedRecordTypes` are used for error reporting, but the reference to `List` in `recordTypes` is overwritten in the loop (at the time of an error report, it may not longer point to the full expected types, and hence cannot be used for error reporting). - PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns [v2]
On Fri, 6 May 2022 15:44:22 GMT, Gavin Bierman wrote:
> > From the JLS specdiff
> > > If the type R names a generic record class then it is a compile-time
> > > error if R is not a parameterized type.
> >
> >
> > The following snippet raises a `MatchException`. Shouldn't it be a
> > compile-time error?
> > ```
> > Box r = new Box<>(null);
> >
> > switch (r) {
> > case Box(String s):
> > System.out.println("match");
> > }
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > If this is Ok and my understanding is wrong, then why that raises an
> > exception at all? I can make it work (as an unconditional) if I define the
> > Box as `record Box` and now I am confused...
> > ping @GavinBierman @lahodaj
>
> A couple of issues here. (1) This should be a compile-time error. (2) upon
> investigation I think there is a bug with the pattern matching code, because
> the compiler is currently saying that the pattern match here: `Box bs
> = new Box<>(null); if (bs instanceof Box(String s)) {
> System.out.println("match!"); }` does not succeed. (It should do). The
> `MatchException` you are seeing is that the exhaustive pattern switch has no
> matching label (if you put in a default, you don't get the exception).
Right. Will fix. Sorry for that.
-
PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns [v2]
On Fri, 6 May 2022 14:09:24 GMT, Jan Lahoda wrote: >> 8262889: Compiler implementation for Record Patterns >> >> A first version of a patch that introduces record patterns into javac as a >> preview feature. For the specification, please see: >> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html >> >> There are two notable tricky parts: >> -in the parser, it was necessary to improve the `analyzePattern` method to >> handle nested/record patterns, while still keeping error recovery reasonable >> -in the `TransPatterns`, the desugaring of the record patterns is very >> straightforward - effectivelly the record patterns are desugared into >> guards/conditions. This will likely be improved in some future >> version/preview >> >> `MatchException` has been extended to cover additional cases related to >> record patterns. > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Reflecting review feedback. src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 4217: > 4215: } > 4216: ListBuffer outBindings = new ListBuffer<>(); > 4217: List recordTypes = expectedRecordTypes; nit: probably a matter of style but why not reusing the already declared `expectedRecordTypes` declaring a new variable seems unnecessary - PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns [v2]
On Fri, 6 May 2022 14:09:24 GMT, Jan Lahoda wrote:
>> 8262889: Compiler implementation for Record Patterns
>>
>> A first version of a patch that introduces record patterns into javac as a
>> preview feature. For the specification, please see:
>> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html
>>
>> There are two notable tricky parts:
>> -in the parser, it was necessary to improve the `analyzePattern` method to
>> handle nested/record patterns, while still keeping error recovery reasonable
>> -in the `TransPatterns`, the desugaring of the record patterns is very
>> straightforward - effectivelly the record patterns are desugared into
>> guards/conditions. This will likely be improved in some future
>> version/preview
>>
>> `MatchException` has been extended to cover additional cases related to
>> record patterns.
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Reflecting review feedback.
> From the JLS specdiff
>
> > If the type R names a generic record class then it is a compile-time error
> > if R is not a parameterized type.
>
> The following snippet raises a `MatchException`. Shouldn't it be a
> compile-time error?
>
> ```
> Box r = new Box<>(null);
>
> switch (r) {
> case Box(String s):
> System.out.println("match");
> }
> ```
>
> If this is Ok and my understanding is wrong, then why that raises an
> exception at all? I can make it work (as an unconditional) if I define the
> Box as `record Box` and now I am confused...
>
> ping @GavinBierman @lahodaj
A couple of issues here. (1) This should be a compile-time error. (2) upon
investigation I think there is a bug with the pattern matching code, because
the compiler is currently saying that the pattern match here: `Box bs =
new Box<>(null); if (bs instanceof Box(String s)) {
System.out.println("match!"); }` does not succeed. (It should do). The
`MatchException` you are seeing is that the exhaustive pattern switch has no
matching label (if you put in a default, you don't get the exception).
-
PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns [v2]
On Fri, 6 May 2022 14:09:24 GMT, Jan Lahoda wrote:
>> 8262889: Compiler implementation for Record Patterns
>>
>> A first version of a patch that introduces record patterns into javac as a
>> preview feature. For the specification, please see:
>> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html
>>
>> There are two notable tricky parts:
>> -in the parser, it was necessary to improve the `analyzePattern` method to
>> handle nested/record patterns, while still keeping error recovery reasonable
>> -in the `TransPatterns`, the desugaring of the record patterns is very
>> straightforward - effectivelly the record patterns are desugared into
>> guards/conditions. This will likely be improved in some future
>> version/preview
>>
>> `MatchException` has been extended to cover additional cases related to
>> record patterns.
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Reflecting review feedback.
Looks good
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 752:
> 750:Iterable JCCaseLabel> labels) {
> 751: Set coveredSymbols = new HashSet<>();
> 752: Map>
> deconstructionPatternsBySymbol = new HashMap<>();
since you seem to have settled on "recordPattern" for implementation names -
you can probably revisit some of these names to say "record" instead of
"deconstruction".
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 801:
> 799: //i.e. represent all possible combinations.
> 800: //This is done by categorizing the patterns based on the
> type covered by the given
> 801: //starting component.
Example needed here. For instance (I discussed this with @biboudis):
record Outer(R r) { };
sealed interface I { };
class A implements I { };
class B implements I { };
sealed interface R { };
record Foo(I i) implements R { }
record Bar(I i) implements R { }
switch (o) {
case Outer(Foo(A), Foo(A)):
case Outer(Foo(B), Foo(B)):
case Outer(Foo(A), Foo(B)):
case Outer(Foo(B), Foo(A)):
case Outer(Bar(A), Bar(A)):
case Outer(Bar(B), Bar(B)):
case Outer(Bar(A), Bar(B)):
case Outer(Bar(B), Bar(A)):
}
Which generates two sets:
case Outer(Foo(A), Foo(A)):
case Outer(Foo(B), Foo(B)):
case Outer(Foo(A), Foo(B)):
case Outer(Foo(B), Foo(A)):
And
case Outer(Bar(A), Bar(A)):
case Outer(Bar(B), Bar(B)):
case Outer(Bar(A), Bar(B)):
case Outer(Bar(B), Bar(A)):
Sorry for being pedantic - this code is tricky and I'm worried we'll all forget
exactly how it works in 2 months :-)
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java line
1014:
> 1012: pattern = parsePattern(patternPos, mods, type,
> false);
> 1013: } else if (token.kind == LPAREN) {
> 1014: pattern = parsePattern(patternPos, mods, type,
> false);
Nice!
-
Marked as reviewed by mcimadamore (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns [v2]
On Fri, 6 May 2022 14:09:24 GMT, Jan Lahoda wrote:
>> 8262889: Compiler implementation for Record Patterns
>>
>> A first version of a patch that introduces record patterns into javac as a
>> preview feature. For the specification, please see:
>> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html
>>
>> There are two notable tricky parts:
>> -in the parser, it was necessary to improve the `analyzePattern` method to
>> handle nested/record patterns, while still keeping error recovery reasonable
>> -in the `TransPatterns`, the desugaring of the record patterns is very
>> straightforward - effectivelly the record patterns are desugared into
>> guards/conditions. This will likely be improved in some future
>> version/preview
>>
>> `MatchException` has been extended to cover additional cases related to
>> record patterns.
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Reflecting review feedback.
>From the JLS specdiff
> If the type R names a generic record class then it is a compile-time error if
> R is not a parameterized type.
The following snippet raises a `MatchException`. Shouldn't it be a compile-time
error?
Box r = new Box<>(null);
switch (r) {
case Box(String s):
System.out.println("match");
}
-
PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns [v2]
> 8262889: Compiler implementation for Record Patterns > > A first version of a patch that introduces record patterns into javac as a > preview feature. For the specification, please see: > http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html > > There are two notable tricky parts: > -in the parser, it was necessary to improve the `analyzePattern` method to > handle nested/record patterns, while still keeping error recovery reasonable > -in the `TransPatterns`, the desugaring of the record patterns is very > straightforward - effectivelly the record patterns are desugared into > guards/conditions. This will likely be improved in some future version/preview > > `MatchException` has been extended to cover additional cases related to > record patterns. Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Reflecting review feedback. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8516/files - new: https://git.openjdk.java.net/jdk/pull/8516/files/56020b0b..90b37c3a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8516=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8516=00-01 Stats: 193 lines in 18 files changed: 67 ins; 19 del; 107 mod Patch: https://git.openjdk.java.net/jdk/pull/8516.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8516/head:pull/8516 PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns
On Wed, 4 May 2022 10:03:13 GMT, Maurizio Cimadamore
wrote:
>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 4180:
>>
>>> 4178: type = attribTree(tree.var.vartype, env, varInfo);
>>> 4179: } else {
>>> 4180: type = resultInfo.pt;
>>
>> Looks good - infers the binging var type from the record component being
>> processed. If not in a record, then I suspect resultInfo.pt is just the
>> target expression type (e.g. var in toplevel environment).
>
> That said, I'm not sure how this connects with `instanceof`. This patch
> doesn't seem to alter `visitTypeTest`. In the current code I can see this:
>
> if (tree.pattern.getTag() == BINDINGPATTERN ||
>tree.pattern.getTag() == PARENTHESIZEDPATTERN) {
>attribTree(tree.pattern, env, unknownExprInfo);
>...
>
>
> This seems wrong for two reasons:
>
> * it doesn't take into account the new pattern tag
> * it uses an "unknown" result info when attributing, meaning that any
> toplevel `var` pattern will not be attributed correctly
>
> But we seem to have tests covering record patterns and instanceof. so I'm
> wondering if I'm missing some code update?
Some of the handling inside this ifs is only needed for type test and
parenthesized patterns (as record patterns are never unconditional (total)). I
have an upcoming patch that should improve the tests here, however.
For `var` - the specification does not allow `var` at the top level (14.30.1,
"The LocalVariableType in a top-level type pattern denotes a reference type
(and furthermore is not var)."). In my upcoming patch, I am adding a test to
ensure meaningful behavior for top-level type test patterns with `var`.
-
PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns
On Fri, 6 May 2022 10:51:33 GMT, Jan Lahoda wrote:
>> I now believe that the check is needed to properly classify patterns based
>> on the type of the i-th component. That said, not sure this should be a
>> subtyping check, or a type equality
>
> A good question. Consider code like:
>
> private void test(R r) {
> switch (r) {
> case R(A a, A v) -> {}
> case R(B b, A v) -> {}
> case R(I i, B v) -> {}
> }
> }
> final class A implements I {}
> sealed interface I permits A, B {}
> final class B implements I {}
> record R(I i1, I i2) {}
>
>
> The switch is exhaustive - all the possible combinations are covered. When
> checking the first component, the code will categorize the patterns like:
>
> A -> R(A a, A v), R(I i, B v)
> B -> R(B b, A v), R(I i, B v)
> I -> R(I i, B v)
>
> this categorization is done using the subtype check, so that `R(I i, B v)`
> will appear in the list for `A`. When checking the second component, the
> possibility for `I` is not exhaustive (does not cover `A` in the second
> component), but the possibilities for `A` and `B` are exhaustive, and they
> together cover `I`.
Ah - makes sense of course - I "just" needed a more convoluted example :-)
-
PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns
On Thu, 5 May 2022 15:17:11 GMT, Maurizio Cimadamore
wrote:
>> You are right. It is the ii) which iteratively checks the component pattern
>> list L.
>
> I now believe that the check is needed to properly classify patterns based on
> the type of the i-th component. That said, not sure this should be a
> subtyping check, or a type equality
A good question. Consider code like:
private void test(R r) {
switch (r) {
case R(A a, A v) -> {}
case R(B b, A v) -> {}
case R(I i, B v) -> {}
}
}
final class A implements I {}
sealed interface I permits A, B {}
final class B implements I {}
record R(I i1, I i2) {}
The switch is exhaustive - all the possible combinations are covered. When
checking the first component, the code will categorize the patterns like:
A -> R(A a, A v), R(I i, B v)
B -> R(B b, A v), R(I i, B v)
I -> R(I i, B v)
this categorization is done using the subtype check, so that `R(I i, B v)` will
appear in the list for `A`. When checking the second component, the possibility
for `I` is not exhaustive (does not cover `A` in the second component), but the
possibilities for `A` and `B` are exhaustive, and they together cover `I`.
-
PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns
On Thu, 5 May 2022 11:57:34 GMT, Aggelos Biboudis
wrote:
>> 8262889: Compiler implementation for Record Patterns
>>
>> A first version of a patch that introduces record patterns into javac as a
>> preview feature. For the specification, please see:
>> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html
>>
>> There are two notable tricky parts:
>> -in the parser, it was necessary to improve the `analyzePattern` method to
>> handle nested/record patterns, while still keeping error recovery reasonable
>> -in the `TransPatterns`, the desugaring of the record patterns is very
>> straightforward - effectivelly the record patterns are desugared into
>> guards/conditions. This will likely be improved in some future
>> version/preview
>>
>> `MatchException` has been extended to cover additional cases related to
>> record patterns.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 783:
>
>> 781: }
>> 782: for (Entry> e :
>> categorizedDeconstructionPatterns.entrySet()) {
>> 783: if (coversDeconstructionStartingFromComponent(pos,
>> targetType, e.getValue(), 0)) {
>
> Here, the result of `e.getValue` is a reversed list of the observed patterns.
>
> For the switch below,
>
>
> switch (r) {
> case R(A a, A b) -> 1;
> case R(A a, B b) -> 2;
> case R(B a, A b) -> 3;
> case R(B a, B b) -> 4;
> }
>
>
> The 0th element of that list is the `R(B a, B b)` pattern, the 1st the `R(B
> a, A b)`. I am 99% sure that this is not a problem but I am pointing it out
> regardless, in case there is any underlying danger to that.
Order doesn't matter. I triple checked.
-
PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns
On Thu, 5 May 2022 15:21:49 GMT, Maurizio Cimadamore
wrote:
>> 8262889: Compiler implementation for Record Patterns
>>
>> A first version of a patch that introduces record patterns into javac as a
>> preview feature. For the specification, please see:
>> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html
>>
>> There are two notable tricky parts:
>> -in the parser, it was necessary to improve the `analyzePattern` method to
>> handle nested/record patterns, while still keeping error recovery reasonable
>> -in the `TransPatterns`, the desugaring of the record patterns is very
>> straightforward - effectivelly the record patterns are desugared into
>> guards/conditions. This will likely be improved in some future
>> version/preview
>>
>> `MatchException` has been extended to cover additional cases related to
>> record patterns.
>
> src/java.base/share/classes/jdk/internal/javac/PreviewFeature.java line 64:
>
>> 62: public enum Feature {
>> 63: SWITCH_PATTERN_MATCHING,
>> 64: DECONSTRUCTION_PATTERNS,
>
> The spec and JEP talks about record patterns - I think we should follow the
> correct name here
yup, I agree
-
PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns
On Tue, 3 May 2022 12:07:50 GMT, Jan Lahoda wrote:
> 8262889: Compiler implementation for Record Patterns
>
> A first version of a patch that introduces record patterns into javac as a
> preview feature. For the specification, please see:
> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html
>
> There are two notable tricky parts:
> -in the parser, it was necessary to improve the `analyzePattern` method to
> handle nested/record patterns, while still keeping error recovery reasonable
> -in the `TransPatterns`, the desugaring of the record patterns is very
> straightforward - effectivelly the record patterns are desugared into
> guards/conditions. This will likely be improved in some future version/preview
>
> `MatchException` has been extended to cover additional cases related to
> record patterns.
I've added some renaming suggestions for the code in Flow, after some
discussions with @biboudis.
More generally, that code should contain comments, with example of what it's
trying to do - e.g. how it's partitioning the set of patterns, etc.
src/java.base/share/classes/jdk/internal/javac/PreviewFeature.java line 64:
> 62: public enum Feature {
> 63: SWITCH_PATTERN_MATCHING,
> 64: DECONSTRUCTION_PATTERNS,
The spec and JEP talks about record patterns - I think we should follow the
correct name here
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 751:
> 749:Iterable JCCaseLabel> labels) {
> 750: Set constants = new HashSet<>();
> 751: Map>
> categorizedDeconstructionPatterns = new HashMap<>();
maybe rename to `deconstructionPatternsBySymbol` ?
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 790:
> 788: }
> 789:
> 790: private boolean
> coversDeconstructionStartingFromComponent(DiagnosticPosition pos,
maybe rename as `coverDeconstructionFromComponent`/`coverDeconstructionAt`
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 792:
> 790: private boolean
> coversDeconstructionStartingFromComponent(DiagnosticPosition pos,
> 791: Type
> targetType,
> 792:
> List patterns,
patterns -> "deconstructionPatterns"
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 800:
> 798: }
> 799:
> 800: Type parameterizedComponentType =
> types.memberType(targetType, components.get(component));
maybe `instantiatedComponentType`
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 802:
> 800: Type parameterizedComponentType =
> types.memberType(targetType, components.get(component));
> 801: List nestedComponentPatterns = patterns.map(d ->
> d.nested.get(component));
> 802: Set nestedCovered = coveredSymbols(pos,
> parameterizedComponentType,
maybe `coveredComponents`
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 807:
> 805: Set covered = new HashSet<>();
> 806:
> 807: for (JCDeconstructionPattern subTypeCandidate : patterns) {
`subTypeCandidate` -> `deconstructionPattern`
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 809:
> 807: for (JCDeconstructionPattern subTypeCandidate : patterns) {
> 808: JCPattern nestedPattern =
> subTypeCandidate.nested.get(component);
> 809: Symbol currentPatternType;
`currentPatternType` -> `componentPatternType`
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 823:
> 821: }
> 822: }
> 823: for (Symbol currentType : nestedCovered) {
`currentType` -> `coveredComponent`
src/jdk.compiler/share/classes/com/sun/tools/javac/tree/JCTree.java line 246:
> 244: PARENTHESIZEDPATTERN,
> 245:
> 246: DECONSTRUCTIONPATTERN,
might want to rename to RECORDPATTERN (but this is impl dependent, so less
important to fix)
src/jdk.compiler/share/classes/com/sun/tools/javac/tree/JCTree.java line 2373:
> 2371: }
> 2372:
> 2373: public static class JCDeconstructionPattern extends JCPattern
JCRecordPattern (although this is impl-only, so less important to fix)
-
PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns
On Thu, 5 May 2022 12:28:42 GMT, Aggelos Biboudis wrote: >>> I think this is i) from the domination relation: >>> >>> > A record pattern with type R and record component pattern list L >>> > dominates another record pattern with type S and record component pattern >>> > list M if (i) the erasure of S is a subtype of the erasure of R, and (ii) >>> > every pattern, if any, in L dominates the corresponding pattern in M. >> >> But this subtyping test seems to happen at the level of the component >> pattern list, not at the R/S level, right? > > You are right. It is the ii) which iteratively checks the component pattern > list L. I now believe that the check is needed to properly classify patterns based on the type of the i-th component. That said, not sure this should be a subtyping check, or a type equality - PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns
On Thu, 5 May 2022 12:16:23 GMT, Maurizio Cimadamore wrote: >> I think this is i) from the domination relation: >> >>> A record pattern with type R and record component pattern list L dominates >>> another record pattern with type S and record component pattern list M if >>> (i) the erasure of S is a subtype of the erasure of R, and (ii) every >>> pattern, if any, in L dominates the corresponding pattern in M. > >> I think this is i) from the domination relation: >> >> > A record pattern with type R and record component pattern list L dominates >> > another record pattern with type S and record component pattern list M if >> > (i) the erasure of S is a subtype of the erasure of R, and (ii) every >> > pattern, if any, in L dominates the corresponding pattern in M. > > But this subtyping test seems to happen at the level of the component pattern > list, not at the R/S level, right? You are right. It is the ii) which iteratively checks the component pattern list L. - PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns
On Thu, 5 May 2022 12:06:38 GMT, Aggelos Biboudis wrote: > I think this is i) from the domination relation: > > > A record pattern with type R and record component pattern list L dominates > > another record pattern with type S and record component pattern list M if > > (i) the erasure of S is a subtype of the erasure of R, and (ii) every > > pattern, if any, in L dominates the corresponding pattern in M. But this subtyping test seems to happen at the level of the component pattern list, not at the R/S level, right? - PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns
On Tue, 3 May 2022 12:07:50 GMT, Jan Lahoda wrote:
> 8262889: Compiler implementation for Record Patterns
>
> A first version of a patch that introduces record patterns into javac as a
> preview feature. For the specification, please see:
> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html
>
> There are two notable tricky parts:
> -in the parser, it was necessary to improve the `analyzePattern` method to
> handle nested/record patterns, while still keeping error recovery reasonable
> -in the `TransPatterns`, the desugaring of the record patterns is very
> straightforward - effectivelly the record patterns are desugared into
> guards/conditions. This will likely be improved in some future version/preview
>
> `MatchException` has been extended to cover additional cases related to
> record patterns.
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 783:
> 781: }
> 782: for (Entry> e :
> categorizedDeconstructionPatterns.entrySet()) {
> 783: if (coversDeconstructionStartingFromComponent(pos,
> targetType, e.getValue(), 0)) {
Here, the result of `e.getValue` is a reversed list of the observed patterns.
For the switch below,
switch (r) {
case R(A a, A b) -> 1;
case R(A a, B b) -> 2;
case R(B a, A b) -> 3;
case R(B a, B b) -> 4;
}
The 0th element of that list is the `R(B a, B b)` pattern, the 1st the `R(B a,
A b)`. I am 99% sure that this is not a problem but I am pointing it out
regardless, in case there is any underlying danger to that.
-
PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns
On Wed, 4 May 2022 10:51:38 GMT, Maurizio Cimadamore
wrote:
>> 8262889: Compiler implementation for Record Patterns
>>
>> A first version of a patch that introduces record patterns into javac as a
>> preview feature. For the specification, please see:
>> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html
>>
>> There are two notable tricky parts:
>> -in the parser, it was necessary to improve the `analyzePattern` method to
>> handle nested/record patterns, while still keeping error recovery reasonable
>> -in the `TransPatterns`, the desugaring of the record patterns is very
>> straightforward - effectivelly the record patterns are desugared into
>> guards/conditions. This will likely be improved in some future
>> version/preview
>>
>> `MatchException` has been extended to cover additional cases related to
>> record patterns.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 824:
>
>> 822: }
>> 823: for (Symbol currentType : nestedCovered) {
>> 824: if (types.isSubtype(types.erasure(currentType.type),
>
> Not 100% what this test does
I think this is i) from the domination relation:
> A record pattern with type R and record component pattern list L dominates
> another record pattern with type S and record component pattern list M if (i)
> the erasure of S is a subtype of the erasure of R, and (ii) every pattern, if
> any, in L dominates the corresponding pattern in M.
-
PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8262889: Compiler implementation for Record Patterns
On Tue, 3 May 2022 12:07:50 GMT, Jan Lahoda wrote:
> 8262889: Compiler implementation for Record Patterns
>
> A first version of a patch that introduces record patterns into javac as a
> preview feature. For the specification, please see:
> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html
>
> There are two notable tricky parts:
> -in the parser, it was necessary to improve the `analyzePattern` method to
> handle nested/record patterns, while still keeping error recovery reasonable
> -in the `TransPatterns`, the desugaring of the record patterns is very
> straightforward - effectivelly the record patterns are desugared into
> guards/conditions. This will likely be improved in some future version/preview
>
> `MatchException` has been extended to cover additional cases related to
> record patterns.
Changes look generally good. Personally, I found the changes in Flow the harder
to follow - but I that part is just hard (even in the spec), as it has to
perform a search in both breadth (as records have more than one component) and
in depth (as patterns can be nested). I've added some comments to check my
understanding.
src/java.base/share/classes/java/lang/MatchException.java line 41:
> 39: *constants at runtime than it had at compilation time, or the
> type hierarchy has changed
> 40: *in incompatible ways between compile time and run time.
> 41: *Null targets and sealed types. If an interface or abstract
> class {@code C} is sealed
Should this be `null targets and nested sealed type test patterns` ?
Also, not sure `null` target is the right expression - maybe `null and nested
xyz` would work better?
src/jdk.compiler/share/classes/com/sun/source/tree/DeconstructionPatternTree.java
line 43:
> 41: * @return the deconstructed type
> 42: */
> 43: Tree getDeconstructor();
Shouldn't this return an ExpressionTree?
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 4180:
> 4178: type = attribTree(tree.var.vartype, env, varInfo);
> 4179: } else {
> 4180: type = resultInfo.pt;
Looks good - infers the binging var type from the record component being
processed. If not in a record, then I suspect resultInfo.pt is just the target
expression type (e.g. var in toplevel environment).
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 750:
> 748: private Set coveredSymbols(DiagnosticPosition pos, Type
> targetType,
> 749:Iterable JCCaseLabel> labels) {
> 750: Set constants = new HashSet<>();
Should this be called `constants` ?
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 824:
> 822: }
> 823: for (Symbol currentType : nestedCovered) {
> 824: if (types.isSubtype(types.erasure(currentType.type),
Not 100% what this test does
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 836:
> 834: for (Entry> e :
> componentType2Patterns.entrySet()) {
> 835: if (coversDeconstructionStartingFromComponent(pos,
> targetType, e.getValue(), component + 1)) {
> 836: covered.add(e.getKey());
So... it seems to me that what this code is doing is that, for a component
index _i_, we get all its recursively covered symbols - then we add them to the
set of covered symbols of component _i_, but only if components _w_, where _w_
> _i_ are also covered? That is, if in `R(P1, P2, ... Pn)`, we see that `P1` is
covered, but `P2` is not, we also consider `P1` not covered (which in turn
makes `R` not covered).
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java line
788:
> 786: }
> 787: if (token.kind == LPAREN) {
> 788: ListBuffer nested = new ListBuffer<>();
should we add comment saying "deconstruction pattern"
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java line
808:
> 806: pattern = toP(F.at(pos).DeconstructionPattern(e,
> nested.toList(), var));
> 807: } else {
> 808: JCVariableDecl var = toP(F.at(token.pos).VarDef(mods,
> ident(), e, null));
and, here, a comment saying "type test pattern" ?
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavacParser.java line
1004:
> 1002: JCExpression type = unannotatedType(false);
> 1003: if (token.kind == IDENTIFIER) {
> 1004: checkSourceLevel(token.pos,
> Feature.PATTERN_MATCHING_IN_INSTANCEOF);
Two question/comments:
* I believe that `checkSourceLevel` is called here, but not in `analyzePattern`
because `analyzePattern` is also called in the `switch` code, in which case we
already check for source level which supports pattern in switch, correct?
* For type test pattern, this
Re: RFR: 8262889: Compiler implementation for Record Patterns
On Wed, 4 May 2022 09:59:33 GMT, Maurizio Cimadamore
wrote:
>> 8262889: Compiler implementation for Record Patterns
>>
>> A first version of a patch that introduces record patterns into javac as a
>> preview feature. For the specification, please see:
>> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html
>>
>> There are two notable tricky parts:
>> -in the parser, it was necessary to improve the `analyzePattern` method to
>> handle nested/record patterns, while still keeping error recovery reasonable
>> -in the `TransPatterns`, the desugaring of the record patterns is very
>> straightforward - effectivelly the record patterns are desugared into
>> guards/conditions. This will likely be improved in some future
>> version/preview
>>
>> `MatchException` has been extended to cover additional cases related to
>> record patterns.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 4180:
>
>> 4178: type = attribTree(tree.var.vartype, env, varInfo);
>> 4179: } else {
>> 4180: type = resultInfo.pt;
>
> Looks good - infers the binging var type from the record component being
> processed. If not in a record, then I suspect resultInfo.pt is just the
> target expression type (e.g. var in toplevel environment).
That said, I'm not sure how this connects with `instanceof`. This patch doesn't
seem to alter `visitTypeTest`. In the current code I can see this:
if (tree.pattern.getTag() == BINDINGPATTERN ||
tree.pattern.getTag() == PARENTHESIZEDPATTERN) {
attribTree(tree.pattern, env, unknownExprInfo);
...
This seems wrong for two reasons:
* it doesn't take into account the new pattern tag
* it uses an "unknown" result info when attributing, meaning that any toplevel
`var` pattern will not be attributed correctly
But we seem to have tests covering record patterns and instanceof. so I'm
wondering if I'm missing some code update?
-
PR: https://git.openjdk.java.net/jdk/pull/8516
RFR: 8262889: Compiler implementation for Record Patterns
8262889: Compiler implementation for Record Patterns A first version of a patch that introduces record patterns into javac as a preview feature. For the specification, please see: http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html There are two notable tricky parts: -in the parser, it was necessary to improve the `analyzePattern` method to handle nested/record patterns, while still keeping error recovery reasonable -in the `TransPatterns`, the desugaring of the record patterns is very straightforward - effectivelly the record patterns are desugared into guards/conditions. This will likely be improved in some future version/preview `MatchException` has been extended to cover additional cases related to record patterns. - Depends on: https://git.openjdk.java.net/jdk/pull/8182 Commit messages: - 8262889: Compiler implementation for Record Patterns Changes: https://git.openjdk.java.net/jdk/pull/8516/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8516=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8262889 Stats: 2008 lines in 45 files changed: 1943 ins; 15 del; 50 mod Patch: https://git.openjdk.java.net/jdk/pull/8516.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8516/head:pull/8516 PR: https://git.openjdk.java.net/jdk/pull/8516
