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). -- 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]
