[Java 11] RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE false positive

boris-petrov commented 5 years ago

I updated our code from Java 10 to Java 11 and a bunch of warnings appeared in SpotBugs:

DMC_DUBIOUS_MAP_COLLECTION
NP_LOAD_OF_KNOWN_NULL_VALUE
PCOA_PARTIALLY_CONSTRUCTED_OBJECT_ACCESS
RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE
RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE
RCN_REDUNDANT_NULLCHECK_OF_NULL_VALUE

Not sure why they would appear after updating Java. I haven't gone though them all but at least the one in the title of this issue is bogus:

try (InputStream stream = MyClass.class.getClassLoader().getResourceAsStream("some-file")) {
    System.err.println(stream);
} catch (IOException exception) {
    System.err.println("Error");
}

Something like this gives the warning on the catch line... again, this happens only on Java 11, not Java 10. Using OpenJDK 11.0.1.

P.S. Also, UPM_UNCALLED_PRIVATE_METHOD and WOC_WRITE_ONLY_COLLECTION_FIELD are also given and both are wrong. There is something amiss with fb-contrib on Java 11. How can I help?

boris-petrov commented 5 years ago

@mebigfatguy - just a heads-up on this one - PCOA_PARTIALLY_CONSTRUCTED_OBJECT_ACCESS seems to be fixed in the new release but the rest remain broken on Java 11. Any ideas there?

mebigfatguy commented 5 years ago

Well the RCN, NP and UCM ones are actually spotbugs proper. I'm on the hook for DMC and WOC. Do you have examples of DMC and WOC failing?

boris-petrov commented 5 years ago

Ah, sorry, I keep forgetting that there are built-in rules. :D I'll open an issue there then.

Here is a reproduction for both DMC and WOC:

class X<K, V> implements Map<K, V> {
    private final Set<String> foo = new HashSet<>();

    @Override
    public int size() {
        return 0;
    }

    @Override
    public boolean isEmpty() {
        return false;
    }

    @Override
    public boolean containsKey(Object key) {
        return false;
    }

    @Override
    public boolean containsValue(Object value) {
        return false;
    }

    @Override
    public V get(Object key) {
        return null;
    }

    @Override
    public V put(K key, V value) {
        foo.add("");
        return null;
    }

    @Override
    public V remove(Object key) {
        return null;
    }

    @Override
    public void putAll(Map<? extends K, ? extends V> m) {
    }

    @Override
    public void clear() {
    }

    @Override
    public Set<K> keySet() {
        return null;
    }

    @Override
    public Collection<V> values() {
        return null;
    }

    @Override
    public Set<Map.Entry<K, V>> entrySet() {
        return new EntrySet<>(this);
    }

    private static final class EntrySet<K, V> implements Set<Map.Entry<K, V>> {
        private final X<K, V> map;

        public EntrySet(X<K, V> map) {
            this.map = map;
        }

        private static final class Entry<K, V> implements Map.Entry<K, V> {
            private final Map<K, V> map;
            private final K key;

            public Entry(Map<K, V> map, K key) {
                this.map = map;
                this.key = key;
            }

            @Override
            public K getKey() {
                return key;
            }

            @Override
            public V getValue() {
                return map.get(key);
            }

            @Override
            public V setValue(V value) {
                return map.put(key, value);
            }
        }

        @Override
        public int size() {
            return map.size();
        }

        @Override
        public boolean isEmpty() {
            return map.foo.remove("");
        }

        @Override
        public boolean contains(Object o) {
            throw new UnsupportedOperationException();
        }

        @Override
        public Iterator<Map.Entry<K, V>> iterator() {
            Set<K> keySet = map.keySet();
            Iterator<K> iterator = keySet.iterator();

            return new Iterator<>() {
                @Override
                public boolean hasNext() {
                    return iterator.hasNext();
                }

                @Override
                public Map.Entry<K, V> next() {
                    return new EntrySet.Entry<>(map, iterator.next());
                }

                @Override
                public void remove() {
                    iterator.remove();
                }
            };
        }

