Nullable check of java:S2259 does not respect optional code flow

Sonarqube 9.7.1

    var foo= Optional.ofNullable(fooValidatable) // this is the trigger to guess fooValidatable could be null
                                          .map(Validatable::getValue)
                                          .orElse(Boolean.FALSE); // or else and early exit not respected in code flow analysis

    if (foo.equals(Boolean.FALSE)) {
      // not enabled so ok
      return true;
    } else  {
      fooValidatable.addDistinctError(new ValidationMessage()); // here sonar thinks fooValidatable is nullable
      return false;
    }

As described in the comments the code flow that sonar tries to construct for nullable access can not happen because the orElse use on Optional.

Same when using checks on Optional for isPresent or isEmpty, its not considered by the rule producing hundreds of false positives!!

Kind regards,
Michael

Hello @reitzmichnicht,
Thanks for your message. I was able to reproduce the issue, so here is a ticket to fix it:

https://sonarsource.atlassian.net/browse/SONARJAVA-4400

Meanwhile I don’t feel like the fix will be trivial and handled soon. Anyway thanks for reporting. About the other issues, do you mean cases like this:

 Optional<Boolean> optionalBoolean = Optional.ofNullable(fooValidatable);

    if (optionalBoolean.isEmpty()) {
    } else {
      fooValidatable.booleanValue(); // here sonar thinks fooValidatable is nullable
    }

The thing with actually all issues (and the one you reported in the first place) like this is that we can’t guarantee that fooValidatable wasn’t reassigned (it’s not even final var). So basically the correct way would be to write it optionalBoolean.get().booleanValue();. Anyway, there is much space for improvement, we can identify the final(effectively final) variables and learn more constraints for them and this problem is already known by our team.

Regards,
Margarita

Hi Margarita,

yes your second example is another FP we see.

I understand that a fix may not be trivial, but to be honest if your logic can not determine this case correctly yet it should not raise complaints at all.
As this is a common coding pattern in hundreds of files, FPs are really not making our developers happy if it stops them from being able to merge there PRs.

Kind regards,
Michael

This topic was automatically closed 7 days after the last reply. New replies are no longer allowed.