fb-contrib
fb-contrib copied to clipboard
mebigfatguy
[Java 11] RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE false positive
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?
@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?
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?
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.
Does this reflect real code? I can't fathom what real-world situation would look like this.
@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.
Mind posting a
javap -v -private X
output for this above class? I can confirm it's fine in jdk8
@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