diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/SpringHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/SpringHandler.java index 811e09ba16..0c02a77335 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/SpringHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/SpringHandler.java @@ -20,12 +20,19 @@ public class SpringHandler implements Handler { /** * Matches a SpEL fragment like {@code #{...}} when it contains {@code null} as a standalone * token. This lets us distinguish Spring {@code @Value} expressions that may produce {@code null} - * from plain property placeholders or string literals containing the letters {@code null}. This - * is a heuristic match and may have false positives. + * from plain property placeholders or string literals containing the letters {@code null}. */ private static final Pattern VALUE_NULL_SPEL_PATTERN = Pattern.compile("#\\{[^}]*\\bnull\\b[^}]*}"); + /** + * Matches {@code null} used as an operand in equality comparisons ({@code == null}, {@code != + * null}, {@code null ==}, {@code null !=}). These occurrences do not produce a {@code null} value + * and should be excluded from the SpEL null detection heuristic. + */ + private static final Pattern NULL_COMPARISON_PATTERN = + Pattern.compile("[!=]=\\s*\\bnull\\b|\\bnull\\b\\s*[!=]="); + @Override public FieldSkipResult shouldSkipFieldInitializationCheck( Symbol.ClassSymbol classSymbol, Symbol fieldSymbol, VisitorState state) { @@ -45,6 +52,12 @@ public FieldSkipResult shouldSkipFieldInitializationCheck( } private static boolean containsNullSpELExpression(String annotationValue) { - return VALUE_NULL_SPEL_PATTERN.matcher(annotationValue).find(); + if (!VALUE_NULL_SPEL_PATTERN.matcher(annotationValue).find()) { + return false; + } + // Strip null occurrences that are only used in equality comparisons (e.g., != null, == null) + // and re-check whether any standalone null token remains as a potential return value. + String withoutComparisons = NULL_COMPARISON_PATTERN.matcher(annotationValue).replaceAll(""); + return VALUE_NULL_SPEL_PATTERN.matcher(withoutComparisons).find(); } } diff --git a/nullaway/src/main/java/com/uber/nullaway/handlers/contract/fieldcontract/RequiresNonNullHandler.java b/nullaway/src/main/java/com/uber/nullaway/handlers/contract/fieldcontract/RequiresNonNullHandler.java index 8b7896bae8..eb2f574bd8 100644 --- a/nullaway/src/main/java/com/uber/nullaway/handlers/contract/fieldcontract/RequiresNonNullHandler.java +++ b/nullaway/src/main/java/com/uber/nullaway/handlers/contract/fieldcontract/RequiresNonNullHandler.java @@ -106,7 +106,7 @@ protected void validateOverridingRules( StringBuilder errorMessage = new StringBuilder(); errorMessage.append( "precondition inheritance is violated, method in child class cannot have a stricter precondition than its closest overridden method, adding @requiresNonNull for fields ["); - Iterator iterator = overriddenFieldNames.iterator(); + Iterator iterator = overridingFieldNames.iterator(); while (iterator.hasNext()) { errorMessage.append(iterator.next()); if (iterator.hasNext()) { diff --git a/nullaway/src/test/java/com/uber/nullaway/EnsuresNonNullTests.java b/nullaway/src/test/java/com/uber/nullaway/EnsuresNonNullTests.java index 453146879d..31c651afa0 100644 --- a/nullaway/src/test/java/com/uber/nullaway/EnsuresNonNullTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/EnsuresNonNullTests.java @@ -141,7 +141,7 @@ public void test3() { a.call(); } @RequiresNonNull("b") - // BUG: Diagnostic contains: precondition inheritance is violated, method in child class cannot have a stricter precondition than its closest overridden method, adding @requiresNonNull for fields [a] makes this method precondition stricter + // BUG: Diagnostic contains: precondition inheritance is violated, method in child class cannot have a stricter precondition than its closest overridden method, adding @requiresNonNull for fields [b] makes this method precondition stricter public void test4() { // BUG: Diagnostic contains: dereferenced expression a is @Nullable a.call(); @@ -482,4 +482,44 @@ public void negativeRequiresNonnull() { "Item.java", "package com.uber;", "class Item {", " public void call() { }", "}") .doTest(); } + + @Test + public void requiresNonNullOverridingErrorMessageListsExtraFields() { + defaultCompilationHelper + .addSourceLines( + "SuperClass.java", + """ + package com.uber; + import javax.annotation.Nullable; + import com.uber.nullaway.annotations.RequiresNonNull; + class SuperClass { + @Nullable Item a; + @RequiresNonNull("a") + public void doWork() { + a.call(); + } + } + """) + .addSourceLines( + "ChildClass.java", + """ + package com.uber; + import javax.annotation.Nullable; + import com.uber.nullaway.annotations.RequiresNonNull; + class ChildClass extends SuperClass { + @Nullable Item b; + @Nullable Item c; + @RequiresNonNull({"a", "b", "c"}) + // BUG: Diagnostic contains: adding @requiresNonNull for fields [b, c] makes this method precondition stricter + public void doWork() { + a.call(); + b.call(); + c.call(); + } + } + """) + .addSourceLines( + "Item.java", "package com.uber;", "class Item {", " public void call() { }", "}") + .doTest(); + } } diff --git a/nullaway/src/test/java/com/uber/nullaway/FrameworkTests.java b/nullaway/src/test/java/com/uber/nullaway/FrameworkTests.java index bf8d873882..70df86cdde 100644 --- a/nullaway/src/test/java/com/uber/nullaway/FrameworkTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/FrameworkTests.java @@ -1502,4 +1502,106 @@ public boolean bar(@Nullable Path p) { """) .doTest(); } + + @Test + public void springValueSpelWithNullInConditional() { + defaultCompilationHelper + .addSourceLines( + "Value.java", + """ + package org.springframework.beans.factory.annotation; + import java.lang.annotation.ElementType; + import java.lang.annotation.Retention; + import java.lang.annotation.RetentionPolicy; + import java.lang.annotation.Target; + @Target({ElementType.FIELD, ElementType.PARAMETER, ElementType.METHOD}) + @Retention(RetentionPolicy.RUNTIME) + public @interface Value { + String value(); + } + """) + .addSourceLines( + "Test.java", + """ + package com.uber; + import org.springframework.beans.factory.annotation.Value; + class Test { + // SpEL conditional: 'null' is used only as a comparison operand, not as a + // return value. The heuristic correctly strips comparison-only nulls. + @Value("#{someBean != null ? someBean.value : 'default'}") + String spelConditionalNullCheck; + } + """) + .doTest(); + } + + @Test + public void springValueSpelNullAsReturnValue() { + defaultCompilationHelper + .addSourceLines( + "Value.java", + """ + package org.springframework.beans.factory.annotation; + import java.lang.annotation.ElementType; + import java.lang.annotation.Retention; + import java.lang.annotation.RetentionPolicy; + import java.lang.annotation.Target; + @Target({ElementType.FIELD, ElementType.PARAMETER, ElementType.METHOD}) + @Retention(RetentionPolicy.RUNTIME) + public @interface Value { + String value(); + } + """) + .addSourceLines( + "Test.java", + """ + package com.uber; + import org.springframework.beans.factory.annotation.Value; + class Test { + // Boundary: null appears in a comparison AND as a ternary return value (then-branch). + @Value("#{someBean != null ? null : 'default'}") + // BUG: Diagnostic contains: @NonNull field nullInThenBranch not initialized + String nullInThenBranch; + // Boundary: null appears in a comparison AND as a ternary return value (else-branch). + @Value("#{someBean != null ? someBean.value : null}") + // BUG: Diagnostic contains: @NonNull field nullInElseBranch not initialized + String nullInElseBranch; + } + """) + .doTest(); + } + + @Test + public void springValueSpelNullComparisonLeftSide() { + defaultCompilationHelper + .addSourceLines( + "Value.java", + """ + package org.springframework.beans.factory.annotation; + import java.lang.annotation.ElementType; + import java.lang.annotation.Retention; + import java.lang.annotation.RetentionPolicy; + import java.lang.annotation.Target; + @Target({ElementType.FIELD, ElementType.PARAMETER, ElementType.METHOD}) + @Retention(RetentionPolicy.RUNTIME) + public @interface Value { + String value(); + } + """) + .addSourceLines( + "Test.java", + """ + package com.uber; + import org.springframework.beans.factory.annotation.Value; + class Test { + // Boundary: null on the LEFT side of != comparison (symmetric case). + @Value("#{null != someBean ? someBean.value : 'default'}") + String nullOnLeftNeq; + // Boundary: null on the LEFT side of == comparison (symmetric case). + @Value("#{null == someBean ? 'default' : someBean.value}") + String nullOnLeftEq; + } + """) + .doTest(); + } } diff --git a/nullaway/src/test/java/com/uber/nullaway/JakartaPersistenceTests.java b/nullaway/src/test/java/com/uber/nullaway/JakartaPersistenceTests.java index 4984b26d45..dc14d8a6c5 100644 --- a/nullaway/src/test/java/com/uber/nullaway/JakartaPersistenceTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/JakartaPersistenceTests.java @@ -503,4 +503,37 @@ class Sub extends Super { """) .doTest(); } + + @Test + public void jakartaPersistenceMixedAccessBothMappingsIsUnknown() { + addJpaAnnotationStubs(defaultCompilationHelper) + .addSourceLines( + "MixedAccessEntity.java", + """ + package com.uber; + import jakarta.persistence.Entity; + import jakarta.persistence.Column; + import jakarta.persistence.Id; + @Entity + class MixedAccessEntity { + // @Column on a field -> hasFieldMapping = true + @Column + // BUG: Diagnostic contains: @NonNull field name not initialized + String name; + // BUG: Diagnostic contains: @NonNull field id not initialized + Long id; + // @Id on a getter -> hasPropertyMapping = true + // Both field and property mappings present -> access type is UNKNOWN + // -> shouldSkipFieldInitializationCheck returns false for all fields + @Id + public Long getId() { + return id; + } + public void setId(Long id) { + this.id = id; + } + } + """) + .doTest(); + } } diff --git a/nullaway/src/test/java/com/uber/nullaway/SyncLambdasTests.java b/nullaway/src/test/java/com/uber/nullaway/SyncLambdasTests.java index 262538351d..2cf4f8dd12 100644 --- a/nullaway/src/test/java/com/uber/nullaway/SyncLambdasTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/SyncLambdasTests.java @@ -175,4 +175,40 @@ public void test1() { """) .doTest(); } + + @Test + public void forEachOnMapWithAnonymousClass() { + defaultCompilationHelper + .addSourceLines( + "Test.java", + """ + package com.uber; + import java.util.Map; + import java.util.HashMap; + import java.util.function.BiConsumer; + import org.jspecify.annotations.Nullable; + public class Test { + private @Nullable Map target; + private @Nullable Map resolved; + public void initialize() { + if (this.target == null) { + throw new IllegalArgumentException(); + } + this.resolved = new HashMap<>(); + // Unlike a lambda callback, an anonymous class callback does NOT propagate + // nullness facts from the enclosing scope. 'Test.this.resolved' is known + // non-null at this point, but inside the anonymous class method NullAway + // cannot use that fact because the access path changes (this$0.resolved). + this.target.forEach(new BiConsumer() { + @Override + public void accept(Object key, Object value) { + // BUG: Diagnostic contains: dereferenced expression Test.this.resolved is @Nullable + Test.this.resolved.put(key, value); + } + }); + } + } + """) + .doTest(); + } }