[
https://issues.apache.org/jira/browse/GROOVY-12208?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18100004#comment-18100004
]
ASF GitHub Bot commented on GROOVY-12208:
-----------------------------------------
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.
> NullChecker: broaden null-guard recognition
> -------------------------------------------
>
> Key: GROOVY-12208
> URL: https://issues.apache.org/jira/browse/GROOVY-12208
> Project: Groovy
> Issue Type: Improvement
> Components: groovy-typecheckers
> Reporter: Paul King
> Priority: Major
>
> The incubating {{groovy.typecheckers.NullChecker}} currently recognizes a
> limited in-body guard vocabulary: single {{x != null}} / {{x == null}} binary
> comparisons (including identity forms) as {{if}} conditions, and early-exit
> patterns ({{if (x == null) return/throw}}), plus safe navigation.
> (Separately, the groovy-contracts bridge already infers non-nullness from
> {{@Requires}} conditions including conjunctions, via
> {{StaticTypesMarker.INFERRED_NON_NULL}} — this issue is about guard
> recognition inside method bodies, where conjunctions and other idiomatic
> forms are not yet recognized.)
> Idiomatic Groovy uses several other null-guard forms, each currently
> producing false positives that discourage adoption:
> * Groovy-truth guards: {{if (x) { x.foo() } }} — for reference types,
> truthiness implies non-null within the guarded block (Groovy truth being
> false for empty strings/collections does not affect nullness soundness).
> * Boolean conjunctions in conditions: {{if (x != null && x.foo())}}, and
> short-circuit dereferences {{x != null && x.foo()}} in expression position;
> Groovy-truth conjuncts {{x && x.foo()}}.
> * {{instanceof}} checks: {{if (x instanceof Foo)}} implies {{x}} is non-null
> (aligns with the flow typing the static type checker already performs for
> {{instanceof}}).
> * {{Objects.nonNull(x)}} / {{Objects.isNull(x)}} in the positions the binary
> forms are recognized today.
> * {{assert x}} and {{assert x != null}} statements.
> * Guard conditions in {{while}} loops and ternary conditions, mirroring the
> {{if}} handling.
> Each addition removes a class of false positives without changing the
> checker's architecture. Test coverage should include the negated forms and
> else-branch behaviour for each new guard shape, following the existing
> patterns in {{NullCheckerTest}} (cf. {{testNullableWithNotNullGuard}} et al.).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