        @Override
        public Object[] toArray() {
            throw new UnsupportedOperationException();
        }

        @Override
        public <T> T[] toArray(T[] a) {
            throw new UnsupportedOperationException();
        }

        @Override
        public boolean add(Map.Entry<K, V> e) {
            throw new UnsupportedOperationException();
        }

        @Override
        public boolean remove(Object o) {
            throw new UnsupportedOperationException();
        }

        @Override
        public boolean containsAll(Collection<?> c) {
            throw new UnsupportedOperationException();
        }

        @Override
        public boolean addAll(Collection<? extends Map.Entry<K, V>> c) {
            throw new UnsupportedOperationException();
        }

        @Override
        public boolean retainAll(Collection<?> c) {
            throw new UnsupportedOperationException();
        }

        @Override
        public boolean removeAll(Collection<?> c) {
            throw new UnsupportedOperationException();
        }

        @Override
        public void clear() {
            throw new UnsupportedOperationException();
        }
    }
}

Sorry it's a bit long but I guess it will do the job.

I guess for both of them the problem comes from passing/using the same variable in an inner class.

For WOC check the map.foo.remove(""); line - it is not caught by fb-contrib and it thinks the foo variable is only added to.

For DMC - the map variable in X.EntrySet is passed along in new EntrySet.Entry<>(map, iterator.next()); - that's where fb-contrib fails.

ThrawnCA commented 5 years ago

Does this reflect real code? I can't fathom what real-world situation would look like this.

boris-petrov commented 5 years ago

@ThrawnCA - it is a very real-world situation. We have an implementation of the Map interface that does what I've done here (and lots more, of course, which I've stripped down). The private final Set<String> foo = new HashSet<>(); thing is from a completely different place in our code, I just merged the two issues together here because I saw that both happen because of an inner class.

mebigfatguy commented 5 years ago

Mind posting a

javap -v -private X

output for this above class? I can confirm it's fine in jdk8

boris-petrov commented 5 years ago

@mebigfatguy:

%  javap -v -private X
Classfile /home/boris/X.class
  Last modified May 10, 2019; size 1731 bytes
  MD5 checksum e4a36bd6bd0ef7cbcc4a813771112156
  Compiled from "X.java"
class X<K extends java.lang.Object, V extends java.lang.Object> extends java.lang.Object implements java.util.Map<K, V>
  minor version: 0
  major version: 55
  flags: (0x0020) ACC_SUPER
  this_class: #9                          // X
  super_class: #10                        // java/lang/Object
  interfaces: 1, fields: 1, methods: 13, attributes: 4
