spotbugs / spotbugs

SpotBugs is FindBugs' successor. A tool for static analysis to look for bugs in Java code.
https://spotbugs.github.io/
GNU Lesser General Public License v2.1
3.46k stars 585 forks source link

NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE with `@CheckForNull` annotation #1778

Open hgschmie opened 2 years ago

hgschmie commented 2 years ago

This code:

package testcode;

import static java.util.Objects.requireNonNull;

import java.util.function.Function;
import javax.annotation.CheckForNull;

public class SpotbugsTest {

    @CheckForNull
    public static Function<String, String> get() {
        return k -> "nope";
    }

    public static String getMapping(String value) {

        requireNonNull(value);

        Function<String, String> f = get();
        requireNonNull(f);
        return f.apply(value);
    }
}

reports [ERROR] Medium: Possible null pointer dereference in testcode.SpotbugsTest.getMapping(String, String) due to return value of called method [testcode.SpotbugsTest, testcode.SpotbugsTest] Method invoked at SpotbugsTest.java:[line 20]Known null at SpotbugsTest.java:[line 19] NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE

Objects#requireNonNull is supposed to do the null check, but is flagged as dereferencing its parameter.

Removing the @CheckForNull annotation makes the spotbugs check pass.

IntelliJ flow analysis shows this correctly:

(with requireNonNull commented out):

Screen Shot 2021-10-27 at 20 08 49

There is no such warning with the method call.

hgschmie commented 2 years ago

pretty ping?

hgschmie commented 2 years ago

pretty pretty ping?

Kidlike commented 2 years ago

I hit the same problem (bug?).

I got rid of the warning by replacing Objects#requireNonNull with Optional#ofNullable, which in the end I liked more, because I can control the exception being thrown.

hgschmie commented 2 years ago

I can still reproduce this bug with spotbugs 4.7.0

hgschmie commented 1 year ago

another pretty ping. This is now open for 18 months.

gtoison commented 1 year ago

Sorry for the belated answer, the project is a bit low on manpower and it is quite complicated as well (it is static flow analysis after all...)

Can you try: value = requireNonNull(value); Instead of: requireNonNull(value);

I've seen elsewhere that SpotBugs knows that the return of requireNonNull is non null, but it is not smart enough that an exception is thrown otherwise

malogulko commented 1 month ago

Hi! Thanks for your proposal, but value = requireNonNull(value); still triggers spot bugs failure.