RFR: 8302344: Compiler Implementation for Unnamed patterns and variables (Preview) [v4]

[Jan Lahoda]

On Mon, 1 May 2023 22:29:21 GMT, Aggelos Biboudis <abimpoudis at openjdk.org> wrote:

>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java line 1777:
>> 
>>> 1775:                                 log.error(guard.pos(), Errors.GuardHasConstantExpressionFalse);
>>> 1776:                             }
>>> 1777:                         } else if (guard != null) {
>> 
>> I guess this is caused by something like:
>> 
>> case _ when ...
>> 
>> If now a guard can appear regardless of how many labels there are, should be refactor this code so that if `guard != null` it is always attributed? And then we do other stuff in case `labels.tail.isEmpty()` ? Note that the code for attributing the guard is slightly different in the two cases in this patch: the original code creates a nested scope - while the new code doesn't.
>
> Actually this is for the case where we have `case A(_), B(C _) when ... `. In that case we are sure that the only scope we can use is the switch environment (since no bindings are introduced). Hm, I don't see another way of refactoring this without depending on `labels.tail.isEmpty()`

I was looking into this again, and I am afraid this is not quite correct. One of the issues is that this attrib will place any binding variables from the guard into the current `matchBindings`, and when the guard is attributed again, the variables in the `matchBindings` will collide. For example:

    String unnamedGuardAddsBindings(Object o1, Object o2) {
        return switch (o1) {
            case String _, Object _ when o2 instanceof String s: yield s;
            case Object _: yield "any";
        };
    }


My current proposal would be to (try to):
 - attribute the guard when handling the first pattern (so that we can detect whether or not it is constant)
 - but, do not merge the bindings from the guard immediately, but only after all the patterns are handled

It could look like this:

diff --git a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
index e9c3ca27f1c..bade6b530eb 100644
--- a/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
+++ b/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
@@ -1688,7 +1688,7 @@ public class Attr extends JCTree.Visitor {
                     wasError = true;
                 }
                 MatchBindings currentBindings = null;
-                boolean wasUnconditionalPattern = hasUnconditionalPattern;
+                MatchBindings guardBindings = null;
                 for (List<JCCaseLabel> labels = c.labels; labels.nonEmpty(); labels = labels.tail) {
                     JCCaseLabel label = labels.head;
                     if (label instanceof JCConstantCaseLabel constLabel) {
@@ -1761,7 +1761,7 @@ public class Attr extends JCTree.Visitor {
                         checkCastablePattern(pat.pos(), seltype, primaryType);
                         Type patternType = types.erasure(primaryType);
                         JCExpression guard = c.guard;
-                        if (labels.tail.isEmpty() && guard != null) {
+                        if (labels == c.labels && guard != null) {
                             MatchBindings afterPattern = matchBindings;
                             Env<AttrContext> bodyEnv = bindingEnv(switchEnv, matchBindings.bindingsWhenTrue);
                             try {
@@ -1769,14 +1769,13 @@ public class Attr extends JCTree.Visitor {
                             } finally {
                                 bodyEnv.info.scope.leave();
                             }
-                            matchBindings = matchBindingsComputer.caseGuard(c, afterPattern, matchBindings);
+
+                            guardBindings = matchBindings;
+                            matchBindings = afterPattern;
 
                             if (TreeInfo.isBooleanWithValue(guard, 0)) {
                                 log.error(guard.pos(), Errors.GuardHasConstantExpressionFalse);
                             }
-                        } else if (guard != null) {
-                            // if the label has multiple patterns and a guard (with binding from the switch environment)
-                            attribExpr(guard, switchEnv, syms.booleanType);
                         }
                         boolean unguarded = TreeInfo.unguardedCase(c) && !pat.hasTag(RECORDPATTERN);
                         boolean unconditional =
@@ -1799,6 +1798,10 @@ public class Attr extends JCTree.Visitor {
                     currentBindings = matchBindingsComputer.switchCase(label, currentBindings, matchBindings);
                 }
 
+                if (guardBindings != null) {
+                    currentBindings = matchBindingsComputer.caseGuard(c, currentBindings, guardBindings);
+                }
+
                 Env<AttrContext> caseEnv =
                         bindingEnv(switchEnv, c, currentBindings.bindingsWhenTrue);
                 try {


(I didn't run tests on this, though.)

>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 746:
>> 
>>> 744: 
>>> 745:         sealed interface PatternDescription {
>>> 746:             public static PatternDescription from(Types types, Type selectorType, JCPattern pattern, Symtab syms) {
>> 
>> Optional comment: the fact that this factory needs so much compiler state makes me think that perhaps a better place for it would be as an instance method of Flow, rather than a static method in PatternDescription.
>
> @lahodaj WDYT? Looks like it can be easily refactored and it is used only in one place (apart from the recursive call). I can refactor it in this PR or u can do it in #13074.

I somewhat liked the "factory method" aspect to this, but I admit the parameter list is getting longer. So, up to you, I think.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13528#discussion_r1185881273
PR Review Comment: https://git.openjdk.org/jdk/pull/13528#discussion_r1185817955