From 0644776d6ea405719859bec5c35b4123254bb93c Mon Sep 17 00:00:00 2001 From: Daniel Sun Date: Sat, 8 Aug 2026 14:08:03 +0900 Subject: [PATCH 1/6] GROOVY-12242: instanceof pattern variable scope is not aligned with Java flow scoping (JEP 394) --- .../ast/expr/DeclarationExpression.java | 11 + .../classgen/InstanceofFlowBindings.java | 249 ++++++++++++++++ .../groovy/classgen/VariableScopeVisitor.java | 111 ++++++- .../classgen/asm/BinaryExpressionHelper.java | 132 ++++++--- .../groovy/classgen/asm/CompileStack.java | 25 ++ .../asm/InstanceofFlowSlotPublisher.java | 155 ++++++++++ .../groovy/classgen/asm/StatementWriter.java | 56 +++- .../groovy/InstanceofFlowBindingsTest.groovy | 206 +++++++++++++ src/test/groovy/groovy/InstanceofTest.groovy | 275 ++++++++++++++++++ 9 files changed, 1159 insertions(+), 61 deletions(-) create mode 100644 src/main/java/org/codehaus/groovy/classgen/InstanceofFlowBindings.java create mode 100644 src/main/java/org/codehaus/groovy/classgen/asm/InstanceofFlowSlotPublisher.java create mode 100644 src/test/groovy/groovy/InstanceofFlowBindingsTest.groovy diff --git a/src/main/java/org/codehaus/groovy/ast/expr/DeclarationExpression.java b/src/main/java/org/codehaus/groovy/ast/expr/DeclarationExpression.java index e4970bca8d6..a7e6ec2abae 100644 --- a/src/main/java/org/codehaus/groovy/ast/expr/DeclarationExpression.java +++ b/src/main/java/org/codehaus/groovy/ast/expr/DeclarationExpression.java @@ -43,6 +43,12 @@ * you can use the method "TupleExpression getTupleExpression()" method. * Calling either of these expression getters when the "isMultipleAssignment" condition * is not appropriate is unsafe and will result in a ClassCastException. + *

+ * JEP 394 type patterns reuse this node as the right-hand side of + * {@code instanceof}: {@code e instanceof String s} is modelled as an + * {@code instanceof} {@link BinaryExpression} whose RHS is a + * {@code DeclarationExpression} with an {@link EmptyExpression} initializer. + * The pattern variable's scope is flow-sensitive (see GROOVY-12242). */ public class DeclarationExpression extends BinaryExpression { @@ -130,6 +136,11 @@ public TupleExpression getTupleExpression() { : null; } + /** + * Returns the type of the declared variable (or of the tuple for a multiple + * assignment). For a JEP 394 pattern used as the RHS of {@code instanceof}, + * this is the pattern type {@code T} in {@code e instanceof T t}. + */ @Override public ClassNode getType() { return (isMultipleAssignmentDeclaration() ? getTupleExpression() : getVariableExpression()).getType(); diff --git a/src/main/java/org/codehaus/groovy/classgen/InstanceofFlowBindings.java b/src/main/java/org/codehaus/groovy/classgen/InstanceofFlowBindings.java new file mode 100644 index 00000000000..84e1d27a15d --- /dev/null +++ b/src/main/java/org/codehaus/groovy/classgen/InstanceofFlowBindings.java @@ -0,0 +1,249 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.codehaus.groovy.classgen; + +import org.codehaus.groovy.ast.CodeVisitorSupport; +import org.codehaus.groovy.ast.expr.BinaryExpression; +import org.codehaus.groovy.ast.expr.BooleanExpression; +import org.codehaus.groovy.ast.expr.DeclarationExpression; +import org.codehaus.groovy.ast.expr.Expression; +import org.codehaus.groovy.ast.expr.NotExpression; +import org.codehaus.groovy.ast.expr.VariableExpression; +import org.codehaus.groovy.syntax.Types; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Set; + +/** + * Flow-sensitive analysis of JEP 394 {@code instanceof} pattern bindings + * (GROOVY-12242). + *

+ * This is pure semantic analysis: given a boolean expression, which + * pattern variables are definitely bound when the expression is + * {@code true} versus {@code false}? (Same idea as compiler “flow info” / + * JEP 394 flow scoping — not a bytecode construct.) + *

+ * Covered shapes: {@code e instanceof T t}, negation / {@code !instanceof}, + * {@code &&} (union of true bindings), {@code ||} (union of false bindings). + * Other shapes contribute nothing (conservative). + *

+ * Consumers: + *

