[
https://issues.apache.org/jira/browse/GROOVY-11894?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18070318#comment-18070318
]
ASF GitHub Bot commented on GROOVY-11894:
-----------------------------------------
Copilot commented on code in PR #2426:
URL: https://github.com/apache/groovy/pull/2426#discussion_r3021709367
##########
subprojects/groovy-typecheckers/src/main/groovy/org/apache/groovy/typecheckers/NullCheckingVisitor.groovy:
##########
@@ -0,0 +1,350 @@
+/*
+ * 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.apache.groovy.typecheckers
+
+import groovy.lang.Tuple2
+import org.codehaus.groovy.ast.AnnotatedNode
+import org.codehaus.groovy.ast.FieldNode
+import org.codehaus.groovy.ast.MethodNode
+import org.codehaus.groovy.ast.Variable
+import org.codehaus.groovy.ast.expr.BinaryExpression
+import org.codehaus.groovy.ast.expr.ConstantExpression
+import org.codehaus.groovy.ast.expr.DeclarationExpression
+import org.codehaus.groovy.ast.expr.EmptyExpression
+import org.codehaus.groovy.ast.expr.Expression
+import org.codehaus.groovy.ast.expr.MethodCallExpression
+import org.codehaus.groovy.ast.expr.PropertyExpression
+import org.codehaus.groovy.ast.expr.StaticMethodCallExpression
+import org.codehaus.groovy.ast.expr.TupleExpression
+import org.codehaus.groovy.ast.expr.VariableExpression
+import org.codehaus.groovy.ast.stmt.BlockStatement
+import org.codehaus.groovy.ast.stmt.EmptyStatement
+import org.codehaus.groovy.ast.stmt.IfStatement
+import org.codehaus.groovy.ast.stmt.ReturnStatement
+import org.codehaus.groovy.ast.stmt.Statement
+import org.codehaus.groovy.ast.stmt.ThrowStatement
+import org.codehaus.groovy.syntax.Types
+import org.codehaus.groovy.transform.stc.GroovyTypeCheckingExtensionSupport
+import org.codehaus.groovy.transform.stc.StaticTypesMarker
+
+import static org.codehaus.groovy.ast.ClassHelper.VOID_TYPE
+import static org.codehaus.groovy.ast.ClassHelper.isPrimitiveType
+import static org.codehaus.groovy.syntax.Types.isAssignment
+
+/**
+ * Shared visitor implementation for null-safety type checking.
+ * Used by both {@code NullChecker} (annotation-only) and {@code
StrictNullChecker} (annotation + flow-sensitive).
+ *
+ * @see groovy.typecheckers.NullChecker
+ * @see groovy.typecheckers.StrictNullChecker
+ */
+class NullCheckingVisitor extends CheckingVisitor {
+
+ private static final Set<String> NULLABLE_ANNOS = Set.of('Nullable',
'CheckForNull', 'MonotonicNonNull')
+ private static final Set<String> NONNULL_ANNOS = Set.of('NonNull',
'NotNull', 'Nonnull')
+ private static final Set<String> MONOTONIC_ANNOS =
Set.of('MonotonicNonNull', 'Lazy')
+ private static final Set<String> NULLCHECK_ANNOS = Set.of('NullCheck',
'ParametersAreNonnullByDefault', 'ParametersAreNonNullByDefault')
+ private static final Set<String> NONNULL_BY_DEFAULT_ANNOS =
Set.of('NonNullByDefault', 'NonnullByDefault')
+
+ private final boolean flowSensitive
+ private final Closure errorReporter
+ private final MethodNode method
+ private final boolean methodNonNull
+ private final Set<Variable> nullableVars = new HashSet<>()
+ private final Set<Variable> monotonicInitialized = new HashSet<>()
+ private final Set<Variable> guardedVars = new HashSet<>()
+
+ NullCheckingVisitor(boolean flowSensitive, MethodNode method, Closure
errorReporter) {
+ this.flowSensitive = flowSensitive
+ this.errorReporter = errorReporter
+ this.method = method
+ boolean classNonNullByDefault = method.declaringClass != null &&
hasNonNullByDefaultAnno(method.declaringClass)
+ this.methodNonNull = method.returnType != VOID_TYPE &&
(hasNonNullAnno(method) || (classNonNullByDefault && !hasNullableAnno(method)))
+ for (param in method.parameters) {
+ if (hasNullableAnno(param)) {
+ nullableVars.add(param)
+ }
+ }
+ }
+
+ /**
+ * Installs the null-checking visitor into a type checking DSL.
+ * Called from the DSL's {@code run()} method to set up the {@code
afterVisitMethod} callback.
+ *
+ * @param dsl the type checking DSL instance
+ * @param flowSensitive whether to enable flow-sensitive null tracking
+ */
+ static void install(GroovyTypeCheckingExtensionSupport.TypeCheckingDSL
dsl, boolean flowSensitive) {
+ dsl.afterVisitMethod { MethodNode method ->
+ def reporter = { String msg, node -> dsl.addStaticTypeError(msg,
node) }
+ method.code?.visit(new NullCheckingVisitor(flowSensitive, method,
reporter))
+ }
+ }
+
+
//--------------------------------------------------------------------------
+
+ @Override
+ void visitDeclarationExpression(DeclarationExpression decl) {
+ super.visitDeclarationExpression(decl)
+ def ve = decl.variableExpression
+ if (ve == null) return
+ if (decl.rightExpression instanceof ConstantExpression) {
+ localConstVars.put(ve, decl.rightExpression)
+ }
+ if (hasNonNullAnno(ve) && isNullExpr(decl.rightExpression)) {
+ reportError("Cannot assign null to @NonNull variable
'${ve.name}'", decl)
+ }
+ // Uninitialized non-primitive declaration (e.g. "String x") is
implicitly null
+ boolean implicitlyNull = decl.rightExpression instanceof
EmptyExpression && !isPrimitiveType(ve.type)
+ if (hasNullableAnno(ve) || isNullExpr(decl.rightExpression) ||
(flowSensitive && implicitlyNull)) {
+ nullableVars.add(ve)
+ }
+ if (flowSensitive) {
+ trackNullableReturn(ve, decl.rightExpression)
+ }
+ }
+
+ @Override
+ void visitBinaryExpression(BinaryExpression expression) {
+ super.visitBinaryExpression(expression)
+ if (isAssignment(expression.operation.type) &&
expression.leftExpression instanceof VariableExpression) {
+ def target = findTargetVariable(expression.leftExpression)
+ boolean fieldNonNull = target instanceof AnnotatedNode &&
hasNonNullAnno(target)
+ if (!fieldNonNull && target instanceof FieldNode &&
!hasNullableAnno(target)) {
+ fieldNonNull = target.declaringClass != null &&
hasNonNullByDefaultAnno(target.declaringClass)
+ }
+ if (fieldNonNull && isNullExpr(expression.rightExpression)) {
+ reportError("Cannot assign null to @NonNull variable
'${expression.leftExpression.name}'", expression)
+ }
+ // @MonotonicNonNull: once initialized with non-null, cannot
assign null again
+ if (target instanceof AnnotatedNode && hasMonotonicAnno(target)) {
+ if (!isNullExpr(expression.rightExpression)) {
+ monotonicInitialized.add(target)
+ } else if (monotonicInitialized.contains(target)) {
+ reportError("Cannot assign null to @MonotonicNonNull
variable '${expression.leftExpression.name}' after non-null assignment",
expression)
+ }
+ }
Review Comment:
The flow-sensitive assignment logic has several edge cases that aren't
currently exercised by tests: (1) casted-null assignments (e.g. `def x =
(String) null`) should be treated as nullable in `StrictNullChecker`, (2)
propagation of nullability through variable-to-variable assignment (`def y =
x`) when `x` is nullable, and (3) `@MonotonicNonNull` should not be considered
initialized when assigned from a value that may be null. Adding focused tests
for these cases would prevent regressions and validate the intended
flow-sensitive behavior.
##########
subprojects/groovy-typecheckers/src/main/groovy/org/apache/groovy/typecheckers/NullCheckingVisitor.groovy:
##########
@@ -0,0 +1,350 @@
+/*
+ * 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.apache.groovy.typecheckers
+
+import groovy.lang.Tuple2
+import org.codehaus.groovy.ast.AnnotatedNode
+import org.codehaus.groovy.ast.FieldNode
+import org.codehaus.groovy.ast.MethodNode
+import org.codehaus.groovy.ast.Variable
+import org.codehaus.groovy.ast.expr.BinaryExpression
+import org.codehaus.groovy.ast.expr.ConstantExpression
+import org.codehaus.groovy.ast.expr.DeclarationExpression
+import org.codehaus.groovy.ast.expr.EmptyExpression
+import org.codehaus.groovy.ast.expr.Expression
+import org.codehaus.groovy.ast.expr.MethodCallExpression
+import org.codehaus.groovy.ast.expr.PropertyExpression
+import org.codehaus.groovy.ast.expr.StaticMethodCallExpression
+import org.codehaus.groovy.ast.expr.TupleExpression
+import org.codehaus.groovy.ast.expr.VariableExpression
+import org.codehaus.groovy.ast.stmt.BlockStatement
+import org.codehaus.groovy.ast.stmt.EmptyStatement
+import org.codehaus.groovy.ast.stmt.IfStatement
+import org.codehaus.groovy.ast.stmt.ReturnStatement
+import org.codehaus.groovy.ast.stmt.Statement
+import org.codehaus.groovy.ast.stmt.ThrowStatement
+import org.codehaus.groovy.syntax.Types
+import org.codehaus.groovy.transform.stc.GroovyTypeCheckingExtensionSupport
+import org.codehaus.groovy.transform.stc.StaticTypesMarker
+
+import static org.codehaus.groovy.ast.ClassHelper.VOID_TYPE
+import static org.codehaus.groovy.ast.ClassHelper.isPrimitiveType
+import static org.codehaus.groovy.syntax.Types.isAssignment
+
+/**
+ * Shared visitor implementation for null-safety type checking.
+ * Used by both {@code NullChecker} (annotation-only) and {@code
StrictNullChecker} (annotation + flow-sensitive).
+ *
+ * @see groovy.typecheckers.NullChecker
+ * @see groovy.typecheckers.StrictNullChecker
+ */
+class NullCheckingVisitor extends CheckingVisitor {
+
+ private static final Set<String> NULLABLE_ANNOS = Set.of('Nullable',
'CheckForNull', 'MonotonicNonNull')
+ private static final Set<String> NONNULL_ANNOS = Set.of('NonNull',
'NotNull', 'Nonnull')
+ private static final Set<String> MONOTONIC_ANNOS =
Set.of('MonotonicNonNull', 'Lazy')
+ private static final Set<String> NULLCHECK_ANNOS = Set.of('NullCheck',
'ParametersAreNonnullByDefault', 'ParametersAreNonNullByDefault')
+ private static final Set<String> NONNULL_BY_DEFAULT_ANNOS =
Set.of('NonNullByDefault', 'NonnullByDefault')
+
+ private final boolean flowSensitive
+ private final Closure errorReporter
+ private final MethodNode method
+ private final boolean methodNonNull
+ private final Set<Variable> nullableVars = new HashSet<>()
+ private final Set<Variable> monotonicInitialized = new HashSet<>()
+ private final Set<Variable> guardedVars = new HashSet<>()
+
+ NullCheckingVisitor(boolean flowSensitive, MethodNode method, Closure
errorReporter) {
+ this.flowSensitive = flowSensitive
+ this.errorReporter = errorReporter
+ this.method = method
+ boolean classNonNullByDefault = method.declaringClass != null &&
hasNonNullByDefaultAnno(method.declaringClass)
+ this.methodNonNull = method.returnType != VOID_TYPE &&
(hasNonNullAnno(method) || (classNonNullByDefault && !hasNullableAnno(method)))
+ for (param in method.parameters) {
+ if (hasNullableAnno(param)) {
+ nullableVars.add(param)
+ }
+ }
+ }
+
+ /**
+ * Installs the null-checking visitor into a type checking DSL.
+ * Called from the DSL's {@code run()} method to set up the {@code
afterVisitMethod} callback.
+ *
+ * @param dsl the type checking DSL instance
+ * @param flowSensitive whether to enable flow-sensitive null tracking
+ */
+ static void install(GroovyTypeCheckingExtensionSupport.TypeCheckingDSL
dsl, boolean flowSensitive) {
+ dsl.afterVisitMethod { MethodNode method ->
+ def reporter = { String msg, node -> dsl.addStaticTypeError(msg,
node) }
+ method.code?.visit(new NullCheckingVisitor(flowSensitive, method,
reporter))
+ }
+ }
+
+
//--------------------------------------------------------------------------
+
+ @Override
+ void visitDeclarationExpression(DeclarationExpression decl) {
+ super.visitDeclarationExpression(decl)
+ def ve = decl.variableExpression
+ if (ve == null) return
+ if (decl.rightExpression instanceof ConstantExpression) {
+ localConstVars.put(ve, decl.rightExpression)
+ }
+ if (hasNonNullAnno(ve) && isNullExpr(decl.rightExpression)) {
+ reportError("Cannot assign null to @NonNull variable
'${ve.name}'", decl)
+ }
+ // Uninitialized non-primitive declaration (e.g. "String x") is
implicitly null
+ boolean implicitlyNull = decl.rightExpression instanceof
EmptyExpression && !isPrimitiveType(ve.type)
+ if (hasNullableAnno(ve) || isNullExpr(decl.rightExpression) ||
(flowSensitive && implicitlyNull)) {
+ nullableVars.add(ve)
+ }
+ if (flowSensitive) {
+ trackNullableReturn(ve, decl.rightExpression)
+ }
+ }
+
+ @Override
+ void visitBinaryExpression(BinaryExpression expression) {
+ super.visitBinaryExpression(expression)
+ if (isAssignment(expression.operation.type) &&
expression.leftExpression instanceof VariableExpression) {
+ def target = findTargetVariable(expression.leftExpression)
+ boolean fieldNonNull = target instanceof AnnotatedNode &&
hasNonNullAnno(target)
+ if (!fieldNonNull && target instanceof FieldNode &&
!hasNullableAnno(target)) {
+ fieldNonNull = target.declaringClass != null &&
hasNonNullByDefaultAnno(target.declaringClass)
+ }
+ if (fieldNonNull && isNullExpr(expression.rightExpression)) {
+ reportError("Cannot assign null to @NonNull variable
'${expression.leftExpression.name}'", expression)
+ }
+ // @MonotonicNonNull: once initialized with non-null, cannot
assign null again
+ if (target instanceof AnnotatedNode && hasMonotonicAnno(target)) {
+ if (!isNullExpr(expression.rightExpression)) {
+ monotonicInitialized.add(target)
+ } else if (monotonicInitialized.contains(target)) {
+ reportError("Cannot assign null to @MonotonicNonNull
variable '${expression.leftExpression.name}' after non-null assignment",
expression)
+ }
+ }
+ if (isNullExpr(expression.rightExpression)) {
+ nullableVars.add(target)
+ } else {
+ nullableVars.remove(target)
+ if (flowSensitive) {
+ trackNullableReturn(target, expression.rightExpression)
Review Comment:
In flow-sensitive mode, nullability does not propagate through assignments
like `y = x` when `x` is known nullable (either annotated `@Nullable` or
tracked in `nullableVars`). Because the `else` branch unconditionally removes
the target from `nullableVars` unless RHS is a literal `null` (or a
nullable-returning method via `trackNullableReturn`), `StrictNullChecker` can
miss `y` being nullable. Consider treating
`isKnownNullable(expression.rightExpression)` as nullable and keeping/adding
the target in `nullableVars` (and similarly avoid marking `@MonotonicNonNull`
as initialized when the assigned value may be null).
```suggestion
Expression rhs = expression.rightExpression
boolean rhsIsNull = isNullExpr(rhs)
boolean rhsKnownNullable = isKnownNullable(rhs)
if (fieldNonNull && rhsIsNull) {
reportError("Cannot assign null to @NonNull variable
'${expression.leftExpression.name}'", expression)
}
// @MonotonicNonNull: once initialized with non-null, cannot
assign null again
if (target instanceof AnnotatedNode && hasMonotonicAnno(target))
{
if (!rhsIsNull && !rhsKnownNullable) {
monotonicInitialized.add(target)
} else if (rhsIsNull &&
monotonicInitialized.contains(target)) {
reportError("Cannot assign null to @MonotonicNonNull
variable '${expression.leftExpression.name}' after non-null assignment",
expression)
}
}
if (rhsIsNull || rhsKnownNullable) {
nullableVars.add(target)
} else {
nullableVars.remove(target)
if (flowSensitive) {
trackNullableReturn(target, rhs)
```
##########
subprojects/groovy-typecheckers/src/main/groovy/org/apache/groovy/typecheckers/NullCheckingVisitor.groovy:
##########
@@ -0,0 +1,350 @@
+/*
+ * 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.apache.groovy.typecheckers
+
+import groovy.lang.Tuple2
+import org.codehaus.groovy.ast.AnnotatedNode
+import org.codehaus.groovy.ast.FieldNode
+import org.codehaus.groovy.ast.MethodNode
+import org.codehaus.groovy.ast.Variable
+import org.codehaus.groovy.ast.expr.BinaryExpression
+import org.codehaus.groovy.ast.expr.ConstantExpression
+import org.codehaus.groovy.ast.expr.DeclarationExpression
+import org.codehaus.groovy.ast.expr.EmptyExpression
+import org.codehaus.groovy.ast.expr.Expression
+import org.codehaus.groovy.ast.expr.MethodCallExpression
+import org.codehaus.groovy.ast.expr.PropertyExpression
+import org.codehaus.groovy.ast.expr.StaticMethodCallExpression
+import org.codehaus.groovy.ast.expr.TupleExpression
+import org.codehaus.groovy.ast.expr.VariableExpression
+import org.codehaus.groovy.ast.stmt.BlockStatement
+import org.codehaus.groovy.ast.stmt.EmptyStatement
+import org.codehaus.groovy.ast.stmt.IfStatement
+import org.codehaus.groovy.ast.stmt.ReturnStatement
+import org.codehaus.groovy.ast.stmt.Statement
+import org.codehaus.groovy.ast.stmt.ThrowStatement
+import org.codehaus.groovy.syntax.Types
+import org.codehaus.groovy.transform.stc.GroovyTypeCheckingExtensionSupport
+import org.codehaus.groovy.transform.stc.StaticTypesMarker
+
+import static org.codehaus.groovy.ast.ClassHelper.VOID_TYPE
+import static org.codehaus.groovy.ast.ClassHelper.isPrimitiveType
+import static org.codehaus.groovy.syntax.Types.isAssignment
+
+/**
+ * Shared visitor implementation for null-safety type checking.
+ * Used by both {@code NullChecker} (annotation-only) and {@code
StrictNullChecker} (annotation + flow-sensitive).
+ *
+ * @see groovy.typecheckers.NullChecker
+ * @see groovy.typecheckers.StrictNullChecker
+ */
+class NullCheckingVisitor extends CheckingVisitor {
+
+ private static final Set<String> NULLABLE_ANNOS = Set.of('Nullable',
'CheckForNull', 'MonotonicNonNull')
+ private static final Set<String> NONNULL_ANNOS = Set.of('NonNull',
'NotNull', 'Nonnull')
+ private static final Set<String> MONOTONIC_ANNOS =
Set.of('MonotonicNonNull', 'Lazy')
+ private static final Set<String> NULLCHECK_ANNOS = Set.of('NullCheck',
'ParametersAreNonnullByDefault', 'ParametersAreNonNullByDefault')
+ private static final Set<String> NONNULL_BY_DEFAULT_ANNOS =
Set.of('NonNullByDefault', 'NonnullByDefault')
+
+ private final boolean flowSensitive
+ private final Closure errorReporter
+ private final MethodNode method
+ private final boolean methodNonNull
+ private final Set<Variable> nullableVars = new HashSet<>()
+ private final Set<Variable> monotonicInitialized = new HashSet<>()
+ private final Set<Variable> guardedVars = new HashSet<>()
+
+ NullCheckingVisitor(boolean flowSensitive, MethodNode method, Closure
errorReporter) {
+ this.flowSensitive = flowSensitive
+ this.errorReporter = errorReporter
+ this.method = method
+ boolean classNonNullByDefault = method.declaringClass != null &&
hasNonNullByDefaultAnno(method.declaringClass)
+ this.methodNonNull = method.returnType != VOID_TYPE &&
(hasNonNullAnno(method) || (classNonNullByDefault && !hasNullableAnno(method)))
+ for (param in method.parameters) {
+ if (hasNullableAnno(param)) {
+ nullableVars.add(param)
+ }
+ }
+ }
+
+ /**
+ * Installs the null-checking visitor into a type checking DSL.
+ * Called from the DSL's {@code run()} method to set up the {@code
afterVisitMethod} callback.
+ *
+ * @param dsl the type checking DSL instance
+ * @param flowSensitive whether to enable flow-sensitive null tracking
+ */
+ static void install(GroovyTypeCheckingExtensionSupport.TypeCheckingDSL
dsl, boolean flowSensitive) {
+ dsl.afterVisitMethod { MethodNode method ->
+ def reporter = { String msg, node -> dsl.addStaticTypeError(msg,
node) }
+ method.code?.visit(new NullCheckingVisitor(flowSensitive, method,
reporter))
+ }
+ }
+
+
//--------------------------------------------------------------------------
+
+ @Override
+ void visitDeclarationExpression(DeclarationExpression decl) {
+ super.visitDeclarationExpression(decl)
+ def ve = decl.variableExpression
+ if (ve == null) return
+ if (decl.rightExpression instanceof ConstantExpression) {
+ localConstVars.put(ve, decl.rightExpression)
+ }
+ if (hasNonNullAnno(ve) && isNullExpr(decl.rightExpression)) {
+ reportError("Cannot assign null to @NonNull variable
'${ve.name}'", decl)
+ }
+ // Uninitialized non-primitive declaration (e.g. "String x") is
implicitly null
+ boolean implicitlyNull = decl.rightExpression instanceof
EmptyExpression && !isPrimitiveType(ve.type)
+ if (hasNullableAnno(ve) || isNullExpr(decl.rightExpression) ||
(flowSensitive && implicitlyNull)) {
+ nullableVars.add(ve)
+ }
+ if (flowSensitive) {
+ trackNullableReturn(ve, decl.rightExpression)
+ }
+ }
+
+ @Override
+ void visitBinaryExpression(BinaryExpression expression) {
+ super.visitBinaryExpression(expression)
+ if (isAssignment(expression.operation.type) &&
expression.leftExpression instanceof VariableExpression) {
+ def target = findTargetVariable(expression.leftExpression)
+ boolean fieldNonNull = target instanceof AnnotatedNode &&
hasNonNullAnno(target)
+ if (!fieldNonNull && target instanceof FieldNode &&
!hasNullableAnno(target)) {
+ fieldNonNull = target.declaringClass != null &&
hasNonNullByDefaultAnno(target.declaringClass)
+ }
+ if (fieldNonNull && isNullExpr(expression.rightExpression)) {
+ reportError("Cannot assign null to @NonNull variable
'${expression.leftExpression.name}'", expression)
+ }
+ // @MonotonicNonNull: once initialized with non-null, cannot
assign null again
+ if (target instanceof AnnotatedNode && hasMonotonicAnno(target)) {
+ if (!isNullExpr(expression.rightExpression)) {
+ monotonicInitialized.add(target)
+ } else if (monotonicInitialized.contains(target)) {
+ reportError("Cannot assign null to @MonotonicNonNull
variable '${expression.leftExpression.name}' after non-null assignment",
expression)
+ }
+ }
+ if (isNullExpr(expression.rightExpression)) {
+ nullableVars.add(target)
+ } else {
+ nullableVars.remove(target)
+ if (flowSensitive) {
+ trackNullableReturn(target, expression.rightExpression)
+ }
+ }
+ }
+ }
+
+ @Override
+ void visitMethodCallExpression(MethodCallExpression call) {
+ super.visitMethodCallExpression(call)
+ if (!call.safe && !call.implicitThis) {
+ checkDereference(call.objectExpression, call)
+ }
+ checkMethodArguments(call)
+ }
+
+ @Override
+ void visitStaticMethodCallExpression(StaticMethodCallExpression call) {
+ super.visitStaticMethodCallExpression(call)
+ checkMethodArguments(call)
+ }
+
+ @Override
+ void visitPropertyExpression(PropertyExpression expression) {
+ super.visitPropertyExpression(expression)
+ if (!expression.safe) {
+ checkDereference(expression.objectExpression, expression)
+ }
+ }
+
+ @Override
+ void visitReturnStatement(ReturnStatement statement) {
+ super.visitReturnStatement(statement)
+ if (methodNonNull) {
+ if (isNullExpr(statement.expression)) {
+ reportError("Cannot return null from @NonNull method
'${method.name}'", statement)
+ } else if (isKnownNullable(statement.expression)) {
+ reportError("Cannot return @Nullable value from @NonNull
method '${method.name}'", statement)
+ }
+ }
+ }
+
+ @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)
+ }
+ }
+
+
//--------------------------------------------------------------------------
+
+ 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)
+ } 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)
+ }
+ // Early exit: if (x == null) return/throw → x is non-null after
+ if (isEarlyExit(ifElse.ifBlock)) {
+ nullableVars.remove(guardVar)
+ guardedVars.add(guardVar)
+ }
+ }
+ }
+
+ private void checkDereference(Expression receiver, Expression context) {
+ if (isNullExpr(receiver)) {
+ reportError('Cannot dereference null', context)
+ return
+ }
+ if (receiver instanceof VariableExpression) {
+ if (receiver.isThisExpression() || receiver.isSuperExpression())
return
+ def target = findTargetVariable(receiver)
+ boolean isMonotonicAndInitialized = target instanceof
AnnotatedNode && hasMonotonicAnno(target) &&
monotonicInitialized.contains(target)
+ if (target instanceof AnnotatedNode && hasNullableAnno(target) &&
!guardedVars.contains(target) && !isMonotonicAndInitialized) {
+ reportError("Potential null dereference: '${receiver.name}' is
@Nullable", context)
+ } else if (flowSensitive && nullableVars.contains(target) &&
!guardedVars.contains(target)) {
+ reportError("Potential null dereference: '${receiver.name}'
may be null", context)
+ }
+ } else if (receiver instanceof MethodCallExpression || receiver
instanceof StaticMethodCallExpression) {
+ def targetMethod =
receiver.getNodeMetaData(StaticTypesMarker.DIRECT_METHOD_CALL_TARGET)
+ if (targetMethod instanceof MethodNode &&
hasNullableAnno(targetMethod)) {
+ reportError("Potential null dereference:
'${targetMethod.name}()' may return null", context)
+ }
+ }
+ }
+
+ private void checkMethodArguments(call) {
+ def target =
call.getNodeMetaData(StaticTypesMarker.DIRECT_METHOD_CALL_TARGET)
+ if (!(target instanceof MethodNode)) return
+ def args = call.arguments
+ if (!(args instanceof TupleExpression)) return
+ def params = target.parameters
+ // @NullCheck/@ParametersAreNonnullByDefault/@NonNullByDefault on
method or class makes non-primitive params effectively @NonNull
+ def declaringClass = target.declaringClass
+ boolean nullChecked = hasNullCheckAnno(target) ||
hasNonNullByDefaultAnno(target) ||
+ (declaringClass != null && (hasNullCheckAnno(declaringClass) ||
hasNonNullByDefaultAnno(declaringClass)))
+ int limit = Math.min(args.expressions.size(), params.length)
+ for (int i = 0; i < limit; i++) {
+ def arg = args.getExpression(i)
+ boolean paramIsNonNull = hasNonNullAnno(params[i]) || (nullChecked
&& !isPrimitiveType(params[i].type) && !hasNullableAnno(params[i]))
+ if (paramIsNonNull) {
+ if (isNullExpr(arg)) {
+ reportError("Cannot pass null to @NonNull parameter
'${params[i].name}' of '${target.name}'", call)
+ } else if (isKnownNullable(arg)) {
+ reportError("Cannot pass @Nullable value to @NonNull
parameter '${params[i].name}' of '${target.name}'", call)
+ }
+ }
+ }
+ }
+
+ private boolean isKnownNullable(Expression expr) {
+ if (expr instanceof VariableExpression) {
+ def target = findTargetVariable(expr)
+ if (target instanceof AnnotatedNode && hasNullableAnno(target))
return true
+ if (nullableVars.contains(target)) return true
+ }
+ false
+ }
+
+ private void trackNullableReturn(Variable variable, Expression rhs) {
+ if (rhs instanceof MethodCallExpression || rhs instanceof
StaticMethodCallExpression) {
+ def target =
rhs.getNodeMetaData(StaticTypesMarker.DIRECT_METHOD_CALL_TARGET)
+ if (target instanceof MethodNode && hasNullableAnno(target)) {
+ nullableVars.add(variable)
+ }
+ }
+ }
+
+ private Tuple2<Variable, Boolean> findNullGuard(Expression condition) {
+ if (condition instanceof BinaryExpression) {
+ def op = condition.operation.type
+ boolean isNotEqual = (op == Types.COMPARE_NOT_EQUAL || op ==
Types.COMPARE_NOT_IDENTICAL)
+ boolean isEqual = (op == Types.COMPARE_EQUAL || op ==
Types.COMPARE_IDENTICAL)
+ if (isNotEqual || isEqual) {
+ Variable var = null
+ if (isNullExpr(condition.rightExpression) &&
condition.leftExpression instanceof VariableExpression) {
+ var = findTargetVariable(condition.leftExpression)
+ } else if (isNullExpr(condition.leftExpression) &&
condition.rightExpression instanceof VariableExpression) {
+ var = findTargetVariable(condition.rightExpression)
+ }
+ if (var != null) return new Tuple2<>(var, isNotEqual)
+ }
+ }
+ null
+ }
+
+ private void reportError(String msg, node) {
+ errorReporter.call(msg, node)
+ }
+
+
//--------------------------------------------------------------------------
+
+ static boolean hasNullableAnno(AnnotatedNode node) {
+ node.annotations?.any { it.classNode?.nameWithoutPackage in
NULLABLE_ANNOS } ?: false
+ }
+
+ static boolean hasNonNullAnno(AnnotatedNode node) {
+ node.annotations?.any { it.classNode?.nameWithoutPackage in
NONNULL_ANNOS } ?: false
+ }
+
+ static boolean hasMonotonicAnno(AnnotatedNode node) {
+ node.annotations?.any { it.classNode?.nameWithoutPackage in
MONOTONIC_ANNOS } ?: false
+ }
+
+ static boolean hasNullCheckAnno(AnnotatedNode node) {
+ node.annotations?.any { it.classNode?.nameWithoutPackage in
NULLCHECK_ANNOS } ?: false
+ }
+
+ static boolean hasNonNullByDefaultAnno(AnnotatedNode node) {
+ node.annotations?.any { it.classNode?.nameWithoutPackage in
NONNULL_BY_DEFAULT_ANNOS } ?: false
+ }
+
+ static boolean isNullExpr(Expression expr) {
+ expr instanceof ConstantExpression && ((ConstantExpression)
expr).isNullExpression()
+ }
Review Comment:
`isNullExpr` only recognizes a bare `ConstantExpression(null)`. Common
patterns like `def x = (String) null` (casted null) or `x = null as String`
won't be treated as null, which breaks flow-sensitive tracking and `@NonNull`
assignment checks in those cases. Consider unwrapping wrapper expressions
(e.g., `CastExpression`) before checking for a null constant.
##########
subprojects/groovy-typecheckers/src/main/groovy/org/apache/groovy/typecheckers/NullCheckingVisitor.groovy:
##########
@@ -0,0 +1,350 @@
+/*
+ * 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.apache.groovy.typecheckers
+
+import groovy.lang.Tuple2
+import org.codehaus.groovy.ast.AnnotatedNode
+import org.codehaus.groovy.ast.FieldNode
+import org.codehaus.groovy.ast.MethodNode
+import org.codehaus.groovy.ast.Variable
+import org.codehaus.groovy.ast.expr.BinaryExpression
+import org.codehaus.groovy.ast.expr.ConstantExpression
+import org.codehaus.groovy.ast.expr.DeclarationExpression
+import org.codehaus.groovy.ast.expr.EmptyExpression
+import org.codehaus.groovy.ast.expr.Expression
+import org.codehaus.groovy.ast.expr.MethodCallExpression
+import org.codehaus.groovy.ast.expr.PropertyExpression
+import org.codehaus.groovy.ast.expr.StaticMethodCallExpression
+import org.codehaus.groovy.ast.expr.TupleExpression
+import org.codehaus.groovy.ast.expr.VariableExpression
+import org.codehaus.groovy.ast.stmt.BlockStatement
+import org.codehaus.groovy.ast.stmt.EmptyStatement
+import org.codehaus.groovy.ast.stmt.IfStatement
+import org.codehaus.groovy.ast.stmt.ReturnStatement
+import org.codehaus.groovy.ast.stmt.Statement
+import org.codehaus.groovy.ast.stmt.ThrowStatement
+import org.codehaus.groovy.syntax.Types
+import org.codehaus.groovy.transform.stc.GroovyTypeCheckingExtensionSupport
+import org.codehaus.groovy.transform.stc.StaticTypesMarker
+
+import static org.codehaus.groovy.ast.ClassHelper.VOID_TYPE
+import static org.codehaus.groovy.ast.ClassHelper.isPrimitiveType
+import static org.codehaus.groovy.syntax.Types.isAssignment
+
+/**
+ * Shared visitor implementation for null-safety type checking.
+ * Used by both {@code NullChecker} (annotation-only) and {@code
StrictNullChecker} (annotation + flow-sensitive).
+ *
+ * @see groovy.typecheckers.NullChecker
+ * @see groovy.typecheckers.StrictNullChecker
+ */
+class NullCheckingVisitor extends CheckingVisitor {
+
+ private static final Set<String> NULLABLE_ANNOS = Set.of('Nullable',
'CheckForNull', 'MonotonicNonNull')
+ private static final Set<String> NONNULL_ANNOS = Set.of('NonNull',
'NotNull', 'Nonnull')
+ private static final Set<String> MONOTONIC_ANNOS =
Set.of('MonotonicNonNull', 'Lazy')
+ private static final Set<String> NULLCHECK_ANNOS = Set.of('NullCheck',
'ParametersAreNonnullByDefault', 'ParametersAreNonNullByDefault')
+ private static final Set<String> NONNULL_BY_DEFAULT_ANNOS =
Set.of('NonNullByDefault', 'NonnullByDefault')
+
+ private final boolean flowSensitive
+ private final Closure errorReporter
+ private final MethodNode method
+ private final boolean methodNonNull
+ private final Set<Variable> nullableVars = new HashSet<>()
+ private final Set<Variable> monotonicInitialized = new HashSet<>()
+ private final Set<Variable> guardedVars = new HashSet<>()
+
+ NullCheckingVisitor(boolean flowSensitive, MethodNode method, Closure
errorReporter) {
+ this.flowSensitive = flowSensitive
+ this.errorReporter = errorReporter
+ this.method = method
+ boolean classNonNullByDefault = method.declaringClass != null &&
hasNonNullByDefaultAnno(method.declaringClass)
+ this.methodNonNull = method.returnType != VOID_TYPE &&
(hasNonNullAnno(method) || (classNonNullByDefault && !hasNullableAnno(method)))
+ for (param in method.parameters) {
+ if (hasNullableAnno(param)) {
+ nullableVars.add(param)
+ }
+ }
+ }
+
+ /**
+ * Installs the null-checking visitor into a type checking DSL.
+ * Called from the DSL's {@code run()} method to set up the {@code
afterVisitMethod} callback.
+ *
+ * @param dsl the type checking DSL instance
+ * @param flowSensitive whether to enable flow-sensitive null tracking
+ */
+ static void install(GroovyTypeCheckingExtensionSupport.TypeCheckingDSL
dsl, boolean flowSensitive) {
+ dsl.afterVisitMethod { MethodNode method ->
+ def reporter = { String msg, node -> dsl.addStaticTypeError(msg,
node) }
+ method.code?.visit(new NullCheckingVisitor(flowSensitive, method,
reporter))
+ }
+ }
+
+
//--------------------------------------------------------------------------
+
+ @Override
+ void visitDeclarationExpression(DeclarationExpression decl) {
+ super.visitDeclarationExpression(decl)
+ def ve = decl.variableExpression
+ if (ve == null) return
+ if (decl.rightExpression instanceof ConstantExpression) {
+ localConstVars.put(ve, decl.rightExpression)
+ }
+ if (hasNonNullAnno(ve) && isNullExpr(decl.rightExpression)) {
+ reportError("Cannot assign null to @NonNull variable
'${ve.name}'", decl)
+ }
+ // Uninitialized non-primitive declaration (e.g. "String x") is
implicitly null
+ boolean implicitlyNull = decl.rightExpression instanceof
EmptyExpression && !isPrimitiveType(ve.type)
+ if (hasNullableAnno(ve) || isNullExpr(decl.rightExpression) ||
(flowSensitive && implicitlyNull)) {
+ nullableVars.add(ve)
+ }
+ if (flowSensitive) {
+ trackNullableReturn(ve, decl.rightExpression)
+ }
+ }
+
+ @Override
+ void visitBinaryExpression(BinaryExpression expression) {
+ super.visitBinaryExpression(expression)
+ if (isAssignment(expression.operation.type) &&
expression.leftExpression instanceof VariableExpression) {
+ def target = findTargetVariable(expression.leftExpression)
+ boolean fieldNonNull = target instanceof AnnotatedNode &&
hasNonNullAnno(target)
+ if (!fieldNonNull && target instanceof FieldNode &&
!hasNullableAnno(target)) {
+ fieldNonNull = target.declaringClass != null &&
hasNonNullByDefaultAnno(target.declaringClass)
+ }
+ if (fieldNonNull && isNullExpr(expression.rightExpression)) {
+ reportError("Cannot assign null to @NonNull variable
'${expression.leftExpression.name}'", expression)
+ }
+ // @MonotonicNonNull: once initialized with non-null, cannot
assign null again
+ if (target instanceof AnnotatedNode && hasMonotonicAnno(target)) {
+ if (!isNullExpr(expression.rightExpression)) {
+ monotonicInitialized.add(target)
+ } else if (monotonicInitialized.contains(target)) {
+ reportError("Cannot assign null to @MonotonicNonNull
variable '${expression.leftExpression.name}' after non-null assignment",
expression)
+ }
+ }
+ if (isNullExpr(expression.rightExpression)) {
+ nullableVars.add(target)
+ } else {
+ nullableVars.remove(target)
+ if (flowSensitive) {
+ trackNullableReturn(target, expression.rightExpression)
+ }
+ }
Review Comment:
`guardedVars` can be made to contain a variable after an early-exit guard
(`if (x == null) return`) and is then consulted to suppress dereference
warnings. However, assignments never clear `guardedVars`, so `x` can be
re-assigned to `null` later and still be treated as guarded, causing false
negatives in `StrictNullChecker`. A concrete fix is to remove the assignment
target from `guardedVars` on any write (and only re-add it within the
appropriate guard scope / post-early-exit state).
> Provide a NullChecker for groovy-typecheckers
> ---------------------------------------------
>
> Key: GROOVY-11894
> URL: https://issues.apache.org/jira/browse/GROOVY-11894
> Project: Groovy
> Issue Type: New Feature
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
