RFR: 8287236: Reorganize AST related to pattern matching for switch [v2]

[Maurizio Cimadamore]

On Tue, 31 May 2022 17:08:32 GMT, Jan Lahoda <jlahoda at openjdk.org> wrote:

>> Under JEP 427, case label elements may be: a) expressions; b) patterns; c) the default. Currently, this is modeled in the Trees API in a way where all expressions and patterns extend `CaseLabelTree`.
>> 
>> When guarded patterns were removed in favor of guards on pattern case label element, it was necessary to augment all patterns with a guard, which is only used when the pattern is used as a case label element. This is somewhat inconsistent for the Trees API.
>> 
>> A use a layer of indirection is proposed here: - the `CaseLabelTree` has three subtypes: `ConstantCaseLabelTree`, which contains `ExpressionTree` as a subnode; `PatternCaseLabelTree`, which contains `PatternTree` and a guard (`ExpressionTree`) as subnodes; and the `DefaultCaseLabelTree` for the default clause.
>> 
>> This patch also fixes `TreeScanner.visitCase`, which currently iterates only over `CaseTree.getExpressions()`, ignoring patterns. It is changed to iterate over `CaseTree.getLabels()`, to get all labels.
>
> Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fixing test.

Overall looks an improvement modelling-wise. Unfortunately the code seem to have gotten a bit more verbose, with all the `hasTag` check which sometimes are followed by casts. I added some suggestions to use pattern match, but if now, perhaps some helper methods in TreeInfo might help.

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 1719:

> 1717:                 boolean wasUnconditionalPattern = hasUnconditionalPattern;
> 1718:                 for (JCCaseLabel label : c.labels) {
> 1719:                     if (label.hasTag(CONSTANTCASELABEL)) {

Note here how we could use sealed types do do tests and cast with instanceof patterns. I know we have not used sealed types elsewhere, but in this particular case it seems particularly worthy, since you have a relatively small number of options, all of which need to be looked at. But even w/o sealing, pattern + instanceof could be useful. Not necessarily something to act on now - but I think methods like `hasTag` are starting to get a bit stale :-)

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 741:

> 739:         private Set<Symbol> coveredSymbolsForCases(DiagnosticPosition pos,
> 740:                                                    JCExpression selector, List<JCCase> cases) {
> 741:             HashSet<JCTree> labels = cases.stream()

Is the name `labels` correct to identify the set of things returned by the stream? Aren't these either patterns or constant expressions? We probably miss a term for that entity. But it's confusing in the current code to just call them lables, because you can see in the stream expression that you go deeper inside the labels.

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 756:

> 754:             Map<Symbol, List<JCRecordPattern>> deconstructionPatternsBySymbol = new HashMap<>();
> 755: 
> 756:             for (JCTree label : labels) {

Similar naming problem with `label` here.

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 2587:

> 2585:                     l.tail.head.labels.size() == 1 &&
> 2586:                     l.tail.head.labels.head.hasTag(CONSTANTCASELABEL) &&
> 2587:                     TreeInfo.isNull(((JCConstantCaseLabel) l.tail.head.labels.head).expr)) {

Patterns can help quite a bit here too


l.tail.head.labels.head instanceof JCConstantCaseLabel constantLabel &&
TreeInfo.isNull(constantLabel.expr)

src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Gen.java line 1302:

> 1300:             List<JCCase> l = cases;
> 1301:             for (int i = 0; i < labels.length; i++) {
> 1302:                 if (l.head.labels.head.hasTag(CONSTANTCASELABEL)) {

Another good one for patterns

src/jdk.compiler/share/classes/com/sun/tools/javac/jvm/Gen.java line 1304:

> 1302:                 if (l.head.labels.head.hasTag(CONSTANTCASELABEL)) {
> 1303:                     Assert.check(l.head.labels.size() == 1);
> 1304:                     int val = ((Number)((JCConstantCaseLabel) l.head.labels.head).expr.type.constValue()).intValue();

And here

-------------

PR: https://git.openjdk.java.net/jdk/pull/8959