You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@groovy.apache.org by pa...@apache.org on 2026/04/23 12:13:08 UTC
(groovy) branch master updated: GROOVY-11894: Provide a NullChecker for groovy-typecheckers (some combinations of NullChecker and @Requires/@Ensures weren't working)
This is an automated email from the ASF dual-hosted git repository.
paulk-asert pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/groovy.git
The following commit(s) were added to refs/heads/master by this push:
new e36d2565be GROOVY-11894: Provide a NullChecker for groovy-typecheckers (some combinations of NullChecker and @Requires/@Ensures weren't working)
e36d2565be is described below
commit e36d2565bece33e5b2441eee06f9976d83549531
Author: Paul King <pa...@asert.com.au>
AuthorDate: Thu Apr 23 22:12:54 2026 +1000
GROOVY-11894: Provide a NullChecker for groovy-typecheckers (some combinations of NullChecker and @Requires/@Ensures weren't working)
---
.../groovy/transform/stc/StaticTypesMarker.java | 6 +-
.../ast/visitor/AnnotationClosureVisitor.java | 102 +++++++++++++++++
.../groovy/groovy/typecheckers/NullChecker.groovy | 9 ++
.../src/spec/doc/typecheckers.adoc | 44 ++++++++
.../src/spec/test/NullCheckerTest.groovy | 50 +++++++++
.../groovy/typecheckers/NullCheckerTest.groovy | 125 +++++++++++++++++++++
6 files changed, 335 insertions(+), 1 deletion(-)
diff --git a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypesMarker.java b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypesMarker.java
index 8435b0fa5d..cd8e9ddff2 100644
--- a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypesMarker.java
+++ b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypesMarker.java
@@ -57,5 +57,9 @@ public enum StaticTypesMarker {
/** used to store the condition expression type of the switch-case statement */
SWITCH_CONDITION_EXPRESSION_TYPE,
/** used to store the result of {@link StaticTypeCheckingVisitor#getType} */
- TYPE
+ TYPE,
+ /** indicates a parameter or method return is known to be non-null (e.g., inferred from {@code @Requires}/{@code @Ensures} contracts) */
+ INFERRED_NON_NULL,
+ /** list of {@code return null} statements recorded on a method before its body is rewritten, so a downstream checker can still report them as non-null violations */
+ INFERRED_NON_NULL_RETURN_VIOLATIONS
}
diff --git a/subprojects/groovy-contracts/src/main/java/org/apache/groovy/contracts/ast/visitor/AnnotationClosureVisitor.java b/subprojects/groovy-contracts/src/main/java/org/apache/groovy/contracts/ast/visitor/AnnotationClosureVisitor.java
index 5f418364c2..e6a38415c7 100644
--- a/subprojects/groovy-contracts/src/main/java/org/apache/groovy/contracts/ast/visitor/AnnotationClosureVisitor.java
+++ b/subprojects/groovy-contracts/src/main/java/org/apache/groovy/contracts/ast/visitor/AnnotationClosureVisitor.java
@@ -49,6 +49,7 @@ import org.codehaus.groovy.ast.expr.BinaryExpression;
import org.codehaus.groovy.ast.expr.BooleanExpression;
import org.codehaus.groovy.ast.expr.CastExpression;
import org.codehaus.groovy.ast.expr.ClosureExpression;
+import org.codehaus.groovy.ast.expr.ConstantExpression;
import org.codehaus.groovy.ast.expr.ConstructorCallExpression;
import org.codehaus.groovy.ast.expr.Expression;
import org.codehaus.groovy.ast.expr.MethodCallExpression;
@@ -59,10 +60,12 @@ import org.codehaus.groovy.ast.expr.StaticMethodCallExpression;
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.ReturnStatement;
import org.codehaus.groovy.control.SourceUnit;
import org.codehaus.groovy.control.io.ReaderSource;
import org.codehaus.groovy.syntax.Token;
import org.codehaus.groovy.syntax.Types;
+import org.codehaus.groovy.transform.stc.StaticTypesMarker;
import org.objectweb.asm.Opcodes;
import java.util.ArrayList;
@@ -71,6 +74,7 @@ import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
+import java.util.Set;
import static org.codehaus.groovy.ast.tools.GeneralUtils.args;
import static org.codehaus.groovy.ast.tools.GeneralUtils.callX;
@@ -175,6 +179,8 @@ public class AnnotationClosureVisitor extends BaseVisitor implements ASTNodeMeta
replaceWithClosureClassReference(annotationNode, methodNode);
}
+ recordNonNullReturnViolations(methodNode);
+
markProcessed(methodNode);
super.visitConstructorOrMethod(methodNode, isConstructor);
@@ -201,6 +207,8 @@ public class AnnotationClosureVisitor extends BaseVisitor implements ASTNodeMeta
List<BooleanExpression> booleanExpressions = ExpressionUtils.getBooleanExpression(closureExpression);
if (booleanExpressions == null || booleanExpressions.isEmpty()) return;
+ inferNonNullFromContract(methodNode, annotationNode, booleanExpressions);
+
boolean isConstructor = methodNode.isConstructor();
boolean isPostcondition = AnnotationUtils.hasAnnotationOfType(annotationNode.getClassNode(), POSTCONDITION_TYPE_NAME);
@@ -276,6 +284,100 @@ public class AnnotationClosureVisitor extends BaseVisitor implements ASTNodeMeta
someNode.setNodeMetaData(PROCESSED, Boolean.TRUE);
}
+ /**
+ * Records non-null facts derivable from top-level {@code x != null} conjuncts in a
+ * {@code @Requires} or {@code @Ensures} closure, as {@link StaticTypesMarker#INFERRED_NON_NULL}
+ * metadata on the matching {@link Parameter} (or the {@link MethodNode} itself when the closure
+ * references {@code result} in a postcondition).
+ */
+ private static void inferNonNullFromContract(MethodNode methodNode, AnnotationNode annotationNode, List<BooleanExpression> booleanExpressions) {
+ String simpleName = annotationNode.getClassNode().getNameWithoutPackage();
+ boolean isEnsures = "Ensures".equals(simpleName);
+ if (!("Requires".equals(simpleName) || isEnsures)) return;
+ for (BooleanExpression booleanExpression : booleanExpressions) {
+ if (booleanExpression == null) continue;
+ collectNonNullFacts(booleanExpression.getExpression(), methodNode, isEnsures);
+ }
+ }
+
+ private static void collectNonNullFacts(Expression expr, MethodNode methodNode, boolean isEnsures) {
+ if (!(expr instanceof BinaryExpression bin)) return;
+ int op = bin.getOperation().getType();
+ if (op == Types.LOGICAL_AND) {
+ collectNonNullFacts(bin.getLeftExpression(), methodNode, isEnsures);
+ collectNonNullFacts(bin.getRightExpression(), methodNode, isEnsures);
+ return;
+ }
+ if (op != Types.COMPARE_NOT_EQUAL && op != Types.COMPARE_NOT_IDENTICAL) return;
+ String name = matchVarAgainstNull(bin.getLeftExpression(), bin.getRightExpression());
+ if (name == null) name = matchVarAgainstNull(bin.getRightExpression(), bin.getLeftExpression());
+ if (name == null) return;
+ if (isEnsures && "result".equals(name)) {
+ methodNode.putNodeMetaData(StaticTypesMarker.INFERRED_NON_NULL, Boolean.TRUE);
+ return;
+ }
+ for (Parameter p : methodNode.getParameters()) {
+ if (p.getName().equals(name)) {
+ p.putNodeMetaData(StaticTypesMarker.INFERRED_NON_NULL, Boolean.TRUE);
+ return;
+ }
+ }
+ }
+
+ private static String matchVarAgainstNull(Expression maybeVar, Expression maybeNull) {
+ if (!(maybeNull instanceof ConstantExpression) || !((ConstantExpression) maybeNull).isNullExpression()) return null;
+ if (maybeVar instanceof VariableExpression ve) return ve.getName();
+ return null;
+ }
+
+ private static final Set<String> NONNULL_ANNO_SIMPLE_NAMES = Set.of("NonNull", "NotNull", "Nonnull");
+
+ /**
+ * Records {@code return null} statements on a method whose return is effectively {@code @NonNull}
+ * and whose body will be rewritten by the contracts transform (postcondition or class invariant).
+ * The stashed list is read later by a downstream checker (e.g. NullChecker) which would otherwise
+ * no longer see the literal null after the rewrite.
+ */
+ private static void recordNonNullReturnViolations(MethodNode methodNode) {
+ if (methodNode.isVoidMethod() || methodNode.isAbstract() || methodNode.getCode() == null) return;
+ if (!isEffectivelyNonNullReturn(methodNode)) return;
+ if (!willHavePostconditionRewrite(methodNode)) return;
+ List<ASTNode> violations = null;
+ for (ReturnStatement rs : AssertStatementCreationUtility.getReturnStatements(methodNode)) {
+ Expression expr = rs.getExpression();
+ if (expr instanceof ConstantExpression ce && ce.isNullExpression()) {
+ if (violations == null) violations = new ArrayList<>();
+ violations.add(rs);
+ }
+ }
+ if (violations != null) {
+ methodNode.putNodeMetaData(StaticTypesMarker.INFERRED_NON_NULL_RETURN_VIOLATIONS, violations);
+ }
+ }
+
+ private static boolean isEffectivelyNonNullReturn(MethodNode method) {
+ if (Boolean.TRUE.equals(method.getNodeMetaData(StaticTypesMarker.INFERRED_NON_NULL))) return true;
+ for (AnnotationNode a : method.getAnnotations()) {
+ if (NONNULL_ANNO_SIMPLE_NAMES.contains(a.getClassNode().getNameWithoutPackage())) return true;
+ }
+ return false;
+ }
+
+ private static boolean willHavePostconditionRewrite(MethodNode method) {
+ for (AnnotationNode a : method.getAnnotations()) {
+ String name = a.getClassNode().getName();
+ if ("groovy.contracts.Ensures".equals(name) || "groovy.contracts.EnsuresConditions".equals(name)) return true;
+ }
+ ClassNode cls = method.getDeclaringClass();
+ if (cls != null) {
+ for (AnnotationNode a : cls.getAnnotations()) {
+ String name = a.getClassNode().getName();
+ if ("groovy.contracts.Invariant".equals(name) || "groovy.contracts.Invariants".equals(name)) return true;
+ }
+ }
+ return false;
+ }
+
//--------------------------------------------------------------------------
static class ClosureExpressionValidator extends ClassCodeVisitorSupport {
diff --git a/subprojects/groovy-typecheckers/src/main/groovy/groovy/typecheckers/NullChecker.groovy b/subprojects/groovy-typecheckers/src/main/groovy/groovy/typecheckers/NullChecker.groovy
index 0c9f352dc9..71ed0e24f6 100644
--- a/subprojects/groovy-typecheckers/src/main/groovy/groovy/typecheckers/NullChecker.groovy
+++ b/subprojects/groovy-typecheckers/src/main/groovy/groovy/typecheckers/NullChecker.groovy
@@ -118,6 +118,14 @@ class NullChecker extends GroovyTypeCheckingExtensionSupport.TypeCheckingDSL {
private CheckingVisitor makeVisitor(boolean flowSensitive, MethodNode method) {
boolean classNonNullByDefault = method.declaringClass != null && hasNonNullByDefaultAnno(method.declaringClass)
boolean methodNonNull = method.returnType != VOID_TYPE && (hasNonNullAnno(method) || (classNonNullByDefault && !hasNullableAnno(method)))
+ if (methodNonNull) {
+ def stash = method.getNodeMetaData(StaticTypesMarker.INFERRED_NON_NULL_RETURN_VIOLATIONS)
+ if (stash instanceof List) {
+ stash.each { node ->
+ addStaticTypeError("Cannot return null from @NonNull method '${method.name}'", node)
+ }
+ }
+ }
def initialNullable = method.parameters.findAll { hasNullableAnno(it) } as Set<Variable>
new CheckingVisitor() {
@@ -348,6 +356,7 @@ class NullChecker extends GroovyTypeCheckingExtensionSupport.TypeCheckingDSL {
}
private static boolean hasNonNullAnno(AnnotatedNode node) {
+ if (node.getNodeMetaData(StaticTypesMarker.INFERRED_NON_NULL) == Boolean.TRUE) return true
node.annotations?.any { it.classNode?.nameWithoutPackage in NONNULL_ANNOS } ?: false
}
diff --git a/subprojects/groovy-typecheckers/src/spec/doc/typecheckers.adoc b/subprojects/groovy-typecheckers/src/spec/doc/typecheckers.adoc
index b73c097aff..45fc8da3e6 100644
--- a/subprojects/groovy-typecheckers/src/spec/doc/typecheckers.adoc
+++ b/subprojects/groovy-typecheckers/src/spec/doc/typecheckers.adoc
@@ -765,6 +765,50 @@ We would see the following error at compile-time:
include::../test/NullCheckerTest.groovy[tags=nonnull_by_default_message,indent=0]
----
+=== Integration with @Requires and @Ensures
+
+When `groovy-contracts` is on the classpath, `NullChecker` recognises null-safety
+facts expressed through Design-by-Contract annotations. Top-level `x != null`
+conjuncts inside a `@Requires` closure mark the corresponding parameter as
+effectively `@NonNull`; a `result != null` conjunct inside an `@Ensures` closure
+marks the method return the same way. Both orderings (`x != null` and
+`null != x`) are recognised. Disjunctions (`||`) are deliberately not used for
+inference, since they do not establish non-nullness of any individual operand.
+
+[source,groovy]
+----
+include::../test/NullCheckerTest.groovy[tags=requires_integration,indent=0]
+----
+
+We would see the following error at compile-time:
+
+[source]
+----
+include::../test/NullCheckerTest.groovy[tags=requires_integration_message,indent=0]
+----
+
+The same applies on the return side with `@Ensures`:
+
+[source,groovy]
+----
+include::../test/NullCheckerTest.groovy[tags=ensures_integration,indent=0]
+----
+
+We would see the following error at compile-time:
+
+[source]
+----
+include::../test/NullCheckerTest.groovy[tags=ensures_integration_message,indent=0]
+----
+
+This coexists with explicit `@NonNull` annotations: a method marked `@NonNull`
+that also carries any `@Ensures` postcondition, or that lives in a class with
+an `@Invariant` class invariant, still has its literal `return null` statements
+flagged. Without this coordination, the `groovy-contracts` AST transform would
+rewrite the return statement before the type-checking pass runs, hiding the
+literal from the checker. The runtime postcondition continues to guard non-literal
+cases.
+
=== Flow-sensitive mode with strict option
`NullChecker` only flags issues involving annotated code. `NullChecker(strict: true)` adds
diff --git a/subprojects/groovy-typecheckers/src/spec/test/NullCheckerTest.groovy b/subprojects/groovy-typecheckers/src/spec/test/NullCheckerTest.groovy
index 5d3dae7629..e0d856af0b 100644
--- a/subprojects/groovy-typecheckers/src/spec/test/NullCheckerTest.groovy
+++ b/subprojects/groovy-typecheckers/src/spec/test/NullCheckerTest.groovy
@@ -416,4 +416,54 @@ class NullCheckerTest {
''')
}
+ @Test
+ void testRequiresIntegration() {
+ def err = shouldFail('''
+ import groovy.transform.TypeChecked
+ import groovy.contracts.Requires
+
+ // tag::requires_integration[]
+ @TypeChecked(extensions='groovy.typecheckers.NullChecker')
+ class Greeter {
+ @Requires({ name != null })
+ static String greet(name) { "Hello, $name!" }
+
+ static void main(String[] args) {
+ greet(null) // caught at compile time
+ }
+ }
+ // end::requires_integration[]
+ ''')
+ def expectedError = '''\
+ # tag::requires_integration_message[]
+ [Static type checking] - Cannot pass null to @NonNull parameter 'name' of 'greet'
+ # end::requires_integration_message[]
+ '''
+ assert err.message.contains(expectedError.readLines()[1].trim())
+ }
+
+ @Test
+ void testEnsuresIntegration() {
+ def err = shouldFail('''
+ import groovy.transform.TypeChecked
+ import groovy.contracts.Ensures
+
+ // tag::ensures_integration[]
+ @TypeChecked(extensions='groovy.typecheckers.NullChecker')
+ class Greeter {
+ @Ensures({ result != null })
+ String greet() {
+ return null // caught at compile time
+ }
+ }
+ // end::ensures_integration[]
+ ''')
+ def expectedError = '''\
+ # tag::ensures_integration_message[]
+ [Static type checking] - Cannot return null from @NonNull method 'greet'
+ # end::ensures_integration_message[]
+ '''
+ assert err.message.contains(expectedError.readLines()[1].trim())
+ }
+
}
diff --git a/subprojects/groovy-typecheckers/src/test/groovy/groovy/typecheckers/NullCheckerTest.groovy b/subprojects/groovy-typecheckers/src/test/groovy/groovy/typecheckers/NullCheckerTest.groovy
index ecace95c67..1297733f11 100644
--- a/subprojects/groovy-typecheckers/src/test/groovy/groovy/typecheckers/NullCheckerTest.groovy
+++ b/subprojects/groovy-typecheckers/src/test/groovy/groovy/typecheckers/NullCheckerTest.groovy
@@ -994,4 +994,129 @@ final class NullCheckerTest {
assert y.toString() == 'hello'
'''
}
+
+ // === groovy-contracts: @Requires/@Ensures inferring @NonNull ===
+
+ @Test
+ void testRequiresNotEqualNullInfersNonNullParam() {
+ def err = shouldFail shell, '''
+ import groovy.contracts.Requires
+ class Foo {
+ @Requires({ x != null })
+ String bar(x) { x.toString() }
+ }
+ new Foo().bar(null)
+ '''
+ assert err.message.contains("Cannot pass null to @NonNull parameter 'x'")
+ }
+
+ @Test
+ void testRequiresReverseNullComparisonInfersNonNullParam() {
+ def err = shouldFail shell, '''
+ import groovy.contracts.Requires
+ class Foo {
+ @Requires({ null != x })
+ String bar(x) { x.toString() }
+ }
+ new Foo().bar(null)
+ '''
+ assert err.message.contains("Cannot pass null to @NonNull parameter 'x'")
+ }
+
+ @Test
+ void testRequiresConjunctionInfersNonNullForEachParam() {
+ def err = shouldFail shell, '''
+ import groovy.contracts.Requires
+ class Foo {
+ @Requires({ left != null && right != null && right.size() > 0 })
+ String concat(left, middle, right) { left + middle + right }
+ }
+ new Foo().concat('a', 'b', null)
+ '''
+ assert err.message.contains("Cannot pass null to @NonNull parameter 'right'")
+ }
+
+ @Test
+ void testRequiresNonNullParamAcceptsNonNullArg() {
+ assertScript shell, '''
+ import groovy.contracts.Requires
+ class Foo {
+ @Requires({ x != null })
+ String bar(x) { x.toString() }
+ }
+ assert new Foo().bar('hi') == 'hi'
+ '''
+ }
+
+ @Test
+ void testEnsuresResultNonNullRejectsNullReturn() {
+ def err = shouldFail shell, '''
+ import groovy.contracts.Ensures
+ class Foo {
+ @Ensures({ result != null })
+ String bar() {
+ return null
+ }
+ }
+ '''
+ assert err.message.contains("Cannot return null from @NonNull method 'bar'")
+ }
+
+ @Test
+ void testEnsuresResultNonNullAcceptsNonNullReturn() {
+ assertScript shell, '''
+ import groovy.contracts.Ensures
+ class Foo {
+ @Ensures({ result != null })
+ String bar() { return 'hi' }
+ }
+ assert new Foo().bar() == 'hi'
+ '''
+ }
+
+ @Test
+ void testExplicitNonNullWithEnsuresRejectsNullReturn() {
+ // Contracts rewrites the return statement (because of @Ensures), which
+ // would otherwise hide the literal null from NullChecker. The stashed
+ // violation list carries the fact through.
+ def err = shouldFail shell, ANNOS + '''
+ import groovy.contracts.Ensures
+ class Foo {
+ @NonNull
+ @Ensures({ result.size() >= 0 })
+ String bar() {
+ return null
+ }
+ }
+ '''
+ assert err.message.contains("Cannot return null from @NonNull method 'bar'")
+ }
+
+ @Test
+ void testExplicitNonNullWithClassInvariantRejectsNullReturn() {
+ def err = shouldFail shell, ANNOS + '''
+ import groovy.contracts.Invariant
+ @Invariant({ true })
+ class Foo {
+ @NonNull
+ String bar() {
+ return null
+ }
+ }
+ '''
+ assert err.message.contains("Cannot return null from @NonNull method 'bar'")
+ }
+
+ @Test
+ void testRequiresDisjunctionDoesNotInferNonNull() {
+ // x != null || y != null does not guarantee either is non-null
+ assertScript shell, '''
+ import groovy.contracts.Requires
+ class Foo {
+ @Requires({ x != null || y != null })
+ String bar(x, y) { 'ok' }
+ }
+ assert new Foo().bar(null, 'present') == 'ok'
+ '''
+ }
}