[jdk17] RFR: 8269146: Missing unreported constraints on pattern and other case label combination [v2]

[Maurizio Cimadamore]

On Thu, 1 Jul 2021 13:26:25 GMT, Jan Lahoda <jlahoda at openjdk.org> wrote:

>> There is a number of constraints on the case label forms - patterns cannot be combined with patterns, etc. Currently, the checks are implemented by checking if there are clashing bindings. But this misses some of the cases which should lead to errors. This patch proposes to implement explicit checks on the structure of the case labels in `Attr.handleSwitch`, rather than relying on clashing bindings.
>
> Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fixing fallthrough.

I wonder if all the `wasDefault` etc tests, to check compatibility between different kind of labels couldn't be moved when at parse time. The code in attr is already quite convoluted, and it seems to be doing many things at once - if there are checks which depend on flow analysis, of course these should remain in Attr - but there seems to be a lot of structural checks which do not really depend on attribution which could be caught a lot earlier, and probably giving better error messages too.

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

> 1682:             boolean hasTotalPattern = false;      // Is there a total pattern?
> 1683:             boolean hasNullPattern = false;       // Is there a null pattern?
> 1684:             boolean wasConstant = false;          // Seen a constant in the same case label

I'd suggest to move the `wasXYZ` variables inside the case label loop - since they are not meant to be variables that are global to the entire switch construct?

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

> 1785:                         if (wasPattern || wasConstant || wasDefault ||
> 1786:                             (wasNullPattern && (!isTypePattern || wasNonEmptyFallThrough))) {
> 1787:                             log.error(pat.pos(), Errors.FlowsThroughToPattern);

Is this a good error message for the condition? E.g. it seems like here we're mostly complaining about some illegal mix of constant, default and pattern labels - which seems more something to do with well-formedness than with fallthrough?

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

> 1827:                 boolean completesNormally = c.caseKind == CaseTree.CaseKind.STATEMENT ? flow.aliveAfter(caseEnv, c, make) : false;
> 1828: 
> 1829:                 if (c.stats.nonEmpty()) {

This initialization should probably go at the beginning of the for loop block?

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

PR: https://git.openjdk.java.net/jdk17/pull/194