daniellansun commented on code in PR #2773: URL: https://github.com/apache/groovy/pull/2773#discussion_r3740592977
########## 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 { Review Comment: The concern is well-taken and worth explaining carefully. **`@Internal` annotation**: `InstanceofFlowBindings` is a compiler-internal type that is not part of Groovy's public API surface. The `@Internal` annotation (`groovy.transform.Internal`) has been added to the class declaration in the follow-up to make this intent explicit and machine-checkable (binary-compatible check tooling already honours the annotation). **Dual-phase design — why sharing is intentional**: The class is consumed by two distinct compiler phases, *not* only by `VariableScopeVisitor`: 1. **`VariableScopeVisitor`** (semantic analysis) — uses the binding sets to declare pattern-variable names only on the definitively-live path, so that subsequent name resolution resolves them correctly. 2. **`InstanceofFlowSlotPublisher` / `StatementWriter`** (code generation) — uses the *same* binding sets to publish/hide CompileStack slots on the matching control-flow arm, keeping bytecode slot visibility consistent with the resolved scopes. Both phases need to answer exactly the same question: *"which pattern-variable names are definitely bound on the true path vs. the false path of this boolean condition?"* Answering that question is a single, pure semantic analysis with no side effects. Sharing the answer via one value type avoids two independent implementations that could silently diverge — which is the actual separation-of-concerns risk. Placing the class in `org.codehaus.groovy.classgen` (the parent package of both `VariableScopeVisitor` and the `asm` sub-package) follows naturally from that shared role. A new sub-package (e.g. `varscope`) would be a reasonable long-term home if the project decides to modularise the classgen layer more finely. This can be deferred — the `@Internal` marker already prevents accidental API entrenchment. The class-level Javadoc has been updated with a **"Design note — dual use across compiler phases"** section that makes this reasoning explicit in the source code so future readers do not have to reconstruct it from PR threads. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
