[
https://issues.apache.org/jira/browse/GROOVY-12209?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18100016#comment-18100016
]
ASF GitHub Bot commented on GROOVY-12209:
-----------------------------------------
Copilot commented on code in PR #2747:
URL: https://github.com/apache/groovy/pull/2747#discussion_r3673122086
##########
subprojects/groovy-typecheckers/src/main/groovy/groovy/typecheckers/NullChecker.groovy:
##########
@@ -539,6 +584,31 @@ class NullChecker extends
GroovyTypeCheckingExtensionSupport.TypeCheckingDSL {
false
}
+ /**
+ * A safe-navigation result ({@code a?.b} or {@code a?.b()}) is null
whenever its receiver is.
+ */
+ private static boolean isSafeNavResult(Expression expr) {
+ (expr instanceof PropertyExpression && expr.safe) || (expr instanceof
MethodCallExpression && expr.safe)
+ }
Review Comment:
The docstring is potentially misleading: safe-navigation results are null
when the receiver is null, but they can also be null for other reasons (e.g.,
the accessed property value is null, or the method returns null). Consider
rewording to something like “may be null at least when its receiver is null” to
better match the semantics and avoid future misuse of this helper.
##########
subprojects/groovy-typecheckers/src/spec/doc/typecheckers.adoc:
##########
@@ -721,9 +721,22 @@ patterns also combines with the early-exit form, e.g.
include::../test/NullCheckerTest.groovy[tags=broader_guards,indent=0]
----
-*Ternary and elvis expressions*: in flow-sensitive mode (`NullChecker(strict:
true)`), the checker
-examines both branches of ternary (`condition ? a : b`) and elvis (`x ?:
fallback`) expressions.
-If either branch can be null, the result is treated as nullable. The elvis
assignment
+*Ternary and elvis expressions*: the checker examines the branches of ternary
+(`condition ? a : b`) and elvis (`x ?: fallback`) expressions. A ternary is
treated as
+nullable if either branch can be null, judging each branch under the
condition's own
+guard facts, so `s != null ? s : 'default'` is not considered nullable. An
elvis
+expression is nullable only if its fallback is, since a Groovy-truthy value is
never
+null — making `x ?: 'default'` a recognized way to pass a `@Nullable` value
where a
+non-null value is required:
Review Comment:
These lines appear to have dropped the leading table-cell delimiter (`|`)
that existed previously (e.g. `|*Ternary...`). If this section is inside an
AsciiDoc table (likely, given the prior `|`), removing it will break table
structure/formatting by merging content into the previous cell. Restore the
leading `|` (or adjust the surrounding table markup) so the row/cell boundaries
remain valid.
> NullChecker: track nullness through ?., ?:, and ternary results
> ---------------------------------------------------------------
>
> Key: GROOVY-12209
> URL: https://issues.apache.org/jira/browse/GROOVY-12209
> Project: Groovy
> Issue Type: Improvement
> Components: groovy-typecheckers
> Reporter: Paul King
> Priority: Major
>
> The incubating {{groovy.typecheckers.NullChecker}} tracks nullability per
> *variable*, and already handles several expression forms: dereferencing a
> {{@Nullable}}-returning method call is flagged, strict mode merges
> null-literal branches of ternaries, and elvis assignments over null-literal
> fallbacks are tracked (see {{testNullableMethodReturnDereference}},
> {{testStrictTernary*}}, {{testStrictElvis*}}).
> However, nullness of expression *results* is not propagated in general — the
> null-literal flows work while nullable-expression flows don't. Confirmed gaps:
> * Safe-navigation results: the documented behaviour is unchanged —
> {{input?.length()}} stays OK, since {{?.}} guards the receiver it is applied
> to. The gap is the *result* of a safe navigation used as an unguarded
> receiver afterwards: {{a?.b.c}} parses as {{(a?.b).c}} and throws NPE at
> runtime when {{a}} is null, so it should be flagged ({{a?.b}} is nullable by
> definition, even when {{b}} is {{@NonNull}}), while {{a?.b?.c}} is fine.
> Currently only {{VariableExpression}} and {{MethodCallExpression}} receivers
> are checked in {{checkDereference}}, so a {{PropertyExpression}} receiver
> passes silently.
> * Nullable expressions as arguments: {{isKnownNullable}} recognizes variables
> only, so passing a {{@Nullable}}-returning call or a safe-navigation result
> directly to a {{@NonNull}} parameter (e.g. {{process(repo.findById(id))}}) is
> not caught — only when first assigned to a variable.
> * Ternary/elvis over nullable non-literals: {{cond ? maybeNull : other}} and
> {{x ?: maybeNullCall()}} where a branch is a nullable *variable or call*
> (rather than a null literal) are not merged into the result's nullness.
> * {{@Nullable}} getters dereferenced via property syntax mid-chain: {{x.a.b}}
> where {{getA()}} is {{@Nullable}} is caught when written as {{x.getA().b}}
> but not in property form.
> * Cast and parenthesized expressions should be transparent to nullness in
> {{isKnownNullable}} (they already are in {{isNullExpr}}).
> Together with broadened guard recognition (companion issue), this moves the
> checker from "annotated variables" coverage to "annotated APIs used
> idiomatically" coverage — values obtained from framework calls and used
> inline, which is where most real null-mishandling occurs.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
