On Tue, 4 May 2021 20:48:34 GMT, Maurizio Cimadamore <mcimadam...@openjdk.org> wrote:
>> This is a preview of a patch implementing JEP 406: Pattern Matching for >> switch (Preview): >> https://bugs.openjdk.java.net/browse/JDK-8213076 >> >> The current draft of the specification is here: >> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html >> >> A summary of notable parts of the patch: >> -to support cases expressions and patterns in cases, there is a new common >> superinterface for expressions and patterns, `CaseLabelTree`, which >> expressions and patterns implement, and a list of case labels is returned >> from `CaseTree.getLabels()`. >> -to support `case default`, there is an implementation of `CaseLabelTree` >> that represents it (`DefaultCaseLabelTree`). It is used also to represent >> the conventional `default` internally and in the newly added methods. >> -in the parser, parenthesized patterns and expressions need to be >> disambiguated when parsing case labels. >> -Lower has been enhanced to handle `case null` for ordinary >> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed >> primitives, as there is no value that is not part of the input domain so >> that could be used to represent `case null`. Requires a bit shuffling with >> values. >> -TransPatterns has been enhanced to handle the pattern matching switch. It >> produces code that delegates to a new bootstrap method, that will classify >> the input value to the switch and return the case number, to which the >> switch then jumps. To support guards, the switches (and the bootstrap >> method) are restartable. The bootstrap method as such is written very simply >> so far, but could be much more optimized later. >> -nullable type patterns are `case String s, null`/`case null, String >> s`/`case null: case String s:`/`case String s: case null:`, handling of >> these required a few tricks in `Attr`, `Flow` and `TransPatterns`. >> >> The specdiff for the change is here (to be updated): >> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html > > Good work. There's a lot to take in here. I think overall, it holds up well - > I like how case labels have been extended to accommodate for patterns. In the > front-end, I think there are some questions over the split between Attr and > Flow - maybe it is unavoidable, but I'm not sure why some control-flow > analysis happens in Attr instead of Flow and I left some questions to make > sure I understand what's happening. > > In the backend it's mostly good work - but overall the structure of the code, > both in Lower and in TransPattern is getting very difficult to follow, given > there are many different kind of switches each with its own little twist > attached to it. It would be nice, I think (maybe in a separate cleanup?) if > the code paths serving the different switch kinds could be made more > separate, so that, when reading the code we can worry only about one possible > code shape. That would make the code easier to document as well. @mcimadamore, thanks a lot for the comments! I tried to reflect most of them in https://github.com/openjdk/jdk/pull/3863/commits/1a5a424139a52d0f93e16980c6b42cf29dd908ef - please let me know how that looks. Thanks! > src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 1790: > >> 1788: if (isTotal) { >> 1789: if (hasTotalPattern) { >> 1790: log.error(pat.pos(), >> Errors.DuplicateTotalPattern); > > Hard to explain - but a lot of the code here feels more like it belongs to > `Flow` than to `Attr`. E.g. these "duplicateXYZ" labels really want to say > "unreachable code", I think. Is there a strong reason as to why all this code > shouldn't (apart from code computing bindings) move to Flow? Seems that the > logic is partially duplicated anyway... `Attr` was always checking duplicated constants and duplicated default, so checking for pattern dominance (which could be seen as an extension to duplicate constant labels) and total patterns (which is an extension to duplicated default) here seemed reasonable. We also have a couple of other similar checks performed by `Attr`, like duplicated statement labels, or that exception types in multicatch are disjoint. These are checks that don't need DA/DU, while I think `Flow` does mostly checks that require DA/DU. > src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java line 3649: > >> 3647: if (!enumSwitch && !stringSwitch && >> !selector.type.isPrimitive() && >> 3648: cases.stream().anyMatch(c -> >> TreeInfo.isNull(c.labels.head))) { >> 3649: //a switch over a boxed primitive, with a null case. Pick >> two constants that are > > Is this comment correct? If we're here, can't this be just any pattern switch? Patterns switches are resolved before Lower, so in Lower there should be no pattern switches. I'll add a comment in this sense at an appropriate place. > src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties > line 501: > >> 499: >> 500: compiler.err.pattern.dominated=\ >> 501: this pattern is dominated by a preceding pattern > > Not sure whether the concept of "dominance" is the one that will work best > here. As I said elsewhere, this is, morally, unreachable code. The specification uses "dominance", so it seemed appropriate to use the same term that is used by the specification. We can say "unreachable code", of course, but it will not be consistent with what the specification says, and also with what we do for duplicate constant labels. Also considering code like: switch (o) { case A a -> {} case B b -> {} //error on this line } It may not be obvious why the code is "unreachable", while saying "dominated" might be more helpful/clear. ------------- PR: https://git.openjdk.java.net/jdk/pull/3863
