S1696: "NullPointerException" should not be caught - Map.contains can throw an NPE during null check

Rules and Languages Report False-positive / False-negative...
1
  • Language: Java
  • Rule: S1696: “NullPointerException” should not be caught
  • Why it can be a false positive:
    • ImmutableCollections will throw an NPE if you check them for null. Of note, it is part of the method contract for contains. Specifically, from List#contains

      @throws NullPointerException if the specified element is null and this list does not permit null elements

  • Software:
    • SonarLint: 10.6.2.78685
      • Binding: https://josm.openstreetmap.de/sonar/
      • Project key: josm
    • IntelliJ IDEA: 2024.1.3
    • SonarQube: 9.9.4 (for the binding server)
  • Reproducer
import java.util.Arrays;
import java.util.Collection;
import java.util.List;

class Scratch {
    public static void main(String[] args) {
        checkNull(Arrays.asList(null, ""));
        checkNull(List.of(""));
    }

    static void checkNull(Collection<Object> collection) {
        try {
            if (collection.contains(null)) {
                System.out.println("No null");
            } else {
                System.out.println("Null found");
            }
        } catch (NullPointerException e) {
            System.out.println("Collection does not support null");
        }
    }
}
3

Hey @tsmock!

I just tried it out in VSCode (after enabling S1696, which is not in the default Qualtiy Profile). The issue raised as expected;

Screenshot 2024-07-09 at 08.50.39
Screenshot 2024-07-09 at 08.50.391920×991 144 KB

Can you double check the rule is enabled in the Quality Profile used for the project you’re binding to?

4

I think we have a communications issue. I was reporting a false positive, not a false negative.
My understanding of false positive: an issue is raised when it should not be.
My understanding of false negative: an issue is not raised when it should be.

The rule is enabled for my projects quality profile.
The false positive is that there are some method contracts which cannot be safely checked for null (in this case Collections#contains). For methods in the JDK modules, the rule should ignore catching NullPointerExceptions where there is not a sane alternative to avoid it.

5

Ah, I’m sorry. I sped through this thread too fast. I’ll flag an expert.

1 Like
9

Hi @tsmock,

Thank you for reporting.
I reviewed your example and am trying to understand your use case better.
It is correct that null is not allowed in java.util.List . So why do you try to check if the List contains null values?
Can you do that when inserting elements in the List? In my opinion, that is where the problem originates.

All the best,

Irina

10

The example has a function which takes an arbitrary collection which does stuff (for the example, just print if the collection contains null or not).
This is due to the fact that whether a collection allows null values is up to the collection implementation. Some lists allow null (like Arrays.asList) while other lists don’t (like List.of). Unfortunately there is not a good way to check whether or not a list allows null; you just have to catch a NullPointerException.

13

Hi,

I agree that this is a false positive, but the behavior you describe is extremely specific and I do not think that adding an exception for it is a good course of action.

So we will just accept this case as a false positive. If you think this is a serious issue, please provide us with a more common example.

Best,
Gabriel