Copilot commented on code in PR #2746:
URL: https://github.com/apache/groovy/pull/2746#discussion_r3672807715
##########
subprojects/groovy-typecheckers/src/main/groovy/groovy/typecheckers/NullChecker.groovy:
##########
@@ -231,43 +251,158 @@ class NullChecker extends
GroovyTypeCheckingExtensionSupport.TypeCheckingDSL {
@Override
void visitIfElse(IfStatement ifElse) {
- ifElse.booleanExpression.visit(this)
- def guard = findNullGuard(ifElse.booleanExpression.expression)
- if (guard != null) {
- handleNullGuard(ifElse, guard.v1, guard.v2)
- } else {
- ifElse.ifBlock.visit(this)
- ifElse.elseBlock.visit(this)
+ def condition = ifElse.booleanExpression.expression
+ visitCondition(condition)
+ def facts = analyzeCondition(condition)
+ withGuards(facts.whenTrue) { ifElse.ifBlock.visit(this) }
+ withGuards(facts.whenFalse) { ifElse.elseBlock.visit(this) }
+ // Early exit: if (x == null) return/throw → x is non-null
after (and vice versa)
+ if (isEarlyExit(ifElse.ifBlock)) {
+ applyFacts(facts.whenFalse)
+ }
+ if (isEarlyExit(ifElse.elseBlock)) {
+ applyFacts(facts.whenTrue)
+ }
+ }
+
+ @Override
+ void visitWhileLoop(WhileStatement loop) {
+ def condition = loop.booleanExpression.expression
+ visitCondition(condition)
+ withGuards(analyzeCondition(condition).whenTrue) {
loop.loopBlock.visit(this) }
+ }
+
+ @Override
+ void visitTernaryExpression(TernaryExpression expression) {
+ def condition = expression.booleanExpression.expression
+ visitCondition(condition)
+ def facts = analyzeCondition(condition)
+ // for elvis, the true expression is the (already visited)
condition
+ if (!condition.is(expression.trueExpression) &&
!expression.booleanExpression.is(expression.trueExpression)) {
+ withGuards(facts.whenTrue) {
expression.trueExpression.visit(this) }
}
+ withGuards(facts.whenFalse) {
expression.falseExpression.visit(this) }
+ }
+
+ @Override
+ void visitAssertStatement(AssertStatement statement) {
+ def condition = statement.booleanExpression.expression
+ visitCondition(condition)
+ statement.messageExpression?.visit(this)
+ // assert x/assert x != null → x is non-null for the rest of
the block
+ applyFacts(analyzeCondition(condition).whenTrue)
}
//------------------------------------------------------------------
- private void handleNullGuard(IfStatement ifElse, Variable
guardVar, boolean isNotNull) {
- if (isNotNull) {
- // if (x != null) { ... } else { ... }
- def saved = new HashSet<>(guardedVars)
- guardedVars.add(guardVar)
- ifElse.ifBlock.visit(this)
- guardedVars.clear()
- guardedVars.addAll(saved)
- ifElse.elseBlock.visit(this)
+ /**
+ * Visits a condition applying short-circuit guard semantics: in
+ * {@code x != null && x.foo()} the right operand is protected by
the
+ * left operand's null check (and similarly after {@code x == null
||}).
+ */
+ private void visitCondition(Expression condition) {
+ if (condition instanceof BooleanExpression) {
+ visitCondition(condition.expression)
+ } else if (condition instanceof BinaryExpression &&
condition.operation.type == Types.LOGICAL_AND) {
+ visitCondition(condition.leftExpression)
+
withGuards(analyzeCondition(condition.leftExpression).whenTrue) {
+ visitCondition(condition.rightExpression)
+ }
+ } else if (condition instanceof BinaryExpression &&
condition.operation.type == Types.LOGICAL_OR) {
+ visitCondition(condition.leftExpression)
+
withGuards(analyzeCondition(condition.leftExpression).whenFalse) {
+ visitCondition(condition.rightExpression)
+ }
Review Comment:
`analyzeCondition(condition.leftExpression)` is recomputed immediately after
`visitCondition(condition.leftExpression)` and can be invoked repeatedly for
nested boolean expressions, creating avoidable allocations and repeated AST
walks. A tangible improvement would be to compute the left facts once (store in
a local) and/or cache `GuardFacts` per `Expression` node (e.g., via node
metadata) during a single visitation pass.
##########
subprojects/groovy-typecheckers/src/main/groovy/groovy/typecheckers/NullChecker.groovy:
##########
@@ -231,43 +251,158 @@ class NullChecker extends
GroovyTypeCheckingExtensionSupport.TypeCheckingDSL {
@Override
void visitIfElse(IfStatement ifElse) {
- ifElse.booleanExpression.visit(this)
- def guard = findNullGuard(ifElse.booleanExpression.expression)
- if (guard != null) {
- handleNullGuard(ifElse, guard.v1, guard.v2)
- } else {
- ifElse.ifBlock.visit(this)
- ifElse.elseBlock.visit(this)
+ def condition = ifElse.booleanExpression.expression
+ visitCondition(condition)
+ def facts = analyzeCondition(condition)
+ withGuards(facts.whenTrue) { ifElse.ifBlock.visit(this) }
+ withGuards(facts.whenFalse) { ifElse.elseBlock.visit(this) }
+ // Early exit: if (x == null) return/throw → x is non-null
after (and vice versa)
+ if (isEarlyExit(ifElse.ifBlock)) {
+ applyFacts(facts.whenFalse)
+ }
+ if (isEarlyExit(ifElse.elseBlock)) {
+ applyFacts(facts.whenTrue)
+ }
+ }
+
+ @Override
+ void visitWhileLoop(WhileStatement loop) {
+ def condition = loop.booleanExpression.expression
+ visitCondition(condition)
+ withGuards(analyzeCondition(condition).whenTrue) {
loop.loopBlock.visit(this) }
+ }
+
+ @Override
+ void visitTernaryExpression(TernaryExpression expression) {
+ def condition = expression.booleanExpression.expression
+ visitCondition(condition)
+ def facts = analyzeCondition(condition)
+ // for elvis, the true expression is the (already visited)
condition
+ if (!condition.is(expression.trueExpression) &&
!expression.booleanExpression.is(expression.trueExpression)) {
+ withGuards(facts.whenTrue) {
expression.trueExpression.visit(this) }
}
+ withGuards(facts.whenFalse) {
expression.falseExpression.visit(this) }
+ }
+
+ @Override
+ void visitAssertStatement(AssertStatement statement) {
+ def condition = statement.booleanExpression.expression
+ visitCondition(condition)
+ statement.messageExpression?.visit(this)
+ // assert x/assert x != null → x is non-null for the rest of
the block
+ applyFacts(analyzeCondition(condition).whenTrue)
}
//------------------------------------------------------------------
- private void handleNullGuard(IfStatement ifElse, Variable
guardVar, boolean isNotNull) {
- if (isNotNull) {
- // if (x != null) { ... } else { ... }
- def saved = new HashSet<>(guardedVars)
- guardedVars.add(guardVar)
- ifElse.ifBlock.visit(this)
- guardedVars.clear()
- guardedVars.addAll(saved)
- ifElse.elseBlock.visit(this)
+ /**
+ * Visits a condition applying short-circuit guard semantics: in
+ * {@code x != null && x.foo()} the right operand is protected by
the
+ * left operand's null check (and similarly after {@code x == null
||}).
+ */
+ private void visitCondition(Expression condition) {
+ if (condition instanceof BooleanExpression) {
+ visitCondition(condition.expression)
+ } else if (condition instanceof BinaryExpression &&
condition.operation.type == Types.LOGICAL_AND) {
+ visitCondition(condition.leftExpression)
+
withGuards(analyzeCondition(condition.leftExpression).whenTrue) {
+ visitCondition(condition.rightExpression)
+ }
+ } else if (condition instanceof BinaryExpression &&
condition.operation.type == Types.LOGICAL_OR) {
+ visitCondition(condition.leftExpression)
+
withGuards(analyzeCondition(condition.leftExpression).whenFalse) {
+ visitCondition(condition.rightExpression)
+ }
} else {
- // if (x == null) { ... } else { ... }
- ifElse.ifBlock.visit(this)
- if (!(ifElse.elseBlock instanceof EmptyStatement)) {
- def saved = new HashSet<>(guardedVars)
- guardedVars.add(guardVar)
- ifElse.elseBlock.visit(this)
- guardedVars.clear()
- guardedVars.addAll(saved)
+ condition.visit(this)
+ }
+ }
+
+ /**
+ * Analyzes a condition into the sets of variables known to be
non-null
+ * when the condition evaluates true and when it evaluates false.
+ */
+ private GuardFacts analyzeCondition(Expression condition) {
+ def facts = new GuardFacts()
+ if (condition instanceof NotExpression) {
+ def inner = analyzeCondition(condition.expression)
+ facts.whenTrue = inner.whenFalse
+ facts.whenFalse = inner.whenTrue
+ } else if (condition instanceof BooleanExpression) {
+ facts = analyzeCondition(condition.expression)
+ } else if (condition instanceof BinaryExpression) {
+ int op = condition.operation.type
+ if (op == Types.LOGICAL_AND) {
+ def left = analyzeCondition(condition.leftExpression)
+ def right = analyzeCondition(condition.rightExpression)
+ facts.whenTrue = left.whenTrue + right.whenTrue
+ facts.whenFalse =
left.whenFalse.intersect(right.whenFalse)
+ } else if (op == Types.LOGICAL_OR) {
+ def left = analyzeCondition(condition.leftExpression)
+ def right = analyzeCondition(condition.rightExpression)
+ facts.whenTrue =
left.whenTrue.intersect(right.whenTrue)
+ facts.whenFalse = left.whenFalse + right.whenFalse
+ } else if (op == Types.KEYWORD_INSTANCEOF) {
Review Comment:
The code assigns the results of `+` and `intersect(...)` directly into
fields declared as `Set<Variable>`. In Groovy, these operations can yield a
generic `Collection` (and the concrete type may vary), which makes the
`GuardFacts` invariants less explicit and can introduce subtle behavior
differences (e.g., duplicates/order) and extra allocations. Consider explicitly
constructing `HashSet`s for union/intersection (e.g., copy + `addAll` /
`retainAll`) so `whenTrue/whenFalse` are always concrete sets with predictable
semantics.
##########
subprojects/groovy-typecheckers/src/spec/doc/typecheckers.adoc:
##########
@@ -707,6 +707,19 @@
include::../test/NullCheckerTest.groovy[tags=null_guard,indent=0]
include::../test/NullCheckerTest.groovy[tags=early_return,indent=0]
----
+*Broader guard patterns* are also recognized: Groovy-truth guards (`if (x)`),
+`instanceof` checks, `Objects.nonNull(x)`/`Objects.isNull(x)`, assert
statements
+(`assert x`, `assert x != null`), and boolean conjunctions and disjunctions
with
+short-circuit semantics (`x != null && x.length() > 0`,
+`if (x == null || x.isEmpty()) return`). Negated forms (`if (!x) return`,
+`x !instanceof Foo`) are understood as well. Guard conditions are honored in
+`if` statements, `while` loops, and ternary expressions:
Review Comment:
The “short-circuit semantics” examples mix a pure expression with a full
statement (`if (x == null || x.isEmpty()) return`) inside the same
parenthetical list, which makes the list harder to scan and slightly ambiguous
about what is being recognized (the boolean expression vs. the early-exit
statement form). Consider rephrasing to keep this portion as expressions only
(e.g., `x == null || x.isEmpty()`) and call out the early-exit `if (...)
return/throw` form separately.
--
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]
