Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible Nullpointer dereference - False positive #456

Closed
phoenix384 opened this issue Oct 17, 2017 · 14 comments
Closed

Possible Nullpointer dereference - False positive #456

phoenix384 opened this issue Oct 17, 2017 · 14 comments

Comments

@phoenix384
Copy link

File[] content = new File(".").listFiles();
Objects.requireNonNull(content);

results in "Possible Nullpointer dereference due to return value of called method" for the second line.

@iloveeclipse
Copy link
Member

Why do you need Objects.requireNonNull()? If the array is null, next access to it will throw NPE, same as Objects thing - but probably you simply want to do something with the array only if it is non null?

But yea, we could consider that method in the NPE analysis similar to an "assert".

@phoenix384
Copy link
Author

Because with Object.requireNonNull I can add a specific error message like
Objects.requireNonNull(content, "Unable to read directory");
That is better or more helpful than just the normal NPE.

Just like
Validate.notNull(content, "...");
from Apache Commons Lang3 which is NOT marked by SpotBugs, although it's doing the same thing.

@Maaartinus
Copy link

Maaartinus commented Oct 20, 2017

TL;DR Using Objects.requireNonNull means that the NPE is an expected outcome and should produce no warning.


But yea, we could consider that method in the NPE analysis similar to an "assert".

AFAICT it's not similar.

When I do Objects.requireNonNull or Preconditions.checkNotNull, then I'm well aware of the possibility of NPE and definitely want no warning for this line. Later, the object is known not to be null.

After an assert, the object is not known not to be null. When I do

assert started != null;
if (started == null) ...

then I hope that started isn't null, but I'm not perfectly sure and the conditional is needed to deal with it (and should produce no warning like in findbugsproject/findbugs/issues/166).

@xenoterracide
Copy link
Contributor

xenoterracide commented Oct 23, 2017

TL;DR Using Objects.requireNonNull means that the NPE is an expected outcome and should produce no warning.

plus one, we have this problem in findbugs, and it's obnoxious. If I explicitly throw an NPE, whether it's by if ( i == null ) { throw new NullPointerException() } or use Objects.requireNonNull it should be the same thing. Often I use requireNonNull so it doesn't fail farther down in a method, and usually it's to catch programming errors, vs actual runtime errors. Still grumpy that requireNonNull doesn't throw an IllegalArgumentException

assert wise, IIRC you have to enable something in the JVM in order for those to actually trigger in a production run.

@YEMEAC
Copy link

YEMEAC commented Jul 18, 2018

you can use if (method == null) throw new AssertionError("error message) also

@Octogonapus
Copy link

Sure, but this is still a bug. Is anyone working on this, or is there any place for someone else to start?

@xpyct707
Copy link

xpyct707 commented Dec 3, 2018

There is a quote from Bloch's 'Effective Java' (3rd Edition) - Item 49: Check parameters for validity:

The Objects.requireNonNull method, added in Java 7, is flexible and convenient, so there’s no reason to perform null checks manually anymore. You can specify your own exception detail message if you wish. The method returns its input, so you can perform a null check at the same time as you use a value:
// Inline use of Java's null-checking facility
this.strategy = Objects.requireNonNull(strategy, "strategy");
You can also ignore the return value and use Objects.requireNonNull as a freestanding null check where that suits your needs.

So this issue is definitely a bug and it must be resolved.

@Dichotomia
Copy link
Contributor

Same problem here.

@bbottema
Copy link

Same here. Now I've got to manually suppress this warning for the whole method, unfortunately.

@bbottema
Copy link

Crap, just ran into it again in a new situation when I noticed I already added myself on the list here. I would +1 myself if I could :P

@victornoel
Copy link

I think this is a duplicate of #651

@stealthrabbi
Copy link

This issue is older than #651, so this can't be the duplicate.

@victornoel
Copy link

@stealthrabbi I mean that #651 is an issue that is actually considered as valid by the spotbugs team, contrary to this one which has been nicely ignored for 3 years :) hence, to focus effort of everybody interested in this to happen, we should close this one and all go complain on the other one :)

@hazendaz
Copy link
Member

fixed by #2709

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests