Rule not being triggered - Optional.get() S3655

Clean Code Report False-positive / False-negative...
, ,
1

While setting up our first repository and pipelines I added some code to test our pull request analysis.

I copied some bug that was reported in another part of the codebase and to my surprise it was not reported.

Speicifically the use of Optional.get() in Java without doint a isPresent() or isEmpty()

I then played around with the code and suddenly it popped up while the reason for being there didn’t change…

I don’t really care about the specific rule, I care about the fact that certain things are not being detected due to something unrelated…

sonarcloud

package mypackage;


import java.util.Optional;
import java.util.UUID;

class FullOfBugs {
    public void f() {
        String someString = null;
        System.out.println(someString.isEmpty());

        Optional<String> f = geto();
        String s = f.get();
        System.out.println(s);
    }

    public Optional<String> geto() {
        if (UUID.randomUUID().toString().startsWith("a")) {
            return Optional.of("sdf");
        } else {
            return Optional.ofNullable(null);
        }
    }
}
4

Hello Tom,

Welcome to the community, and thanks for your report! And for the nice detailed presentation of it :slight_smile:

It may be not strictly a bug, but I admit that the report is confusing. In your example, line 10:

  System.out.println(someString.isEmpty());

always throws a NullPointerException, and therefore line 13 can never be reached:

  String s = f.get();

Our control flow analyzer detects that and hence, does not report a problem for line 13. This is a minimal example where the same problem occurs:

  public String f(String s, Optional<String> opt) {
    s = null;
    s.length();
    return opt.get(); // not reported, because line 3 will throw a NPE
  }

I created a ticket here for a better handling of such cases.

Best,
Marco

5

Thanks for the reply.

I considered that might have been the reason. But still found it confusing because other lines are reported . For example the println() later in the method even if these are not bugs.

6

Yes, I agree, it’s confusing. If we would report the NPE at least, the situation would be more clear to the user. I suggested that as a possible solution in the ticket.

As for why some rules still do report: there’s a technical reason for that. Some of our rules require control flow information and some do not. Those that don’t need it ignore it entirely, and will consequently also report for unreachable positions.