[
https://issues.apache.org/jira/browse/GROOVY-12242?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18103029#comment-18103029
]
ASF GitHub Bot commented on GROOVY-12242:
-----------------------------------------
daniellansun commented on code in PR #2773:
URL: https://github.com/apache/groovy/pull/2773#discussion_r3740601359
##########
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).
+ * <p>
+ * This is pure <em>semantic</em> analysis: given a boolean expression, which
+ * pattern variables are <em>definitely bound</em> when the expression is
+ * {@code true} versus {@code false}? (Same idea as compiler “flow info” /
+ * JEP 394 flow scoping — not a bytecode construct.)
+ * <ul>
+ * <li>{@link #of(Expression)} — true/false binding sets for a condition</li>
+ * <li>{@link #containsPattern(Expression)} — nested type-pattern presence
+ * (e.g. whether an expression statement needs CompileStack
isolation)</li>
+ * </ul>
+ * 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).
+ * <p>
+ * Consumers:
+ * <ul>
+ * <li>{@link VariableScopeVisitor} — declare names on the live path</li>
+ * <li>{@link org.codehaus.groovy.classgen.asm.InstanceofFlowSlotPublisher} —
+ * publish/hide CompileStack slots from these bindings</li>
+ * </ul>
+ *
+ * @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<VariableExpression> whenTrue;
+ private final List<VariableExpression> whenFalse;
+
+ private InstanceofFlowBindings(final List<VariableExpression> whenTrue,
+ final List<VariableExpression>
whenFalse) {
+ this.whenTrue = whenTrue;
+ this.whenFalse = whenFalse;
+ }
+
+ /**
+ * Pattern variables that are definitely assigned when the analysed
expression
+ * evaluates to {@code true}.
+ */
+ public List<VariableExpression> whenTrue() {
+ return whenTrue;
+ }
+
+ /**
+ * Pattern variables that are definitely assigned when the analysed
expression
+ * evaluates to {@code false}.
+ */
+ public List<VariableExpression> 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<String> whenTrueNames() {
+ return names(whenTrue);
+ }
+
+ /**
+ * Names of pattern variables bound when the expression is {@code false}.
+ */
+ public Set<String> whenFalseNames() {
+ return names(whenFalse);
+ }
+
+ /**
+ * All pattern-variable names appearing in either path (stable encounter
order).
+ */
+ public Set<String> allNames() {
+ if (isEmpty()) return Collections.emptySet();
+ Set<String> 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<String> names(final List<VariableExpression> vars) {
+ if (vars.isEmpty()) return Collections.emptySet();
+ Set<String> 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));
+ }
+ }
Review Comment:
There are two methods with deliberately different traversal strategies:
**`containsPattern(Expression)` — intentionally traverses the full subtree.**
This method answers a conservative question: *"does any descendant node in
this expression
tree contain a type pattern?"* It is used for expression-statement isolation
(deciding
whether the statement's value needs CompileStack sandboxing). Even a pattern
buried
arbitrarily deep (e.g. inside a method-call argument: `print(o instanceof
String s)`) may
have caused `evaluateInstanceof` to define a CompileStack slot. That
worst-case question
requires a full subtree scan. Short-circuit optimisation (`found[0]`
early-return) keeps
the typical cost proportional to the depth of the first match, not the full
tree size.
**`analyse(Expression)` — does *not* traverse the full tree.** It is a
recursive structural
descent that follows only the operators that propagate *definite-assignment*
(`instanceof`,
`!instanceof`, `&&`, `||`, `!`) and returns `EMPTY` conservatively for any
other shape.
For a typical condition `o instanceof String s && s.length() > 0` the
descent visits exactly
three nodes.
Both methods now have explicit **"Why a full subtree walk?"** / **"Why not a
full subtree
walk?"** Javadoc paragraphs to document this distinction for future
contributors.
> instanceof pattern variable scope is not aligned with Java flow scoping (JEP
> 394)
> ---------------------------------------------------------------------------------
>
> Key: GROOVY-12242
> URL: https://issues.apache.org/jira/browse/GROOVY-12242
> Project: Groovy
> Issue Type: Bug
> Reporter: Daniel Sun
> Priority: Major
>
> h2. Summary
> After {{instanceof}} type patterns landed in GROOVY-11229, pattern variables
> were still scoped with a coarse lexical approximation. That diverges from
> Java’s *flow scoping* (JEP 394): a pattern variable must be visible only
> where the pattern has *definitely* matched.
> The gaps appear as:
> # variables missing where Java allows them
> # variables leaking past the statement that introduced them
> # name resolution and bytecode disagreeing, so an “out of scope” use can
> still load a local slot
> h2. Background
> * GROOVY-11229 added {{e instanceof T t}} (parser, AST, store-on-match).
> * Java (JEP 394 / JLS): scope follows boolean flow and abrupt completion,
> not simple block poison.
> * Groovy initially limited leakage with push/pop around statements, but did
> not implement true/false-path binding or CompileStack polarity.
> h2. Problems (before the fix)
> ||#||Scenario||Java||Groovy (before)||
> |1|negated {{instanceof}} — use pattern var in else|in scope|missing|
> |2|negated {{instanceof}} + early {{return}} — use pattern var after if|in
> scope|missing|
> |3|positive {{instanceof}} + abrupt else — use pattern var after if|in
> scope|missing|
> |4|{{boolean b = (o instanceof String s)}} then use {{s}}|not in
> scope|CompileStack leak (local still loadable)|
> |5|expression statement with pattern, then use pattern var|not in
> scope|CompileStack leak|
> |6|type-checked: pattern var used on RHS of logical-or|error on RHS|often
> accepted|
> |7|type-checked ternary false arm uses pattern var|error|often accepted|
> |8|negated {{instanceof}} — use pattern var in then-branch|not in scope|could
> ALOAD unassigned local (null)|
> h2. Steps to reproduce
> h3. A. Negated instanceof — else branch (should see {{{}s{}}})
> {code:groovy}
> def f = { Object o ->
> if (!(o instanceof String s)) {
> return 'not'
> } else {
> return s.toUpperCase() // expected: OK when o is String
> }
> }
> assert f('hi') == 'HI'
> {code}
> h3. B. Early return after negation (should see {{s}} after if)
> {code:groovy}
> def f = { Object o ->
> if (!(o instanceof String s)) return 'early'
> return s.toUpperCase() // expected: OK when o is String
> }
> assert f('hi') == 'HI'
> {code}
> h3. C. Leak after declaration (must *not* see {{{}s{}}})
> {code:groovy}
> class C {
> Object m(Object o) {
> boolean b = (o instanceof String s)
> return s // expected: MissingPropertyException /
> undeclared
> }
> }
> new C().m('hi')
> {code}
> h3. D. Type-checked {{||}} RHS must not see true-path binding
> {code:groovy}
> @groovy.transform.TypeChecked
> class C {
> static void m(Object o) {
> if (o instanceof String s || s.length() > 0) {
> // expected: undeclared / apparent variable s on RHS of ||
> }
> }
> }
> {code}
> h2. Expected behaviour
> Align with Java JEP 394 flow scoping for the common shapes:
> * true-path bindings (e.g. {{{}e instanceof T t{}}}) live in then-blocks,
> {{&&}} RHS, and ternary true arm
> * false-path bindings (e.g. {{{}!(e instanceof T t){}}}) live in
> else-blocks, after abrupt then, and the matching ternary arm
> * pattern variables do not leak past the introducing statement (declaration
> RHS, expression statement, …)
> * VariableScope (names) and CompileStack (locals) agree on which path a
> pattern local is live
> h2. Actual behaviour (before fix)
> * Lexical push/pop approximated “no leak past statement” but not true/false
> path polarity.
> * CompileStack could keep pattern slots after VariableScope had dropped the
> name (silent local load vs property miss).
> * Negation and abrupt-completion cases from Java were not supported.
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
