GROOVY-11894: Provide a NullChecker for groovy-typecheckers (some combinations of NullChecker and @Requires/@ensures weren't working)

Author: paulk-asert

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
 }

subprojects/groovy-contracts/src/main/java/org/apache/groovy/contracts/ast/visitor/AnnotationClosureVisitor.java

@@ -49,6 +49,7 @@
 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.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.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 void visitConstructorOrMethod(MethodNode methodNode, boolean isConstructo
             replaceWithClosureClassReference(annotationNode, methodNode);
         }
 
+        recordNonNullReturnViolations(methodNode);
+
         markProcessed(methodNode);
 
         super.visitConstructorOrMethod(methodNode, isConstructor);
@@ -201,6 +207,8 @@ private void replaceWithClosureClassReference(AnnotationNode annotationNode, Met
         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 @@ private void markProcessed(ASTNode someNode) {
             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 {

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
     }
 

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

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())
+    }
+
 }