+ * + * @see org.codehaus.groovy.classgen.asm.InstanceofFlowSlotPublisher + * @since 6.0.0 + */ +public final class InstanceofFlowBindings { + + private static final InstanceofFlowBindings EMPTY = + new InstanceofFlowBindings(List.of(), List.of()); + + private final List whenTrue; + private final List whenFalse; + + private InstanceofFlowBindings(final List whenTrue, + final List whenFalse) { + this.whenTrue = whenTrue; + this.whenFalse = whenFalse; + } + + /** + * Pattern variables that are definitely assigned when the analysed expression + * evaluates to {@code true}. + */ + public List whenTrue() { + return whenTrue; + } + + /** + * Pattern variables that are definitely assigned when the analysed expression + * evaluates to {@code false}. + */ + public List whenFalse() { + return whenFalse; + } + + /** Whether any pattern variable is bound on either path. */ + public boolean isEmpty() { + return whenTrue.isEmpty() && whenFalse.isEmpty(); + } + + /** + * Names of pattern variables bound when the expression is {@code true}. + */ + public Set whenTrueNames() { + return names(whenTrue); + } + + /** + * Names of pattern variables bound when the expression is {@code false}. + */ + public Set whenFalseNames() { + return names(whenFalse); + } + + /** + * All pattern-variable names appearing in either path (stable encounter order). + */ + public Set allNames() { + if (isEmpty()) return Collections.emptySet(); + Set names = new LinkedHashSet<>(whenTrue.size() + whenFalse.size()); + for (VariableExpression ve : whenTrue) names.add(ve.getName()); + for (VariableExpression ve : whenFalse) names.add(ve.getName()); + return names; + } + + private static Set names(final List vars) { + if (vars.isEmpty()) return Collections.emptySet(); + Set result = new LinkedHashSet<>(vars.size()); + for (VariableExpression ve : vars) { + result.add(ve.getName()); + } + return result; + } + + /** + * Analyses {@code expression} for definite {@code instanceof} pattern bindings. + * + * @param expression a boolean condition (may be a {@link BooleanExpression} wrapper) + * @return the true/false binding sets; never {@code null} + */ + public static InstanceofFlowBindings of(final Expression expression) { + if (expression == null) { + return EMPTY; + } + return analyse(expression); + } + + /** + * Returns {@code true} if {@code expression} contains any JEP 394 type + * pattern ({@code e instanceof T t} or {@code e !instanceof T t}), including + * nested subexpressions. Used to decide whether expression-statement + * CompileStack isolation is required. + * + * @param expression any expression; {@code null} yields {@code false} + */ + public static boolean containsPattern(final Expression expression) { + if (expression == null) return false; + boolean[] found = {false}; + expression.visit(new CodeVisitorSupport() { + @Override + public void visitBinaryExpression(final BinaryExpression be) { + if (found[0]) return; + int op = be.getOperation().getType(); + if ((op == Types.KEYWORD_INSTANCEOF || op == Types.COMPARE_NOT_INSTANCEOF) + && isTypePattern(be.getRightExpression())) { + found[0] = true; + return; + } + super.visitBinaryExpression(be); + } + }); + return found[0]; + } + + private static boolean isTypePattern(final Expression right) { + return right instanceof DeclarationExpression decl + && !decl.isMultipleAssignmentDeclaration() + && decl.getVariableExpression() != null; + } + + private static InstanceofFlowBindings analyse(final Expression expression) { + Expression expr = expression; + + // Unwrap BooleanExpression wrappers; NotExpression is handled below so that + // nested negations compose correctly. + while (expr instanceof BooleanExpression && !(expr instanceof NotExpression)) { + expr = ((BooleanExpression) expr).getExpression(); + } + + if (expr instanceof NotExpression not) { + return analyse(not.getExpression()).negated(); + } + + if (expr instanceof BinaryExpression binary) { + int op = binary.getOperation().getType(); + if (op == Types.KEYWORD_INSTANCEOF) { + return ofInstanceof(binary); + } + if (op == Types.COMPARE_NOT_INSTANCEOF) { + // AST may still carry !instanceof before codegen rewrites it to !(… instanceof …). + return ofInstanceof(binary).negated(); + } + if (op == Types.LOGICAL_AND) { + InstanceofFlowBindings left = analyse(binary.getLeftExpression()); + InstanceofFlowBindings right = analyse(binary.getRightExpression()); + // True path evaluates both; false path is not definite for either side alone. + return new InstanceofFlowBindings( + union(left.whenTrue, right.whenTrue), + List.of()); + } + if (op == Types.LOGICAL_OR) { + InstanceofFlowBindings left = analyse(binary.getLeftExpression()); + InstanceofFlowBindings right = analyse(binary.getRightExpression()); + // False path evaluates both; true path is not definite for either side alone. + return new InstanceofFlowBindings( + List.of(), + union(left.whenFalse, right.whenFalse)); + } + } + + return EMPTY; + } + + private static InstanceofFlowBindings ofInstanceof(final BinaryExpression binary) { + Expression right = binary.getRightExpression(); + if (isTypePattern(right)) { + VariableExpression patternVar = ((DeclarationExpression) right).getVariableExpression(); + return new InstanceofFlowBindings(List.of(patternVar), List.of()); + } + return EMPTY; + } + + private InstanceofFlowBindings negated() { + if (isEmpty()) return this; + return new InstanceofFlowBindings(whenFalse, whenTrue); + } + + private static List union(final List a, + final List b) { + if (a.isEmpty()) return b; + if (b.isEmpty()) return a; + List result = new ArrayList<>(a.size() + b.size()); + Set seen = new LinkedHashSet<>(); + for (VariableExpression ve : a) { + if (seen.add(ve.getName())) result.add(ve); + } + for (VariableExpression ve : b) { + if (seen.add(ve.getName())) result.add(ve); + } + return List.copyOf(result); + } +} diff --git a/src/main/java/org/codehaus/groovy/classgen/VariableScopeVisitor.java b/src/main/java/org/codehaus/groovy/classgen/VariableScopeVisitor.java index b9c7126c371..c2acdcadfd1 100644 --- a/src/main/java/org/codehaus/groovy/classgen/VariableScopeVisitor.java +++ b/src/main/java/org/codehaus/groovy/classgen/VariableScopeVisitor.java @@ -45,6 +45,7 @@ import org.codehaus.groovy.ast.expr.FieldExpression; import org.codehaus.groovy.ast.expr.MethodCallExpression; import org.codehaus.groovy.ast.expr.PropertyExpression; +import org.codehaus.groovy.ast.expr.TernaryExpression; import org.codehaus.groovy.ast.expr.TupleExpression; import org.codehaus.groovy.ast.expr.VariableExpression; import org.codehaus.groovy.ast.stmt.AssertStatement; @@ -66,6 +67,7 @@ import java.util.HashSet; import java.util.Iterator; import java.util.LinkedList; +import java.util.List; import java.util.Optional; import java.util.Set; import java.util.function.BiConsumer; @@ -76,10 +78,17 @@ import static org.apache.groovy.ast.tools.MethodNodeUtils.getPropertyName; import static org.apache.groovy.ast.tools.MethodNodeUtils.withDefaultArgumentMethods; import static org.codehaus.groovy.ast.tools.GeneralUtils.getAllProperties; +import static org.codehaus.groovy.ast.tools.GeneralUtils.maybeFallsThrough; import static org.codehaus.groovy.transform.trait.Traits.isTrait; /** * Initializes the variable scopes for an AST. + *

+ * For JEP 394 {@code instanceof} pattern variables (GROOVY-12242), name + * resolution follows {@link InstanceofFlowBindings} (flow analysis): a pattern + * variable is only declared where the pattern has definitely matched. Bytecode + * slot visibility applies the same analysis via + * {@link org.codehaus.groovy.classgen.asm.InstanceofFlowSlotPublisher}. */ public class VariableScopeVisitor extends ClassCodeVisitorSupport { @@ -623,20 +632,45 @@ public void visitForLoop(final ForStatement statement) { } /** - * {@inheritDoc} + * Visits an {@code if}/{@code else} with Java-aligned flow scoping for + * {@code instanceof} pattern variables (GROOVY-12242 / JEP 394). + *

+ * Pattern variables that bind when the condition is {@code true} are in + * scope in the then-block; those that bind when it is {@code false} are in + * scope in the else-block. When a branch cannot complete normally, bindings + * from the opposite path remain in scope for subsequent statements (e.g. + * {@code if (!(o instanceof String s)) return; s.length()}). */ @Override public void visitIfElse(final IfStatement statement) { + InstanceofFlowBindings bindings = InstanceofFlowBindings.of(statement.getBooleanExpression()); + + // Condition: pattern vars are available for short-circuit RHS (e.g. &&). pushState(); visitStatement(statement); statement.getBooleanExpression().visit(this); + popState(); + + // Then-block: only true-path bindings. pushState(); + declarePatternVariables(bindings.whenTrue()); statement.getIfBlock().visit(this); popState(); - popState(); + + // Else-block: only false-path bindings. pushState(); + declarePatternVariables(bindings.whenFalse()); statement.getElseBlock().visit(this); popState(); + + // After the if: Java keeps the opposite path's bindings when a branch + // cannot complete normally (early return / throw). + if (!maybeFallsThrough(statement.getIfBlock())) { + declarePatternVariables(bindings.whenFalse()); + } + if (!statement.getElseBlock().isEmpty() && !maybeFallsThrough(statement.getElseBlock())) { + declarePatternVariables(bindings.whenTrue()); + } } /** @@ -660,7 +694,9 @@ public void visitSwitch(final SwitchStatement statement) { } /** - * {@inheritDoc} + * Visits a {@code while} loop. Pattern variables introduced by the condition + * are in scope for the condition's short-circuit RHS and for the loop body + * (GROOVY-12242). They do not leak past the loop. */ @Override public void visitWhileLoop(final WhileStatement statement) { @@ -669,6 +705,20 @@ public void visitWhileLoop(final WhileStatement statement) { popState(); } + /** + * Declares {@code instanceof} pattern variables into the current scope so + * that subsequent visits resolve them as locals. Skips names already present + * in this scope (re-declaring the same pattern variable object after a + * condition visit is a no-op). + */ + private void declarePatternVariables(final List patternVariables) { + for (VariableExpression variable : patternVariables) { + if (currentScope.getDeclaredVariable(variable.getName()) == null) { + declare(variable); + } + } + } + // expressions: /** @@ -681,17 +731,66 @@ public void visitArrayExpression(final ArrayExpression expression) { } /** - * {@inheritDoc} + * Visits binary expressions with flow-aware scoping for {@code &&} / {@code ||} + * so pattern variables follow Java short-circuit rules (GROOVY-12242 / JEP 394): + *

    + *
  • {@code a && b} — true-path bindings of {@code a} are in scope in {@code b}
    • + *
  • {@code a || b} — true-path bindings of {@code a} are not in scope in {@code b}; + * false-path bindings of {@code a} are
    • + *
*/ @Override public void visitBinaryExpression(final BinaryExpression expression) { - super.visitBinaryExpression(expression); + int op = expression.getOperation().getType(); + if (op == Types.LOGICAL_AND) { + // Left first; its true-path pattern vars stay in the current scope for the right. + expression.getLeftExpression().visit(this); + expression.getRightExpression().visit(this); + } else if (op == Types.LOGICAL_OR) { + // Left's true-path bindings must not leak into the right (Java rejects + // `o instanceof String s || s.isEmpty()`). False-path bindings of the + // left are in scope on the right (`!(o instanceof String s) || s.isEmpty()`). + InstanceofFlowBindings leftBindings = InstanceofFlowBindings.of(expression.getLeftExpression()); + pushState(); + expression.getLeftExpression().visit(this); + popState(); + pushState(); + declarePatternVariables(leftBindings.whenFalse()); + expression.getRightExpression().visit(this); + popState(); + } else { + super.visitBinaryExpression(expression); + } - if (Types.isAssignment(expression.getOperation().getType())) { + if (Types.isAssignment(op)) { checkFinalFieldAccess(expression.getLeftExpression()); } } + /** + * Visits a ternary / Elvis expression with flow scoping for pattern variables: + * true-path bindings are in scope in the then-branch; false-path bindings in + * the else-branch (GROOVY-12242 / JEP 394). + */ + @Override + public void visitTernaryExpression(final TernaryExpression expression) { + InstanceofFlowBindings bindings = InstanceofFlowBindings.of(expression.getBooleanExpression()); + + pushState(); + expression.getBooleanExpression().visit(this); + popState(); + + pushState(); + declarePatternVariables(bindings.whenTrue()); + expression.getTrueExpression().visit(this); + popState(); + + pushState(); + declarePatternVariables(bindings.whenFalse()); + expression.getFalseExpression().visit(this); + popState(); + } + /** * {@inheritDoc} */ diff --git a/src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java b/src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java index 01c3e3dac3e..951168493e0 100644 --- a/src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java +++ b/src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java @@ -47,6 +47,7 @@ import org.codehaus.groovy.ast.tools.WideningCategories; import org.codehaus.groovy.classgen.AsmClassGenerator; import org.codehaus.groovy.classgen.BytecodeExpression; +import org.codehaus.groovy.classgen.InstanceofFlowBindings; import org.codehaus.groovy.runtime.MultipleAssignmentSupport; import org.codehaus.groovy.runtime.ScriptBytecodeAdapter; import org.codehaus.groovy.syntax.Token; @@ -565,47 +566,44 @@ public void evaluateEqual(final BinaryExpression expression, final boolean defin } // evaluate RHS and store its value - if (lhsType.isArray() && rightExpression instanceof ListExpression) { // array = [ ... ] - Expression array = new ArrayExpression(lhsType.getComponentType(), ((ListExpression) rightExpression).getExpressions()); - array.setSourcePosition(rightExpression); - array.setType(lhsType); - array.visit(acg); - } else if (rightExpression instanceof EmptyExpression) { // define field - CompileStack.pushInitValue(lhsType, mv); - operandStack.push(lhsType); - } else { - rightExpression.visit(acg); - } - - ClassNode rhsType = operandStack.getTopOperand(); - + // GROOVY-12242: for a single-variable declaration, evaluate the RHS in a nested + // CompileStack state so instanceof pattern variables do not leak past the + // declaration while the LHS local is defined in the outer state. if (directAssignment) { - VariableExpression var = (VariableExpression) leftExpression; - if (var.isClosureSharedVariable() && ClassHelper.isPrimitiveType(rhsType)) { - // GROOVY-5570: if a closure shared variable is a primitive type, it must be boxed - rhsType = ClassHelper.getWrapper(rhsType); - operandStack.box(); - } + compileStack.pushState(); + try { + evaluateRightHandSide(lhsType, rightExpression, acg, mv, operandStack); + ClassNode rhsType = operandStack.getTopOperand(); + VariableExpression var = (VariableExpression) leftExpression; + if (var.isClosureSharedVariable() && ClassHelper.isPrimitiveType(rhsType)) { + // GROOVY-5570: if a closure shared variable is a primitive type, it must be boxed + rhsType = ClassHelper.getWrapper(rhsType); + operandStack.box(); + } - // ensure we try to unbox null to cause a runtime NPE in case we assign - // null to a primitive typed variable, even if it is used only in boxed - // form as it is closure shared - if (var.isClosureSharedVariable() && ClassHelper.isPrimitiveType(var.getOriginType()) && isNullConstant(rightExpression)) { - operandStack.doGroovyCast(var.getOriginType()); - // these two are never reached in bytecode and only there - // to avoid verify errors and compiler infrastructure hazzle - operandStack.box(); - operandStack.doGroovyCast(lhsType); - } - // normal type transformation - if (!ClassHelper.isPrimitiveType(lhsType) && isNullConstant(rightExpression)) { - operandStack.replace(lhsType); - } else { - operandStack.doGroovyCast(lhsType); + // ensure we try to unbox null to cause a runtime NPE in case we assign + // null to a primitive typed variable, even if it is used only in boxed + // form as it is closure shared + if (var.isClosureSharedVariable() && ClassHelper.isPrimitiveType(var.getOriginType()) && isNullConstant(rightExpression)) { + operandStack.doGroovyCast(var.getOriginType()); + // these two are never reached in bytecode and only there + // to avoid verify errors and compiler infrastructure hazzle + operandStack.box(); + operandStack.doGroovyCast(lhsType); + } + // normal type transformation + if (!ClassHelper.isPrimitiveType(lhsType) && isNullConstant(rightExpression)) { + operandStack.replace(lhsType); + } else { + operandStack.doGroovyCast(lhsType); + } + } finally { + // Drop RHS pattern locals before defining the LHS in the outer state + compileStack.pop(); } // store value - BytecodeVariable v = compileStack.defineVariable(var, lhsType, true); + BytecodeVariable v = compileStack.defineVariable((Variable) leftExpression, lhsType, true); operandStack.remove(1); if (returnRightValue) { new VariableSlotLoader(lhsType, v.getIndex(), operandStack).visit(acg); @@ -613,6 +611,9 @@ public void evaluateEqual(final BinaryExpression expression, final boolean defin return; } + evaluateRightHandSide(lhsType, rightExpression, acg, mv, operandStack); + ClassNode rhsType = operandStack.getTopOperand(); + // GROOVY-10918: direct store to local variable or parameter (no temp) if (!defineVariable && leftExpression instanceof VariableExpression) { BytecodeVariable v = compileStack.getVariable(leftExpression.getText(), false); @@ -1110,6 +1111,37 @@ protected void evaluateCompoundAssign(final String assignName, final String base controller.getCompileStack().popLHS(); } + /** + * Evaluates the right-hand side of an assignment onto the operand stack. + */ + private void evaluateRightHandSide(final ClassNode lhsType, final Expression rightExpression, + final AsmClassGenerator acg, final MethodVisitor mv, + final OperandStack operandStack) { + if (lhsType.isArray() && rightExpression instanceof ListExpression) { // array = [ ... ] + Expression array = new ArrayExpression(lhsType.getComponentType(), ((ListExpression) rightExpression).getExpressions()); + array.setSourcePosition(rightExpression); + array.setType(lhsType); + array.visit(acg); + } else if (rightExpression instanceof EmptyExpression) { // define field + CompileStack.pushInitValue(lhsType, mv); + operandStack.push(lhsType); + } else { + rightExpression.visit(acg); + } + } + + /** + * Emits bytecode for {@code e instanceof T} and, when the right-hand side is + * a JEP 394 type pattern ({@code e instanceof T t}), conditionally stores + * the checked value into the pattern variable {@code t}. + *

+ * The pattern variable's visibility is governed by flow scoping in + * {@link org.codehaus.groovy.classgen.VariableScopeVisitor} and + * {@link StatementWriter#writeIfElse}; this method only performs the store. + * + * @param expression an {@code instanceof} binary expression + * @see org.codehaus.groovy.classgen.InstanceofFlowBindings + */ private void evaluateInstanceof(final BinaryExpression expression) { CompileStack compileStack = controller.getCompileStack(); OperandStack operandStack = controller.getOperandStack(); @@ -1117,25 +1149,24 @@ private void evaluateInstanceof(final BinaryExpression expression) { expression.getLeftExpression().visit(controller.getAcg()); operandStack.box(); // TODO: support instanceof primitives - //ClassNode sourceType = operandStack.getTopOperand(); ClassNode targetType = expression.getRightExpression().getType(); - - var jep394 = !(expression.getRightExpression() instanceof ClassExpression); - if (jep394) { - operandStack.dup(); // stash value for use by JEP 394 pattern variable + // JEP 394: RHS is DeclarationExpression (Type name) rather than ClassExpression + boolean patternMatch = !(expression.getRightExpression() instanceof ClassExpression); + if (patternMatch) { + operandStack.dup(); // stash value for the pattern variable store } String typeName = BytecodeHelper.getClassInternalName(targetType); controller.getMethodVisitor().visitTypeInsn(INSTANCEOF, typeName); operandStack.replace(ClassHelper.boolean_TYPE); - if (jep394) { + if (patternMatch) { var variable = (Variable) ((BinaryExpression) expression.getRightExpression()).getLeftExpression(); BytecodeVariable v = compileStack.defineVariable(variable, targetType, false); MethodVisitor mv = controller.getMethodVisitor(); mv.visitInsn(DUP_X1); // stack: ..., check, value, check - Label l0 = operandStack.jump(IFEQ); // skip store if not instanceof + Label notInstance = operandStack.jump(IFEQ); // skip store if not instanceof mv.visitTypeInsn(CHECKCAST, typeName); if (!v.isHolder()) { @@ -1146,12 +1177,12 @@ private void evaluateInstanceof(final BinaryExpression expression) { mv.visitInsn(SWAP); mv.visitMethodInsn(INVOKEVIRTUAL, "groovy/lang/Reference", "set", "(Ljava/lang/Object;)V", false); } - Label l1 = operandStack.jump(GOTO); + Label done = operandStack.jump(GOTO); - mv.visitLabel(l0); // stack: ..., check, value + mv.visitLabel(notInstance); // stack: ..., check, value mv.visitInsn(POP); - mv.visitLabel(l1); // stack: ..., check + mv.visitLabel(done); // stack: ..., check operandStack.push(ClassHelper.boolean_TYPE); } } @@ -1441,23 +1472,30 @@ private void evaluateTernaryExpression(final TernaryExpression expression) { ClassNode commonType = WideningCategories.lowestUpperBound(truePartType, falsePartType); // write "x?y:z" as "x?T(y):T(z)" where T is common type of y and z + CompileStack compileStack = controller.getCompileStack(); OperandStack operandStack = controller.getOperandStack(); MethodVisitor mv = controller.getMethodVisitor(); - // load x + // load x; hide pattern locals then publish only on the live arm (GROOVY-12242) boolPart.visit(controller.getAcg()); + InstanceofFlowSlotPublisher slotPublisher = InstanceofFlowSlotPublisher.captureAndHide( + compileStack, InstanceofFlowBindings.of(expression.getBooleanExpression())); Label l0 = operandStack.jump(IFEQ); // true path: load y and cast to T + slotPublisher.publishTrue(compileStack); truePart.visit(controller.getAcg()); operandStack.doGroovyCast(commonType); + slotPublisher.hideTrue(compileStack); Label l1 = new Label(); mv.visitJumpInsn(GOTO, l1); // false path: load z and cast to T mv.visitLabel(l0); + slotPublisher.publishFalse(compileStack); falsePart.visit(controller.getAcg()); operandStack.doGroovyCast(commonType); + slotPublisher.hideFalse(compileStack); // finish up mv.visitLabel(l1); diff --git a/src/main/java/org/codehaus/groovy/classgen/asm/CompileStack.java b/src/main/java/org/codehaus/groovy/classgen/asm/CompileStack.java index 796617f32f4..5854e07ac06 100644 --- a/src/main/java/org/codehaus/groovy/classgen/asm/CompileStack.java +++ b/src/main/java/org/codehaus/groovy/classgen/asm/CompileStack.java @@ -908,6 +908,31 @@ public boolean containsVariable(final String name) { return stackVariables.containsKey(name); } + /** + * Re-publishes a previously defined local into the current state. + * Used by {@link InstanceofFlowSlotPublisher} for path-scoped pattern variables + * (GROOVY-12242 / JEP 394). + * + * @param variable the bytecode variable to make visible again + */ + public void putVariable(final BytecodeVariable variable) { + if (variable != null) { + stackVariables.put(variable.getName(), variable); + } + } + + /** + * Removes a named local from the current state without affecting temporary + * variables or the free-register cursor. Used by {@link InstanceofFlowSlotPublisher} + * to hide pattern slots that are not live on the current control-flow path. + * + * @param name the variable name to remove + * @return the removed variable, or {@code null} if it was not present + */ + public BytecodeVariable removeVariable(final String name) { + return stackVariables.remove(name); + } + /** * Calculates the index of the next free register stores it * and sets the current variable index to the old value diff --git a/src/main/java/org/codehaus/groovy/classgen/asm/InstanceofFlowSlotPublisher.java b/src/main/java/org/codehaus/groovy/classgen/asm/InstanceofFlowSlotPublisher.java new file mode 100644 index 00000000000..f2f875227f7 --- /dev/null +++ b/src/main/java/org/codehaus/groovy/classgen/asm/InstanceofFlowSlotPublisher.java @@ -0,0 +1,155 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.codehaus.groovy.classgen.asm; + +import org.codehaus.groovy.classgen.InstanceofFlowBindings; + +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.Set; + +/** + * Codegen helper that publishes/hides {@link CompileStack} local slots + * for {@code instanceof} pattern variables according to + * {@link InstanceofFlowBindings} (GROOVY-12242 / JEP 394). + *

+ * Distinct from {@link InstanceofFlowBindings}: that type is AST flow + * analysis; this type is bytecode slot control. + *

+ * {@code evaluateInstanceof} defines pattern slots while the condition runs + * (needed for short-circuit {@code &&} RHS). After the condition, this helper + * hides every captured slot and then publishes only names + * live on the current control-flow path: + *

    + *
  • then-block → {@link InstanceofFlowBindings#whenTrueNames()}
    • + *
  • else-block → {@link InstanceofFlowBindings#whenFalseNames()}
    • + *
  • after the if → opposite path when a branch cannot complete normally
    • + *
+ * That “hide all, publish path” rule keeps CompileStack polarity aligned with + * {@link org.codehaus.groovy.classgen.VariableScopeVisitor}. + * + * @see InstanceofFlowBindings + * @since 6.0.0 + */ +final class InstanceofFlowSlotPublisher { + + private static final InstanceofFlowSlotPublisher NONE = + new InstanceofFlowSlotPublisher(InstanceofFlowBindings.of(null), Map.of()); + + private final InstanceofFlowBindings bindings; + private final Map captured; + + private InstanceofFlowSlotPublisher(final InstanceofFlowBindings bindings, + final Map captured) { + this.bindings = bindings; + this.captured = captured; + } + + /** + * Snapshots pattern slots defined while evaluating {@code bindings}' condition + * and removes them from {@code compileStack} so no branch sees unscoped slots. + * + * @param compileStack current compile stack (condition already evaluated) + * @param bindings flow-analysis result for that condition + * @return a publisher for path-scoped reintroduction of the captured slots + */ + static InstanceofFlowSlotPublisher captureAndHide(final CompileStack compileStack, + final InstanceofFlowBindings bindings) { + if (bindings == null || bindings.isEmpty()) { + return NONE; + } + Map captured = new HashMap<>(); + for (String name : bindings.allNames()) { + BytecodeVariable bv = compileStack.getVariable(name, false); + if (bv != null) { + captured.put(name, bv); + compileStack.removeVariable(name); + } + } + if (captured.isEmpty()) { + return NONE; + } + return new InstanceofFlowSlotPublisher(bindings, Collections.unmodifiableMap(captured)); + } + + boolean isEmpty() { + return captured.isEmpty(); + } + + /** Makes true-path pattern locals visible on the current CompileStack frame. */ + void publishTrue(final CompileStack compileStack) { + publish(compileStack, bindings.whenTrueNames()); + } + + /** Makes false-path pattern locals visible on the current CompileStack frame. */ + void publishFalse(final CompileStack compileStack) { + publish(compileStack, bindings.whenFalseNames()); + } + + /** Hides true-path pattern locals (end of then-block). */ + void hideTrue(final CompileStack compileStack) { + hide(compileStack, bindings.whenTrueNames()); + } + + /** Hides false-path pattern locals (end of else-block). */ + void hideFalse(final CompileStack compileStack) { + hide(compileStack, bindings.whenFalseNames()); + } + + /** + * Publishes bindings that remain in scope after the if, matching Java's + * abrupt-completion rule: opposite-path bindings survive when a branch + * cannot complete normally. + * + * @param ifFallsThrough whether the then-block may complete normally + * @param elseEmpty whether there is no else branch + * @param elseFallsThrough whether the else-block may complete normally + */ + void publishAfterIf(final CompileStack compileStack, + final boolean ifFallsThrough, + final boolean elseEmpty, + final boolean elseFallsThrough) { + if (!ifFallsThrough) { + publishFalse(compileStack); + } + if (!elseEmpty && !elseFallsThrough) { + publishTrue(compileStack); + } + } + + private void publish(final CompileStack compileStack, final Set names) { + if (captured.isEmpty() || names.isEmpty()) return; + for (String name : names) { + BytecodeVariable bv = captured.get(name); + if (bv != null) { + compileStack.putVariable(bv); + } + } + } + + private void hide(final CompileStack compileStack, final Set names) { + if (captured.isEmpty() || names.isEmpty()) return; + for (String name : names) { + if (captured.containsKey(name)) { + compileStack.removeVariable(name); + } + } + } +} diff --git a/src/main/java/org/codehaus/groovy/classgen/asm/StatementWriter.java b/src/main/java/org/codehaus/groovy/classgen/asm/StatementWriter.java index 2154f9ef009..f5af5e83cc9 100644 --- a/src/main/java/org/codehaus/groovy/classgen/asm/StatementWriter.java +++ b/src/main/java/org/codehaus/groovy/classgen/asm/StatementWriter.java @@ -23,6 +23,7 @@ import org.codehaus.groovy.ast.VariableScope; import org.codehaus.groovy.ast.expr.BinaryExpression; import org.codehaus.groovy.ast.expr.ClosureListExpression; +import org.codehaus.groovy.ast.expr.DeclarationExpression; import org.codehaus.groovy.ast.expr.EmptyExpression; import org.codehaus.groovy.ast.expr.Expression; import org.codehaus.groovy.ast.expr.MethodCall; @@ -45,6 +46,7 @@ import org.codehaus.groovy.ast.stmt.TryCatchStatement; import org.codehaus.groovy.ast.stmt.WhileStatement; import org.codehaus.groovy.classgen.AsmClassGenerator; +import org.codehaus.groovy.classgen.InstanceofFlowBindings; import org.codehaus.groovy.classgen.asm.CompileStack.BlockRecorder; import org.objectweb.asm.Label; import org.objectweb.asm.MethodVisitor; @@ -470,6 +472,11 @@ public void writeDoWhileLoop(final DoWhileStatement statement) { /** * Generates bytecode for an if/else statement. + *

+ * GROOVY-12242 / JEP 394: after the condition runs, pattern locals are hidden + * then re-published only on the live path (then / else / after abrupt + * completion) via {@link InstanceofFlowSlotPublisher}, matching + * {@link org.codehaus.groovy.classgen.VariableScopeVisitor}. * * @param statement the if statement to compile */ @@ -477,22 +484,38 @@ public void writeIfElse(final IfStatement statement) { controller.getAcg().onLineNumber(statement, "visitIfElse"); writeStatementLabel(statement); - Label exitPath = controller.getCompileStack().pushBreakable(statement.getStatementLabels()); // GROOVY-7463 + CompileStack compileStack = controller.getCompileStack(); + InstanceofFlowBindings bindings = InstanceofFlowBindings.of(statement.getBooleanExpression()); + + Label exitPath = compileStack.pushBreakable(statement.getStatementLabels()); // GROOVY-7463 statement.getBooleanExpression().visit(controller.getAcg()); + // Hide every pattern slot; publish only path-live bindings below. + InstanceofFlowSlotPublisher slotPublisher = InstanceofFlowSlotPublisher.captureAndHide(compileStack, bindings); + Label elsePath = controller.getOperandStack().jump(IFEQ); + slotPublisher.publishTrue(compileStack); statement.getIfBlock().visit(controller.getAcg()); - controller.getCompileStack().pop(); + slotPublisher.hideTrue(compileStack); + compileStack.pop(); // ends breakable + + boolean ifFallsThrough = maybeFallsThrough(statement.getIfBlock()); + boolean elseEmpty = statement.getElseBlock().isEmpty(); + boolean elseFallsThrough = elseEmpty || maybeFallsThrough(statement.getElseBlock()); MethodVisitor mv = controller.getMethodVisitor(); - if (statement.getElseBlock().isEmpty()) { + if (elseEmpty) { mv.visitLabel(elsePath); } else { - if (maybeFallsThrough(statement.getIfBlock())) { + if (ifFallsThrough) { mv.visitJumpInsn(GOTO, exitPath); } mv.visitLabel(elsePath); + slotPublisher.publishFalse(compileStack); statement.getElseBlock().visit(controller.getAcg()); + slotPublisher.hideFalse(compileStack); } + + slotPublisher.publishAfterIf(compileStack, ifFallsThrough, elseEmpty, elseFallsThrough); mv.visitLabel(exitPath); } @@ -883,6 +906,10 @@ public void writeReturn(final ReturnStatement statement) { * Evaluates the expression and discards any value left on the operand stack. * Marks method-call and binary expressions so that unused return values * are elided rather than boxed. + *

+ * GROOVY-12242: non-declaration statements that contain an {@code instanceof} + * type pattern run in a nested CompileStack state so pattern locals cannot + * leak. Detection uses {@link InstanceofFlowBindings#containsPattern}. * * @param statement the expression statement to compile */ @@ -895,9 +922,22 @@ public void writeExpressionStatement(final ExpressionStatement statement) { if (expression instanceof MethodCall || expression instanceof BinaryExpression) expression.putNodeMetaData(AsmClassGenerator.ELIDE_EXPRESSION_VALUE, Boolean.TRUE); - var operandStack = controller.getOperandStack(); - int mark = operandStack.getStackLength(); - expression.visit(controller.getAcg()); - operandStack.popDownTo(mark); + CompileStack compileStack = controller.getCompileStack(); + // Declaration LHS isolation is in evaluateEqual; multi-assign must not be wrapped. + boolean isolatesPatternVars = !(expression instanceof DeclarationExpression) + && InstanceofFlowBindings.containsPattern(expression); + if (isolatesPatternVars) { + compileStack.pushState(); + } + try { + var operandStack = controller.getOperandStack(); + int mark = operandStack.getStackLength(); + expression.visit(controller.getAcg()); + operandStack.popDownTo(mark); + } finally { + if (isolatesPatternVars) { + compileStack.pop(); + } + } } } diff --git a/src/test/groovy/groovy/InstanceofFlowBindingsTest.groovy b/src/test/groovy/groovy/InstanceofFlowBindingsTest.groovy new file mode 100644 index 00000000000..72a4d61aff8 --- /dev/null +++ b/src/test/groovy/groovy/InstanceofFlowBindingsTest.groovy @@ -0,0 +1,206 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package groovy + +import org.codehaus.groovy.ast.DynamicVariable +import org.codehaus.groovy.ast.expr.Expression +import org.codehaus.groovy.ast.expr.VariableExpression +import org.codehaus.groovy.ast.stmt.IfStatement +import org.codehaus.groovy.classgen.InstanceofFlowBindings +import org.codehaus.groovy.control.CompilationUnit +import org.codehaus.groovy.control.Phases +import org.codehaus.groovy.ast.CodeVisitorSupport +import org.junit.jupiter.api.Test + +final class InstanceofFlowBindingsTest { + + @Test + void testBindingsOfPositiveInstanceof() { + def b = InstanceofFlowBindings.of(parseCondition('o instanceof String s')) + assert b.whenTrue()*.name == ['s'] + assert b.whenFalse().isEmpty() + assert b.allNames() as List == ['s'] + } + + @Test + void testBindingsOfNegatedInstanceof() { + def b = InstanceofFlowBindings.of(parseCondition('!(o instanceof String s)')) + assert b.whenTrue().isEmpty() + assert b.whenFalse()*.name == ['s'] + assert b.allNames() as List == ['s'] + } + + @Test + void testBindingsOfAnd() { + def b = InstanceofFlowBindings.of(parseCondition('o instanceof String s && s.length() > 0')) + assert b.whenTrue()*.name == ['s'] + assert b.whenFalse().isEmpty() + } + + @Test + void testBindingsOfOr() { + def b = InstanceofFlowBindings.of(parseCondition('o instanceof String s || s.length() > 0')) + assert b.whenTrue().isEmpty() + assert b.whenFalse().isEmpty() + assert b.isEmpty() + } + + @Test + void testBindingsOfNegatedOrFalsePath() { + def b = InstanceofFlowBindings.of(parseCondition('!(o instanceof String s) || s.isEmpty()')) + // left false-path binds s; OR false bindings = left.whenFalse ∪ right.whenFalse = {s} + assert b.whenFalse()*.name == ['s'] + assert b.whenTrue().isEmpty() + } + + @Test + void testBindingsOfPlainInstanceofAreEmpty() { + def b = InstanceofFlowBindings.of(parseCondition('o instanceof String')) + assert b.isEmpty() + } + + @Test + void testOfNullIsEmpty() { + assert InstanceofFlowBindings.of(null).isEmpty() + } + + @Test + void testContainsPattern() { + assert InstanceofFlowBindings.containsPattern(parseCondition('o instanceof String s')) + assert InstanceofFlowBindings.containsPattern(parseCondition('!(o instanceof String s)')) + assert InstanceofFlowBindings.containsPattern(parseCondition('o instanceof String s && s')) + assert !InstanceofFlowBindings.containsPattern(parseCondition('o instanceof String')) + assert !InstanceofFlowBindings.containsPattern(null) + } + + @Test + void testContainsPatternNestedInCall() { + def expr = parseMethodArg('m(o instanceof String s)') + assert InstanceofFlowBindings.containsPattern(expr) + } + + @Test + void testRightOfOrIsDynamicVariable() { + def src = ''' + class C { + def m(Object o) { + if (o instanceof String s || s.length() > 0) { + return 1 + } + return 0 + } + } + ''' + def cu = new CompilationUnit() + cu.addSource('C.groovy', src) + cu.compile(Phases.SEMANTIC_ANALYSIS) + def accesses = [] + cu.ast.classes[0].getMethods('m')[0].code.visit(new CodeVisitorSupport() { + @Override + void visitVariableExpression(VariableExpression ve) { + if (ve.name == 's') { + accesses << ve.accessedVariable + } + super.visitVariableExpression(ve) + } + }) + assert accesses.any { it instanceof DynamicVariable } : + "expected DynamicVariable for RHS of ||, got: ${accesses*.class*.simpleName}" + } + + @Test + void testNegatedIfBranchIsDynamicVariable() { + def src = ''' + class C { + def m(Object o) { + if (!(o instanceof String s)) { + return s + } + return null + } + } + ''' + def cu = new CompilationUnit() + cu.addSource('C.groovy', src) + cu.compile(Phases.SEMANTIC_ANALYSIS) + def accesses = [] + cu.ast.classes[0].getMethods('m')[0].code.visit(new CodeVisitorSupport() { + @Override + void visitVariableExpression(VariableExpression ve) { + if (ve.name == 's') { + accesses << ve.accessedVariable + } + super.visitVariableExpression(ve) + } + }) + // declaration + use in if-branch (must be DynamicVariable) + assert accesses.count { it instanceof DynamicVariable } >= 1 + } + + private static Expression parseCondition(String condition) { + def src = """ + class C { + def m(Object o) { + if ($condition) return 1 + return 0 + } + } + """ + def cu = new CompilationUnit() + cu.addSource('C.groovy', src) + cu.compile(Phases.CONVERSION) + IfStatement ifStmt = null + cu.ast.classes[0].getMethods('m')[0].code.visit(new CodeVisitorSupport() { + @Override + void visitIfElse(IfStatement statement) { + ifStmt = statement + } + }) + ifStmt.booleanExpression.expression + } + + private static Expression parseMethodArg(String statement) { + def src = """ + class C { + def m(Object o) { + $statement + } + } + """ + def cu = new CompilationUnit() + cu.addSource('C.groovy', src) + cu.compile(Phases.CONVERSION) + Expression found = null + cu.ast.classes[0].getMethods('m')[0].code.visit(new CodeVisitorSupport() { + @Override + void visitMethodCallExpression(org.codehaus.groovy.ast.expr.MethodCallExpression call) { + if (call.methodAsString == 'm' || call.objectExpression.text == 'this') { + found = call + } + super.visitMethodCallExpression(call) + } + @Override + void visitExpressionStatement(org.codehaus.groovy.ast.stmt.ExpressionStatement stmt) { + found = stmt.expression + super.visitExpressionStatement(stmt) + } + }) + found + } +} diff --git a/src/test/groovy/groovy/InstanceofTest.groovy b/src/test/groovy/groovy/InstanceofTest.groovy index 7896c65ac9b..4caa6705093 100644 --- a/src/test/groovy/groovy/InstanceofTest.groovy +++ b/src/test/groovy/groovy/InstanceofTest.groovy @@ -223,4 +223,279 @@ final class InstanceofTest { } assert y == 'foobar' } + + // GROOVY-12242: Java-aligned flow scoping for negated instanceof (JEP 394) + @Test + void testVariableScopeNegatedElse() { + def f = { Object o -> + if (!(o instanceof String s)) { + return 'not' + } else { + return s.toUpperCase() + } + } + assert f('hi') == 'HI' + assert f(1) == 'not' + } + + // GROOVY-12242: pattern variable remains in scope after abrupt then-branch + @Test + void testVariableScopeEarlyReturn() { + def f = { Object o -> + if (!(o instanceof String s)) return 'early' + return s.toUpperCase() + } + assert f('hi') == 'HI' + assert f(42) == 'early' + } + + // GROOVY-12242: pattern variable remains after else that cannot complete normally + @Test + void testVariableScopeAfterAbruptElse() { + def f = { Object o -> + if (o instanceof String s) { + // matched + } else { + return 'no' + } + return s.toUpperCase() + } + assert f('ab') == 'AB' + assert f(9) == 'no' + } + + // GROOVY-12242: pattern variable must not leak after a declaration statement + @Test + void testVariableNoLeakAfterDeclaration() { + def err = shouldFail MissingPropertyException, ''' + class C { + Object m(Object o) { + boolean b = (o instanceof String s) + return s + } + } + new C().m('hi') + ''' + assert err.message =~ /No such property: s/ + } + + // GROOVY-12242: pattern variable must not leak after an expression statement + @Test + void testVariableNoLeakAfterExpressionStatement() { + def err = shouldFail MissingPropertyException, ''' + class C { + Object m(Object o) { + o instanceof String s && s.length() > 0 + return s + } + } + new C().m('hi') + ''' + assert err.message =~ /No such property: s/ + } + + // GROOVY-12242: true branch of negated instanceof must not see the pattern local + // (CompileStack polarity must match VariableScope — no silent null ALOAD) + @Test + void testVariableNegatedIfBranchNotInScope() { + def err = shouldFail MissingPropertyException, ''' + class C { + Object m(Object o) { + if (!(o instanceof String s)) { + return s + } + return 'matched' + } + } + new C().m(1) + ''' + assert err.message =~ /No such property: s/ + } + + // GROOVY-12242: true-path binding of left of || is not in scope on the right (Java) + @Test + void testVariableOrRightHandSideNotInScope() { + def shell = GroovyShell.withConfig { + ast groovy.transform.TypeChecked + } + def err = shouldFail shell, ''' + @groovy.transform.TypeChecked + class C { + static void m(Object o) { + if (o instanceof String s || s.length() > 0) { + } + } + } + ''' + assert err.message =~ /The variable .s. is undeclared|Apparent variable .s./ + } + + // GROOVY-12242: false-path binding is in scope on the right of || (Java) + @Test + void testVariableOrRightHandSideFalsePathInScope() { + def f = { Object o -> + // when o is String, left is false, right sees s + return (!(o instanceof String s) || s.isEmpty()) + } + assert f('') == true + assert f('x') == false + assert f(1) == true // left true → short-circuit, s not needed + } + + // GROOVY-12242: ternary false branch must not see true-path pattern variable + @Test + void testVariableTernaryFalseBranchNotInScope() { + def shell = GroovyShell.withConfig { + ast groovy.transform.TypeChecked + } + def err = shouldFail shell, ''' + @groovy.transform.TypeChecked + class C { + static Object m(Object o) { + return o instanceof String s ? 'yes' : s + } + } + ''' + assert err.message =~ /The variable .s. is undeclared|Apparent variable .s./ + } + + // GROOVY-12242: dynamic ternary false branch must not load a pattern local + @Test + void testVariableTernaryFalseBranchNotInScopeDynamic() { + def err = shouldFail MissingPropertyException, ''' + class C { + Object m(Object o) { + return o instanceof String s ? 'yes' : s + } + } + new C().m(1) + ''' + assert err.message =~ /No such property: s/ + } + + // GROOVY-12242: ternary true branch sees pattern variable + @Test + void testVariableTernaryTrueBranch() { + def f = { Object o -> o instanceof String s ? s.toUpperCase() : 'no' } + assert f('ab') == 'AB' + assert f(1) == 'no' + } + + // GROOVY-12242: reassignment of pattern variable (not implicitly final, JEP 394) + @Test + void testVariableReassignment() { + Object o = 'hi' + if (o instanceof String s) { + s = s + '!' + assert s == 'hi!' + } else { + assert false + } + } + + // GROOVY-12242: pattern variable shadows a field only where in scope + @Test + void testVariableFieldShadowing() { + def obj = new Object() { + String s = 'field' + def test(Object o) { + if (o instanceof String s) { + return "pv=$s" + } + return "field=$s" + } + } + assert obj.test('x') == 'pv=x' + assert obj.test(1) == 'field=field' + } + + // GROOVY-12242: && chain uses pattern variable on subsequent operands + @Test + void testVariableAndChain() { + Object o = 'hello' + assert (o instanceof String s && s.length() > 3 && s.startsWith('h')) + assert !(o instanceof String s && s.length() > 99) + } + + // GROOVY-12242: while body can use true-path pattern variable + @Test + void testVariableWhileBody() { + Object o = 'ab' + def n = 0 + while (o instanceof String s && s.length() > 0) { + n += 1 + o = s.substring(1) + } + assert n == 2 + assert o == '' + } + + // GROOVY-12242: reuse the same pattern variable name in successive statements + @Test + void testVariableNameReuse() { + Object a = 'x', b = 1 + def r = [] + if (a instanceof String s) r << s + if (b instanceof Integer s) r << s + assert r == ['x', 1] + } + + // GROOVY-12242: type-checked flow scoping for early return + @Test + void testVariableScopeEarlyReturnTypeChecked() { + def shell = GroovyShell.withConfig { + ast groovy.transform.TypeChecked + } + assert shell.evaluate(''' + @groovy.transform.TypeChecked + class C { + static String m(Object o) { + if (!(o instanceof String s)) return 'early' + return s.toUpperCase() + } + } + assert C.m('hi') == 'HI' + assert C.m(1) == 'early' + true + ''') + } + + // GROOVY-12242: type-checked — positive instanceof still not in else + @Test + void testVariableScopePositiveNotInElseTypeChecked() { + def shell = GroovyShell.withConfig { + ast groovy.transform.TypeChecked + } + def err = shouldFail shell, ''' + Number n = 12345 + if (n instanceof Integer i) { + } else { + i.toString() + } + ''' + assert err.message =~ /The variable .i. is undeclared/ + } + + // GROOVY-12242: type-checked — negated instanceof is in else + @Test + void testVariableScopeNegatedInElseTypeChecked() { + def shell = GroovyShell.withConfig { + ast groovy.transform.TypeChecked + } + assert shell.evaluate(''' + @groovy.transform.TypeChecked + class C { + static String m(Object o) { + if (!(o instanceof String s)) { + return 'not' + } else { + return s.toUpperCase() + } + } + } + assert C.m('hi') == 'HI' + assert C.m(1) == 'not' + true + ''') + } } From 0b93bc71606d4afd9f9a5a432744c87a74e9445a Mon Sep 17 00:00:00 2001 From: Daniel Sun Date: Sat, 8 Aug 2026 20:41:10 +0900 Subject: [PATCH 2/6] GROOVY-12242: add systematic visibility matrix for instanceof pattern variables --- .../classgen/InstanceofFlowBindings.java | 62 +++- .../groovy/InstanceofFlowBindingsTest.groovy | 195 ++++++++++++ src/test/groovy/groovy/InstanceofTest.groovy | 285 ++++++++++++++++++ 3 files changed, 532 insertions(+), 10 deletions(-) diff --git a/src/main/java/org/codehaus/groovy/classgen/InstanceofFlowBindings.java b/src/main/java/org/codehaus/groovy/classgen/InstanceofFlowBindings.java index 84e1d27a15d..5d6394a9000 100644 --- a/src/main/java/org/codehaus/groovy/classgen/InstanceofFlowBindings.java +++ b/src/main/java/org/codehaus/groovy/classgen/InstanceofFlowBindings.java @@ -18,6 +18,7 @@ */ package org.codehaus.groovy.classgen; +import groovy.transform.Internal; import org.codehaus.groovy.ast.CodeVisitorSupport; import org.codehaus.groovy.ast.expr.BinaryExpression; import org.codehaus.groovy.ast.expr.BooleanExpression; @@ -50,16 +51,25 @@ * {@code &&} (union of true bindings), {@code ||} (union of false bindings). * Other shapes contribute nothing (conservative). *

- * Consumers: - *

    - *
  • {@link VariableScopeVisitor} — declare names on the live path
    • - *
  • {@link org.codehaus.groovy.classgen.asm.InstanceofFlowSlotPublisher} — - * publish/hide CompileStack slots from these bindings
    • - *
+ * Design note — dual use across compiler phases.
+ * This class is consumed by two separate compiler phases: + *
    + *
  1. {@link VariableScopeVisitor} (semantic analysis) — uses the binding + * sets to declare pattern-variable names only on the live path, so that + * subsequent name resolution sees the correct scope.
    2. + *
  2. {@link org.codehaus.groovy.classgen.asm.InstanceofFlowSlotPublisher} + * (code generation) — uses the same binding sets to publish/hide bytecode + * locals on the matching control-flow arm, keeping CompileStack slot + * visibility consistent with the resolved scopes.
    3. + *
+ * Both phases need to ask the same question ("which names are live on which + * path?"), so sharing this analysis type avoids two independent implementations + * that could diverge. * * @see org.codehaus.groovy.classgen.asm.InstanceofFlowSlotPublisher * @since 6.0.0 */ +@Internal public final class InstanceofFlowBindings { private static final InstanceofFlowBindings EMPTY = @@ -111,6 +121,11 @@ public Set whenFalseNames() { /** * All pattern-variable names appearing in either path (stable encounter order). + *

+ * Implemented by iterating both lists directly rather than composing + * {@link #whenTrueNames()} and {@link #whenFalseNames()}: that would + * allocate two intermediate {@link Set} objects only to merge them into a + * third, whereas the direct loop allocates only the result set. */ public Set allNames() { if (isEmpty()) return Collections.emptySet(); @@ -145,8 +160,17 @@ public static InstanceofFlowBindings of(final Expression expression) { /** * Returns {@code true} if {@code expression} contains any JEP 394 type * pattern ({@code e instanceof T t} or {@code e !instanceof T t}), including - * nested subexpressions. Used to decide whether expression-statement - * CompileStack isolation is required. + * arbitrarily nested subexpressions (e.g. a pattern buried inside a + * method-call argument or a ternary). + *

+ * Why a full subtree walk? This method is used for + * expression-statement isolation: before discarding an expression-statement's + * value, the code generator needs to know whether any descendant + * node may have allocated a CompileStack slot for a pattern variable, even + * if {@link #of} does not model that node (e.g. a pattern inside a method + * argument). The conservative "does any descendant match?" question requires + * visiting the whole tree. Short-circuiting ({@code found[0]} check) stops + * the walk as soon as the first match is detected. * * @param expression any expression; {@code null} yields {@code false} */ @@ -175,6 +199,20 @@ private static boolean isTypePattern(final Expression right) { && decl.getVariableExpression() != null; } + /** + * Core recursive descent that computes binding sets. + *

+ * Why not a full subtree walk? Unlike {@link #containsPattern}, + * this method only needs to understand the boolean algebra of the + * condition (which variables are definitely bound on each path), + * not to locate patterns anywhere in an arbitrary expression tree. The + * recursive descent follows exactly the operators that can propagate + * definite-assignment ({@code instanceof}, {@code !instanceof}, {@code &&}, + * {@code ||}, {@code !}) and returns {@link #EMPTY} conservatively for any + * other expression shape. This keeps the traversal shallow and + * proportional to the boolean structure of the condition, not to the total + * AST size. + */ private static InstanceofFlowBindings analyse(final Expression expression) { Expression expr = expression; @@ -200,7 +238,9 @@ private static InstanceofFlowBindings analyse(final Expression expression) { if (op == Types.LOGICAL_AND) { InstanceofFlowBindings left = analyse(binary.getLeftExpression()); InstanceofFlowBindings right = analyse(binary.getRightExpression()); - // True path evaluates both; false path is not definite for either side alone. + // True path: the condition succeeds only if both sides are true, so + // both sides' true-bindings are definitely assigned. The false path is + // not definite: either side alone may have caused failure. return new InstanceofFlowBindings( union(left.whenTrue, right.whenTrue), List.of()); @@ -208,7 +248,9 @@ private static InstanceofFlowBindings analyse(final Expression expression) { if (op == Types.LOGICAL_OR) { InstanceofFlowBindings left = analyse(binary.getLeftExpression()); InstanceofFlowBindings right = analyse(binary.getRightExpression()); - // False path evaluates both; true path is not definite for either side alone. + // False path: the condition fails only if both sides are false, so + // both sides' false-bindings are definitely assigned. The true path + // is not definite: only the left side may have been evaluated. return new InstanceofFlowBindings( List.of(), union(left.whenFalse, right.whenFalse)); diff --git a/src/test/groovy/groovy/InstanceofFlowBindingsTest.groovy b/src/test/groovy/groovy/InstanceofFlowBindingsTest.groovy index 72a4d61aff8..85b969c491e 100644 --- a/src/test/groovy/groovy/InstanceofFlowBindingsTest.groovy +++ b/src/test/groovy/groovy/InstanceofFlowBindingsTest.groovy @@ -153,6 +153,175 @@ final class InstanceofFlowBindingsTest { assert accesses.count { it instanceof DynamicVariable } >= 1 } + // ------------------------------------------------------------------------- + // GROOVY-12242: systematic binding-analysis unit tests + // + // Covers every condition shape in the visibility matrix: + // (1) instanceof s + // (2) !instanceof s + // (3) !instanceof s (negated via BooleanExpression) + // (4) instanceof s && cond + // (5) instanceof s || cond + // (6) !instanceof s && cond + // (7) !instanceof s || cond + // (8) !(instanceof s && cond) + // (9) double negation !!(instanceof s) + // ------------------------------------------------------------------------- + + // (4) o instanceof String s && cond — true: {s}, false: {} + @Test + void testAndWithPattern_trueBinds_falseEmpty() { + def b = InstanceofFlowBindings.of(parseCondition('o instanceof String s && true')) + assert b.whenTrue()*.name == ['s'] + assert b.whenFalse().isEmpty() + } + + // (5) o instanceof String s || cond — both paths empty (can't guarantee s on true path, + // and cond alone doesn't bind s on the false path) + @Test + void testOrWithPattern_bothEmpty() { + def b = InstanceofFlowBindings.of(parseCondition('o instanceof String s || true')) + assert b.isEmpty() + } + + // (6) !(o instanceof String s) && cond + // Left's false path binds s, but && propagates no false-bindings. + // Left's true path is empty. Right contributes nothing. + // → true: {}, false: {} + @Test + void testNegatedAndCond_bothEmpty() { + def b = InstanceofFlowBindings.of(parseCondition('!(o instanceof String s) && true')) + assert b.isEmpty() + } + + // (7) !(o instanceof String s) || cond + // Left false-path binds s (since !(s) is false → s matched). + // Right's false path is empty. || false-path = union of false-paths = {s}. + // → true: {}, false: {s} + @Test + void testNegatedOrCond_falseBinds() { + def b = InstanceofFlowBindings.of(parseCondition('!(o instanceof String s) || true')) + assert b.whenFalse()*.name == ['s'] + assert b.whenTrue().isEmpty() + } + + // (8) !(o instanceof String s && s.length() > 0) + // Inner: true:{s}, false:{}. Negated: true:{}, false:{s}. + // → true: {}, false: {s} + @Test + void testNegatedAndPattern_falseBindsAfterNegation() { + def b = InstanceofFlowBindings.of(parseCondition('!(o instanceof String s && s.length() > 0)')) + assert b.whenFalse()*.name == ['s'] + assert b.whenTrue().isEmpty() + } + + // (9) double negation !!(o instanceof String s) ≡ o instanceof String s + // Inner: true:{s}, false:{}. Negated once: true:{}, false:{s}. + // Negated twice: true:{s}, false:{}. + @Test + void testDoubleNegation_sameAsPositive() { + def b = InstanceofFlowBindings.of(parseCondition('!!(o instanceof String s)')) + assert b.whenTrue()*.name == ['s'] + assert b.whenFalse().isEmpty() + } + + // (4) o instanceof String s && p instanceof Integer i (JLS §6.3.1.1) + // Rule B: a&&b when-true = a.whenTrue ∪ b.whenTrue = {s} ∪ {i} = {s, i} + // Rule A: s (introduced by a when true) is definitely matched at b — legal, + // different names, no §6.3.1.1-200-A error. + // No rule for when-false (§6.3.1.1 note: cannot determine which side failed). + @Test + void testAndWithTwoPatterns_bothInTrue() { + def b = InstanceofFlowBindings.of( + parseCondition2('o instanceof String s && p instanceof Integer i')) + assert b.whenTrue()*.name as Set == ['s', 'i'] as Set + assert b.whenFalse().isEmpty() + } + + // (5) o instanceof String s || p instanceof Integer i (JLS §6.3.1.2) + // Rule B: a||b when-false = a.whenFalse ∪ b.whenFalse + // a.whenFalse for (o instanceof String s) = {} (§6.3.1.5: no when-false rule for instanceof) + // b.whenFalse for (p instanceof Integer i) = {} + // a||b when-false = {} ∪ {} = {} + // No rule for when-true (§6.3.1.2 note: cannot determine which side was true). + // Different names s/i → no §6.3.1.2-200-A or -200-B error. + @Test + void testOrWithTwoPatterns_bothEmpty() { + def b = InstanceofFlowBindings.of( + parseCondition2('o instanceof String s || p instanceof Integer i')) + assert b.isEmpty() + } + + // !(a instanceof String s) || !(b instanceof Integer i) (JLS §6.3.1.2 + §6.3.1.3) + // !(o instanceof String s): when-false = {s} (§6.3.1.3: !a when-false = a.whenTrue) + // !(p instanceof Integer i): when-false = {i} + // Rule A: s (introduced by left when false) is in scope at right (§6.3.1.2-100-A). + // Rule B: a||b when-false = {s} ∪ {i} = {s, i} + // Different names s/i → no §6.3.1.2-200-B error. + @Test + void testOrWithTwoNegatedPatterns_falseHasBoth() { + def b = InstanceofFlowBindings.of( + parseCondition2('!(o instanceof String s) || !(p instanceof Integer i)')) + assert b.whenFalse()*.name as Set == ['s', 'i'] as Set + assert b.whenTrue().isEmpty() + } + + // isEmpty() is false when only whenFalse is non-empty + @Test + void testIsEmpty_falseWhenOnlyFalseSide() { + def b = InstanceofFlowBindings.of(parseCondition('!(o instanceof String s)')) + assert !b.isEmpty() + assert b.whenTrue().isEmpty() + assert !b.whenFalse().isEmpty() + } + + // allNames() returns names from both sides (stable order) + @Test + void testAllNames_mergesBothSides() { + // construct a condition that has s on true and t on false: + // (o instanceof String s && true) || !(o instanceof Integer t) + // → true: {} (|| doesn't guarantee), false: {t} (from right) + // Use allNames on something that has both sides: + // !(o instanceof String s) alone: whenFalse={s}, whenTrue={}; allNames={s} + def b1 = InstanceofFlowBindings.of(parseCondition('!(o instanceof String s)')) + assert b1.allNames() == ['s'] as Set + + // positive: whenTrue={s}, whenFalse={} + def b2 = InstanceofFlowBindings.of(parseCondition('o instanceof String s')) + assert b2.allNames() == ['s'] as Set + } + + // whenTrueNames() / whenFalseNames() return Set (not List) + // Uses a legal condition; tests that the return type is a Set. + @Test + void testNameSets_returnSets() { + // o instanceof String s && true → whenTrue introduces {s}, whenFalse is {} + def b = InstanceofFlowBindings.of(parseCondition('o instanceof String s && true')) + assert b.whenTrueNames() instanceof Set + assert b.whenFalseNames() instanceof Set + assert b.whenTrueNames() == ['s'] as Set + assert b.whenFalseNames().isEmpty() + } + + // JLS §6.3.1.1-200-A: it is a compile-time error when the same pattern variable name + // is introduced by BOTH operands of &&. InstanceofFlowBindings is a pre-error analysis + // (it runs before the error is reported) and will conservatively union the sets; the + // resulting program is still rejected at compile time by VariableScopeVisitor. + // This test documents that behavior without asserting on specific names (the analysis + // result for rejected input is unspecified / implementation-defined). + @Test + void testJLS_6_3_1_1_DuplicateNameIsCompileError_analysisStillRuns() { + // 'o instanceof String s && o instanceof String s' — same name s on both sides, + // §6.3.1.1-200-A error. The analysis sees both, union deduplicates to size 1. + // We only check it doesn't throw; the compiler will still reject the source. + def b = InstanceofFlowBindings.of(parseCondition('o instanceof String s && o instanceof String s')) + // whenTrue may contain s (deduped) — at minimum the result is not null / EMPTY + assert b != null + // The union of {s} and {s} should deduplicate to one entry + assert b.whenTrueNames().size() == 1 : 'union should deduplicate same-named vars' + assert b.whenFalseNames().isEmpty() : 'no false-path bindings for &&' + } + private static Expression parseCondition(String condition) { def src = """ class C { @@ -203,4 +372,30 @@ final class InstanceofFlowBindingsTest { }) found } + + /** + * Variant of {@link #parseCondition} for conditions that reference two variables + * ({@code o} and {@code p}), e.g. multi-pattern {@code &&} / {@code ||} combinations. + */ + private static Expression parseCondition2(String condition) { + def src = """ + class C { + def m(Object o, Object p) { + if ($condition) return 1 + return 0 + } + } + """ + def cu = new CompilationUnit() + cu.addSource('C.groovy', src) + cu.compile(Phases.CONVERSION) + IfStatement ifStmt = null + cu.ast.classes[0].getMethods('m')[0].code.visit(new CodeVisitorSupport() { + @Override + void visitIfElse(IfStatement statement) { + ifStmt = statement + } + }) + ifStmt.booleanExpression.expression + } } diff --git a/src/test/groovy/groovy/InstanceofTest.groovy b/src/test/groovy/groovy/InstanceofTest.groovy index 4caa6705093..ba7b6ec8dee 100644 --- a/src/test/groovy/groovy/InstanceofTest.groovy +++ b/src/test/groovy/groovy/InstanceofTest.groovy @@ -498,4 +498,289 @@ final class InstanceofTest { true ''') } + + // ------------------------------------------------------------------------- + // GROOVY-12242: systematic visibility matrix + // + // For each condition shape the test verifies: + // - if-block visibility + // - else-block visibility + // - after-if-else visibility (with and without abrupt completion) + // ------------------------------------------------------------------------- + + // --- (1) simple: o instanceof String s --- + + // pattern var in if-block (already covered by testVariable above); + // here we also test: NOT in else-block, NOT after if-else (no abrupt branch) + @Test + void testSimpleInstanceof_notInElse_notAfterIf() { + def err = shouldFail MissingPropertyException, ''' + class C { + def m(Object o) { + if (o instanceof String s) { /* ok */ } + else { return s } + } + } + new C().m(1) + ''' + assert err.message =~ /No such property: s/ + + err = shouldFail MissingPropertyException, ''' + class C { + def m(Object o) { + if (o instanceof String s) { /* ok */ } + return s + } + } + new C().m('x') + ''' + assert err.message =~ /No such property: s/ + } + + // pattern var IS visible after if when else cannot complete normally + @Test + void testSimpleInstanceof_visibleAfterIf_whenElseAbrupt() { + // else throws → s is visible after + def f = { Object o -> + if (o instanceof String s) { + // matched + } else { + throw new IllegalArgumentException('not a string') + } + s.toUpperCase() + } + assert f('hello') == 'HELLO' + try { f(1); assert false } catch (IllegalArgumentException ignored) {} + } + + // --- (2) simple: !(o instanceof String s) --- + + // true-branch (the !instanceof branch) must NOT see s; + // false-branch (else) MUST see s + @Test + void testNegatedInstanceof_truePathHides_falsePathBinds() { + // else sees s (already covered by testVariableScopeNegatedElse) + // here: if-branch does NOT see s + def err = shouldFail MissingPropertyException, ''' + class C { + def m(Object o) { + if (!(o instanceof String s)) { + return s + } + return 'ok' + } + } + new C().m(1) + ''' + assert err.message =~ /No such property: s/ + } + + // --- (3) !instanceof s, plus return in else block (→ s after if) --- + // (already covered by testVariableScopeEarlyReturn / testVariableScopeNegatedElse; + // repeat here as an explicit cell in the matrix) + @Test + void testNegatedInstanceof_earlyReturnInTrue_visibleAfter() { + def f = { Object o -> + if (!(o instanceof String s)) return 'nope' + s.toUpperCase() + } + assert f('hi') == 'HI' + assert f(42) == 'nope' + } + + // --- (4) o instanceof String s && cond --- + + // if-block: s visible; else-block: s NOT visible; after: NOT visible + @Test + void testAndChain_ifBlockVisible_elseNotVisible_afterNotVisible() { + // if-block is visible (tested by testVariableAndChain and testVariableScope) + // else-block: NOT visible + def err = shouldFail MissingPropertyException, ''' + class C { + def m(Object o) { + if (o instanceof String s && s.length() > 0) { + /* ok */ + } else { + return s + } + } + } + new C().m(1) + ''' + assert err.message =~ /No such property: s/ + + // after if: NOT visible (even if both branches complete normally) + err = shouldFail MissingPropertyException, ''' + class C { + def m(Object o) { + if (o instanceof String s && s.length() > 0) { /* ok */ } + return s + } + } + new C().m('x') + ''' + assert err.message =~ /No such property: s/ + } + + // --- (5) o instanceof String s || cond --- + + // In dynamic Groovy the evaluateInstanceof always allocates the slot, so s can be + // accessed in the if-block at runtime (even though flow scoping says it's not + // guaranteed). TypeChecked enforces the stricter Java rule: true-path binding of + // left of || is NOT in scope on the right (Java rule) and NOT in the if-block. + @Test + void testOrChain_noVisibilityAnywhere() { + def shell = GroovyShell.withConfig { + ast groovy.transform.TypeChecked + } + // s must not be visible in the if-block when condition is instanceof s || ... + def err = shouldFail shell, ''' + @groovy.transform.TypeChecked + class C { + static Object m(Object o) { + if (o instanceof String s || true) { + return s + } + return 'ok' + } + } + ''' + assert err.message =~ /The variable .s. is undeclared|Apparent variable .s./ + } + + // --- (6) !(o instanceof String s) && cond --- + // De Morgan: ≡ (!instanceof s) && cond + // In dynamic mode, evaluateInstanceof defines the slot during condition evaluation, + // so s resolves dynamically even in the if-body. TypeChecked enforces the strict rule: + // && propagates no false-bindings, so if-block (true path) does NOT see s. + @Test + void testNegatedAndCond_noVisibility() { + def shell = GroovyShell.withConfig { + ast groovy.transform.TypeChecked + } + def err = shouldFail shell, ''' + @groovy.transform.TypeChecked + class C { + static Object m(Object o) { + if (!(o instanceof String s) && true) { + return s + } + return 'out' + } + } + ''' + assert err.message =~ /The variable .s. is undeclared|Apparent variable .s./ + } + + // --- (7) !(o instanceof String s) && cond, plus return in else block --- + // After the if: the if-block's true-path has !(s bound) && cond, no s guarantee. + // TypeChecked enforces that s is NOT visible after the if statement. + @Test + void testNegatedAndCond_withElseReturn_noVisibilityAfter() { + def shell = GroovyShell.withConfig { + ast groovy.transform.TypeChecked + } + def err = shouldFail shell, ''' + @groovy.transform.TypeChecked + class C { + static Object m(Object o) { + if (!(o instanceof String s) && true) { + // true path: s not definitely bound + } else { + return 'out' + } + return s + } + } + ''' + assert err.message =~ /The variable .s. is undeclared|Apparent variable .s./ + } + + // --- (8) !(o instanceof String s) || cond --- + // Equivalent to !s || cond. False path of || = both sides false → s is bound when + // left is false (i.e. o instanceof String s) AND right is false. + // So else-block sees s; after-if with abrupt if-block sees s. + @Test + void testNegatedOr_elseBlockSees_afterAbruptIfBlockSees() { + // else-block: !(!(s)) || cond is false → !(o instanceof s) is false → s bound + def f = { Object o -> + if (!(o instanceof String s) || s.isEmpty()) { + return 'branch-true' + } else { + // here s is definitely bound (the !instanceof was false, so instanceof matched) + return 'has-s:' + s + } + } + assert f('hello') == 'has-s:hello' // !instanceof false, so else + assert f('') == 'branch-true' // !instanceof false but s.isEmpty true → if + assert f(42) == 'branch-true' // !instanceof true → if + + // after if with abrupt else (throw): s visible after + def g = { Object o -> + if (!(o instanceof String s) || s.isEmpty()) { + /* fell through */ + } else { + throw new IllegalStateException('non-empty string') + } + // s NOT visible here (if-block can complete normally without binding s) + } + g('') // no exception + g(42) // no exception + } + + // --- (9) !(o instanceof String s) || cond, plus return in else block --- + // true-path completes normally → s NOT visible after if-else when else abruptly returns + // (because the if-path does not guarantee s is bound) + @Test + void testNegatedOr_elseReturn_noVisibilityAfter() { + def err = shouldFail MissingPropertyException, ''' + class C { + def m(Object o) { + if (!(o instanceof String s) || s.isEmpty()) { + /* true branch: no guarantee s is bound */ + } else { + return 'else' + } + return s + } + } + new C().m('') + ''' + assert err.message =~ /No such property: s/ + } + + // ------------------------------------------------------------------------- + // Additional De Morgan / compound cases + // ------------------------------------------------------------------------- + + // De Morgan: !(a && b) ≡ !a || !b + // !(o instanceof String s && cond) — no binding anywhere (conservative) + @Test + void testDeMorgan_notAndNegation_noBinding() { + def err = shouldFail MissingPropertyException, ''' + class C { + def m(Object o) { + if (!(o instanceof String s && s.length() > 0)) { + return s + } + return 'ok' + } + } + new C().m(1) + ''' + assert err.message =~ /No such property: s/ + } + + // Double negation: !!(o instanceof String s) ≡ o instanceof String s + @Test + void testDoubleNegation_positiveBinding() { + def f = { Object o -> + if (!!(o instanceof String s)) { + return s.toUpperCase() + } + return 'no' + } + assert f('ab') == 'AB' + assert f(1) == 'no' + } } From 5738634ae2df9ae7d36864690c29e5099d1827a9 Mon Sep 17 00:00:00 2001 From: Daniel Sun Date: Mon, 10 Aug 2026 02:37:42 +0900 Subject: [PATCH 3/6] GROOVY-12242: drive pattern-slot visibility from CompileStack push/hide/pop --- .../groovy/classgen/AsmClassGenerator.java | 22 +- .../classgen/InstanceofFlowBindings.java | 291 -------- .../groovy/classgen/VariableScopeVisitor.java | 373 ++++++++- .../classgen/asm/BinaryExpressionHelper.java | 39 +- .../groovy/classgen/asm/CompileStack.java | 114 ++- .../asm/InstanceofFlowSlotPublisher.java | 155 ---- .../groovy/classgen/asm/StatementWriter.java | 96 ++- .../groovy/InstanceofFlowBindingsTest.groovy | 146 +++- .../groovy/groovy/InstanceofScopeTest.groovy | 705 ++++++++++++++++++ src/test/groovy/groovy/InstanceofTest.groovy | 274 ++++++- 10 files changed, 1639 insertions(+), 576 deletions(-) delete mode 100644 src/main/java/org/codehaus/groovy/classgen/InstanceofFlowBindings.java delete mode 100644 src/main/java/org/codehaus/groovy/classgen/asm/InstanceofFlowSlotPublisher.java create mode 100644 src/test/groovy/groovy/InstanceofScopeTest.groovy diff --git a/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java b/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java index 3bf8e5e7bdb..3b83617bf5f 100644 --- a/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java +++ b/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java @@ -27,6 +27,7 @@ import org.codehaus.groovy.ast.ClassHelper; import org.codehaus.groovy.ast.ClassNode; import org.codehaus.groovy.ast.ConstructorNode; +import org.codehaus.groovy.ast.DynamicVariable; import org.codehaus.groovy.ast.FieldNode; import org.codehaus.groovy.ast.GenericsType; import org.codehaus.groovy.ast.InnerClassNode; @@ -1673,14 +1674,27 @@ public void visitVariableExpression(final VariableExpression expression) { return; } - BytecodeVariable variable = compileStack.getVariable(expression.getName(), /*throwIfMissing*/false); - if (variable != null) { - controller.getOperandStack().loadOrStoreVariable(variable, expression.isUseReferenceDirectly()); - } else { + // GROOVY-12242: VariableScopeVisitor marks out-of-scope pattern references + // as DynamicVariable. Name-based CompileStack lookup must not bypass that: + // pattern slots are allocated during condition evaluation and can still be + // present on the stack for short-circuit RHS of || (where the pattern is + // not definitely bound) or briefly on a non-live arm before hide. Treat + // DynamicVariable as property access even when a same-named slot exists. + if (expression.getAccessedVariable() instanceof DynamicVariable) { PropertyExpression pexp = thisPropX(/*implicit-this*/true, expression.getName()); pexp.getProperty().setSourcePosition(expression); pexp.copyNodeMetaData(expression); pexp.visit(this); + } else { + BytecodeVariable variable = compileStack.getVariable(expression.getName(), /*throwIfMissing*/false); + if (variable != null) { + controller.getOperandStack().loadOrStoreVariable(variable, expression.isUseReferenceDirectly()); + } else { + PropertyExpression pexp = thisPropX(/*implicit-this*/true, expression.getName()); + pexp.getProperty().setSourcePosition(expression); + pexp.copyNodeMetaData(expression); + pexp.visit(this); + } } if (!compileStack.isLHS()) { diff --git a/src/main/java/org/codehaus/groovy/classgen/InstanceofFlowBindings.java b/src/main/java/org/codehaus/groovy/classgen/InstanceofFlowBindings.java deleted file mode 100644 index 5d6394a9000..00000000000 --- a/src/main/java/org/codehaus/groovy/classgen/InstanceofFlowBindings.java +++ /dev/null @@ -1,291 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -package org.codehaus.groovy.classgen; - -import groovy.transform.Internal; -import org.codehaus.groovy.ast.CodeVisitorSupport; -import org.codehaus.groovy.ast.expr.BinaryExpression; -import org.codehaus.groovy.ast.expr.BooleanExpression; -import org.codehaus.groovy.ast.expr.DeclarationExpression; -import org.codehaus.groovy.ast.expr.Expression; -import org.codehaus.groovy.ast.expr.NotExpression; -import org.codehaus.groovy.ast.expr.VariableExpression; -import org.codehaus.groovy.syntax.Types; - -import java.util.ArrayList; -import java.util.Collections; -import java.util.LinkedHashSet; -import java.util.List; -import java.util.Set; - -/** - * Flow-sensitive analysis of JEP 394 {@code instanceof} pattern bindings - * (GROOVY-12242). - *

- * This is pure semantic analysis: given a boolean expression, which - * pattern variables are definitely bound when the expression is - * {@code true} versus {@code false}? (Same idea as compiler “flow info” / - * JEP 394 flow scoping — not a bytecode construct.) - *

    - *
  • {@link #of(Expression)} — true/false binding sets for a condition
    • - *
  • {@link #containsPattern(Expression)} — nested type-pattern presence - * (e.g. whether an expression statement needs CompileStack isolation)
    • - *
- * Covered shapes: {@code e instanceof T t}, negation / {@code !instanceof}, - * {@code &&} (union of true bindings), {@code ||} (union of false bindings). - * Other shapes contribute nothing (conservative). - *

- * Design note — dual use across compiler phases.
- * This class is consumed by two separate compiler phases: - *

    - *
  1. {@link VariableScopeVisitor} (semantic analysis) — uses the binding - * sets to declare pattern-variable names only on the live path, so that - * subsequent name resolution sees the correct scope.
    2. - *
  2. {@link org.codehaus.groovy.classgen.asm.InstanceofFlowSlotPublisher} - * (code generation) — uses the same binding sets to publish/hide bytecode - * locals on the matching control-flow arm, keeping CompileStack slot - * visibility consistent with the resolved scopes.
    3. - *
- * Both phases need to ask the same question ("which names are live on which - * path?"), so sharing this analysis type avoids two independent implementations - * that could diverge. - * - * @see org.codehaus.groovy.classgen.asm.InstanceofFlowSlotPublisher - * @since 6.0.0 - */ -@Internal -public final class InstanceofFlowBindings { - - private static final InstanceofFlowBindings EMPTY = - new InstanceofFlowBindings(List.of(), List.of()); - - private final List whenTrue; - private final List whenFalse; - - private InstanceofFlowBindings(final List whenTrue, - final List whenFalse) { - this.whenTrue = whenTrue; - this.whenFalse = whenFalse; - } - - /** - * Pattern variables that are definitely assigned when the analysed expression - * evaluates to {@code true}. - */ - public List whenTrue() { - return whenTrue; - } - - /** - * Pattern variables that are definitely assigned when the analysed expression - * evaluates to {@code false}. - */ - public List whenFalse() { - return whenFalse; - } - - /** Whether any pattern variable is bound on either path. */ - public boolean isEmpty() { - return whenTrue.isEmpty() && whenFalse.isEmpty(); - } - - /** - * Names of pattern variables bound when the expression is {@code true}. - */ - public Set whenTrueNames() { - return names(whenTrue); - } - - /** - * Names of pattern variables bound when the expression is {@code false}. - */ - public Set whenFalseNames() { - return names(whenFalse); - } - - /** - * All pattern-variable names appearing in either path (stable encounter order). - *

- * Implemented by iterating both lists directly rather than composing - * {@link #whenTrueNames()} and {@link #whenFalseNames()}: that would - * allocate two intermediate {@link Set} objects only to merge them into a - * third, whereas the direct loop allocates only the result set. - */ - public Set allNames() { - if (isEmpty()) return Collections.emptySet(); - Set names = new LinkedHashSet<>(whenTrue.size() + whenFalse.size()); - for (VariableExpression ve : whenTrue) names.add(ve.getName()); - for (VariableExpression ve : whenFalse) names.add(ve.getName()); - return names; - } - - private static Set names(final List vars) { - if (vars.isEmpty()) return Collections.emptySet(); - Set result = new LinkedHashSet<>(vars.size()); - for (VariableExpression ve : vars) { - result.add(ve.getName()); - } - return result; - } - - /** - * Analyses {@code expression} for definite {@code instanceof} pattern bindings. - * - * @param expression a boolean condition (may be a {@link BooleanExpression} wrapper) - * @return the true/false binding sets; never {@code null} - */ - public static InstanceofFlowBindings of(final Expression expression) { - if (expression == null) { - return EMPTY; - } - return analyse(expression); - } - - /** - * Returns {@code true} if {@code expression} contains any JEP 394 type - * pattern ({@code e instanceof T t} or {@code e !instanceof T t}), including - * arbitrarily nested subexpressions (e.g. a pattern buried inside a - * method-call argument or a ternary). - *

- * Why a full subtree walk? This method is used for - * expression-statement isolation: before discarding an expression-statement's - * value, the code generator needs to know whether any descendant - * node may have allocated a CompileStack slot for a pattern variable, even - * if {@link #of} does not model that node (e.g. a pattern inside a method - * argument). The conservative "does any descendant match?" question requires - * visiting the whole tree. Short-circuiting ({@code found[0]} check) stops - * the walk as soon as the first match is detected. - * - * @param expression any expression; {@code null} yields {@code false} - */ - public static boolean containsPattern(final Expression expression) { - if (expression == null) return false; - boolean[] found = {false}; - expression.visit(new CodeVisitorSupport() { - @Override - public void visitBinaryExpression(final BinaryExpression be) { - if (found[0]) return; - int op = be.getOperation().getType(); - if ((op == Types.KEYWORD_INSTANCEOF || op == Types.COMPARE_NOT_INSTANCEOF) - && isTypePattern(be.getRightExpression())) { - found[0] = true; - return; - } - super.visitBinaryExpression(be); - } - }); - return found[0]; - } - - private static boolean isTypePattern(final Expression right) { - return right instanceof DeclarationExpression decl - && !decl.isMultipleAssignmentDeclaration() - && decl.getVariableExpression() != null; - } - - /** - * Core recursive descent that computes binding sets. - *

- * Why not a full subtree walk? Unlike {@link #containsPattern}, - * this method only needs to understand the boolean algebra of the - * condition (which variables are definitely bound on each path), - * not to locate patterns anywhere in an arbitrary expression tree. The - * recursive descent follows exactly the operators that can propagate - * definite-assignment ({@code instanceof}, {@code !instanceof}, {@code &&}, - * {@code ||}, {@code !}) and returns {@link #EMPTY} conservatively for any - * other expression shape. This keeps the traversal shallow and - * proportional to the boolean structure of the condition, not to the total - * AST size. - */ - private static InstanceofFlowBindings analyse(final Expression expression) { - Expression expr = expression; - - // Unwrap BooleanExpression wrappers; NotExpression is handled below so that - // nested negations compose correctly. - while (expr instanceof BooleanExpression && !(expr instanceof NotExpression)) { - expr = ((BooleanExpression) expr).getExpression(); - } - - if (expr instanceof NotExpression not) { - return analyse(not.getExpression()).negated(); - } - - if (expr instanceof BinaryExpression binary) { - int op = binary.getOperation().getType(); - if (op == Types.KEYWORD_INSTANCEOF) { - return ofInstanceof(binary); - } - if (op == Types.COMPARE_NOT_INSTANCEOF) { - // AST may still carry !instanceof before codegen rewrites it to !(… instanceof …). - return ofInstanceof(binary).negated(); - } - if (op == Types.LOGICAL_AND) { - InstanceofFlowBindings left = analyse(binary.getLeftExpression()); - InstanceofFlowBindings right = analyse(binary.getRightExpression()); - // True path: the condition succeeds only if both sides are true, so - // both sides' true-bindings are definitely assigned. The false path is - // not definite: either side alone may have caused failure. - return new InstanceofFlowBindings( - union(left.whenTrue, right.whenTrue), - List.of()); - } - if (op == Types.LOGICAL_OR) { - InstanceofFlowBindings left = analyse(binary.getLeftExpression()); - InstanceofFlowBindings right = analyse(binary.getRightExpression()); - // False path: the condition fails only if both sides are false, so - // both sides' false-bindings are definitely assigned. The true path - // is not definite: only the left side may have been evaluated. - return new InstanceofFlowBindings( - List.of(), - union(left.whenFalse, right.whenFalse)); - } - } - - return EMPTY; - } - - private static InstanceofFlowBindings ofInstanceof(final BinaryExpression binary) { - Expression right = binary.getRightExpression(); - if (isTypePattern(right)) { - VariableExpression patternVar = ((DeclarationExpression) right).getVariableExpression(); - return new InstanceofFlowBindings(List.of(patternVar), List.of()); - } - return EMPTY; - } - - private InstanceofFlowBindings negated() { - if (isEmpty()) return this; - return new InstanceofFlowBindings(whenFalse, whenTrue); - } - - private static List union(final List a, - final List b) { - if (a.isEmpty()) return b; - if (b.isEmpty()) return a; - List result = new ArrayList<>(a.size() + b.size()); - Set seen = new LinkedHashSet<>(); - for (VariableExpression ve : a) { - if (seen.add(ve.getName())) result.add(ve); - } - for (VariableExpression ve : b) { - if (seen.add(ve.getName())) result.add(ve); - } - return List.copyOf(result); - } -} diff --git a/src/main/java/org/codehaus/groovy/classgen/VariableScopeVisitor.java b/src/main/java/org/codehaus/groovy/classgen/VariableScopeVisitor.java index c2acdcadfd1..6bc7be3f52b 100644 --- a/src/main/java/org/codehaus/groovy/classgen/VariableScopeVisitor.java +++ b/src/main/java/org/codehaus/groovy/classgen/VariableScopeVisitor.java @@ -20,7 +20,9 @@ import org.codehaus.groovy.ast.ASTNode; import org.codehaus.groovy.ast.AnnotationNode; +import groovy.transform.Internal; import org.codehaus.groovy.ast.ClassCodeVisitorSupport; +import org.codehaus.groovy.ast.CodeVisitorSupport; import org.codehaus.groovy.ast.ClassHelper; import org.codehaus.groovy.ast.ClassNode; import org.codehaus.groovy.ast.DynamicVariable; @@ -35,6 +37,7 @@ import org.codehaus.groovy.ast.expr.AnnotationConstantExpression; import org.codehaus.groovy.ast.expr.ArrayExpression; import org.codehaus.groovy.ast.expr.BinaryExpression; +import org.codehaus.groovy.ast.expr.BooleanExpression; import org.codehaus.groovy.ast.expr.CastExpression; import org.codehaus.groovy.ast.expr.ClassExpression; import org.codehaus.groovy.ast.expr.ClosureExpression; @@ -42,6 +45,7 @@ import org.codehaus.groovy.ast.expr.ConstructorCallExpression; import org.codehaus.groovy.ast.expr.DeclarationExpression; import org.codehaus.groovy.ast.expr.Expression; +import org.codehaus.groovy.ast.expr.NotExpression; import org.codehaus.groovy.ast.expr.FieldExpression; import org.codehaus.groovy.ast.expr.MethodCallExpression; import org.codehaus.groovy.ast.expr.PropertyExpression; @@ -63,9 +67,12 @@ import org.codehaus.groovy.control.SourceUnit; import org.codehaus.groovy.syntax.Types; +import java.util.ArrayList; +import java.util.Collections; import java.util.Deque; import java.util.HashSet; import java.util.Iterator; +import java.util.LinkedHashSet; import java.util.LinkedList; import java.util.List; import java.util.Optional; @@ -84,11 +91,23 @@ /** * Initializes the variable scopes for an AST. *

- * For JEP 394 {@code instanceof} pattern variables (GROOVY-12242), name - * resolution follows {@link InstanceofFlowBindings} (flow analysis): a pattern - * variable is only declared where the pattern has definitely matched. Bytecode - * slot visibility applies the same analysis via - * {@link org.codehaus.groovy.classgen.asm.InstanceofFlowSlotPublisher}. + * For JEP 394 {@code instanceof} pattern variables (GROOVY-12242), this + * class is the single authoritative source of scope decisions. + * The nested {@link InstanceofFlowBindings} analysis determines which pattern + * variables are definitely bound on each path; this visitor: + *

    + *
  • declares each pattern variable only in the lexical scopes where it is + * live, so name resolution outside those scopes yields a + * {@link org.codehaus.groovy.ast.DynamicVariable} (runtime + * {@link groovy.lang.MissingPropertyException} in dynamic Groovy — the + * same rule {@code @TypeChecked} enforces at compile time); and
    • + *
  • attaches {@link InstanceofPathLiveNames} metadata to {@code if}/ternary + * nodes so later phases can consume the path-live name sets without + * re-running the flow analysis.
    • + *
+ * + * @see InstanceofFlowBindings + * @see InstanceofPathLiveNames */ public class VariableScopeVisitor extends ClassCodeVisitorSupport { @@ -632,18 +651,31 @@ public void visitForLoop(final ForStatement statement) { } /** - * Visits an {@code if}/{@code else} with Java-aligned flow scoping for - * {@code instanceof} pattern variables (GROOVY-12242 / JEP 394). + * Visits an {@code if}/{@code else} statement, establishing correct + * lexical scopes for JEP 394 {@code instanceof} pattern variables + * (GROOVY-12242) and recording {@link InstanceofPathLiveNames} on the + * statement so later phases need not re-derive the flow analysis. *

- * Pattern variables that bind when the condition is {@code true} are in - * scope in the then-block; those that bind when it is {@code false} are in - * scope in the else-block. When a branch cannot complete normally, bindings - * from the opposite path remain in scope for subsequent statements (e.g. - * {@code if (!(o instanceof String s)) return; s.length()}). + * Rules applied (JLS §6.3.2.2 / JEP 394): + *

    + *
  • §6.3.2.2-200-A: {@code e.whenTrue()} are in scope in the then-block (S).
    • + *
  • §6.3.2.2-200-B: {@code e.whenFalse()} are in scope in the else-block (T).
    • + *
  • §6.3.2.2-200-C-A: if T cannot complete normally and S can, and the var + * is in {@code e.whenTrue()}, the var is introduced after the if-else.
    • + *
  • §6.3.2.2-200-C-B: if S cannot complete normally and T can (or there is + * no T), and the var is in {@code e.whenFalse()}, the var is introduced + * after the if-else.
    • + *
+ * In dynamic Groovy, undeclared references resolve to + * {@link org.codehaus.groovy.ast.DynamicVariable} (runtime + * {@link groovy.lang.MissingPropertyException}). {@code @TypeChecked} + * enforces the same rules at compile time; both modes share this visitor. */ @Override public void visitIfElse(final IfStatement statement) { InstanceofFlowBindings bindings = InstanceofFlowBindings.of(statement.getBooleanExpression()); + // Enrich the AST so classgen can adjust CompileStack without re-analysis. + statement.putNodeMetaData(InstanceofPathLiveNames.KEY, InstanceofPathLiveNames.of(bindings)); // Condition: pattern vars are available for short-circuit RHS (e.g. &&). pushState(); @@ -651,23 +683,24 @@ public void visitIfElse(final IfStatement statement) { statement.getBooleanExpression().visit(this); popState(); - // Then-block: only true-path bindings. + // Then-block: §6.3.2.2-200-A — only e.whenTrue() bindings. pushState(); declarePatternVariables(bindings.whenTrue()); statement.getIfBlock().visit(this); popState(); - // Else-block: only false-path bindings. + // Else-block: §6.3.2.2-200-B — only e.whenFalse() bindings. pushState(); declarePatternVariables(bindings.whenFalse()); statement.getElseBlock().visit(this); popState(); - // After the if: Java keeps the opposite path's bindings when a branch - // cannot complete normally (early return / throw). + // After the if-else: + // §6.3.2.2-200-C-B: if-block (S) is abrupt → e.whenFalse() survive after. if (!maybeFallsThrough(statement.getIfBlock())) { declarePatternVariables(bindings.whenFalse()); } + // §6.3.2.2-200-C-A: else-block (T) is abrupt → e.whenTrue() survive after. if (!statement.getElseBlock().isEmpty() && !maybeFallsThrough(statement.getElseBlock())) { declarePatternVariables(bindings.whenTrue()); } @@ -770,11 +803,13 @@ public void visitBinaryExpression(final BinaryExpression expression) { /** * Visits a ternary / Elvis expression with flow scoping for pattern variables: * true-path bindings are in scope in the then-branch; false-path bindings in - * the else-branch (GROOVY-12242 / JEP 394). + * the else-branch (GROOVY-12242 / JEP 394). Also records + * {@link InstanceofPathLiveNames} on the expression for later phases. */ @Override public void visitTernaryExpression(final TernaryExpression expression) { InstanceofFlowBindings bindings = InstanceofFlowBindings.of(expression.getBooleanExpression()); + expression.putNodeMetaData(InstanceofPathLiveNames.KEY, InstanceofPathLiveNames.of(bindings)); pushState(); expression.getBooleanExpression().visit(this); @@ -978,4 +1013,308 @@ public void visitVariableExpression(final VariableExpression expression) { checkVariableContextAccess(variable, expression); } } + + // ========================================================================= + // Nested class: InstanceofPathLiveNames + // ========================================================================= + + /** + * Path-live pattern-variable names for an {@code if}/ternary + * condition (GROOVY-12242 / JEP 394). + *

+ * Produced once by {@link VariableScopeVisitor} and attached as node + * metadata under {@link #KEY}. Downstream code generation reads these + * sets to decide which CompileStack names to hide on each path; it must + * not re-run the flow analysis. + * + * @since 6.0.0 + */ + @Internal + public static final class InstanceofPathLiveNames { + + /** Metadata key used with {@link ASTNode#putNodeMetaData}/{@code getNodeMetaData}. */ + public static final Object KEY = InstanceofPathLiveNames.class; + + private static final InstanceofPathLiveNames EMPTY = + new InstanceofPathLiveNames(Collections.emptySet(), Collections.emptySet()); + + private final Set whenTrue; + private final Set whenFalse; + + private InstanceofPathLiveNames(final Set whenTrue, final Set whenFalse) { + this.whenTrue = whenTrue; + this.whenFalse = whenFalse; + } + + /** Names definitely bound when the condition is {@code true}. */ + public Set whenTrue() { return whenTrue; } + + /** Names definitely bound when the condition is {@code false}. */ + public Set whenFalse() { return whenFalse; } + + /** Whether both path sets are empty. */ + public boolean isEmpty() { + return whenTrue.isEmpty() && whenFalse.isEmpty(); + } + + /** + * Builds an immutable name-set pair from a flow-analysis result. + */ + static InstanceofPathLiveNames of(final InstanceofFlowBindings bindings) { + if (bindings == null || bindings.isEmpty()) return EMPTY; + return new InstanceofPathLiveNames( + Set.copyOf(bindings.whenTrueNames()), + Set.copyOf(bindings.whenFalseNames())); + } + + /** + * Returns the metadata attached to {@code node}, or an empty instance + * if none is present (no pattern variables on either path). + */ + public static InstanceofPathLiveNames get(final ASTNode node) { + if (node == null) return EMPTY; + InstanceofPathLiveNames names = node.getNodeMetaData(KEY); + return names != null ? names : EMPTY; + } + } + + // ========================================================================= + // Nested class: InstanceofFlowBindings + // ========================================================================= + + /** + * Flow-sensitive analysis of JEP 394 {@code instanceof} pattern + * bindings (GROOVY-12242). + *

+ * Given a boolean condition, this type answers: which pattern variables are + * definitely bound when the condition is {@code true} vs + * {@code false}? It is the Groovy equivalent of the JLS §6.3.1 + * “introduced by” sets. + *

+ * Pure semantic analysis — no bytecode knowledge. Consumed only by + * {@link VariableScopeVisitor}, which declares names into scopes and + * publishes {@link InstanceofPathLiveNames} metadata for later phases. + * Code generation must not call {@link #of(Expression)} itself. + *

+ * Covered condition shapes: + * {@code e instanceof T t}, {@code e !instanceof T t}, {@code !expr}, + * {@code a && b}, {@code a || b}. All other shapes return + * {@link #EMPTY} (conservative: no bindings on either path). + * + * @see VariableScopeVisitor + * @see InstanceofPathLiveNames + * @since 6.0.0 + */ + @Internal + public static final class InstanceofFlowBindings { + + /** Singleton for "no pattern variables on either path". */ + public static final InstanceofFlowBindings EMPTY = new InstanceofFlowBindings(List.of(), List.of()); + + private final List whenTrue; + private final List whenFalse; + + private InstanceofFlowBindings(final List whenTrue, + final List whenFalse) { + this.whenTrue = whenTrue; + this.whenFalse = whenFalse; + } + + /** + * Pattern variables definitely assigned when the condition is {@code true}. + */ + public List whenTrue() { return whenTrue; } + + /** + * Pattern variables definitely assigned when the condition is {@code false}. + */ + public List whenFalse() { return whenFalse; } + + /** Whether any pattern variable is bound on either path. */ + public boolean isEmpty() { + return whenTrue.isEmpty() && whenFalse.isEmpty(); + } + + /** Names of pattern variables bound when the condition is {@code true}. */ + public Set whenTrueNames() { return names(whenTrue); } + + /** Names of pattern variables bound when the condition is {@code false}. */ + public Set whenFalseNames() { return names(whenFalse); } + + /** + * All pattern-variable names in either path (stable encounter order). + */ + public Set allNames() { + if (isEmpty()) return Collections.emptySet(); + Set names = new LinkedHashSet<>(whenTrue.size() + whenFalse.size()); + for (VariableExpression ve : whenTrue) names.add(ve.getName()); + for (VariableExpression ve : whenFalse) names.add(ve.getName()); + return names; + } + + private static Set names(final List vars) { + if (vars.isEmpty()) return Collections.emptySet(); + Set result = new LinkedHashSet<>(vars.size()); + for (VariableExpression ve : vars) result.add(ve.getName()); + return result; + } + + // ----------------------------------------------------------------- + // Factory methods + // ----------------------------------------------------------------- + + /** + * Analyses {@code expression} for definite {@code instanceof} pattern + * bindings. + * + * @param expression a boolean condition (may be a + * {@link BooleanExpression} wrapper); {@code null} + * yields {@link #EMPTY} + * @return the true/false binding sets; never {@code null} + */ + public static InstanceofFlowBindings of(final Expression expression) { + return expression == null ? EMPTY : analyse(expression); + } + + /** + * Returns {@code true} if {@code expression} contains any JEP 394 + * type-pattern node ({@code e instanceof T t} or + * {@code e !instanceof T t}), at any depth in the expression tree. + *

+ * Unlike {@link #of}, which only follows boolean-algebra operators, this + * performs a full subtree walk. Used by tests and diagnostics; classgen + * has its own structural check for expression-statement isolation. + * + * @param expression any expression; {@code null} yields {@code false} + */ + public static boolean containsPattern(final Expression expression) { + if (expression == null) return false; + boolean[] found = {false}; + expression.visit(new CodeVisitorSupport() { + @Override + public void visitBinaryExpression(final BinaryExpression be) { + if (found[0]) return; + int op = be.getOperation().getType(); + if ((op == Types.KEYWORD_INSTANCEOF || op == Types.COMPARE_NOT_INSTANCEOF) + && isTypePattern(be.getRightExpression())) { + found[0] = true; + return; + } + super.visitBinaryExpression(be); + } + }); + return found[0]; + } + + /** + * Returns the names of all pattern variables that appear + * anywhere in {@code expression}, regardless of which flow path they + * are definitely assigned on. + *

+ * Unlike {@link #allNames()} (which covers only names in the + * {@code whenTrue}/{@code whenFalse} sets), this method walks the + * entire expression tree. Useful in tests and diagnostics; code + * generation tracks allocated pattern slots via CompileStack instead. + * + * @param expression the expression to walk; {@code null} yields an + * empty set + * @return an unmodifiable set of all pattern variable names + */ + public static Set allPatternNames(final Expression expression) { + if (expression == null) return Collections.emptySet(); + Set names = new LinkedHashSet<>(); + expression.visit(new CodeVisitorSupport() { + @Override + public void visitBinaryExpression(final BinaryExpression be) { + int op = be.getOperation().getType(); + if ((op == Types.KEYWORD_INSTANCEOF || op == Types.COMPARE_NOT_INSTANCEOF) + && isTypePattern(be.getRightExpression())) { + names.add(((DeclarationExpression) be.getRightExpression()) + .getVariableExpression().getName()); + return; + } + super.visitBinaryExpression(be); + } + }); + return Collections.unmodifiableSet(names); + } + + // ----------------------------------------------------------------- + // Internal recursive descent + // ----------------------------------------------------------------- + + /** + * Recursive descent over the boolean algebra of the condition. + *

+ * Only the operators that can propagate definite-assignment are + * followed ({@code instanceof}, {@code !instanceof}, {@code !}, + * {@code &&}, {@code ||}). All other shapes return {@link #EMPTY} + * conservatively. This keeps the traversal proportional to the + * boolean structure, not to the total AST size. + */ + private static InstanceofFlowBindings analyse(final Expression expression) { + Expression expr = expression; + // Unwrap BooleanExpression wrappers; handle NotExpression explicitly + // so nested negations compose correctly. + while (expr instanceof BooleanExpression && !(expr instanceof NotExpression)) { + expr = ((BooleanExpression) expr).getExpression(); + } + if (expr instanceof NotExpression not) { + return analyse(not.getExpression()).negated(); + } + if (expr instanceof BinaryExpression binary) { + int op = binary.getOperation().getType(); + if (op == Types.KEYWORD_INSTANCEOF) { + return ofInstanceof(binary); + } + if (op == Types.COMPARE_NOT_INSTANCEOF) { + return ofInstanceof(binary).negated(); + } + if (op == Types.LOGICAL_AND) { + InstanceofFlowBindings left = analyse(binary.getLeftExpression()); + InstanceofFlowBindings right = analyse(binary.getRightExpression()); + // True only when both sides are true → union true-sets. + return new InstanceofFlowBindings(union(left.whenTrue, right.whenTrue), List.of()); + } + if (op == Types.LOGICAL_OR) { + InstanceofFlowBindings left = analyse(binary.getLeftExpression()); + InstanceofFlowBindings right = analyse(binary.getRightExpression()); + // False only when both sides are false → union false-sets. + return new InstanceofFlowBindings(List.of(), union(left.whenFalse, right.whenFalse)); + } + } + return EMPTY; + } + + private static InstanceofFlowBindings ofInstanceof(final BinaryExpression binary) { + Expression right = binary.getRightExpression(); + if (isTypePattern(right)) { + VariableExpression patternVar = + ((DeclarationExpression) right).getVariableExpression(); + return new InstanceofFlowBindings(List.of(patternVar), List.of()); + } + return EMPTY; + } + + private InstanceofFlowBindings negated() { + return isEmpty() ? this : new InstanceofFlowBindings(whenFalse, whenTrue); + } + + private static List union(final List a, + final List b) { + if (a.isEmpty()) return b; + if (b.isEmpty()) return a; + List result = new ArrayList<>(a.size() + b.size()); + Set seen = new LinkedHashSet<>(); + for (VariableExpression ve : a) if (seen.add(ve.getName())) result.add(ve); + for (VariableExpression ve : b) if (seen.add(ve.getName())) result.add(ve); + return List.copyOf(result); + } + + private static boolean isTypePattern(final Expression right) { + return right instanceof DeclarationExpression decl + && !decl.isMultipleAssignmentDeclaration() + && decl.getVariableExpression() != null; + } + } } diff --git a/src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java b/src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java index 951168493e0..269edbac328 100644 --- a/src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java +++ b/src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java @@ -47,13 +47,17 @@ import org.codehaus.groovy.ast.tools.WideningCategories; import org.codehaus.groovy.classgen.AsmClassGenerator; import org.codehaus.groovy.classgen.BytecodeExpression; -import org.codehaus.groovy.classgen.InstanceofFlowBindings; +import org.codehaus.groovy.classgen.VariableScopeVisitor.InstanceofPathLiveNames; import org.codehaus.groovy.runtime.MultipleAssignmentSupport; import org.codehaus.groovy.runtime.ScriptBytecodeAdapter; import org.codehaus.groovy.syntax.Token; import org.objectweb.asm.Label; import org.objectweb.asm.MethodVisitor; +import java.util.Collections; +import java.util.Map; +import java.util.Set; + import static org.apache.groovy.ast.tools.ExpressionUtils.isNullConstant; import static org.codehaus.groovy.ast.tools.GeneralUtils.args; import static org.codehaus.groovy.ast.tools.GeneralUtils.binX; @@ -1135,12 +1139,12 @@ private void evaluateRightHandSide(final ClassNode lhsType, final Expression rig * a JEP 394 type pattern ({@code e instanceof T t}), conditionally stores * the checked value into the pattern variable {@code t}. *

- * The pattern variable's visibility is governed by flow scoping in - * {@link org.codehaus.groovy.classgen.VariableScopeVisitor} and - * {@link StatementWriter#writeIfElse}; this method only performs the store. + * The slot is allocated and {@linkplain CompileStack#recordPatternVariable + * recorded} immediately so a short-circuit {@code &&} RHS can reference + * {@code t}. Path-level name visibility after the condition is administered + * by CompileStack hide/push/pop (see {@link StatementWriter#writeIfElse}). * * @param expression an {@code instanceof} binary expression - * @see org.codehaus.groovy.classgen.InstanceofFlowBindings */ private void evaluateInstanceof(final BinaryExpression expression) { CompileStack compileStack = controller.getCompileStack(); @@ -1163,6 +1167,7 @@ private void evaluateInstanceof(final BinaryExpression expression) { if (patternMatch) { var variable = (Variable) ((BinaryExpression) expression.getRightExpression()).getLeftExpression(); BytecodeVariable v = compileStack.defineVariable(variable, targetType, false); + compileStack.recordPatternVariable(v); MethodVisitor mv = controller.getMethodVisitor(); mv.visitInsn(DUP_X1); // stack: ..., check, value, check @@ -1476,26 +1481,32 @@ private void evaluateTernaryExpression(final TernaryExpression expression) { OperandStack operandStack = controller.getOperandStack(); MethodVisitor mv = controller.getMethodVisitor(); - // load x; hide pattern locals then publish only on the live arm (GROOVY-12242) + // load x; path-hide pattern locals via CompileStack push/hide/pop (GROOVY-12242) + InstanceofPathLiveNames pathNames = InstanceofPathLiveNames.get(expression); + Map beforePatterns = compileStack.snapshotPatternVariables(); boolPart.visit(controller.getAcg()); - InstanceofFlowSlotPublisher slotPublisher = InstanceofFlowSlotPublisher.captureAndHide( - compileStack, InstanceofFlowBindings.of(expression.getBooleanExpression())); + Set introduced = compileStack.patternVariablesIntroducedSince(beforePatterns); Label l0 = operandStack.jump(IFEQ); - // true path: load y and cast to T - slotPublisher.publishTrue(compileStack); + // true path: only whenTrue names visible among those this condition introduced + compileStack.pushState(); + compileStack.hidePatternVariablesExcept(introduced, pathNames.whenTrue()); truePart.visit(controller.getAcg()); operandStack.doGroovyCast(commonType); - slotPublisher.hideTrue(compileStack); + compileStack.pop(); Label l1 = new Label(); mv.visitJumpInsn(GOTO, l1); - // false path: load z and cast to T + // false path: only whenFalse names visible mv.visitLabel(l0); - slotPublisher.publishFalse(compileStack); + compileStack.pushState(); + compileStack.hidePatternVariablesExcept(introduced, pathNames.whenFalse()); falsePart.visit(controller.getAcg()); operandStack.doGroovyCast(commonType); - slotPublisher.hideFalse(compileStack); + compileStack.pop(); + + // After ternary, names introduced by this condition leave scope. + compileStack.hidePatternVariablesExcept(introduced, Collections.emptySet()); // finish up mv.visitLabel(l1); diff --git a/src/main/java/org/codehaus/groovy/classgen/asm/CompileStack.java b/src/main/java/org/codehaus/groovy/classgen/asm/CompileStack.java index 5854e07ac06..c708a287c0b 100644 --- a/src/main/java/org/codehaus/groovy/classgen/asm/CompileStack.java +++ b/src/main/java/org/codehaus/groovy/classgen/asm/CompileStack.java @@ -36,9 +36,11 @@ import java.util.Collections; import java.util.Deque; import java.util.HashMap; +import java.util.HashSet; import java.util.LinkedList; import java.util.List; import java.util.Map; +import java.util.Set; import static org.objectweb.asm.Opcodes.ACONST_NULL; import static org.objectweb.asm.Opcodes.ASTORE; @@ -101,6 +103,14 @@ public class CompileStack { private final Deque temporaryVariables = new LinkedList<>(); /** overall used variables for a method/constructor */ private final Deque usedVariables = new LinkedList<>(); + /** + * Pattern variables defined by {@code instanceof} type patterns in this method. + * Keys are names; values are the slots allocated by {@code evaluateInstanceof}. + * The name→slot mapping in {@link #stackVariables} may be hidden/restored via + * {@link #hideVariable(String)} and {@link #pushState()}/{@link #pop()}, but + * the slot indices recorded here remain valid for the whole method. + */ + private final Map patternVariables = new HashMap<>(); /** map containing named labels of parenting blocks */ private Map superBlockNamedLabels = new HashMap<>(); /** map containing named labels of current block */ @@ -476,6 +486,7 @@ public void clear() { untypedExceptions.clear(); stackVariables.clear(); usedVariables.clear(); + patternVariables.clear(); finallyBlocks.clear(); resetVariableIndex(false); superBlockNamedLabels.clear(); @@ -909,28 +920,105 @@ public boolean containsVariable(final String name) { } /** - * Re-publishes a previously defined local into the current state. - * Used by {@link InstanceofFlowSlotPublisher} for path-scoped pattern variables - * (GROOVY-12242 / JEP 394). + * Records that {@code variable} was allocated as an {@code instanceof} pattern + * binding. The slot is already present in {@link #stackVariables}; this registry + * lets later control-flow boundaries hide names while keeping the slot indices. + *

+ * GROOVY-12242 / JEP 394: pattern slots are defined during condition evaluation + * (needed for short-circuit {@code &&} RHS). After the condition, names that are + * not live on a path must be hidden so (1) a same-named local can be declared + * on that path and (2) name lookup does not load a slot that scoping forbids. + *

+ * Re-recording the same name replaces the previous registry entry (a later + * condition may re-bind the name to a new slot). * - * @param variable the bytecode variable to make visible again + * @param variable the pattern local just defined; ignored if {@code null} + * @see #hideVariable(String) + * @see #hidePatternVariablesExcept(Collection, Collection) */ - public void putVariable(final BytecodeVariable variable) { + public void recordPatternVariable(final BytecodeVariable variable) { if (variable != null) { - stackVariables.put(variable.getName(), variable); + patternVariables.put(variable.getName(), variable); } } /** - * Removes a named local from the current state without affecting temporary - * variables or the free-register cursor. Used by {@link InstanceofFlowSlotPublisher} - * to hide pattern slots that are not live on the current control-flow path. + * Hides a named variable from name resolution while leaving its slot index + * allocated. The name becomes free in the current state frame only. + *

+ * Call this inside a {@link #pushState()}/{@link #pop()} region so that + * {@code pop} restores the previous name map (and thus re-exposes the variable + * if it was visible in the outer frame). Using hide without a matching push + * permanently removes the name from the current frame — the intended form for + * “after this construct the name is no longer in scope”. + *

+ * This is the CompileStack-native mechanism for flow-scoped pattern variables + * (GROOVY-12242). It deliberately does not free the register: the value may + * still be needed on another path that re-exposes the same slot via pop or a + * later path that still lists the name as live. + * + * @param name the variable name to hide; no-op if absent + */ + public void hideVariable(final String name) { + stackVariables.remove(name); + } + + /** + * Snapshot of the pattern-variable registry (name → slot). Used to compute + * which names a condition evaluation introduced or re-bound by + * comparing {@link BytecodeVariable} identity before and after the visit. + * A name-only set is insufficient when a later condition reuses a pattern name. * - * @param name the variable name to remove - * @return the removed variable, or {@code null} if it was not present + * @return an immutable copy of the registry; empty if none recorded */ - public BytecodeVariable removeVariable(final String name) { - return stackVariables.remove(name); + public Map snapshotPatternVariables() { + if (patternVariables.isEmpty()) return Collections.emptyMap(); + return Map.copyOf(patternVariables); + } + + /** + * Names whose registry entry is new or whose {@link BytecodeVariable} identity + * differs from {@code before} (re-bind of the same name). + * + * @param before snapshot from {@link #snapshotPatternVariables()} taken before + * evaluating the condition; may be empty + * @return names introduced or re-bound by the condition that just ran + */ + public Set patternVariablesIntroducedSince(final Map before) { + if (patternVariables.isEmpty()) return Collections.emptySet(); + if (before == null || before.isEmpty()) { + return Set.copyOf(patternVariables.keySet()); + } + Set introduced = new HashSet<>(); + for (Map.Entry e : patternVariables.entrySet()) { + if (before.get(e.getKey()) != e.getValue()) { + introduced.add(e.getKey()); + } + } + return introduced; + } + + /** + * Among {@code candidates}, hides every name that is not in {@code liveNames}. + * Only names that are both candidates and recorded pattern variables are affected; + * outer pattern variables not listed in {@code candidates} stay untouched. + *

+ * Must be used with {@link #pushState()}/{@link #pop()} (or as a permanent hide + * on the current frame) as described by {@link #hideVariable(String)}. + * + * @param candidates names introduced by the current condition; may be empty + * @param liveNames names that must remain visible; may be empty or {@code null} + * (both mean hide every candidate) + */ + public void hidePatternVariablesExcept(final Collection candidates, + final Collection liveNames) { + if (candidates == null || candidates.isEmpty()) return; + for (String name : candidates) { + if (!patternVariables.containsKey(name)) continue; + if (liveNames == null || !liveNames.contains(name)) { + hideVariable(name); + } + } } /** diff --git a/src/main/java/org/codehaus/groovy/classgen/asm/InstanceofFlowSlotPublisher.java b/src/main/java/org/codehaus/groovy/classgen/asm/InstanceofFlowSlotPublisher.java deleted file mode 100644 index f2f875227f7..00000000000 --- a/src/main/java/org/codehaus/groovy/classgen/asm/InstanceofFlowSlotPublisher.java +++ /dev/null @@ -1,155 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -package org.codehaus.groovy.classgen.asm; - -import org.codehaus.groovy.classgen.InstanceofFlowBindings; - -import java.util.Collections; -import java.util.HashMap; -import java.util.Map; -import java.util.Set; - -/** - * Codegen helper that publishes/hides {@link CompileStack} local slots - * for {@code instanceof} pattern variables according to - * {@link InstanceofFlowBindings} (GROOVY-12242 / JEP 394). - *

- * Distinct from {@link InstanceofFlowBindings}: that type is AST flow - * analysis; this type is bytecode slot control. - *

- * {@code evaluateInstanceof} defines pattern slots while the condition runs - * (needed for short-circuit {@code &&} RHS). After the condition, this helper - * hides every captured slot and then publishes only names - * live on the current control-flow path: - *

    - *
  • then-block → {@link InstanceofFlowBindings#whenTrueNames()}
    • - *
  • else-block → {@link InstanceofFlowBindings#whenFalseNames()}
    • - *
  • after the if → opposite path when a branch cannot complete normally
    • - *
- * That “hide all, publish path” rule keeps CompileStack polarity aligned with - * {@link org.codehaus.groovy.classgen.VariableScopeVisitor}. - * - * @see InstanceofFlowBindings - * @since 6.0.0 - */ -final class InstanceofFlowSlotPublisher { - - private static final InstanceofFlowSlotPublisher NONE = - new InstanceofFlowSlotPublisher(InstanceofFlowBindings.of(null), Map.of()); - - private final InstanceofFlowBindings bindings; - private final Map captured; - - private InstanceofFlowSlotPublisher(final InstanceofFlowBindings bindings, - final Map captured) { - this.bindings = bindings; - this.captured = captured; - } - - /** - * Snapshots pattern slots defined while evaluating {@code bindings}' condition - * and removes them from {@code compileStack} so no branch sees unscoped slots. - * - * @param compileStack current compile stack (condition already evaluated) - * @param bindings flow-analysis result for that condition - * @return a publisher for path-scoped reintroduction of the captured slots - */ - static InstanceofFlowSlotPublisher captureAndHide(final CompileStack compileStack, - final InstanceofFlowBindings bindings) { - if (bindings == null || bindings.isEmpty()) { - return NONE; - } - Map captured = new HashMap<>(); - for (String name : bindings.allNames()) { - BytecodeVariable bv = compileStack.getVariable(name, false); - if (bv != null) { - captured.put(name, bv); - compileStack.removeVariable(name); - } - } - if (captured.isEmpty()) { - return NONE; - } - return new InstanceofFlowSlotPublisher(bindings, Collections.unmodifiableMap(captured)); - } - - boolean isEmpty() { - return captured.isEmpty(); - } - - /** Makes true-path pattern locals visible on the current CompileStack frame. */ - void publishTrue(final CompileStack compileStack) { - publish(compileStack, bindings.whenTrueNames()); - } - - /** Makes false-path pattern locals visible on the current CompileStack frame. */ - void publishFalse(final CompileStack compileStack) { - publish(compileStack, bindings.whenFalseNames()); - } - - /** Hides true-path pattern locals (end of then-block). */ - void hideTrue(final CompileStack compileStack) { - hide(compileStack, bindings.whenTrueNames()); - } - - /** Hides false-path pattern locals (end of else-block). */ - void hideFalse(final CompileStack compileStack) { - hide(compileStack, bindings.whenFalseNames()); - } - - /** - * Publishes bindings that remain in scope after the if, matching Java's - * abrupt-completion rule: opposite-path bindings survive when a branch - * cannot complete normally. - * - * @param ifFallsThrough whether the then-block may complete normally - * @param elseEmpty whether there is no else branch - * @param elseFallsThrough whether the else-block may complete normally - */ - void publishAfterIf(final CompileStack compileStack, - final boolean ifFallsThrough, - final boolean elseEmpty, - final boolean elseFallsThrough) { - if (!ifFallsThrough) { - publishFalse(compileStack); - } - if (!elseEmpty && !elseFallsThrough) { - publishTrue(compileStack); - } - } - - private void publish(final CompileStack compileStack, final Set names) { - if (captured.isEmpty() || names.isEmpty()) return; - for (String name : names) { - BytecodeVariable bv = captured.get(name); - if (bv != null) { - compileStack.putVariable(bv); - } - } - } - - private void hide(final CompileStack compileStack, final Set names) { - if (captured.isEmpty() || names.isEmpty()) return; - for (String name : names) { - if (captured.containsKey(name)) { - compileStack.removeVariable(name); - } - } - } -} diff --git a/src/main/java/org/codehaus/groovy/classgen/asm/StatementWriter.java b/src/main/java/org/codehaus/groovy/classgen/asm/StatementWriter.java index f5af5e83cc9..9378122585d 100644 --- a/src/main/java/org/codehaus/groovy/classgen/asm/StatementWriter.java +++ b/src/main/java/org/codehaus/groovy/classgen/asm/StatementWriter.java @@ -45,15 +45,20 @@ import org.codehaus.groovy.ast.stmt.ThrowStatement; import org.codehaus.groovy.ast.stmt.TryCatchStatement; import org.codehaus.groovy.ast.stmt.WhileStatement; +import org.codehaus.groovy.ast.CodeVisitorSupport; import org.codehaus.groovy.classgen.AsmClassGenerator; -import org.codehaus.groovy.classgen.InstanceofFlowBindings; +import org.codehaus.groovy.classgen.VariableScopeVisitor.InstanceofPathLiveNames; import org.codehaus.groovy.classgen.asm.CompileStack.BlockRecorder; +import org.codehaus.groovy.syntax.Types; import org.objectweb.asm.Label; import org.objectweb.asm.MethodVisitor; +import java.util.HashSet; import java.util.Iterator; import java.util.List; +import java.util.Map; import java.util.Optional; +import java.util.Set; import java.util.function.Consumer; import static org.apache.groovy.ast.tools.ExpressionUtils.isNullConstant; @@ -473,10 +478,14 @@ public void writeDoWhileLoop(final DoWhileStatement statement) { /** * Generates bytecode for an if/else statement. *

- * GROOVY-12242 / JEP 394: after the condition runs, pattern locals are hidden - * then re-published only on the live path (then / else / after abrupt - * completion) via {@link InstanceofFlowSlotPublisher}, matching - * {@link org.codehaus.groovy.classgen.VariableScopeVisitor}. + * GROOVY-12242 / JEP 394: {@code evaluateInstanceof} allocates pattern slots + * during the condition (needed for short-circuit {@code &&} RHS). After the + * condition, CompileStack hides names that are not live on the current path + * using {@link CompileStack#hidePatternVariablesExcept} inside + * {@link CompileStack#pushState}/{@link CompileStack#pop} frames. Path-live + * name sets come from {@link InstanceofPathLiveNames} metadata written by + * {@link org.codehaus.groovy.classgen.VariableScopeVisitor} — classgen does + * not re-run the flow analysis. * * @param statement the if statement to compile */ @@ -485,23 +494,28 @@ public void writeIfElse(final IfStatement statement) { writeStatementLabel(statement); CompileStack compileStack = controller.getCompileStack(); - InstanceofFlowBindings bindings = InstanceofFlowBindings.of(statement.getBooleanExpression()); + InstanceofPathLiveNames pathNames = InstanceofPathLiveNames.get(statement); - Label exitPath = compileStack.pushBreakable(statement.getStatementLabels()); // GROOVY-7463 + // Define pattern slots on the *outer* frame so that after the breakable + // push/pop the survivors remain ordinary stackVariables entries (no + // put-back / show API). GROOVY-7463 breakable still wraps the arms only. + Map beforePatterns = compileStack.snapshotPatternVariables(); statement.getBooleanExpression().visit(controller.getAcg()); - // Hide every pattern slot; publish only path-live bindings below. - InstanceofFlowSlotPublisher slotPublisher = InstanceofFlowSlotPublisher.captureAndHide(compileStack, bindings); + Set introduced = compileStack.patternVariablesIntroducedSince(beforePatterns); - Label elsePath = controller.getOperandStack().jump(IFEQ); - slotPublisher.publishTrue(compileStack); - statement.getIfBlock().visit(controller.getAcg()); - slotPublisher.hideTrue(compileStack); - compileStack.pop(); // ends breakable + Label exitPath = compileStack.pushBreakable(statement.getStatementLabels()); boolean ifFallsThrough = maybeFallsThrough(statement.getIfBlock()); boolean elseEmpty = statement.getElseBlock().isEmpty(); boolean elseFallsThrough = elseEmpty || maybeFallsThrough(statement.getElseBlock()); + // Then path: only whenTrue names among those this condition introduced. + Label elsePath = controller.getOperandStack().jump(IFEQ); + compileStack.pushState(); + compileStack.hidePatternVariablesExcept(introduced, pathNames.whenTrue()); + statement.getIfBlock().visit(controller.getAcg()); + compileStack.pop(); + MethodVisitor mv = controller.getMethodVisitor(); if (elseEmpty) { mv.visitLabel(elsePath); @@ -510,12 +524,28 @@ public void writeIfElse(final IfStatement statement) { mv.visitJumpInsn(GOTO, exitPath); } mv.visitLabel(elsePath); - slotPublisher.publishFalse(compileStack); + // Else path: only whenFalse names among those this condition introduced. + compileStack.pushState(); + compileStack.hidePatternVariablesExcept(introduced, pathNames.whenFalse()); statement.getElseBlock().visit(controller.getAcg()); - slotPublisher.hideFalse(compileStack); + compileStack.pop(); } - slotPublisher.publishAfterIf(compileStack, ifFallsThrough, elseEmpty, elseFallsThrough); + // Survivors per JLS §6.3.2.2-200-C (abrupt-completion rule). + Set survivors = new HashSet<>(); + if (!ifFallsThrough) { + survivors.addAll(pathNames.whenFalse()); + } + if (!elseEmpty && !elseFallsThrough) { + survivors.addAll(pathNames.whenTrue()); + } + survivors.retainAll(introduced); + + // Leave the breakable frame: outer map still holds all pattern slots from + // the condition (they were defined before pushBreakable). Permanently hide + // non-survivors on the outer frame. + compileStack.pop(); // ends breakable + compileStack.hidePatternVariablesExcept(introduced, survivors); mv.visitLabel(exitPath); } @@ -908,8 +938,8 @@ public void writeReturn(final ReturnStatement statement) { * are elided rather than boxed. *

* GROOVY-12242: non-declaration statements that contain an {@code instanceof} - * type pattern run in a nested CompileStack state so pattern locals cannot - * leak. Detection uses {@link InstanceofFlowBindings#containsPattern}. + * type pattern run in a nested CompileStack state ({@code pushState}/{@code pop}) + * so pattern locals defined during evaluation cannot leak past the statement. * * @param statement the expression statement to compile */ @@ -925,7 +955,7 @@ public void writeExpressionStatement(final ExpressionStatement statement) { CompileStack compileStack = controller.getCompileStack(); // Declaration LHS isolation is in evaluateEqual; multi-assign must not be wrapped. boolean isolatesPatternVars = !(expression instanceof DeclarationExpression) - && InstanceofFlowBindings.containsPattern(expression); + && containsTypePattern(expression); if (isolatesPatternVars) { compileStack.pushState(); } @@ -940,4 +970,30 @@ public void writeExpressionStatement(final ExpressionStatement statement) { } } } + + /** + * Structural check: does {@code expression} contain any JEP 394 type + * pattern ({@code e instanceof T t})? Not a flow analysis — used only to + * decide whether an expression statement needs CompileStack isolation. + */ + private static boolean containsTypePattern(final Expression expression) { + if (expression == null) return false; + boolean[] found = {false}; + expression.visit(new CodeVisitorSupport() { + @Override + public void visitBinaryExpression(final BinaryExpression be) { + if (found[0]) return; + int op = be.getOperation().getType(); + if ((op == Types.KEYWORD_INSTANCEOF || op == Types.COMPARE_NOT_INSTANCEOF) + && be.getRightExpression() instanceof DeclarationExpression decl + && !decl.isMultipleAssignmentDeclaration() + && decl.getVariableExpression() != null) { + found[0] = true; + return; + } + super.visitBinaryExpression(be); + } + }); + return found[0]; + } } diff --git a/src/test/groovy/groovy/InstanceofFlowBindingsTest.groovy b/src/test/groovy/groovy/InstanceofFlowBindingsTest.groovy index 85b969c491e..2645ab4086e 100644 --- a/src/test/groovy/groovy/InstanceofFlowBindingsTest.groovy +++ b/src/test/groovy/groovy/InstanceofFlowBindingsTest.groovy @@ -22,7 +22,7 @@ import org.codehaus.groovy.ast.DynamicVariable import org.codehaus.groovy.ast.expr.Expression import org.codehaus.groovy.ast.expr.VariableExpression import org.codehaus.groovy.ast.stmt.IfStatement -import org.codehaus.groovy.classgen.InstanceofFlowBindings +import org.codehaus.groovy.classgen.VariableScopeVisitor.InstanceofFlowBindings import org.codehaus.groovy.control.CompilationUnit import org.codehaus.groovy.control.Phases import org.codehaus.groovy.ast.CodeVisitorSupport @@ -95,6 +95,54 @@ final class InstanceofFlowBindingsTest { assert InstanceofFlowBindings.containsPattern(expr) } + // allPatternNames() returns ALL pattern variable names in the expression tree, + // regardless of which flow path (whenTrue / whenFalse) they appear on. + // This differs from allNames() which only covers names in the binding sets. + @Test + void testAllPatternNames_positiveInstanceof() { + // simple instanceof: appears in both allNames() and allPatternNames() + def b = InstanceofFlowBindings.of(parseCondition('o instanceof String s')) + def expr = parseCondition('o instanceof String s') + assert InstanceofFlowBindings.allPatternNames(expr) == ['s'] as Set + } + + @Test + void testAllPatternNames_orCondition_returnsNameEvenWhenBindingsEmpty() { + // o instanceof String s || true: bindings EMPTY (no whenTrue, no whenFalse), + // so allNames() returns {}. But allPatternNames() still returns {s} because + // evaluateInstanceof allocates the slot regardless. + def expr = parseCondition('o instanceof String s || true') + def b = InstanceofFlowBindings.of(expr) + assert b.isEmpty() : 'bindings are empty for || shape' + assert b.allNames().isEmpty() : 'allNames() returns {} for || shape' + assert InstanceofFlowBindings.allPatternNames(expr) == ['s'] as Set : 'allPatternNames() returns {s}' + } + + @Test + void testAllPatternNames_andCondition_sameAsAllNames() { + // o instanceof String s && cond: whenTrue={s}, allNames()={s}, allPatternNames()={s} + def expr = parseCondition('o instanceof String s && s.length() > 0') + def b = InstanceofFlowBindings.of(expr) + assert InstanceofFlowBindings.allPatternNames(expr) == b.allNames() + } + + @Test + void testAllPatternNames_nullReturnsEmpty() { + assert InstanceofFlowBindings.allPatternNames(null).isEmpty() + } + + @Test + void testAllPatternNames_noPattern_returnsEmpty() { + def expr = parseCondition('o instanceof String') // no pattern variable + assert InstanceofFlowBindings.allPatternNames(expr).isEmpty() + } + + @Test + void testAllPatternNames_twoPatternsTwoNames() { + def expr = parseCondition2('o instanceof String s && p instanceof Integer i') + assert InstanceofFlowBindings.allPatternNames(expr) == ['s', 'i'] as Set + } + @Test void testRightOfOrIsDynamicVariable() { def src = ''' @@ -154,21 +202,22 @@ final class InstanceofFlowBindingsTest { } // ------------------------------------------------------------------------- - // GROOVY-12242: systematic binding-analysis unit tests + // GROOVY-12242: systematic binding-analysis unit tests aligned with JLS §6.3.1 // - // Covers every condition shape in the visibility matrix: - // (1) instanceof s - // (2) !instanceof s - // (3) !instanceof s (negated via BooleanExpression) - // (4) instanceof s && cond - // (5) instanceof s || cond - // (6) !instanceof s && cond - // (7) !instanceof s || cond - // (8) !(instanceof s && cond) - // (9) double negation !!(instanceof s) + // JLS §6.3.1 defines flow scoping for pattern variables in expressions. + // The following tests map directly to each sub-section: + // + // §6.3.1.5 (instanceof): e instanceof T t → whenTrue: {t}, whenFalse: {} (no rule) + // §6.3.1.3 (!): !a → whenTrue = a.whenFalse, whenFalse = a.whenTrue + // §6.3.1.1 (&&): a && b → whenTrue = a.whenTrue ∪ b.whenTrue, whenFalse = {} (no rule) + // §6.3.1.2 (||): a || b → whenFalse = a.whenFalse ∪ b.whenFalse, whenTrue = {} (no rule) + // §6.3.1.7 (parens): (a) → same as a (transparent) + // §6.3.1.4 (?:): a ? b : c → no whenTrue / whenFalse bindings (conservative) + // §6.3.1.1-200-A error: same name in a.whenTrue and b.whenTrue of && → compile-time error // ------------------------------------------------------------------------- - // (4) o instanceof String s && cond — true: {s}, false: {} + // JLS §6.3.1.1 Rule B: a&&b when-true = a.whenTrue ∪ b.whenTrue = {s} ∪ {} = {s} + // JLS §6.3.1.1 (note): no rule for when-false of &&. @Test void testAndWithPattern_trueBinds_falseEmpty() { def b = InstanceofFlowBindings.of(parseCondition('o instanceof String s && true')) @@ -176,28 +225,28 @@ final class InstanceofFlowBindingsTest { assert b.whenFalse().isEmpty() } - // (5) o instanceof String s || cond — both paths empty (can't guarantee s on true path, - // and cond alone doesn't bind s on the false path) + // JLS §6.3.1.2 (note): no rule for when-true of ||. + // JLS §6.3.1.2 Rule B: a||b when-false = a.whenFalse ∪ b.whenFalse. + // For (o instanceof String s): whenFalse = {} (§6.3.1.5: no when-false for instanceof). + // For true: whenFalse = {}. So a||b when-false = {} ∪ {} = {}. Both paths empty. @Test void testOrWithPattern_bothEmpty() { def b = InstanceofFlowBindings.of(parseCondition('o instanceof String s || true')) assert b.isEmpty() } - // (6) !(o instanceof String s) && cond - // Left's false path binds s, but && propagates no false-bindings. - // Left's true path is empty. Right contributes nothing. - // → true: {}, false: {} + // JLS §6.3.1.3: !(o instanceof String s) when-true = a.whenFalse = {} (§6.3.1.5 no when-false) + // JLS §6.3.1.1 Rule B: (left.whenTrue={}) && (right.whenTrue={}) → whenTrue = {} + // JLS §6.3.1.1 (note): no rule for when-false of &&. → whenFalse = {} @Test void testNegatedAndCond_bothEmpty() { def b = InstanceofFlowBindings.of(parseCondition('!(o instanceof String s) && true')) assert b.isEmpty() } - // (7) !(o instanceof String s) || cond - // Left false-path binds s (since !(s) is false → s matched). - // Right's false path is empty. || false-path = union of false-paths = {s}. - // → true: {}, false: {s} + // JLS §6.3.1.3: !(o instanceof String s) when-false = a.whenTrue = {s} + // JLS §6.3.1.2 Rule B: a||b when-false = a.whenFalse ∪ b.whenFalse = {s} ∪ {} = {s} + // JLS §6.3.1.2 (note): no rule for when-true of ||. → whenTrue = {} @Test void testNegatedOrCond_falseBinds() { def b = InstanceofFlowBindings.of(parseCondition('!(o instanceof String s) || true')) @@ -205,9 +254,8 @@ final class InstanceofFlowBindingsTest { assert b.whenTrue().isEmpty() } - // (8) !(o instanceof String s && s.length() > 0) - // Inner: true:{s}, false:{}. Negated: true:{}, false:{s}. - // → true: {}, false: {s} + // JLS §6.3.1.1 Rule B: (a&&b) when-true = {s}; negated → when-false = {s}, when-true = {} + // (§6.3.1.3: !expr when-true = expr.whenFalse; !expr when-false = expr.whenTrue) @Test void testNegatedAndPattern_falseBindsAfterNegation() { def b = InstanceofFlowBindings.of(parseCondition('!(o instanceof String s && s.length() > 0)')) @@ -215,9 +263,10 @@ final class InstanceofFlowBindingsTest { assert b.whenTrue().isEmpty() } - // (9) double negation !!(o instanceof String s) ≡ o instanceof String s - // Inner: true:{s}, false:{}. Negated once: true:{}, false:{s}. - // Negated twice: true:{s}, false:{}. + // JLS §6.3.1.3 applied twice (double negation identity): + // o instanceof String s: whenTrue={s}, whenFalse={} + // !(…): whenTrue={}, whenFalse={s} + // !!(…): whenTrue={s}, whenFalse={} — same as the original (§6.3.1.3 is its own inverse) @Test void testDoubleNegation_sameAsPositive() { def b = InstanceofFlowBindings.of(parseCondition('!!(o instanceof String s)')) @@ -225,6 +274,45 @@ final class InstanceofFlowBindingsTest { assert b.whenFalse().isEmpty() } + // JLS §6.3.1.5: a instanceof T t introduces t when true; NO binding when false. + // This is the base axiom — everything else is derived from it. + @Test + void testJLS_6_3_1_5_noWhenFalseForInstanceof() { + // Positive instanceof: whenTrue binds, whenFalse is explicitly empty + def pos = InstanceofFlowBindings.of(parseCondition('o instanceof String s')) + assert pos.whenTrue()*.name == ['s'] : 'JLS §6.3.1.5-100-A: s introduced when true' + assert pos.whenFalse().isEmpty() : 'JLS §6.3.1.5 (note): no rule for when false' + + // Plain instanceof without pattern variable: contributes nothing + def plain = InstanceofFlowBindings.of(parseCondition('o instanceof String')) + assert plain.isEmpty() : 'no type pattern means no binding' + } + + // JLS §6.3.1.7: parenthesized expressions are transparent. + // (a instanceof T t) has exactly the same bindings as a instanceof T t. + @Test + void testJLS_6_3_1_7_parenthesizedExpression_transparent() { + // The Groovy AST wraps the condition in a BooleanExpression; unwrapping happens in analyse(). + // A user-written (o instanceof String s) adds no additional wrapper beyond what the + // if-condition already imposes, so the result must equal the unwrapped case. + def wrapped = InstanceofFlowBindings.of(parseCondition('(o instanceof String s)')) + assert wrapped.whenTrue()*.name == ['s'] : 'JLS §6.3.1.7-100-A: parens transparent for whenTrue' + assert wrapped.whenFalse().isEmpty() : 'JLS §6.3.1.7-100-B: parens transparent for whenFalse' + } + + // JLS §6.3.1.4: conditional operator a ? b : c — no whenTrue/whenFalse bindings. + // "It cannot be determined at compile time whether a will evaluate to true." + // InstanceofFlowBindings.analyse() returns EMPTY conservatively for this shape + // (it is not a boolean-algebra operator that propagates definite-assignment). + @Test + void testJLS_6_3_1_4_ternaryConditional_noBindings() { + // The condition `o instanceof String s ? true : false` cannot propagate + // the binding of s beyond the ternary — no scope rule exists for ?: + // (§6.3.1.4 note). analyse() sees a non-recognised expression shape → EMPTY. + def b = InstanceofFlowBindings.of(parseCondition('o instanceof String s ? true : false')) + assert b.isEmpty() : 'JLS §6.3.1.4 (note): no whenTrue/false rule for ?:' + } + // (4) o instanceof String s && p instanceof Integer i (JLS §6.3.1.1) // Rule B: a&&b when-true = a.whenTrue ∪ b.whenTrue = {s} ∪ {i} = {s, i} // Rule A: s (introduced by a when true) is definitely matched at b — legal, diff --git a/src/test/groovy/groovy/InstanceofScopeTest.groovy b/src/test/groovy/groovy/InstanceofScopeTest.groovy new file mode 100644 index 00000000000..f9a97eeb3b9 --- /dev/null +++ b/src/test/groovy/groovy/InstanceofScopeTest.groovy @@ -0,0 +1,705 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package groovy + +import org.codehaus.groovy.ast.CodeVisitorSupport +import org.codehaus.groovy.ast.DynamicVariable +import org.codehaus.groovy.ast.Variable +import org.codehaus.groovy.ast.expr.VariableExpression +import org.codehaus.groovy.control.CompilationUnit +import org.codehaus.groovy.control.Phases +import org.junit.jupiter.api.Test + +/** + * AST-level scope tests for JEP 394 {@code instanceof} pattern variables + * (GROOVY-12242), verifying that {@link org.codehaus.groovy.classgen.VariableScopeVisitor} + * correctly scopes each pattern variable for every condition shape in the + * visibility matrix. + *

+ * Each test compiles to {@link Phases#SEMANTIC_ANALYSIS} (which runs + * {@code VariableScopeVisitor}) and then inspects + * {@link VariableExpression#getAccessedVariable()} to verify that: + *

    + *
  • References inside the scope resolve to the pattern + * {@link DeclarationExpression}'s {@link VariableExpression} (i.e. the + * declared local, not a dynamic lookup).
    • + *
  • References outside the scope resolve to a + * {@link DynamicVariable} — which at runtime produces a + * {@link MissingPropertyException}, enforcing the JLS §6.3 rule in + * dynamic Groovy without any need for {@code @TypeChecked}.
    • + *
+ * + *

Visibility matrix (JLS §6.3.2.2 / JEP 394)

+ * 
+ *  #  | Condition shape                       | if-block | else-block | after if-else
+ *  ---|--------------------------------------|----------|------------|---------------
+ *  1  | o instanceof String s                 | local    | dynamic    | dynamic (*)
+ *  1b | o instanceof String s, else abrupt    | local    | (abrupt)   | local
+ *  2  | !(o instanceof String s)              | dynamic  | local      | —
+ *  3  | !(o instanceof String s), if abrupt   | (abrupt) | local      | local
+ *  4  | o instanceof String s && cond         | local    | dynamic    | dynamic
+ *  5  | o instanceof String s || cond         | dynamic  | dynamic    | dynamic
+ *  6  | !(o instanceof String s) && cond      | dynamic  | —          | dynamic
+ *  7a | !(o instanceof s) && cond, else abrupt| dynamic  | (abrupt)   | dynamic
+ *  7b | !(o instanceof s), abrupt else only   | dynamic  | (abrupt)   | dynamic (**)
+ *  8  | !(o instanceof String s) || cond      | —        | local      | —
+ *
+ * (*) s NOT visible after when both branches fall through. + * (**) missing case: abrupt else alone is not enough; if-block must + * also be abrupt for §6.3.2.2-200-C-B to apply. + */ +final class InstanceofScopeTest { + + // ------------------------------------------------------------------------- + // Helpers + // ------------------------------------------------------------------------- + + /** + * Compiles {@code src} through {@link Phases#SEMANTIC_ANALYSIS} and + * collects all {@code accessedVariable} values for {@link VariableExpression}s + * that reference a variable named {@code varName} inside method {@code m} of + * class {@code C}, excluding the declaration site itself. + */ + private static List collectAccesses(String src, String varName = 's') { + def cu = new CompilationUnit() + cu.addSource('C.groovy', src) + cu.compile(Phases.SEMANTIC_ANALYSIS) + List accesses = [] + cu.ast.classes.find { it.name == 'C' } + .getMethods('m')[0].code + .visit(new CodeVisitorSupport() { + @Override + void visitVariableExpression(VariableExpression ve) { + // Include only references, not the declaration site. + // The declaration site has accessedVariable == ve (set to itself by declare()). + if (ve.name == varName && ve.accessedVariable !== ve) { + accesses << ve.accessedVariable + } + super.visitVariableExpression(ve) + } + }) + accesses + } + + /** True if the accessed variable is a local (not a DynamicVariable). */ + private static boolean isLocal(Variable v) { + !(v instanceof DynamicVariable) + } + + // ------------------------------------------------------------------------- + // Case 1: o instanceof String s + // ------------------------------------------------------------------------- + + /** + * JLS §6.3.2.2-200-A: e.whenTrue is in scope in the then-block (S). + * e.whenFalse ({}) is in scope in the else-block (T) — so s is dynamic there. + * No abrupt completion → s is NOT introduced after the if-else. + */ + @Test + void testCase1_simpleInstanceof_ifBlockLocal_elseBlockDynamic_afterDynamic() { + def src = ''' + class C { + def m(Object o) { + if (o instanceof String s) { + s.length() // use in if-block → must be local + } else { + s // use in else-block → must be dynamic + } + s // use after if-else → must be dynamic + } + } + ''' + def accesses = collectAccesses(src) + assert accesses.size() == 3 + assert isLocal(accesses[0]) : "if-block: s must be local (JLS §6.3.2.2-200-A)" + assert !isLocal(accesses[1]) : "else-block: s must be dynamic (e.whenFalse={})" + assert !isLocal(accesses[2]) : "after if: s must be dynamic (both branches fall through)" + } + + // ------------------------------------------------------------------------- + // Case 1b: o instanceof String s, else cannot complete normally + // ------------------------------------------------------------------------- + + /** + * JLS §6.3.2.2-200-C-A: introduced by e.whenTrue={s}, S can complete normally, + * T cannot complete normally → s IS introduced after the if-else. + */ + @Test + void testCase1b_simpleInstanceof_abruptElse_afterLocal() { + def src = ''' + class C { + def m(Object o) { + if (o instanceof String s) { + // S falls through + } else { + throw new IllegalArgumentException() + } + s // after: T is abrupt, S falls through → s visible (C-A) + } + } + ''' + def accesses = collectAccesses(src) + assert accesses.size() == 1 + assert isLocal(accesses[0]) : "after if (abrupt else): s must be local (JLS §6.3.2.2-200-C-A)" + } + + // ------------------------------------------------------------------------- + // Case 2: !(o instanceof String s) + // ------------------------------------------------------------------------- + + /** + * JLS §6.3.2.2-200-B: e.whenFalse={s} is in scope in the else-block (T). + * JLS §6.3.2.2-200-A: e.whenTrue={} → if-block does NOT see s. + */ + @Test + void testCase2_negatedInstanceof_ifBlockDynamic_elseBlockLocal() { + def src = ''' + class C { + def m(Object o) { + if (!(o instanceof String s)) { + s // use in if-block → must be dynamic (e.whenTrue={}) + } else { + s.length() // use in else-block → must be local (e.whenFalse={s}) + } + } + } + ''' + def accesses = collectAccesses(src) + assert accesses.size() == 2 + assert !isLocal(accesses[0]) : "if-block: s must be dynamic (JLS §6.3.2.2-200-A, whenTrue={})" + assert isLocal(accesses[1]) : "else-block: s must be local (JLS §6.3.2.2-200-B, whenFalse={s})" + } + + // ------------------------------------------------------------------------- + // Case 3: !(o instanceof String s), abrupt if-block (early return/throw) + // ------------------------------------------------------------------------- + + /** + * JLS §6.3.2.2-200-C-B: introduced by e.whenFalse={s}, S cannot complete + * normally (early return), T can → s IS introduced after the if statement. + */ + @Test + void testCase3_negatedInstanceof_abruptIf_afterLocal() { + def src = ''' + class C { + def m(Object o) { + if (!(o instanceof String s)) { + return null // S cannot complete normally + } + s.length() // after: S abrupt, no else → s visible (C-B) + } + } + ''' + def accesses = collectAccesses(src) + assert accesses.size() == 1 + assert isLocal(accesses[0]) : "after if (abrupt if-block): s must be local (JLS §6.3.2.2-200-C-B)" + } + + // ------------------------------------------------------------------------- + // Case 4: o instanceof String s && cond + // ------------------------------------------------------------------------- + + /** + * JLS §6.3.1.1: a&&b when-true = {s} → if-block sees s. + * JLS §6.3.1.1 (note): no when-false rule → else-block does NOT see s. + * No abrupt completion → after if-else does NOT see s. + */ + @Test + void testCase4_andChain_ifBlockLocal_elseBlockDynamic_afterDynamic() { + def src = ''' + class C { + def m(Object o) { + if (o instanceof String s && s.length() > 0) { + s.toUpperCase() // if-block → local + } else { + s // else-block → dynamic + } + s // after → dynamic + } + } + ''' + def accesses = collectAccesses(src) + // s appears in: condition (RHS of &&, already in scope), if-block, else-block, after + // The && RHS visit also resolves s; let's filter just the block uses: + // accesses are in AST visit order: condition-RHS s, then if-block s, else-block s, after s + assert accesses.size() >= 3 + // The if-block use and after-if uses are tracked; find the else and after ones + // All local ones should be the if-block one, all dynamic ones the else and after + def locals = accesses.findAll { isLocal(it) } + def dynamics = accesses.findAll { !isLocal(it) } + assert !locals.isEmpty() : "if-block (and && RHS) references to s must be local" + assert !dynamics.isEmpty() : "else-block and after-if references to s must be dynamic" + } + + /** + * §6.3.1.1 Rule A: s (introduced by left when true) is definitely matched + * at right — so the RHS of && also sees s as a local. + */ + @Test + void testCase4_andChain_rhs_seesPatternVarAsLocal() { + def src = ''' + class C { + def m(Object o) { + if (o instanceof String s && s.length() > 0) { + return 1 + } + return 0 + } + } + ''' + def accesses = collectAccesses(src) + // The only s reference is in the && RHS: must be local + assert accesses.size() == 1 + assert isLocal(accesses[0]) : "RHS of && must see s as local (JLS §6.3.1.1 Rule A)" + } + + // ------------------------------------------------------------------------- + // Case 5: o instanceof String s || cond + // ------------------------------------------------------------------------- + + /** + * JLS §6.3.1.2 (note): no when-true rule for || → if-block does NOT see s. + * VariableScopeVisitor declares nothing for the if-block → DynamicVariable. + */ + @Test + void testCase5_orChain_ifBlockDynamic() { + def src = ''' + class C { + def m(Object o) { + if (o instanceof String s || true) { + s // if-block → must be dynamic + } + } + } + ''' + def accesses = collectAccesses(src) + assert accesses.size() == 1 + assert !isLocal(accesses[0]) : "if-block with || condition: s must be dynamic (JLS §6.3.1.2 note)" + } + + /** + * JLS §6.3.1.2 Rule A: s introduced by left.whenFalse is in scope in the + * right of ||. But the right of || is NOT where s is declared on a false path — + * that is about the || sub-expression, not the if-block. + * Specifically: `o instanceof String s || s.isEmpty()` — the s in `s.isEmpty()` + * is the RIGHT of ||, which is inside the if's condition, NOT the if-block. + * After || evaluates to true, VariableScopeVisitor gives the if-block e.whenTrue={} + * → still dynamic in the if-block body. + */ + @Test + void testCase5_orChain_rhs_seesPatternFalseVar() { + // Right side of ||: s.isEmpty() — s was introduced by left.whenFalse={s} + // So this s reference should be local (JLS §6.3.1.2-100-A) + def src = ''' + class C { + def m(Object o) { + if (!(o instanceof String s) || s.isEmpty()) { + return 1 + } + return 0 + } + } + ''' + def accesses = collectAccesses(src) + // s in s.isEmpty() is the RHS of ||; it's introduced by left.whenFalse={s} + assert accesses.size() == 1 + assert isLocal(accesses[0]) : "RHS of || (after !(instanceof s)): s must be local (JLS §6.3.1.2-100-A)" + } + + // ------------------------------------------------------------------------- + // Case 6: !(o instanceof String s) && cond + // ------------------------------------------------------------------------- + + /** + * JLS §6.3.1.3 + §6.3.1.1: !(instanceof s).whenTrue = {} → + * (!(instanceof s) && cond).whenTrue = {} → if-block does NOT see s. + */ + @Test + void testCase6_negatedAndCond_ifBlockDynamic() { + def src = ''' + class C { + def m(Object o) { + if (!(o instanceof String s) && true) { + s // if-block → must be dynamic + } + } + } + ''' + def accesses = collectAccesses(src) + assert accesses.size() == 1 + assert !isLocal(accesses[0]) : "if-block: !(instanceof s) && cond → s must be dynamic" + } + + /** + * After if, with abrupt else: JLS §6.3.2.2-200-C-A: + * e.whenTrue={}, T abrupt → C-A doesn't apply (e.whenTrue={}). + * C-B: e.whenFalse={s}, S cannot complete normally? S = !(s) && cond if-block + * can complete normally → C-B doesn't apply either. So s NOT visible after. + */ + @Test + void testCase7a_negatedAndCond_abruptElse_afterDynamic() { + def src = ''' + class C { + def m(Object o) { + if (!(o instanceof String s) && true) { + // S falls through + } else { + throw new RuntimeException() + } + s // after → must be dynamic (C-B doesn't apply) + } + } + ''' + def accesses = collectAccesses(src) + assert accesses.size() == 1 + assert !isLocal(accesses[0]) : "after if (!(instanceof s) && cond, abrupt else): s must be dynamic" + } + + // ------------------------------------------------------------------------- + // Case 7b: missing case — !(o instanceof String s), abrupt else only + // ------------------------------------------------------------------------- + + /** + * explicit missing case: + * 
+     *   if (!(o instanceof String s)) {
+     *     println "not String"   // S can complete normally
+     *   } else {
+     *     println "String"
+     *     return                 // T cannot complete normally
+     *   }
+     *   println s  // s NOT visible: C-B requires S to be abrupt, but S falls through
+     *
+ * + * JLS §6.3.2.2-200-C analysis: + * e = !(o instanceof String s): whenFalse={s} + * C-B: e.whenFalse={s} AND S cannot complete normally AND T can → required but S CAN complete → C-B does NOT apply + * C-A: e.whenTrue={} → nothing + * → s NOT introduced after the if-else statement. + */ + @Test + void testCase7b_negatedInstanceof_abruptElseOnly_afterDynamic() { + def src = ''' + class C { + def m(Object o) { + if (!(o instanceof String s)) { + println "not String" // S can complete normally + } else { + println "String" + return s // T cannot complete normally + } + s // after → must be dynamic (C-B does NOT apply) + } + } + ''' + def accesses = collectAccesses(src) + // s appears in: else-block (return s) and after the if-else + // else-block: e.whenFalse={s} → local + // after: C-B does not apply → dynamic + assert accesses.size() == 2 + assert isLocal(accesses[0]) : "else-block: s must be local (JLS §6.3.2.2-200-B, e.whenFalse={s})" + assert !isLocal(accesses[1]) : "after if (abrupt-else-only): s must be dynamic (C-B does NOT apply)" + } + + // ------------------------------------------------------------------------- + // Case 8: !(o instanceof String s) || cond + // ------------------------------------------------------------------------- + + /** + * JLS §6.3.1.2 + §6.3.1.3: + * !(instanceof s) → whenFalse={s}. + * a || b: b is in scope for left.whenFalse={s} (Rule A). + * a || b: whenFalse = left.whenFalse ∪ right.whenFalse = {s} ∪ {} = {s}. + * → else-block sees s (e.whenFalse={s}, JLS §6.3.2.2-200-B). + * → if-block does NOT see s (JLS §6.3.1.2 note: no whenTrue rule for ||). + */ + @Test + void testCase8_negatedOrCond_elseBlockLocal_ifBlockDynamic() { + def src = ''' + class C { + def m(Object o) { + if (!(o instanceof String s) || s.isEmpty()) { + s // if-block → dynamic (|| has no whenTrue rule) + } else { + s.length() // else-block → local (e.whenFalse={s}) + } + } + } + ''' + def accesses = collectAccesses(src) + // s appears in: condition RHS (s.isEmpty()), if-block, else-block + // condition RHS: s introduced by left.whenFalse={s} via || Rule A → local + // if-block: e.whenTrue={} → dynamic + // else-block: e.whenFalse={s} → local + assert accesses.size() == 3 + assert isLocal(accesses[0]) : "|| RHS (s.isEmpty): s must be local (JLS §6.3.1.2-100-A)" + assert !isLocal(accesses[1]) : "if-block: s must be dynamic (JLS §6.3.1.2 note, no whenTrue rule)" + assert isLocal(accesses[2]) : "else-block: s must be local (JLS §6.3.2.2-200-B, whenFalse={s})" + } + + // ------------------------------------------------------------------------- + // Double negation / De Morgan identities + // ------------------------------------------------------------------------- + + /** + * JLS §6.3.1.3 applied twice: !!(instanceof s) when-true = {s} — same as plain instanceof. + */ + @Test + void testDoubleNegation_ifBlockLocal() { + def src = ''' + class C { + def m(Object o) { + if (!!(o instanceof String s)) { + s.length() // if-block → local + } + } + } + ''' + def accesses = collectAccesses(src) + assert accesses.size() == 1 + assert isLocal(accesses[0]) : "if-block with !! condition: s must be local (§6.3.1.3 applied twice)" + } + + /** + * De Morgan: !(a && b) ≡ !a || !b. No definite binding on either path. + * But the && RHS (b = s.length() > 0) is in scope for the pattern-true + * arm (JLS §6.3.1.1 Rule A), so the condition itself contains a local ref. + */ + @Test + void testDeMorgan_notAndNegation_bothDynamic() { + def src = ''' + class C { + def m(Object o) { + if (!(o instanceof String s && s.length() > 0)) { + s // if-block → dynamic (whenTrue of !(&&) = {}) + } else { + s // else-block → local (whenFalse of !(&&) = {s}) + } + } + } + ''' + def accesses = collectAccesses(src) + // Visit order: condition (s.length() > 0 inside &&), if-block s, else-block s. + // !(a && b): whenTrue = (a&&b).whenFalse = {}; whenFalse = (a&&b).whenTrue = {s} + // condition (&&) RHS: s.length() > 0 — left's whenTrue={s} is in scope (§6.3.1.1 Rule A) → local + // if-block: e.whenTrue={} → dynamic + // else-block: e.whenFalse={s} → local + assert accesses.size() == 3 + assert isLocal(accesses[0]) : "condition (&&) RHS: s must be local (JLS §6.3.1.1 Rule A)" + assert !isLocal(accesses[1]) : "if-block: !(&&) whenTrue={} → s must be dynamic" + assert isLocal(accesses[2]) : "else-block: !(&&) whenFalse={s} → s must be local" + } + + // ------------------------------------------------------------------------- + // Ternary expression (JLS §6.3.1.4) + // ------------------------------------------------------------------------- + + /** + * JLS §6.3.1.4: a ? b : c introduces no bindings for when-true or when-false. + * The condition's whenTrue={s} is visible in b, and whenFalse={} in c. + */ + @Test + void testTernaryExpression_trueExprLocal_falseExprDynamic() { + def src = ''' + class C { + def m(Object o) { + def r = (o instanceof String s) ? s.length() : s // s in false-expr → dynamic + r + } + } + ''' + def accesses = collectAccesses(src) + // s in true-expr (s.length()) → local (condition.whenTrue={s}) + // s in false-expr (s) → dynamic (condition.whenFalse={}) + assert accesses.size() == 2 + assert isLocal(accesses[0]) : "ternary true-expr: s must be local (condition.whenTrue={s})" + assert !isLocal(accesses[1]) : "ternary false-expr: s must be dynamic (condition.whenFalse={})" + } + + // ------------------------------------------------------------------------- + // Redeclaration where pattern variable is NOT visible → new local is OK + // ------------------------------------------------------------------------- + + /** + * Else-block of {@code o instanceof String s}: s is not in scope, so + * {@code def s = ...} declares a fresh local. Uses of s in the else-block + * must resolve to that local (not the pattern binding, not DynamicVariable). + */ + @Test + void testRedeclareInElseBlock_wherePatternNotVisible_isLocal() { + def src = ''' + class C { + def m(Object o) { + if (o instanceof String s) { + s.length() + } else { + def s = 'local' + s + } + } + } + ''' + def accesses = collectAccesses(src) + // if-block s (pattern local), else-block s (new local declaration's use) + assert accesses.size() == 2 + assert isLocal(accesses[0]) : "if-block: pattern s must be local" + assert isLocal(accesses[1]) : "else-block: redeclared s must be a local" + // The two locals must be distinct Variable objects. + assert accesses[0].is(accesses[0]) // sanity + assert !accesses[0].is(accesses[1]) : "else-block s must not be the pattern variable" + } + + /** + * After if-else where both branches fall through, pattern s is not in scope; + * {@code def s = ...} is a fresh local. + */ + @Test + void testRedeclareAfterIf_wherePatternNotVisible_isLocal() { + def src = ''' + class C { + def m(Object o) { + if (o instanceof String s) { + s.length() + } + def s = 42 + s + } + } + ''' + def accesses = collectAccesses(src) + // if-block s (pattern), after-if s (new local) + assert accesses.size() == 2 + assert isLocal(accesses[0]) : "if-block: pattern s must be local" + assert isLocal(accesses[1]) : "after if: redeclared s must be a local" + assert !accesses[0].is(accesses[1]) : "after-if s must not be the pattern variable" + } + + /** + * True-branch of {@code !(o instanceof String s)}: whenTrue is empty, so + * redeclaring s is allowed and yields a fresh local. + */ + @Test + void testRedeclareInNegatedIfBlock_wherePatternNotVisible_isLocal() { + def src = ''' + class C { + def m(Object o) { + if (!(o instanceof String s)) { + def s = 'x' + s + } else { + s.length() + } + } + } + ''' + def accesses = collectAccesses(src) + // if-block s (new local), else-block s (pattern local) + assert accesses.size() == 2 + assert isLocal(accesses[0]) : "if-block: redeclared s must be a local" + assert isLocal(accesses[1]) : "else-block: pattern s must be local" + assert !accesses[0].is(accesses[1]) : "if-block s must not be the pattern variable" + } + + // ------------------------------------------------------------------------- + // shouldNotCompile: cannot redeclare where pattern variable IS visible + // ------------------------------------------------------------------------- + + /** + * Then-block of {@code o instanceof String s}: s is in scope → redeclare fails. + */ + @Test + void testShouldNotCompile_redeclareInIfBlock_wherePatternVisible() { + def err = shouldNotCompile ''' + class C { + def m(Object o) { + if (o instanceof String s) { + def s = 'nope' + } + } + } + ''' + assert err =~ /already contains a variable of the name s/ + } + + /** + * After abrupt else of {@code o instanceof String s}: s survives → redeclare fails. + */ + @Test + void testShouldNotCompile_redeclareAfterAbruptElse_wherePatternVisible() { + def err = shouldNotCompile ''' + class C { + def m(Object o) { + if (o instanceof String s) { + // fall through + } else { + return + } + def s = 'nope' + } + } + ''' + assert err =~ /already contains a variable of the name s/ + } + + /** + * After {@code if (!(o instanceof String s)) return}: s is introduced → redeclare fails. + */ + @Test + void testShouldNotCompile_redeclareAfterEarlyReturn_wherePatternVisible() { + def err = shouldNotCompile ''' + class C { + def m(Object o) { + if (!(o instanceof String s)) return + def s = 'nope' + } + } + ''' + assert err =~ /already contains a variable of the name s/ + } + + /** + * Else-block of {@code !(o instanceof String s)}: whenFalse={s} → redeclare fails. + */ + @Test + void testShouldNotCompile_redeclareInElseOfNegated_wherePatternVisible() { + def err = shouldNotCompile ''' + class C { + def m(Object o) { + if (!(o instanceof String s)) { + // not string + } else { + def s = 'nope' + } + } + } + ''' + assert err =~ /already contains a variable of the name s/ + } + + private static String shouldNotCompile(String src) { + try { + def cu = new CompilationUnit() + cu.addSource('C.groovy', src) + cu.compile(Phases.SEMANTIC_ANALYSIS) + throw new AssertionError("Expected compilation to fail:\n$src") + } catch (Exception e) { + return e.message ?: e.toString() + } + } +} diff --git a/src/test/groovy/groovy/InstanceofTest.groovy b/src/test/groovy/groovy/InstanceofTest.groovy index ba7b6ec8dee..29c4814b0cf 100644 --- a/src/test/groovy/groovy/InstanceofTest.groovy +++ b/src/test/groovy/groovy/InstanceofTest.groovy @@ -20,6 +20,8 @@ package groovy import org.junit.jupiter.api.Test +import org.codehaus.groovy.control.MultipleCompilationErrorsException + import static groovy.test.GroovyAssert.shouldFail final class InstanceofTest { @@ -624,66 +626,57 @@ final class InstanceofTest { // --- (5) o instanceof String s || cond --- - // In dynamic Groovy the evaluateInstanceof always allocates the slot, so s can be - // accessed in the if-block at runtime (even though flow scoping says it's not - // guaranteed). TypeChecked enforces the stricter Java rule: true-path binding of - // left of || is NOT in scope on the right (Java rule) and NOT in the if-block. + // JLS §6.3.1.2 (note): no rule for when-true of || — so VariableScopeVisitor does + // NOT declare s in the if-block scope. Dynamic Groovy resolves the undeclared s as a + // DynamicVariable, which yields MissingPropertyException at runtime (same as TypeChecked). + // There is no need to use @TypeChecked here — the scope decision is made entirely by + // VariableScopeVisitor (JLS §6.3.2.2-200-A: e.whenTrue is {} for this shape). @Test - void testOrChain_noVisibilityAnywhere() { - def shell = GroovyShell.withConfig { - ast groovy.transform.TypeChecked - } + void testOrChain_noVisibilityInIfBlock() { // s must not be visible in the if-block when condition is instanceof s || ... - def err = shouldFail shell, ''' - @groovy.transform.TypeChecked + def err = shouldFail MissingPropertyException, ''' class C { - static Object m(Object o) { + def m(Object o) { if (o instanceof String s || true) { return s } return 'ok' } } + new C().m('hello') ''' - assert err.message =~ /The variable .s. is undeclared|Apparent variable .s./ + assert err.message =~ /No such property: s/ } // --- (6) !(o instanceof String s) && cond --- // De Morgan: ≡ (!instanceof s) && cond - // In dynamic mode, evaluateInstanceof defines the slot during condition evaluation, - // so s resolves dynamically even in the if-body. TypeChecked enforces the strict rule: - // && propagates no false-bindings, so if-block (true path) does NOT see s. + // JLS §6.3.1.3 + §6.3.1.1: !(o instanceof String s).whenTrue = {} (no when-false for instanceof), + // && propagates no false-bindings, so the if-block (true path) does NOT see s. + // VariableScopeVisitor declares nothing in the if-block → DynamicVariable → MissingPropertyException. @Test - void testNegatedAndCond_noVisibility() { - def shell = GroovyShell.withConfig { - ast groovy.transform.TypeChecked - } - def err = shouldFail shell, ''' - @groovy.transform.TypeChecked + void testNegatedAndCond_noVisibilityInIfBlock() { + def err = shouldFail MissingPropertyException, ''' class C { - static Object m(Object o) { + def m(Object o) { if (!(o instanceof String s) && true) { return s } return 'out' } } + new C().m(1) ''' - assert err.message =~ /The variable .s. is undeclared|Apparent variable .s./ + assert err.message =~ /No such property: s/ } - // --- (7) !(o instanceof String s) && cond, plus return in else block --- - // After the if: the if-block's true-path has !(s bound) && cond, no s guarantee. - // TypeChecked enforces that s is NOT visible after the if statement. + // --- (7) !(o instanceof String s) && cond, plus abrupt else-block --- + // After the if: JLS §6.3.2.2-200-C-B: var introduced when true=?, S cannot complete normally, T can. + // Here if-block true-path has no s binding (whenTrue={}), so even with abrupt else, s is NOT after. @Test void testNegatedAndCond_withElseReturn_noVisibilityAfter() { - def shell = GroovyShell.withConfig { - ast groovy.transform.TypeChecked - } - def err = shouldFail shell, ''' - @groovy.transform.TypeChecked + def err = shouldFail MissingPropertyException, ''' class C { - static Object m(Object o) { + def m(Object o) { if (!(o instanceof String s) && true) { // true path: s not definitely bound } else { @@ -692,8 +685,40 @@ final class InstanceofTest { return s } } + new C().m(1) ''' - assert err.message =~ /The variable .s. is undeclared|Apparent variable .s./ + assert err.message =~ /No such property: s/ + } + + // --- (NEW) !(o instanceof String s) with abrupt else-block only --- + // missing case: + // if (!(o instanceof String s)) { println "not String" } else { println "String"; return } + // println s // <-- must be INVALID + // + // JLS §6.3.2.2-200-C analysis: + // e = !(o instanceof String s): whenTrue={}, whenFalse={s} + // S = if-block (println "not String"): can complete normally + // T = else-block (println "String"; return): cannot complete normally + // C-A: e.whenTrue={} -> nothing even if T abrupt + // C-B: e.whenFalse={s}, S cannot complete normally? NO (S falls through) -> C-B does NOT apply + // -> s is NOT introduced after the if-else statement + @Test + void testNegatedInstanceof_abruptElseOnly_noVisibilityAfter() { + def err = shouldFail MissingPropertyException, ''' + class C { + def m(Object o) { + if (!(o instanceof String s)) { + println "not String" + } else { + println "String" + return s + } + return s // s NOT in scope: if-block falls through, C-B does not apply + } + } + new C().m(1) + ''' + assert err.message =~ /No such property: s/ } // --- (8) !(o instanceof String s) || cond --- @@ -783,4 +808,187 @@ final class InstanceofTest { assert f('ab') == 'AB' assert f(1) == 'no' } + + // ------------------------------------------------------------------------- + // GROOVY-12242: redeclaration where pattern variable is / is not visible + // ------------------------------------------------------------------------- + + /** + * Else-block of positive instanceof: s not in scope → fresh local is allowed + * and correctly used at runtime (class generation must free the name). + */ + @Test + void testRedeclareInElse_wherePatternNotVisible_runtime() { + def f = { Object o -> + if (o instanceof String s) { + return 'pat:' + s + } else { + def s = 'local' + return s + } + } + assert f('hi') == 'pat:hi' + assert f(1) == 'local' + } + + /** + * After if with both branches falling through: s not in scope → fresh local OK. + */ + @Test + void testRedeclareAfterIf_wherePatternNotVisible_runtime() { + def f = { Object o -> + if (o instanceof String s) { + // matched; s does not escape + } + def s = 99 + return s + } + assert f('x') == 99 + assert f(1) == 99 + } + + /** + * True-branch of negated instanceof: s not in scope → fresh local OK. + */ + @Test + void testRedeclareInNegatedIf_wherePatternNotVisible_runtime() { + def f = { Object o -> + if (!(o instanceof String s)) { + def s = 'shadow' + return s + } else { + return 'pat:' + s + } + } + assert f(1) == 'shadow' + assert f('hi') == 'pat:hi' + } + + /** + * Nested: outer pattern s remains usable after an inner if that introduces i. + */ + @Test + void testNestedInstanceof_outerPatternStillVisible() { + def f = { Object o, Object p -> + if (o instanceof String s) { + if (p instanceof Integer i) { + return s + ':' + i + } + return s + ':no-i' + } + return 'no-s' + } + assert f('ab', 3) == 'ab:3' + assert f('ab', 'x') == 'ab:no-i' + assert f(1, 3) == 'no-s' + } + + // --- shouldNotCompile: cannot redeclare where pattern s is visible --- + + @Test + void testShouldNotCompile_redeclareInIfBlock_wherePatternVisible() { + def err = shouldFail MultipleCompilationErrorsException, ''' + def m(Object o) { + if (o instanceof String s) { + def s = 'nope' + } + } + ''' + assert err.message =~ /already contains a variable of the name s/ + } + + @Test + void testShouldNotCompile_redeclareAfterEarlyReturn_wherePatternVisible() { + def err = shouldFail MultipleCompilationErrorsException, ''' + def m(Object o) { + if (!(o instanceof String s)) return + def s = 'nope' + } + ''' + assert err.message =~ /already contains a variable of the name s/ + } + + @Test + void testShouldNotCompile_redeclareAfterAbruptElse_wherePatternVisible() { + def err = shouldFail MultipleCompilationErrorsException, ''' + def m(Object o) { + if (o instanceof String s) { + } else { + return + } + def s = 'nope' + } + ''' + assert err.message =~ /already contains a variable of the name s/ + } + + @Test + void testShouldNotCompile_redeclareInElseOfNegated_wherePatternVisible() { + def err = shouldFail MultipleCompilationErrorsException, ''' + def m(Object o) { + if (!(o instanceof String s)) { + } else { + def s = 'nope' + } + } + ''' + assert err.message =~ /already contains a variable of the name s/ + } + + /** + * Successive ifs reusing pattern name {@code s}: second condition must + * re-bind the slot so else-path hide still frees the name for redeclaration. + */ + @Test + void testSuccessiveIfs_reusePatternName_redeclareInSecondElse() { + def f = { Object a, Object b -> + if (a instanceof String s) { + // first binding + } + if (b instanceof Integer s) { + return 'int:' + s + } else { + def s = 'local' + return s + } + } + assert f('x', 7) == 'int:7' + assert f('x', 'y') == 'local' + assert f(1, 7) == 'int:7' + assert f(1, 'y') == 'local' + } + + /** + * Expression-statement pattern then a later if reusing the name — hide must + * still apply (identity-based "introduced", not name-set diff). + */ + @Test + void testIsolatedPatternExpr_thenIfReuseName_redeclareInElse() { + def f = { Object o -> + (o instanceof String s) // isolated; must not leak + if (o instanceof Integer s) { + return 'int:' + s + } else { + def s = 'ok' + return s + } + } + assert f(5) == 'int:5' + assert f('hi') == 'ok' + } + + /** + * After an early-return negated instanceof, survivor {@code s} remains usable + * even when a prior isolated pattern used the same name. + */ + @Test + void testNameReuse_thenSurvivorAfterEarlyReturn() { + def f = { Object o -> + (o instanceof Number n) // different name; isolation + if (!(o instanceof String s)) return 'early' + return s.toUpperCase() + } + assert f('ab') == 'AB' + assert f(1) == 'early' + } } From d332081b98c64a4261b8adc70a45af0839a6cadd Mon Sep 17 00:00:00 2001 From: Daniel Sun Date: Mon, 10 Aug 2026 02:58:04 +0900 Subject: [PATCH 4/6] GROOVY-12242: fold InstanceofPathLiveNames into InstanceofFlowBindings --- .../groovy/classgen/VariableScopeVisitor.java | 251 ++++++++---------- .../classgen/asm/BinaryExpressionHelper.java | 8 +- .../groovy/classgen/asm/StatementWriter.java | 19 +- 3 files changed, 121 insertions(+), 157 deletions(-) diff --git a/src/main/java/org/codehaus/groovy/classgen/VariableScopeVisitor.java b/src/main/java/org/codehaus/groovy/classgen/VariableScopeVisitor.java index 6bc7be3f52b..b5ba95f5a2c 100644 --- a/src/main/java/org/codehaus/groovy/classgen/VariableScopeVisitor.java +++ b/src/main/java/org/codehaus/groovy/classgen/VariableScopeVisitor.java @@ -93,21 +93,20 @@ *

* For JEP 394 {@code instanceof} pattern variables (GROOVY-12242), this * class is the single authoritative source of scope decisions. - * The nested {@link InstanceofFlowBindings} analysis determines which pattern - * variables are definitely bound on each path; this visitor: + * {@link InstanceofFlowBindings} answers which pattern variables are definitely + * bound on each control-flow path; this visitor: *

    - *
  • declares each pattern variable only in the lexical scopes where it is - * live, so name resolution outside those scopes yields a - * {@link org.codehaus.groovy.ast.DynamicVariable} (runtime - * {@link groovy.lang.MissingPropertyException} in dynamic Groovy — the - * same rule {@code @TypeChecked} enforces at compile time); and
    • - *
  • attaches {@link InstanceofPathLiveNames} metadata to {@code if}/ternary - * nodes so later phases can consume the path-live name sets without - * re-running the flow analysis.
    • + *
  • declares each pattern variable only where it is live, so out-of-scope + * references become {@link org.codehaus.groovy.ast.DynamicVariable} + * (runtime {@link groovy.lang.MissingPropertyException} in dynamic + * Groovy — the same rule {@code @TypeChecked} enforces at compile + * time); and
    • + *
  • attaches the same {@link InstanceofFlowBindings} instance as AST + * metadata so later phases (classgen) can read path-live names + * without re-running the analysis.
    • *
* * @see InstanceofFlowBindings - * @see InstanceofPathLiveNames */ public class VariableScopeVisitor extends ClassCodeVisitorSupport { @@ -653,8 +652,8 @@ public void visitForLoop(final ForStatement statement) { /** * Visits an {@code if}/{@code else} statement, establishing correct * lexical scopes for JEP 394 {@code instanceof} pattern variables - * (GROOVY-12242) and recording {@link InstanceofPathLiveNames} on the - * statement so later phases need not re-derive the flow analysis. + * (GROOVY-12242) and attaching {@link InstanceofFlowBindings} metadata so + * later phases need not re-derive the flow analysis. *

* Rules applied (JLS §6.3.2.2 / JEP 394): *

    @@ -674,8 +673,8 @@ public void visitForLoop(final ForStatement statement) { @Override public void visitIfElse(final IfStatement statement) { InstanceofFlowBindings bindings = InstanceofFlowBindings.of(statement.getBooleanExpression()); - // Enrich the AST so classgen can adjust CompileStack without re-analysis. - statement.putNodeMetaData(InstanceofPathLiveNames.KEY, InstanceofPathLiveNames.of(bindings)); + // Same analysis result: declare into scopes + enrich AST for classgen. + InstanceofFlowBindings.put(statement, bindings); // Condition: pattern vars are available for short-circuit RHS (e.g. &&). pushState(); @@ -803,13 +802,13 @@ public void visitBinaryExpression(final BinaryExpression expression) { /** * Visits a ternary / Elvis expression with flow scoping for pattern variables: * true-path bindings are in scope in the then-branch; false-path bindings in - * the else-branch (GROOVY-12242 / JEP 394). Also records - * {@link InstanceofPathLiveNames} on the expression for later phases. + * the else-branch (GROOVY-12242 / JEP 394). Also attaches + * {@link InstanceofFlowBindings} metadata for later phases. */ @Override public void visitTernaryExpression(final TernaryExpression expression) { InstanceofFlowBindings bindings = InstanceofFlowBindings.of(expression.getBooleanExpression()); - expression.putNodeMetaData(InstanceofPathLiveNames.KEY, InstanceofPathLiveNames.of(bindings)); + InstanceofFlowBindings.put(expression, bindings); pushState(); expression.getBooleanExpression().visit(this); @@ -1014,158 +1013,133 @@ public void visitVariableExpression(final VariableExpression expression) { } } - // ========================================================================= - // Nested class: InstanceofPathLiveNames - // ========================================================================= - - /** - * Path-live pattern-variable names for an {@code if}/ternary - * condition (GROOVY-12242 / JEP 394). - *

    - * Produced once by {@link VariableScopeVisitor} and attached as node - * metadata under {@link #KEY}. Downstream code generation reads these - * sets to decide which CompileStack names to hide on each path; it must - * not re-run the flow analysis. - * - * @since 6.0.0 - */ - @Internal - public static final class InstanceofPathLiveNames { - - /** Metadata key used with {@link ASTNode#putNodeMetaData}/{@code getNodeMetaData}. */ - public static final Object KEY = InstanceofPathLiveNames.class; - - private static final InstanceofPathLiveNames EMPTY = - new InstanceofPathLiveNames(Collections.emptySet(), Collections.emptySet()); - - private final Set whenTrue; - private final Set whenFalse; - - private InstanceofPathLiveNames(final Set whenTrue, final Set whenFalse) { - this.whenTrue = whenTrue; - this.whenFalse = whenFalse; - } - - /** Names definitely bound when the condition is {@code true}. */ - public Set whenTrue() { return whenTrue; } - - /** Names definitely bound when the condition is {@code false}. */ - public Set whenFalse() { return whenFalse; } - - /** Whether both path sets are empty. */ - public boolean isEmpty() { - return whenTrue.isEmpty() && whenFalse.isEmpty(); - } - - /** - * Builds an immutable name-set pair from a flow-analysis result. - */ - static InstanceofPathLiveNames of(final InstanceofFlowBindings bindings) { - if (bindings == null || bindings.isEmpty()) return EMPTY; - return new InstanceofPathLiveNames( - Set.copyOf(bindings.whenTrueNames()), - Set.copyOf(bindings.whenFalseNames())); - } - - /** - * Returns the metadata attached to {@code node}, or an empty instance - * if none is present (no pattern variables on either path). - */ - public static InstanceofPathLiveNames get(final ASTNode node) { - if (node == null) return EMPTY; - InstanceofPathLiveNames names = node.getNodeMetaData(KEY); - return names != null ? names : EMPTY; - } - } - // ========================================================================= // Nested class: InstanceofFlowBindings // ========================================================================= /** - * Flow-sensitive analysis of JEP 394 {@code instanceof} pattern - * bindings (GROOVY-12242). - *

    - * Given a boolean condition, this type answers: which pattern variables are - * definitely bound when the condition is {@code true} vs - * {@code false}? It is the Groovy equivalent of the JLS §6.3.1 + * Flow-sensitive result for JEP 394 {@code instanceof} pattern + * bindings (GROOVY-12242) — the Groovy equivalent of the JLS §6.3.1 * “introduced by” sets. *

    - * Pure semantic analysis — no bytecode knowledge. Consumed only by - * {@link VariableScopeVisitor}, which declares names into scopes and - * publishes {@link InstanceofPathLiveNames} metadata for later phases. - * Code generation must not call {@link #of(Expression)} itself. + * One type, two views of the same analysis: + *

      + *
    • {@link #whenTrue()}/{@link #whenFalse()} — pattern + * {@link VariableExpression}s for {@link VariableScopeVisitor} to + * declare into lexical scopes;
      • + *
    • {@link #whenTrueNames()}/{@link #whenFalseNames()} — the same + * sets as names for classgen to path-hide CompileStack slots.
      • + *
    + * {@link VariableScopeVisitor} runs the analysis once via {@link #of}, + * declares from the variable lists, and attaches this instance as AST + * metadata ({@link #put}/{@link #get}). Later phases must use + * {@link #get} only — never re-call {@link #of} on the condition. *

    - * Covered condition shapes: - * {@code e instanceof T t}, {@code e !instanceof T t}, {@code !expr}, - * {@code a && b}, {@code a || b}. All other shapes return - * {@link #EMPTY} (conservative: no bindings on either path). + * Covered shapes: {@code e instanceof T t}, {@code e !instanceof T t}, + * {@code !expr}, {@code a && b}, {@code a || b}. All other shapes yield + * {@link #EMPTY} (conservative: no definite bindings). * * @see VariableScopeVisitor - * @see InstanceofPathLiveNames * @since 6.0.0 */ @Internal public static final class InstanceofFlowBindings { + /** Metadata key for {@link ASTNode#putNodeMetaData}/{@code getNodeMetaData}. */ + public static final Object KEY = InstanceofFlowBindings.class; + /** Singleton for "no pattern variables on either path". */ - public static final InstanceofFlowBindings EMPTY = new InstanceofFlowBindings(List.of(), List.of()); + public static final InstanceofFlowBindings EMPTY = + new InstanceofFlowBindings(List.of(), List.of()); private final List whenTrue; private final List whenFalse; + /** Cached name view of {@link #whenTrue}; never reallocated. */ + private final Set whenTrueNames; + /** Cached name view of {@link #whenFalse}; never reallocated. */ + private final Set whenFalseNames; private InstanceofFlowBindings(final List whenTrue, - final List whenFalse) { + final List whenFalse) { this.whenTrue = whenTrue; this.whenFalse = whenFalse; + this.whenTrueNames = namesOf(whenTrue); + this.whenFalseNames = namesOf(whenFalse); } /** * Pattern variables definitely assigned when the condition is {@code true}. + * Used by {@link VariableScopeVisitor} to declare into scopes. */ public List whenTrue() { return whenTrue; } /** * Pattern variables definitely assigned when the condition is {@code false}. + * Used by {@link VariableScopeVisitor} to declare into scopes. */ public List whenFalse() { return whenFalse; } + /** + * Names of pattern variables bound when the condition is {@code true}. + * Used by classgen for CompileStack path-hide; same set as {@link #whenTrue()}. + */ + public Set whenTrueNames() { return whenTrueNames; } + + /** + * Names of pattern variables bound when the condition is {@code false}. + * Used by classgen for CompileStack path-hide; same set as {@link #whenFalse()}. + */ + public Set whenFalseNames() { return whenFalseNames; } + /** Whether any pattern variable is bound on either path. */ public boolean isEmpty() { return whenTrue.isEmpty() && whenFalse.isEmpty(); } - /** Names of pattern variables bound when the condition is {@code true}. */ - public Set whenTrueNames() { return names(whenTrue); } - - /** Names of pattern variables bound when the condition is {@code false}. */ - public Set whenFalseNames() { return names(whenFalse); } - /** * All pattern-variable names in either path (stable encounter order). */ public Set allNames() { if (isEmpty()) return Collections.emptySet(); - Set names = new LinkedHashSet<>(whenTrue.size() + whenFalse.size()); - for (VariableExpression ve : whenTrue) names.add(ve.getName()); - for (VariableExpression ve : whenFalse) names.add(ve.getName()); - return names; + if (whenFalseNames.isEmpty()) return whenTrueNames; + if (whenTrueNames.isEmpty()) return whenFalseNames; + Set names = new LinkedHashSet<>(whenTrueNames.size() + whenFalseNames.size()); + names.addAll(whenTrueNames); + names.addAll(whenFalseNames); + return Collections.unmodifiableSet(names); } - private static Set names(final List vars) { - if (vars.isEmpty()) return Collections.emptySet(); - Set result = new LinkedHashSet<>(vars.size()); - for (VariableExpression ve : vars) result.add(ve.getName()); - return result; + // ----------------------------------------------------------------- + // AST metadata (enrichment for later phases) + // ----------------------------------------------------------------- + + /** + * Attaches this analysis result to {@code node} for later phases. + * No-op when {@code bindings} is null or {@link #EMPTY}. + */ + public static void put(final ASTNode node, final InstanceofFlowBindings bindings) { + if (node == null || bindings == null || bindings.isEmpty()) return; + node.putNodeMetaData(KEY, bindings); + } + + /** + * Returns the analysis result previously attached to {@code node}, or + * {@link #EMPTY} if none (no path-live pattern variables). + */ + public static InstanceofFlowBindings get(final ASTNode node) { + if (node == null) return EMPTY; + InstanceofFlowBindings bindings = node.getNodeMetaData(KEY); + return bindings != null ? bindings : EMPTY; } // ----------------------------------------------------------------- - // Factory methods + // Analysis entry points // ----------------------------------------------------------------- /** * Analyses {@code expression} for definite {@code instanceof} pattern - * bindings. + * bindings. Call only from {@link VariableScopeVisitor} (or tests); + * classgen must use {@link #get(ASTNode)}. * * @param expression a boolean condition (may be a * {@link BooleanExpression} wrapper); {@code null} @@ -1178,14 +1152,9 @@ public static InstanceofFlowBindings of(final Expression expression) { /** * Returns {@code true} if {@code expression} contains any JEP 394 - * type-pattern node ({@code e instanceof T t} or - * {@code e !instanceof T t}), at any depth in the expression tree. - *

    - * Unlike {@link #of}, which only follows boolean-algebra operators, this - * performs a full subtree walk. Used by tests and diagnostics; classgen - * has its own structural check for expression-statement isolation. - * - * @param expression any expression; {@code null} yields {@code false} + * type-pattern node at any depth. Full subtree walk (unlike {@link #of}). + * Used by tests and diagnostics; classgen has its own structural check + * for expression-statement isolation. */ public static boolean containsPattern(final Expression expression) { if (expression == null) return false; @@ -1207,18 +1176,9 @@ && isTypePattern(be.getRightExpression())) { } /** - * Returns the names of all pattern variables that appear - * anywhere in {@code expression}, regardless of which flow path they - * are definitely assigned on. - *

    - * Unlike {@link #allNames()} (which covers only names in the - * {@code whenTrue}/{@code whenFalse} sets), this method walks the - * entire expression tree. Useful in tests and diagnostics; code - * generation tracks allocated pattern slots via CompileStack instead. - * - * @param expression the expression to walk; {@code null} yields an - * empty set - * @return an unmodifiable set of all pattern variable names + * Names of all pattern variables in {@code expression}, regardless + * of definite-assignment path. Full tree walk; for tests/diagnostics. + * Classgen tracks allocated slots via CompileStack instead. */ public static Set allPatternNames(final Expression expression) { if (expression == null) return Collections.emptySet(); @@ -1245,17 +1205,12 @@ && isTypePattern(be.getRightExpression())) { /** * Recursive descent over the boolean algebra of the condition. - *

    - * Only the operators that can propagate definite-assignment are - * followed ({@code instanceof}, {@code !instanceof}, {@code !}, - * {@code &&}, {@code ||}). All other shapes return {@link #EMPTY} - * conservatively. This keeps the traversal proportional to the - * boolean structure, not to the total AST size. + * Only operators that propagate definite-assignment are followed + * ({@code instanceof}, {@code !instanceof}, {@code !}, {@code &&}, + * {@code ||}). Other shapes return {@link #EMPTY} conservatively. */ private static InstanceofFlowBindings analyse(final Expression expression) { Expression expr = expression; - // Unwrap BooleanExpression wrappers; handle NotExpression explicitly - // so nested negations compose correctly. while (expr instanceof BooleanExpression && !(expr instanceof NotExpression)) { expr = ((BooleanExpression) expr).getExpression(); } @@ -1311,6 +1266,14 @@ private static List union(final List a, return List.copyOf(result); } + private static Set namesOf(final List vars) { + if (vars.isEmpty()) return Collections.emptySet(); + if (vars.size() == 1) return Set.of(vars.get(0).getName()); + Set result = new LinkedHashSet<>(vars.size()); + for (VariableExpression ve : vars) result.add(ve.getName()); + return Collections.unmodifiableSet(result); + } + private static boolean isTypePattern(final Expression right) { return right instanceof DeclarationExpression decl && !decl.isMultipleAssignmentDeclaration() diff --git a/src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java b/src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java index 269edbac328..f2f0d9ffc7a 100644 --- a/src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java +++ b/src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java @@ -47,7 +47,7 @@ import org.codehaus.groovy.ast.tools.WideningCategories; import org.codehaus.groovy.classgen.AsmClassGenerator; import org.codehaus.groovy.classgen.BytecodeExpression; -import org.codehaus.groovy.classgen.VariableScopeVisitor.InstanceofPathLiveNames; +import org.codehaus.groovy.classgen.VariableScopeVisitor.InstanceofFlowBindings; import org.codehaus.groovy.runtime.MultipleAssignmentSupport; import org.codehaus.groovy.runtime.ScriptBytecodeAdapter; import org.codehaus.groovy.syntax.Token; @@ -1482,7 +1482,7 @@ private void evaluateTernaryExpression(final TernaryExpression expression) { MethodVisitor mv = controller.getMethodVisitor(); // load x; path-hide pattern locals via CompileStack push/hide/pop (GROOVY-12242) - InstanceofPathLiveNames pathNames = InstanceofPathLiveNames.get(expression); + InstanceofFlowBindings bindings = InstanceofFlowBindings.get(expression); Map beforePatterns = compileStack.snapshotPatternVariables(); boolPart.visit(controller.getAcg()); Set introduced = compileStack.patternVariablesIntroducedSince(beforePatterns); @@ -1490,7 +1490,7 @@ private void evaluateTernaryExpression(final TernaryExpression expression) { // true path: only whenTrue names visible among those this condition introduced compileStack.pushState(); - compileStack.hidePatternVariablesExcept(introduced, pathNames.whenTrue()); + compileStack.hidePatternVariablesExcept(introduced, bindings.whenTrueNames()); truePart.visit(controller.getAcg()); operandStack.doGroovyCast(commonType); compileStack.pop(); @@ -1500,7 +1500,7 @@ private void evaluateTernaryExpression(final TernaryExpression expression) { // false path: only whenFalse names visible mv.visitLabel(l0); compileStack.pushState(); - compileStack.hidePatternVariablesExcept(introduced, pathNames.whenFalse()); + compileStack.hidePatternVariablesExcept(introduced, bindings.whenFalseNames()); falsePart.visit(controller.getAcg()); operandStack.doGroovyCast(commonType); compileStack.pop(); diff --git a/src/main/java/org/codehaus/groovy/classgen/asm/StatementWriter.java b/src/main/java/org/codehaus/groovy/classgen/asm/StatementWriter.java index 9378122585d..a7ef2be3819 100644 --- a/src/main/java/org/codehaus/groovy/classgen/asm/StatementWriter.java +++ b/src/main/java/org/codehaus/groovy/classgen/asm/StatementWriter.java @@ -47,7 +47,7 @@ import org.codehaus.groovy.ast.stmt.WhileStatement; import org.codehaus.groovy.ast.CodeVisitorSupport; import org.codehaus.groovy.classgen.AsmClassGenerator; -import org.codehaus.groovy.classgen.VariableScopeVisitor.InstanceofPathLiveNames; +import org.codehaus.groovy.classgen.VariableScopeVisitor.InstanceofFlowBindings; import org.codehaus.groovy.classgen.asm.CompileStack.BlockRecorder; import org.codehaus.groovy.syntax.Types; import org.objectweb.asm.Label; @@ -483,9 +483,9 @@ public void writeDoWhileLoop(final DoWhileStatement statement) { * condition, CompileStack hides names that are not live on the current path * using {@link CompileStack#hidePatternVariablesExcept} inside * {@link CompileStack#pushState}/{@link CompileStack#pop} frames. Path-live - * name sets come from {@link InstanceofPathLiveNames} metadata written by - * {@link org.codehaus.groovy.classgen.VariableScopeVisitor} — classgen does - * not re-run the flow analysis. + * names come from {@link InstanceofFlowBindings} metadata attached by + * {@link org.codehaus.groovy.classgen.VariableScopeVisitor} — classgen + * reads names only and does not re-run the flow analysis. * * @param statement the if statement to compile */ @@ -494,7 +494,8 @@ public void writeIfElse(final IfStatement statement) { writeStatementLabel(statement); CompileStack compileStack = controller.getCompileStack(); - InstanceofPathLiveNames pathNames = InstanceofPathLiveNames.get(statement); + // Name view of the visitor's analysis; never re-analyse the condition. + InstanceofFlowBindings bindings = InstanceofFlowBindings.get(statement); // Define pattern slots on the *outer* frame so that after the breakable // push/pop the survivors remain ordinary stackVariables entries (no @@ -512,7 +513,7 @@ public void writeIfElse(final IfStatement statement) { // Then path: only whenTrue names among those this condition introduced. Label elsePath = controller.getOperandStack().jump(IFEQ); compileStack.pushState(); - compileStack.hidePatternVariablesExcept(introduced, pathNames.whenTrue()); + compileStack.hidePatternVariablesExcept(introduced, bindings.whenTrueNames()); statement.getIfBlock().visit(controller.getAcg()); compileStack.pop(); @@ -526,7 +527,7 @@ public void writeIfElse(final IfStatement statement) { mv.visitLabel(elsePath); // Else path: only whenFalse names among those this condition introduced. compileStack.pushState(); - compileStack.hidePatternVariablesExcept(introduced, pathNames.whenFalse()); + compileStack.hidePatternVariablesExcept(introduced, bindings.whenFalseNames()); statement.getElseBlock().visit(controller.getAcg()); compileStack.pop(); } @@ -534,10 +535,10 @@ public void writeIfElse(final IfStatement statement) { // Survivors per JLS §6.3.2.2-200-C (abrupt-completion rule). Set survivors = new HashSet<>(); if (!ifFallsThrough) { - survivors.addAll(pathNames.whenFalse()); + survivors.addAll(bindings.whenFalseNames()); } if (!elseEmpty && !elseFallsThrough) { - survivors.addAll(pathNames.whenTrue()); + survivors.addAll(bindings.whenTrueNames()); } survivors.retainAll(introduced); From 56145dcbb3c068d9a806a8e9d8db308f5679da30 Mon Sep 17 00:00:00 2001 From: Daniel Sun Date: Tue, 11 Aug 2026 00:01:17 +0900 Subject: [PATCH 5/6] GROOVY-12242: document breaking scope change and harden while and if breakable frame --- COMPATIBILITY.md | 28 +++++ .../groovy/classgen/VariableScopeVisitor.java | 39 +++++- .../groovy/classgen/asm/StatementWriter.java | 33 ++--- src/spec/doc/core-semantics.adoc | 39 ++++++ .../groovy/BreakContinueLabelTest.groovy | 44 +++++++ .../groovy/groovy/InstanceofScopeTest.groovy | 115 ++++++++++++++++++ src/test/groovy/groovy/InstanceofTest.groovy | 26 ++++ 7 files changed, 303 insertions(+), 21 deletions(-) diff --git a/COMPATIBILITY.md b/COMPATIBILITY.md index 4b7a3cae005..584cb703ba8 100644 --- a/COMPATIBILITY.md +++ b/COMPATIBILITY.md @@ -206,6 +206,34 @@ planned for deprecation/removal in a future Groovy version. Formal `@Deprecated` may be restored before 6 GA once beta feedback confirms indy remains acceptable for those use cases. +### Groovy 6 — `instanceof` pattern variable flow scoping (GROOVY-12242) + +Groovy 6 aligns JEP 394 / JLS §6.3-style *pattern variable* scoping for +`e instanceof T t` (and `!instanceof`) with Java for the common shapes: +`if`/`else` (including abrupt-completion survivors), short-circuit +`&&` / `||`, ternary/Elvis arms, and true-path bindings in `while` bodies. + +**Who is affected (silent behaviour change in dynamic Groovy).** Code that +previously treated a pattern variable as a local outside its JLS live range +— for example in an `else` branch of `if (o instanceof String s)`, after a +fall-through `if`, or on the RHS of `o instanceof String s || s.isEmpty()` — +now resolves that name as a dynamic property. At runtime that yields +`MissingPropertyException` (or a type-checker error under `@TypeChecked` / +`@CompileStatic`), with **no** additional compile-time warning in dynamic +mode. That is the intended Java-aligned semantics; Groovy 6 is the major +version for the change. + +**What is *not* claimed.** `while` / `do`-`while` deliberately get only +*partial* flow scoping: true-path bindings are available in a `while` body +and short-circuit rules apply in conditions, but Groovy does **not** +introduce false-path bindings after a loop when the body cannot complete +normally (JLS §6.3.2.3). Pattern names never leak past the loop. Full +after-loop introduction is a possible future enhancement, not a 6.0 gap to +back-port silently. + +Release notes for the 6.0 beta line should call this out (JIRA +`breaking` label on GROOVY-12242). + ### Groovy 6 — `CompilerConfiguration` copy constructor copies customizers (GROOVY-9585) `CompilerConfiguration(CompilerConfiguration)` now copies the source diff --git a/src/main/java/org/codehaus/groovy/classgen/VariableScopeVisitor.java b/src/main/java/org/codehaus/groovy/classgen/VariableScopeVisitor.java index b5ba95f5a2c..83b41387e30 100644 --- a/src/main/java/org/codehaus/groovy/classgen/VariableScopeVisitor.java +++ b/src/main/java/org/codehaus/groovy/classgen/VariableScopeVisitor.java @@ -608,7 +608,13 @@ public void visitCatchStatement(final CatchStatement statement) { } /** - * {@inheritDoc} + * Visits a {@code do}/{@code while} loop (GROOVY-12242). + *

    + * The body runs before the condition, so pattern variables from the + * condition are not in scope in the body (same as Java). The + * condition is visited in a nested scope so short-circuit RHS works and + * pattern names do not leak after the loop. Like {@link #visitWhileLoop}, + * there is no after-loop introduction of {@code whenFalse} bindings. */ @Override public void visitDoWhileLoop(final DoWhileStatement statement) { @@ -726,14 +732,37 @@ public void visitSwitch(final SwitchStatement statement) { } /** - * Visits a {@code while} loop. Pattern variables introduced by the condition - * are in scope for the condition's short-circuit RHS and for the loop body - * (GROOVY-12242). They do not leak past the loop. + * Visits a {@code while} loop with partial JEP 394 flow scoping + * for {@code instanceof} pattern variables (GROOVY-12242). + *

    + * What is supported (aligned with the if-then rule for the body): + *

      + *
    • Short-circuit visibility inside the condition ({@code &&} / {@code ||}).
      • + *
    • {@code e.whenTrue()} pattern variables are in scope in the loop body.
      • + *
    + *

    + * Intentional divergence from JLS §6.3.2.3 (while): + * Groovy does not introduce {@code e.whenFalse()} after the loop when + * the body cannot complete normally. Full after-loop introduction would need + * definite abrupt-completion analysis of every exit path (including + * {@code break}/{@code continue} of nested loops) and is left out for 6.0 — + * pattern variables never leak past the loop. Documented as a deliberate + * partial implementation, not an oversight. */ @Override public void visitWhileLoop(final WhileStatement statement) { + InstanceofFlowBindings bindings = InstanceofFlowBindings.of(statement.getBooleanExpression()); + + // Condition: short-circuit RHS only; discard pattern decls after the visit. pushState(); - super.visitWhileLoop(statement); + visitStatement(statement); + statement.getBooleanExpression().visit(this); + popState(); + + // Body: § if-then analogue — only e.whenTrue() (no after-loop whenFalse). + pushState(); + declarePatternVariables(bindings.whenTrue()); + statement.getLoopBlock().visit(this); popState(); } diff --git a/src/main/java/org/codehaus/groovy/classgen/asm/StatementWriter.java b/src/main/java/org/codehaus/groovy/classgen/asm/StatementWriter.java index a7ef2be3819..a412bba7b23 100644 --- a/src/main/java/org/codehaus/groovy/classgen/asm/StatementWriter.java +++ b/src/main/java/org/codehaus/groovy/classgen/asm/StatementWriter.java @@ -478,14 +478,20 @@ public void writeDoWhileLoop(final DoWhileStatement statement) { /** * Generates bytecode for an if/else statement. *

    - * GROOVY-12242 / JEP 394: {@code evaluateInstanceof} allocates pattern slots - * during the condition (needed for short-circuit {@code &&} RHS). After the - * condition, CompileStack hides names that are not live on the current path - * using {@link CompileStack#hidePatternVariablesExcept} inside - * {@link CompileStack#pushState}/{@link CompileStack#pop} frames. Path-live + * GROOVY-7463: a labeled {@code if} registers a breakable frame around the + * then arm only (same region as before GROOVY-12242). The else arm + * runs after that frame is popped so labeled-break scoping for the then + * arm is unchanged. Named break labels remain registered for the method + * (CompileStack does not un-register them on pop). + *

    + * GROOVY-12242 / JEP 394: pattern slots are allocated on the outer frame + * during the condition (needed for short-circuit {@code &&} RHS). Each arm + * then uses {@link CompileStack#pushState}/{@link CompileStack#hidePatternVariablesExcept}/ + * {@link CompileStack#pop} so only path-live names are visible. Path-live * names come from {@link InstanceofFlowBindings} metadata attached by * {@link org.codehaus.groovy.classgen.VariableScopeVisitor} — classgen - * reads names only and does not re-run the flow analysis. + * does not re-run the flow analysis. After both arms, non-survivors are + * permanently hidden on the outer frame (JLS §6.3.2.2-200-C). * * @param statement the if statement to compile */ @@ -497,13 +503,12 @@ public void writeIfElse(final IfStatement statement) { // Name view of the visitor's analysis; never re-analyse the condition. InstanceofFlowBindings bindings = InstanceofFlowBindings.get(statement); - // Define pattern slots on the *outer* frame so that after the breakable - // push/pop the survivors remain ordinary stackVariables entries (no - // put-back / show API). GROOVY-7463 breakable still wraps the arms only. + // Pattern slots on the outer frame so survivors need no put-back after pop. Map beforePatterns = compileStack.snapshotPatternVariables(); statement.getBooleanExpression().visit(controller.getAcg()); Set introduced = compileStack.patternVariablesIntroducedSince(beforePatterns); + // GROOVY-7463: breakable wraps then only (historical region). Label exitPath = compileStack.pushBreakable(statement.getStatementLabels()); boolean ifFallsThrough = maybeFallsThrough(statement.getIfBlock()); @@ -516,6 +521,7 @@ public void writeIfElse(final IfStatement statement) { compileStack.hidePatternVariablesExcept(introduced, bindings.whenTrueNames()); statement.getIfBlock().visit(controller.getAcg()); compileStack.pop(); + compileStack.pop(); // ends breakable (before else — same as pre-GROOVY-12242) MethodVisitor mv = controller.getMethodVisitor(); if (elseEmpty) { @@ -525,14 +531,14 @@ public void writeIfElse(final IfStatement statement) { mv.visitJumpInsn(GOTO, exitPath); } mv.visitLabel(elsePath); - // Else path: only whenFalse names among those this condition introduced. + // Else path (outside breakable): only whenFalse names. compileStack.pushState(); compileStack.hidePatternVariablesExcept(introduced, bindings.whenFalseNames()); statement.getElseBlock().visit(controller.getAcg()); compileStack.pop(); } - // Survivors per JLS §6.3.2.2-200-C (abrupt-completion rule). + // Survivors per JLS §6.3.2.2-200-C on the outer frame. Set survivors = new HashSet<>(); if (!ifFallsThrough) { survivors.addAll(bindings.whenFalseNames()); @@ -541,11 +547,6 @@ public void writeIfElse(final IfStatement statement) { survivors.addAll(bindings.whenTrueNames()); } survivors.retainAll(introduced); - - // Leave the breakable frame: outer map still holds all pattern slots from - // the condition (they were defined before pushBreakable). Permanently hide - // non-survivors on the outer frame. - compileStack.pop(); // ends breakable compileStack.hidePatternVariablesExcept(introduced, survivors); mv.visitLabel(exitPath); } diff --git a/src/spec/doc/core-semantics.adoc b/src/spec/doc/core-semantics.adoc index 1f4319c9819..55dfaed9ec1 100644 --- a/src/spec/doc/core-semantics.adoc +++ b/src/spec/doc/core-semantics.adoc @@ -485,6 +485,45 @@ It is possible to use labels in the `break` instruction as a target for jump, as include::../test/semantics/LabelsTest.groovy[tags=break_label,indent=0] ---- +=== `instanceof` pattern variables (JEP 394) + +Since Groovy 6, a type pattern on `instanceof` / `!instanceof` introduces a +*pattern variable* whose scope follows Java's flow rules (JLS §6.3 / JEP 394) +for the common shapes used in application code: + +[source,groovy] +---- +Object o = 'hi' +if (o instanceof String s) { + assert s.toUpperCase() == 'HI' // s is a local in the then-block +} else { + // s is *not* in scope here (dynamic: MissingPropertyException; static: error) +} +// s is not in scope after a fall-through if either +---- + +Short-circuit boolean operators and ternary arms respect the same “definitely +bound on this path” rules (`&&` exposes true-path bindings on the right-hand +side; `||` does not). Under `@TypeChecked` / `@CompileStatic` the type checker +rejects out-of-scope uses at compile time; in dynamic Groovy the same uses +become dynamic property lookups and typically throw +`groovy.lang.MissingPropertyException` at runtime. + +[NOTE] +==== +*While loops (intentional partial support).* A `while` condition's true-path +pattern variables are in scope in the loop body, and short-circuit rules apply +inside the condition. Groovy does *not* introduce false-path bindings *after* +the loop when the body cannot complete normally (unlike the full JLS while +rule). Pattern names never leak past the loop. Full after-loop introduction may +be considered later; it is not part of the 6.0 contract. +==== + +This is a **breaking change** from earlier Groovy releases that left pattern +locals visible more widely in dynamic mode. See +link:https://issues.apache.org/jira/browse/GROOVY-12242[GROOVY-12242] and +`COMPATIBILITY.md`. + == Expressions Expressions are the building blocks of Groovy programs that are used to reference diff --git a/src/test/groovy/groovy/BreakContinueLabelTest.groovy b/src/test/groovy/groovy/BreakContinueLabelTest.groovy index 43723263bb8..03a08ec31d2 100644 --- a/src/test/groovy/groovy/BreakContinueLabelTest.groovy +++ b/src/test/groovy/groovy/BreakContinueLabelTest.groovy @@ -80,6 +80,50 @@ final class BreakContinueLabelTest { ''' } + /** + * GROOVY-7463 + GROOVY-12242: breakable frame for a labeled {@code if} + * still ends before the else arm (historical region). A break from the + * then-arm of a nested labeled if remains supported; a break from the + * else-arm to the outer if's label is still resolved via the method-level + * named-break map (CompileStack does not un-register names on pop). + */ + @Test + void testBreakLabelFromElseOfLabeledIf() { + assertScript ''' + def log = [] + label: + if (false) { + log << 'then' + assert false + } else { + log << 'else' + break label + log << 'after-break' + } + log << 'after-if' + assert log == ['else', 'after-if'] + ''' + } + + // GROOVY-12242: labeled if + instanceof pattern in then still breaks correctly + @Test + void testBreakLabelInIfWithInstanceofPattern() { + assertScript ''' + def log = [] + Object o = 'x' + label: + if (o instanceof String s) { + log << s + break label + log << 'dead' + } else { + log << 'else' + } + log << 'done' + assert log == ['x', 'done'] + ''' + } + @Test void testBreakLabelInSimpleForLoop() { assertScript ''' diff --git a/src/test/groovy/groovy/InstanceofScopeTest.groovy b/src/test/groovy/groovy/InstanceofScopeTest.groovy index f9a97eeb3b9..18e82cacbee 100644 --- a/src/test/groovy/groovy/InstanceofScopeTest.groovy +++ b/src/test/groovy/groovy/InstanceofScopeTest.groovy @@ -532,6 +532,121 @@ final class InstanceofScopeTest { assert !isLocal(accesses[1]) : "ternary false-expr: s must be dynamic (condition.whenFalse={})" } + // ------------------------------------------------------------------------- + // while / do-while (intentional partial flow scoping) + // ------------------------------------------------------------------------- + + /** + * while body sees e.whenTrue only. Positive instanceof → s local in body. + */ + @Test + void testWhile_positiveInstanceof_bodyLocal_afterDynamic() { + def src = ''' + class C { + def m(Object o) { + while (o instanceof String s) { + s.length() // body → local + break + } + s // after → dynamic (no after-loop introduction) + } + } + ''' + def accesses = collectAccesses(src) + assert accesses.size() == 2 + assert isLocal(accesses[0]) : "while body: s must be local (whenTrue)" + assert !isLocal(accesses[1]) : "after while: s must be dynamic (no JLS after-loop intro)" + } + + /** + * while (!(o instanceof String s)): whenTrue={}, so body must *not* see s + * as a local. After the loop still dynamic (partial support). + */ + @Test + void testWhile_negatedInstanceof_bodyDynamic_afterDynamic() { + def src = ''' + class C { + def m(Object o) { + while (!(o instanceof String s)) { + s // body → dynamic (whenTrue={}) + break + } + s // after → dynamic + } + } + ''' + def accesses = collectAccesses(src) + assert accesses.size() == 2 + assert !isLocal(accesses[0]) : "while body of negated: s must be dynamic" + assert !isLocal(accesses[1]) : "after while: s must be dynamic" + } + + /** + * Even when the while body cannot complete normally, Groovy does not + * introduce whenFalse after the loop (documented divergence from JLS). + */ + @Test + void testWhile_abruptBody_stillNoAfterLoopIntroduction() { + def src = ''' + class C { + def m(Object o) { + while (!(o instanceof String s)) { + return 'in' + } + s // still dynamic — no after-loop whenFalse + } + } + ''' + def accesses = collectAccesses(src) + assert accesses.size() == 1 + assert !isLocal(accesses[0]) : "after while with abrupt body: no whenFalse introduction" + } + + /** + * {@code while (o instanceof String s || cond)}: whenTrue is empty, so the + * body must not see s as a local (|| has no whenTrue rule). + */ + @Test + void testWhile_orCondition_bodyDynamic() { + def src = ''' + class C { + def m(Object o) { + while (o instanceof String s || false) { + s // body → dynamic + break + } + } + } + ''' + def accesses = collectAccesses(src) + assert accesses.size() == 1 + assert !isLocal(accesses[0]) : "while body of || condition: s must be dynamic" + } + + /** + * do-while: body runs before condition — s not in body; condition RHS of && + * may be local. After loop still dynamic. + */ + @Test + void testDoWhile_bodyDynamic_conditionRhsLocal_afterDynamic() { + def src = ''' + class C { + def m(Object o) { + do { + s // body → dynamic + } while (o instanceof String s && s.length() > 0) + s // after → dynamic + } + } + ''' + def accesses = collectAccesses(src) + // body s, condition s.length(), after s + assert accesses.size() == 3 + assert !isLocal(accesses[0]) : "do-while body: s must be dynamic" + assert isLocal(accesses[1]) : "do-while condition && RHS: s must be local" + assert !isLocal(accesses[2]) : "after do-while: s must be dynamic" + } + // ------------------------------------------------------------------------- // Redeclaration where pattern variable is NOT visible → new local is OK // ------------------------------------------------------------------------- diff --git a/src/test/groovy/groovy/InstanceofTest.groovy b/src/test/groovy/groovy/InstanceofTest.groovy index 29c4814b0cf..168e125c339 100644 --- a/src/test/groovy/groovy/InstanceofTest.groovy +++ b/src/test/groovy/groovy/InstanceofTest.groovy @@ -432,6 +432,32 @@ final class InstanceofTest { assert o == '' } + // GROOVY-12242: negated while condition — s not in body; not after (partial JLS) + @Test + void testWhileNegated_bodyAndAfterNotVisible() { + def err = shouldFail MissingPropertyException, ''' + def m(Object o) { + while (!(o instanceof String s)) { + return s + } + return 'out' + } + m(1) + ''' + assert err.message =~ /No such property: s/ + + err = shouldFail MissingPropertyException, ''' + def m(Object o) { + while (!(o instanceof String s)) { + return 'in' + } + return s + } + m('hi') + ''' + assert err.message =~ /No such property: s/ + } + // GROOVY-12242: reuse the same pattern variable name in successive statements @Test void testVariableNameReuse() { From 9aa4dedfb00019bd0593d00389b07144058a2624 Mon Sep 17 00:00:00 2001 From: Daniel Sun Date: Tue, 11 Aug 2026 00:44:17 +0900 Subject: [PATCH 6/6] GROOVY-12242: support type patterns on native !instanceof Parse e !instanceof T t via matchingType (keeping parenthesised (T) form), isolate pattern declare for COMPARE_NOT_INSTANCEOF and short-circuit &&, and cover runtime, AST scope, flow analysis, and parser cases. --- COMPATIBILITY.md | 7 +- src/antlr/GroovyParser.g4 | 11 ++- .../groovy/parser/antlr4/AstBuilder.java | 28 ++++-- .../groovy/classgen/VariableScopeVisitor.java | 28 +++++- .../classgen/asm/BinaryExpressionHelper.java | 13 +++ src/spec/doc/core-semantics.adoc | 21 ++-- .../groovy/InstanceofFlowBindingsTest.groovy | 24 +++++ .../groovy/groovy/InstanceofScopeTest.groovy | 97 +++++++++++++++++++ src/test/groovy/groovy/InstanceofTest.groovy | 89 +++++++++++++++++ .../antlr4/IntersectionCastParserTest.groovy | 8 ++ 10 files changed, 299 insertions(+), 27 deletions(-) diff --git a/COMPATIBILITY.md b/COMPATIBILITY.md index 584cb703ba8..8f026ab81b5 100644 --- a/COMPATIBILITY.md +++ b/COMPATIBILITY.md @@ -209,9 +209,10 @@ indy remains acceptable for those use cases. ### Groovy 6 — `instanceof` pattern variable flow scoping (GROOVY-12242) Groovy 6 aligns JEP 394 / JLS §6.3-style *pattern variable* scoping for -`e instanceof T t` (and `!instanceof`) with Java for the common shapes: -`if`/`else` (including abrupt-completion survivors), short-circuit -`&&` / `||`, ternary/Elvis arms, and true-path bindings in `while` bodies. +`e instanceof T t` and `e !instanceof T t` (equivalent to +`!(e instanceof T t)`) with Java for the common shapes: `if`/`else` +(including abrupt-completion survivors), short-circuit `&&` / `||`, +ternary/Elvis arms, and true-path bindings in `while` bodies. **Who is affected (silent behaviour change in dynamic Groovy).** Code that previously treated a pattern variable as a local outside its JLS live range diff --git a/src/antlr/GroovyParser.g4 b/src/antlr/GroovyParser.g4 index 7384daca9ea..41b3c078bc8 100644 --- a/src/antlr/GroovyParser.g4 +++ b/src/antlr/GroovyParser.g4 @@ -352,10 +352,16 @@ referenceType : qualifiedClassName typeArguments? ; -matchingType // see: instanceof +matchingType // see: instanceof / !instanceof type patterns (JEP 394) : standardType identifier? ; +// RHS of !instanceof: Type / Type name (pattern), or parenthesised (T) / rejected (A & B) +notInstanceofType + : matchingType + | castParExpression + ; + standardType // see: returnType options { baseContext = type; } : annotationsOpt @@ -846,7 +852,8 @@ expression // boolean relational expressions (level 7) | left=expression nls op=INSTANCEOF nls matchingType #relationalExprAlt - | left=expression nls op=(AS | NOT_INSTANCEOF) nls coercionType #relationalExprAlt + | left=expression nls op=NOT_INSTANCEOF nls notInstanceofType #relationalExprAlt + | left=expression nls op=AS nls coercionType #relationalExprAlt | left=expression nls op=(LE | GE | GT | LT | IN | NOT_IN) nls right=expression #relationalExprAlt // equality/inequality (==/!=) (level 8) diff --git a/src/main/java/org/apache/groovy/parser/antlr4/AstBuilder.java b/src/main/java/org/apache/groovy/parser/antlr4/AstBuilder.java index 5152b960edd..e0d05dd7fd0 100644 --- a/src/main/java/org/apache/groovy/parser/antlr4/AstBuilder.java +++ b/src/main/java/org/apache/groovy/parser/antlr4/AstBuilder.java @@ -3232,22 +3232,30 @@ public Expression visitRelationalExprAlt(final RelationalExprAltContext ctx) { ctx); case NOT_INSTANCEOF: { - CoercionTypeContext coercionCtx = ctx.coercionType(); - if (coercionCtx.castParExpression() != null - && coercionCtx.castParExpression().intersectionType().type().size() > 1) { + // GROOVY-12242: !instanceof Type name (JEP 394 pattern) via matchingType; + // parenthesised (T) / rejected intersection (A & B) via castParExpression. + NotInstanceofTypeContext nit = ctx.notInstanceofType(); + if (nit.matchingType() != null) { + nit.matchingType().putNodeMetaData(IS_INSIDE_INSTANCEOF_EXPR, Boolean.TRUE); + return configureAST( + new BinaryExpression( + (Expression) this.visit(ctx.left), + this.createGroovyToken(ctx.op), + this.visitMatchingType(nit.matchingType())), + ctx); + } + CastParExpressionContext castCtx = nit.castParExpression(); + if (castCtx.intersectionType().type().size() > 1) { throw this.createParsingFailedException("Intersection types are not supported as the right-hand side of !instanceof", ctx); } - ClassNode notInstType = this.visitCoercionType(coercionCtx); - // GROOVY-11998: keep IS_INSIDE_INSTANCEOF_EXPR on the parser context for the resolver - (coercionCtx.type() != null - ? coercionCtx.type() - : coercionCtx.castParExpression().intersectionType().type(0) - ).putNodeMetaData(IS_INSIDE_INSTANCEOF_EXPR, Boolean.TRUE); + // GROOVY-11998: mark the type context for the resolver (parameterized RHS) + castCtx.intersectionType().type(0).putNodeMetaData(IS_INSIDE_INSTANCEOF_EXPR, Boolean.TRUE); + ClassNode notInstType = this.visitCastParExpression(castCtx); return configureAST( new BinaryExpression( (Expression) this.visit(ctx.left), this.createGroovyToken(ctx.op), - configureAST(new ClassExpression(notInstType), coercionCtx)), + configureAST(new ClassExpression(notInstType), castCtx)), ctx); } diff --git a/src/main/java/org/codehaus/groovy/classgen/VariableScopeVisitor.java b/src/main/java/org/codehaus/groovy/classgen/VariableScopeVisitor.java index 83b41387e30..330200c4c56 100644 --- a/src/main/java/org/codehaus/groovy/classgen/VariableScopeVisitor.java +++ b/src/main/java/org/codehaus/groovy/classgen/VariableScopeVisitor.java @@ -798,19 +798,28 @@ public void visitArrayExpression(final ArrayExpression expression) { *

  • {@code a && b} — true-path bindings of {@code a} are in scope in {@code b}
    • *
  • {@code a || b} — true-path bindings of {@code a} are not in scope in {@code b}; * false-path bindings of {@code a} are
    • + *
  • {@code e !instanceof T t} / {@code !(e instanceof T t)} — pattern declare is + * isolated on the left of short-circuit ops; only flow-live sets are re-introduced
    • *
*/ @Override public void visitBinaryExpression(final BinaryExpression expression) { int op = expression.getOperation().getType(); if (op == Types.LOGICAL_AND) { - // Left first; its true-path pattern vars stay in the current scope for the right. + // Symmetric to ||: isolate left's declares, then expose only whenTrue on the right. + // Fixes `e !instanceof T t && t.m()` and `!(e instanceof T t) && t.m()` (whenTrue={}). + InstanceofFlowBindings leftBindings = InstanceofFlowBindings.of(expression.getLeftExpression()); + pushState(); expression.getLeftExpression().visit(this); + popState(); + pushState(); + declarePatternVariables(leftBindings.whenTrue()); expression.getRightExpression().visit(this); + popState(); } else if (op == Types.LOGICAL_OR) { // Left's true-path bindings must not leak into the right (Java rejects // `o instanceof String s || s.isEmpty()`). False-path bindings of the - // left are in scope on the right (`!(o instanceof String s) || s.isEmpty()`). + // left are in scope on the right (`o !instanceof String s || s.isEmpty()`). InstanceofFlowBindings leftBindings = InstanceofFlowBindings.of(expression.getLeftExpression()); pushState(); expression.getLeftExpression().visit(this); @@ -819,6 +828,14 @@ public void visitBinaryExpression(final BinaryExpression expression) { declarePatternVariables(leftBindings.whenFalse()); expression.getRightExpression().visit(this); popState(); + } else if (op == Types.COMPARE_NOT_INSTANCEOF) { + // Defence in depth: pattern declare on the RHS must not stick to the + // enclosing scope (whenTrue of !instanceof is empty). Live paths + // re-introduce via declarePatternVariables / short-circuit handlers. + expression.getLeftExpression().visit(this); + pushState(); + expression.getRightExpression().visit(this); + popState(); } else { super.visitBinaryExpression(expression); } @@ -1064,9 +1081,10 @@ public void visitVariableExpression(final VariableExpression expression) { * metadata ({@link #put}/{@link #get}). Later phases must use * {@link #get} only — never re-call {@link #of} on the condition. *

- * Covered shapes: {@code e instanceof T t}, {@code e !instanceof T t}, - * {@code !expr}, {@code a && b}, {@code a || b}. All other shapes yield - * {@link #EMPTY} (conservative: no definite bindings). + * Covered shapes: {@code e instanceof T t}, {@code e !instanceof T t} + * (native form and {@code !(e instanceof T t)}), {@code !expr}, + * {@code a && b}, {@code a || b}. All other shapes yield {@link #EMPTY} + * (conservative: no definite bindings). * * @see VariableScopeVisitor * @since 6.0.0 diff --git a/src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java b/src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java index f2f0d9ffc7a..bf3bf573687 100644 --- a/src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java +++ b/src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionHelper.java @@ -1192,6 +1192,19 @@ private void evaluateInstanceof(final BinaryExpression expression) { } } + /** + * Emits bytecode for {@code e !instanceof T} and for the JEP 394 + * type pattern form {@code e !instanceof T t}. + *

+ * Implemented as {@code !(e instanceof T [t])} so pattern store logic in + * {@link #evaluateInstanceof} is shared: the pattern local is assigned when + * the value is an instance of {@code T} (i.e. when the overall + * {@code !instanceof} result is false). Path visibility follows + * {@code InstanceofFlowBindings} for {@code COMPARE_NOT_INSTANCEOF} + * (whenTrue/whenFalse swapped relative to plain {@code instanceof}). + * + * @param expression a {@code !instanceof} binary expression + */ private void evaluateNotInstanceof(final BinaryExpression expression) { unaryExpressionHelper.writeNotExpression( notX( diff --git a/src/spec/doc/core-semantics.adoc b/src/spec/doc/core-semantics.adoc index 55dfaed9ec1..3efc02e1610 100644 --- a/src/spec/doc/core-semantics.adoc +++ b/src/spec/doc/core-semantics.adoc @@ -485,9 +485,9 @@ It is possible to use labels in the `break` instruction as a target for jump, as include::../test/semantics/LabelsTest.groovy[tags=break_label,indent=0] ---- -=== `instanceof` pattern variables (JEP 394) +=== `instanceof` / `!instanceof` pattern variables (JEP 394) -Since Groovy 6, a type pattern on `instanceof` / `!instanceof` introduces a +Since Groovy 6, a type pattern on `instanceof` or `!instanceof` introduces a *pattern variable* whose scope follows Java's flow rules (JLS §6.3 / JEP 394) for the common shapes used in application code: @@ -500,13 +500,20 @@ if (o instanceof String s) { // s is *not* in scope here (dynamic: MissingPropertyException; static: error) } // s is not in scope after a fall-through if either + +if (o !instanceof Integer i) { + // i is *not* in scope here (condition true means o is not an Integer) +} else { + assert i instanceof Integer // i is bound when !instanceof is false +} ---- -Short-circuit boolean operators and ternary arms respect the same “definitely -bound on this path” rules (`&&` exposes true-path bindings on the right-hand -side; `||` does not). Under `@TypeChecked` / `@CompileStatic` the type checker -rejects out-of-scope uses at compile time; in dynamic Groovy the same uses -become dynamic property lookups and typically throw +The form `e !instanceof T t` is equivalent to `!(e instanceof T t)` for both +binding and scoping. Short-circuit boolean operators and ternary arms respect +the same “definitely bound on this path” rules (`&&` exposes true-path bindings +on the right-hand side; `||` does not). Under `@TypeChecked` / `@CompileStatic` +the type checker rejects out-of-scope uses at compile time; in dynamic Groovy +the same uses become dynamic property lookups and typically throw `groovy.lang.MissingPropertyException` at runtime. [NOTE] diff --git a/src/test/groovy/groovy/InstanceofFlowBindingsTest.groovy b/src/test/groovy/groovy/InstanceofFlowBindingsTest.groovy index 2645ab4086e..b6ba66a08a2 100644 --- a/src/test/groovy/groovy/InstanceofFlowBindingsTest.groovy +++ b/src/test/groovy/groovy/InstanceofFlowBindingsTest.groovy @@ -46,6 +46,30 @@ final class InstanceofFlowBindingsTest { assert b.allNames() as List == ['s'] } + @Test + void testBindingsOfNativeNotInstanceofPattern() { + def b = InstanceofFlowBindings.of(parseCondition('o !instanceof String s')) + assert b.whenTrue().isEmpty() + assert b.whenFalse()*.name == ['s'] + assert b.allNames() as List == ['s'] + // equivalent to !(o instanceof String s) + def negated = InstanceofFlowBindings.of(parseCondition('!(o instanceof String s)')) + assert b.whenTrue()*.name == negated.whenTrue()*.name + assert b.whenFalse()*.name == negated.whenFalse()*.name + } + + @Test + void testContainsPatternNativeNotInstanceof() { + assert InstanceofFlowBindings.containsPattern(parseCondition('o !instanceof String s')) + assert !InstanceofFlowBindings.containsPattern(parseCondition('o !instanceof String')) + } + + @Test + void testAllPatternNamesNativeNotInstanceof() { + def expr = parseCondition('o !instanceof String s') + assert InstanceofFlowBindings.allPatternNames(expr) == ['s'] as Set + } + @Test void testBindingsOfAnd() { def b = InstanceofFlowBindings.of(parseCondition('o instanceof String s && s.length() > 0')) diff --git a/src/test/groovy/groovy/InstanceofScopeTest.groovy b/src/test/groovy/groovy/InstanceofScopeTest.groovy index 18e82cacbee..85f7567552f 100644 --- a/src/test/groovy/groovy/InstanceofScopeTest.groovy +++ b/src/test/groovy/groovy/InstanceofScopeTest.groovy @@ -52,6 +52,7 @@ import org.junit.jupiter.api.Test * 1 | o instanceof String s | local | dynamic | dynamic (*) * 1b | o instanceof String s, else abrupt | local | (abrupt) | local * 2 | !(o instanceof String s) | dynamic | local | — + * 2n | o !instanceof String s (native) | dynamic | local | dynamic (≡ 2) * 3 | !(o instanceof String s), if abrupt | (abrupt) | local | local * 4 | o instanceof String s && cond | local | dynamic | dynamic * 5 | o instanceof String s || cond | dynamic | dynamic | dynamic @@ -63,6 +64,7 @@ import org.junit.jupiter.api.Test * (*) s NOT visible after when both branches fall through. * (**) missing case: abrupt else alone is not enough; if-block must * also be abrupt for §6.3.2.2-200-C-B to apply. + * (2n) native {@code !instanceof} is equivalent to {@code !(instanceof)}. */ final class InstanceofScopeTest { @@ -102,6 +104,101 @@ final class InstanceofScopeTest { !(v instanceof DynamicVariable) } + // ------------------------------------------------------------------------- + // Native !instanceof Type name (equivalent to !(instanceof), JEP 394) + // ------------------------------------------------------------------------- + + /** + * {@code o !instanceof String s} ≡ {@code !(o instanceof String s)}: + * whenTrue={}, whenFalse={s} → then dynamic, else local, after dynamic. + */ + @Test + void testNativeNotInstanceof_ifBlockDynamic_elseLocal_afterDynamic() { + def src = ''' + class C { + def m(Object o) { + if (o !instanceof String s) { + s // then → dynamic + } else { + s.length() // else → local + } + s // after → dynamic + } + } + ''' + def accesses = collectAccesses(src) + assert accesses.size() == 3 + assert !isLocal(accesses[0]) : "then of !instanceof: s must be dynamic" + assert isLocal(accesses[1]) : "else of !instanceof: s must be local" + assert !isLocal(accesses[2]) : "after !instanceof if: s must be dynamic" + } + + /** + * Early return in then of {@code !instanceof}: whenFalse={s} survives after + * (same as {@code !(instanceof)} with abrupt then). + */ + @Test + void testNativeNotInstanceof_abruptThen_afterLocal() { + def src = ''' + class C { + def m(Object o) { + if (o !instanceof String s) return + s.length() // after → local (survivor) + } + } + ''' + def accesses = collectAccesses(src) + assert accesses.size() == 1 + assert isLocal(accesses[0]) : "after abrupt then of !instanceof: s must be local" + } + + /** + * {@code &&} RHS of native {@code !instanceof} and of {@code !(instanceof)} + * must both be dynamic (whenTrue empty) — short-circuit isolation is symmetric. + */ + @Test + void testNotInstanceof_andRhsDynamic_nativeAndNegatedForms() { + for (String cond : [ + 'o !instanceof String s && s.isEmpty()', + '!(o instanceof String s) && s.isEmpty()', + ]) { + def src = """ + class C { + def m(Object o) { + ${cond} + } + } + """ + def accesses = collectAccesses(src) + assert accesses.size() == 1 : "expected one use of s in: $cond" + assert !isLocal(accesses[0]) : "&& RHS must be dynamic for: $cond" + } + } + + /** + * Redeclare in then of {@code !instanceof} is allowed (whenTrue empty). + */ + @Test + void testNativeNotInstanceof_redeclareInThen_isLocal() { + def src = ''' + class C { + def m(Object o) { + if (o !instanceof String s) { + def s = 'x' + s + } else { + s.length() + } + } + } + ''' + def accesses = collectAccesses(src) + assert accesses.size() == 2 + assert isLocal(accesses[0]) : "then redeclare: local" + assert isLocal(accesses[1]) : "else pattern: local" + assert !accesses[0].is(accesses[1]) + } + // ------------------------------------------------------------------------- // Case 1: o instanceof String s // ------------------------------------------------------------------------- diff --git a/src/test/groovy/groovy/InstanceofTest.groovy b/src/test/groovy/groovy/InstanceofTest.groovy index 168e125c339..9257ad8bda7 100644 --- a/src/test/groovy/groovy/InstanceofTest.groovy +++ b/src/test/groovy/groovy/InstanceofTest.groovy @@ -96,6 +96,95 @@ final class InstanceofTest { assert (n instanceof Integer i && i.intValue() == 12345) } + // GROOVY-12242: native !instanceof type pattern (JEP 394) + @Test + void testNotInstanceofPatternVariable() { + def n = (Number) 12345 + if (n !instanceof String s) { + assert n.intValue() == 12345 + } else { + assert false : 'expected non-String' + } + if (n !instanceof Integer i) { + assert false : 'expected Integer (condition false)' + } else { + assert i.intValue() == 12345 + } + } + + // GROOVY-12242: !instanceof pattern + short-circuit / early return survivors + @Test + void testNotInstanceofPatternScope() { + def f = { Object o -> + if (o !instanceof String s) { + return 'not-string' + } else { + return s.toUpperCase() + } + } + assert f('hi') == 'HI' + assert f(1) == 'not-string' + + def g = { Object o -> + if (o !instanceof String s) return 'early' + return s.toUpperCase() + } + assert g('ab') == 'AB' + assert g(9) == 'early' + } + + // GROOVY-12242: !instanceof pattern must not leak into then-block + @Test + void testNotInstanceofPatternThenBlockDynamic() { + def err = shouldFail MissingPropertyException, ''' + def m(Object o) { + if (o !instanceof String s) { + return s + } + return 'ok' + } + m(1) + ''' + assert err.message =~ /No such property: s/ + } + + // GROOVY-12242: ternary with !instanceof pattern + @Test + void testNotInstanceofPatternTernary() { + def f = { Object o -> + (o !instanceof String s) ? 'not' : s.toUpperCase() + } + assert f(1) == 'not' + assert f('xy') == 'XY' + } + + // GROOVY-12242: && / || with !instanceof pattern + @Test + void testNotInstanceofPatternBooleanOps() { + // true path of !instanceof is empty — RHS of && must not see s as bound from left + def err = shouldFail MissingPropertyException, ''' + def m(Object o) { + return (o !instanceof String s && s.isEmpty()) + } + m(1) + ''' + assert err.message =~ /No such property: s/ + + // Same for parenthesised negation form (&& isolation is flow-based) + err = shouldFail MissingPropertyException, ''' + def m(Object o) { + return (!(o instanceof String s) && s.isEmpty()) + } + m(1) + ''' + assert err.message =~ /No such property: s/ + + // false path of !instanceof binds s — RHS of || can use s when left is false + assert (({ Object o -> (o !instanceof String s || s.isEmpty()) }('')) == true) + assert (({ Object o -> (o !instanceof String s || s.length() > 0) }('ab')) == true) + assert (({ Object o -> (o !instanceof String s || s.isEmpty()) }(1)) == true) + } + // GROOVY-11229 @Test void testVariable2() { diff --git a/src/test/groovy/org/apache/groovy/parser/antlr4/IntersectionCastParserTest.groovy b/src/test/groovy/org/apache/groovy/parser/antlr4/IntersectionCastParserTest.groovy index ae278f787dd..f6517f43df8 100644 --- a/src/test/groovy/org/apache/groovy/parser/antlr4/IntersectionCastParserTest.groovy +++ b/src/test/groovy/org/apache/groovy/parser/antlr4/IntersectionCastParserTest.groovy @@ -115,6 +115,14 @@ final class IntersectionCastParserTest { assertTrue(!ast.context.errorCollector.hasErrors()) } + @Test + void 'type pattern accepted for !instanceof'() { + ModuleNode ast = buildAST('def b = x !instanceof String s') + assertNotNull(ast) + assertTrue(!ast.context.errorCollector.hasErrors(), + "Parse should accept !instanceof type pattern; got: ${ast.context.errorCollector.errors}") + } + //-------------------------------------------------------------------------- private static ClassNode singleCastTargetType(String src) {