Fix NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE does not consider null val…

[mdouaihy]

This fixes the issue reported in

• findbugs:NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE does not consider null validation #651
• Possible Nullpointer dereference - False positive #456

NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE is raised when using Objects.requireNonNull whereas it's not raised when using Guava Preconditions.checkNotNull.

---

Make sure these boxes are checked before submitting your PR -- thank you!

• Added an entry into CHANGELOG.md if you have changed SpotBugs code

[gtoison]

Thanks a lot for the contribution @mdouaihy
#651 mentionned other methods than Objects.requireNonNull, did you consider adding things like:

com.google.common.base.Verify.verifyNotNull
com.google.common.base.Preconditions.checkNotNull
org.apache.commons.lang.Validate.notNull

I think it would be good adding unit tests for these too

[hazendaz]

Leaving this one out of 4.8.2 release as a few other items need looked at here still.

[robinjhector]

Hi, this would be great to get merged and released! Any chance you can take a look at the requested changes @mdouaihy ?

[mdouaihy]

hi @robinjhector,

I am looking into applying the requested changes. I am just having a ClassNotFound when adding the tests to cover the changes and I am trying to figure out why.

[JuditKnoll]

hi @robinjhector,

I am looking into applying the requested changes. I am just having a ClassNotFound when adding the tests to cover the changes and I am trying to figure out why.

In this case, it can be a good idea to convert the PR to draft.
Is the class file for not found class added to the performAnalysis() function inside the testclass? If yes and you are stuck, feel free to commit your changes, and I can look into it.

[hazendaz]

@mdouaihy Can you look at last concern here? Would like to include this in next release in 2 weeks.

[mdouaihy]

@hazendaz, due to some personal problems, I was not able to fix the issues before. I'll fix that during the coming couple of days.

[hazendaz]

@JuditKnoll Any chance you could pick this up and do the necessary items you were wanting adjusted? Seems @mdouaihy Has some personal issues to deal with so not available to make this go. If its not too difficult, we could get this included in upcoming release hopefully this weekend :)

[JuditKnoll]

Thanks a lot for the contribution @mdouaihy #651 mentionned other methods than Objects.requireNonNull, did you consider adding things like:

com.google.common.base.Verify.verifyNotNull
com.google.common.base.Preconditions.checkNotNull
org.apache.commons.lang.Validate.notNull

I think it would be good adding unit tests for these too

There is already a unit test for com.google.common.base.Preconditions.checkNotNull with RV_RETURN_VALUE_IGNORED_Guava_Preconditions as sample class tested in PreconditionsCheckNotNullCanIgnoreReturnValueTest, but when running it, the com.google.common.base.Preconditions class needed for analysis was missing, and the test does not fail because of the missing class.

[gtoison]

@JuditKnoll would you know if the missing class error is because of the way we are running unit tests?
I was wondering if it could be a problem similar to #438

[JuditKnoll]

@JuditKnoll would you know if the missing class error is because of the way we are running unit tests? I was wondering if it could be a problem similar to #438

Yes, it seems really similar.
The AbstractIntegrationTest.performAnalysis() adds the auxClassPathEntries from the spotbugsTestCases/lib directory, adding the necessary jars here, removes these messages. I think we should add the aux classpathes listed in the spotbugsTestCases/build/spotbugs/auxclasspath/spotbugsMain file, or if you have a better idea, I would love to hear it, but this should be in a separate PR.

P.S. I'm still looking into whether com.google.common.base.Verify.verifyNotNull(), org.apache.commons.lang3.Validate.notNull() are already handled, or just not found because of some connecting problem in the tests.

[JuditKnoll]

I addressed the issues mentioned in the reviews. However, when running the test separately from the IDE, the following is written to the std err*:

The following classes needed for analysis were missing:
  com.google.common.base.Verify
  org.apache.commons.lang3.Validate
  com.google.common.base.Preconditions

Locally I added these to the spotbugsTestcases/lib, which does solve this issue, and the tests in IssueRequireNotNullTest are still successful, but some other tests fail, which also fail on master if the necessary jars are added.
It looks like the functions are handled by the IsNullValueDataflow, and not by callToAssertionMethod() function.

*When running from terminal using gradle, these missing classes messages does not appear in the output, the build is successful. IMO if there are missing classes, the tests should fail.

[gtoison]

This LGTM, thank you!

[hazendaz]

fixes #651 and fixes #456