Constant pool:
   #1 = Methodref          #10.#56        // java/lang/Object."<init>":()V
   #2 = Class              #57            // java/util/HashSet
   #3 = Methodref          #2.#56         // java/util/HashSet."<init>":()V
   #4 = Fieldref           #9.#58         // X.foo:Ljava/util/Set;
   #5 = String             #59            //
   #6 = InterfaceMethodref #60.#61        // java/util/Set.add:(Ljava/lang/Object;)Z
   #7 = Class              #62            // X$EntrySet
   #8 = Methodref          #7.#63         // X$EntrySet."<init>":(LX;)V
   #9 = Class              #64            // X
  #10 = Class              #65            // java/lang/Object
  #11 = Class              #66            // java/util/Map
  #12 = Utf8               EntrySet
  #13 = Utf8               InnerClasses
  #14 = Utf8               foo
  #15 = Utf8               Ljava/util/Set;
  #16 = Utf8               Signature
  #17 = Utf8               Ljava/util/Set<Ljava/lang/String;>;
  #18 = Utf8               <init>
  #19 = Utf8               ()V
  #20 = Utf8               Code
  #21 = Utf8               LineNumberTable
  #22 = Utf8               size
  #23 = Utf8               ()I
  #24 = Utf8               isEmpty
  #25 = Utf8               ()Z
  #26 = Utf8               containsKey
  #27 = Utf8               (Ljava/lang/Object;)Z
  #28 = Utf8               containsValue
  #29 = Utf8               get
  #30 = Utf8               (Ljava/lang/Object;)Ljava/lang/Object;
  #31 = Utf8               (Ljava/lang/Object;)TV;
  #32 = Utf8               put
  #33 = Utf8               (Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;
  #34 = Utf8               (TK;TV;)TV;
  #35 = Utf8               remove
  #36 = Utf8               putAll
  #37 = Utf8               (Ljava/util/Map;)V
  #38 = Utf8               (Ljava/util/Map<+TK;+TV;>;)V
  #39 = Utf8               clear
  #40 = Utf8               keySet
  #41 = Utf8               ()Ljava/util/Set;
  #42 = Utf8               ()Ljava/util/Set<TK;>;
  #43 = Utf8               values
  #44 = Utf8               ()Ljava/util/Collection;
  #45 = Utf8               ()Ljava/util/Collection<TV;>;
  #46 = Utf8               entrySet
  #47 = Class              #67            // java/util/Map$Entry
  #48 = Utf8               Entry
  #49 = Utf8               ()Ljava/util/Set<Ljava/util/Map$Entry<TK;TV;>;>;
  #50 = Utf8               <K:Ljava/lang/Object;V:Ljava/lang/Object;>Ljava/lang/Object;Ljava/util/Map<TK;TV;>;
  #51 = Utf8               SourceFile
  #52 = Utf8               X.java
  #53 = Utf8               NestMembers
  #54 = Class              #68            // X$EntrySet$Entry
  #55 = Class              #69            // X$EntrySet$1
  #56 = NameAndType        #18:#19        // "<init>":()V
  #57 = Utf8               java/util/HashSet
  #58 = NameAndType        #14:#15        // foo:Ljava/util/Set;
  #59 = Utf8
  #60 = Class              #70            // java/util/Set
  #61 = NameAndType        #71:#27        // add:(Ljava/lang/Object;)Z
  #62 = Utf8               X$EntrySet
  #63 = NameAndType        #18:#72        // "<init>":(LX;)V
  #64 = Utf8               X
  #65 = Utf8               java/lang/Object
  #66 = Utf8               java/util/Map
  #67 = Utf8               java/util/Map$Entry
  #68 = Utf8               X$EntrySet$Entry
  #69 = Utf8               X$EntrySet$1
  #70 = Utf8               java/util/Set
  #71 = Utf8               add
  #72 = Utf8               (LX;)V
{
  private final java.util.Set<java.lang.String> foo;
    descriptor: Ljava/util/Set;
    flags: (0x0012) ACC_PRIVATE, ACC_FINAL
    Signature: #17                          // Ljava/util/Set<Ljava/lang/String;>;

  X();
    descriptor: ()V
    flags: (0x0000)
    Code:
      stack=3, locals=1, args_size=1
         0: aload_0
         1: invokespecial #1                  // Method java/lang/Object."<init>":()V
         4: aload_0
         5: new           #2                  // class java/util/HashSet
         8: dup
         9: invokespecial #3                  // Method java/util/HashSet."<init>":()V
        12: putfield      #4                  // Field foo:Ljava/util/Set;
        15: return
      LineNumberTable:
        line 7: 0
        line 8: 4

  public int size();
    descriptor: ()I
    flags: (0x0001) ACC_PUBLIC
    Code:
      stack=1, locals=1, args_size=1
         0: iconst_0
         1: ireturn
      LineNumberTable:
        line 12: 0

  public boolean isEmpty();
    descriptor: ()Z
    flags: (0x0001) ACC_PUBLIC
    Code:
      stack=1, locals=1, args_size=1
         0: iconst_0
         1: ireturn
      LineNumberTable:
        line 17: 0

  public boolean containsKey(java.lang.Object);
    descriptor: (Ljava/lang/Object;)Z
    flags: (0x0001) ACC_PUBLIC
    Code:
      stack=1, locals=2, args_size=2
         0: iconst_0
         1: ireturn
      LineNumberTable:
        line 22: 0

  public boolean containsValue(java.lang.Object);
    descriptor: (Ljava/lang/Object;)Z
    flags: (0x0001) ACC_PUBLIC
    Code:
      stack=1, locals=2, args_size=2
         0: iconst_0
         1: ireturn
      LineNumberTable:
        line 27: 0

  public V get(java.lang.Object);
    descriptor: (Ljava/lang/Object;)Ljava/lang/Object;
    flags: (0x0001) ACC_PUBLIC
    Code:
      stack=1, locals=2, args_size=2
         0: aconst_null
         1: areturn
      LineNumberTable:
        line 32: 0
    Signature: #31                          // (Ljava/lang/Object;)TV;

  public V put(K, V);
    descriptor: (Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;
    flags: (0x0001) ACC_PUBLIC
    Code:
      stack=2, locals=3, args_size=3
         0: aload_0
         1: getfield      #4                  // Field foo:Ljava/util/Set;
         4: ldc           #5                  // String
         6: invokeinterface #6,  2            // InterfaceMethod java/util/Set.add:(Ljava/lang/Object;)Z
        11: pop
        12: aconst_null
        13: areturn
      LineNumberTable:
        line 37: 0
        line 38: 12
    Signature: #34                          // (TK;TV;)TV;

  public V remove(java.lang.Object);
    descriptor: (Ljava/lang/Object;)Ljava/lang/Object;
    flags: (0x0001) ACC_PUBLIC
    Code:
      stack=1, locals=2, args_size=2
         0: aconst_null
         1: areturn
      LineNumberTable:
        line 43: 0
    Signature: #31                          // (Ljava/lang/Object;)TV;

  public void putAll(java.util.Map<? extends K, ? extends V>);
    descriptor: (Ljava/util/Map;)V
    flags: (0x0001) ACC_PUBLIC
    Code:
      stack=0, locals=2, args_size=2
         0: return
      LineNumberTable:
        line 48: 0
    Signature: #38                          // (Ljava/util/Map<+TK;+TV;>;)V

  public void clear();
    descriptor: ()V
    flags: (0x0001) ACC_PUBLIC
    Code:
      stack=0, locals=1, args_size=1
         0: return
      LineNumberTable:
        line 52: 0

  public java.util.Set<K> keySet();
    descriptor: ()Ljava/util/Set;
    flags: (0x0001) ACC_PUBLIC
    Code:
      stack=1, locals=1, args_size=1
         0: aconst_null
         1: areturn
      LineNumberTable:
        line 56: 0
    Signature: #42                          // ()Ljava/util/Set<TK;>;

  public java.util.Collection<V> values();
    descriptor: ()Ljava/util/Collection;
    flags: (0x0001) ACC_PUBLIC
    Code:
      stack=1, locals=1, args_size=1
         0: aconst_null
         1: areturn
      LineNumberTable:
        line 61: 0
    Signature: #45                          // ()Ljava/util/Collection<TV;>;

  public java.util.Set<java.util.Map$Entry<K, V>> entrySet();
    descriptor: ()Ljava/util/Set;
    flags: (0x0001) ACC_PUBLIC
    Code:
      stack=3, locals=1, args_size=1
         0: new           #7                  // class X$EntrySet
         3: dup
         4: aload_0
         5: invokespecial #8                  // Method X$EntrySet."<init>":(LX;)V
         8: areturn
      LineNumberTable:
        line 66: 0
    Signature: #49                          // ()Ljava/util/Set<Ljava/util/Map$Entry<TK;TV;>;>;
}
Signature: #50                          // <K:Ljava/lang/Object;V:Ljava/lang/Object;>Ljava/lang/Object;Ljava/util/Map<TK;TV;>;
SourceFile: "X.java"
NestMembers:
  X$EntrySet
  X$EntrySet$Entry
  X$EntrySet$1
InnerClasses:
  private static final #12= #7 of #9;     // EntrySet=class X$EntrySet of class X
  public static #48= #47 of #11;          // Entry=class java/util/Map$Entry of class java/util/Map
  private static final #48= #54 of #7;    // Entry=class X$EntrySet$Entry of class X$EntrySet
  #55;                                    // class X$EntrySet$1

mebigfatguy / fb-contrib

[Java 11] RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE false positive #330