NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE false positive when code is …

…formatted on different lines (#2874)

* test: sample for issue #782

* fix: keep looking for an assertion even if we move to a different line

* test: updated bug sample comments

@nullable is @nonnull(when = When.UNKNOWN) so it seems OK that we're not
reporting a bug (i.e. some of the cases do not seem to be false
negatives)

* docs: added changelog entry

CHANGELOG.md

@@ -22,6 +22,7 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
 - Do not report DLS_DEAD_LOCAL_STORE for Java 21's type switches ([#2828](https://github.com/spotbugs/spotbugs/pull/2828))
 - Do not report UWF_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR for fields initialized in method annotated with @PostConstruct, @BeforeEach, etc. ([#2872](https://github.com/spotbugs/spotbugs/pull/2872) [#2870](https://github.com/spotbugs/spotbugs/issues/2870) [#453](https://github.com/spotbugs/spotbugs/issues/453))
 - Do not report DLS_DEAD_LOCAL_STORE for Hibernate bytecode enhancements ([#2865](https://github.com/spotbugs/spotbugs/pull/2865))
+- Fixed NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE false positives due to source code formatting ([#2874](https://github.com/spotbugs/spotbugs/pull/2874))
 
 ### Added
 - New detector `MultipleInstantiationsOfSingletons` and introduced new bug types:

spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/Issue782Test.java

@@ -0,0 +1,41 @@
+package edu.umd.cs.findbugs.detect;
+
+import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly;
+import static org.hamcrest.MatcherAssert.*;
+
+import edu.umd.cs.findbugs.AbstractIntegrationTest;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcher;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder;
+
+import org.junit.jupiter.api.Test;
+
+public class Issue782Test extends AbstractIntegrationTest {
+
+    @Test
+    void testIssue() {
+        performAnalysis(
+                "ghIssues/Issue782.class",
+                "ghIssues/Issue782$MyNullable.class");
+
+        assertBugCount("NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE", 1);
+
+        assertBugAtLine("NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE", 27);
+    }
+
+    private void assertBugCount(String type, int expectedCount) {
+        BugInstanceMatcher bugMatcher = new BugInstanceMatcherBuilder()
+                .bugType(type)
+                .build();
+
+        assertThat(getBugCollection(), containsExactly(expectedCount, bugMatcher));
+    }
+
+    private void assertBugAtLine(String type, int line) {
+        BugInstanceMatcher bugMatcher = new BugInstanceMatcherBuilder()
+                .bugType(type)
+                .atLine(line)
+                .build();
+
+        assertThat(getBugCollection(), containsExactly(1, bugMatcher));
+    }
+}

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindNullDeref.java

@@ -1378,7 +1378,10 @@ boolean callToAssertionMethod(Location loc) {
                 }
             } else {
                 int line = ln.getSourceLine(pos);
-                if (line != firstLine) {
+                // For a call such as checkNotNull(get()) the 'pos' would be 'firstPos' + 3
+                // Break the loop if we moved to a different source line AND moved the PC by at least 3
+                // checkNotNull (or similar) might be formatted on a different line than its argument
+                if (line != firstLine && pos > firstPos + 3) {
                     break;
                 }
             }

spotbugsTestCases/src/java/ghIssues/Issue782.java

@@ -0,0 +1,73 @@
+package ghIssues;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+import static java.util.Objects.requireNonNull;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+import javax.annotation.Nullable;
+import javax.annotation.meta.TypeQualifierNickname;
+import javax.annotation.meta.When;
+
+/**
+ * False negatives and positives of NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE.
+ * Results surprisingly depends on the actual formatting of the source code.
+ */
+public class Issue782 {
+
+    /**
+     * Invoking get() which is annotated with a custom Nullable.
+     */
+    public void func() {
+        Object a = get(); // true positive
+        Object b = checkNotNull(get()); // true negative
+        Object c = checkNotNull(
+                get()); // false positive (fixed by the change)
+        System.out.println(a.toString());
+        System.out.println(b.toString());
+        System.out.println(c.toString());
+    }
+
+    /**
+     * Invoking get2() which is annotated with a javax Nullable, verify return value with Guava checkNotNull.
+     */
+    public void func2() {
+        Object a = get2(); // @Nullable is @Nonnull(when = When.UNKNOWN) so it seems OK that we're not reporting a bug
+        Object b = checkNotNull(get2()); // true negative
+        Object c = checkNotNull(
+                get2()); // true negative
+        System.out.println(a.toString());
+        System.out.println(b.toString());
+        System.out.println(c.toString());
+    }
+
+    /**
+     * Invoking get2() which is annotated with a javax Nullable, verify return value with requireNonNull.
+     */
+    public void func3() {
+        Object a = get2(); // @Nullable is @Nonnull(when = When.UNKNOWN) so it seems OK that we're not reporting a bug
+        Object b = requireNonNull(get2()); // true negative
+        Object c = requireNonNull(
+                get2()); // true negative
+        System.out.println(a.toString());
+        System.out.println(b.toString());
+        System.out.println(c.toString());
+    }
+
+    @MyNullable
+    public Object get() {
+        return null;
+    }
+
+    @Nullable
+    public Object get2() {
+        return null;
+    }
+
+    @TypeQualifierNickname
+    @javax.annotation.Nonnull(when = When.MAYBE)
+    @Retention(RetentionPolicy.CLASS)
+    public @interface MyNullable {
+    }
+}