RV_RETURN_VALUE_IGNORED should not be reported for methods annotated with '@com.google.errorprone.annotations.CanIgnoreReturnValue'

[BrainStone]

Methods annoted with @com.google.errorprone.annotations.CanIgnoreReturnValue generate an RV_RETURN_VALUE_IGNORED error.

For example com.google.common.base.Stopwatch#start causes this issue.

[BrainStone]

This bug appears when using the gradle plugin and using version 3.1.0-RC7. Manually setting the version to 3.1.0-RC6 works fine.

[iloveeclipse]

Can you please pro ide a simple code example and what exactly is reported by SpotBugs?

[iloveeclipse]

Probably related to #429 fix.

[rtack]

Simple code example

import com.google.common.base.Joiner;
public class Foo {
    String[] bars = { "Hello", "World" };
    public void someMethod() {
        StringBuilder sb = new StringBuilder();
        Joiner.on('|').appendTo(sb, bars);
    }
}

Spotbug reports

Bug: Return value of com.google.common.base.Joiner.appendTo(StringBuilder, Object[]) ignored in ch.homegate.services.maps.Foo.someMethod()
The return value of this method should be checked. One common cause of this warning is to invoke a method on an immutable object, thinking that it updates the object. For example, in the following code fragment,

String dateString = getHeaderField(name);
dateString.trim();
the programmer seems to be thinking that the trim() method will update the String referenced by dateString. But since Strings are immutable, the trim() function returns a new String value, which is being ignored here. The code should be corrected to:

String dateString = getHeaderField(name);
dateString = dateString.trim();
Rank: Scary (5), confidence: Normal
Pattern: RV_RETURN_VALUE_IGNORED 
Type: RV, Category: CORRECTNESS (Correctness)

Joiner.appendTo

  @CanIgnoreReturnValue
  public final StringBuilder appendTo(StringBuilder builder, Iterable<?> parts) {
    return appendTo(builder, parts.iterator());
  }

[iloveeclipse]

I believe this has nothing to do with CanIgnoreReturnValue annotation in the sense that the detector simply does not know anything about it.

So what you probably want is that SpotBugs knows that such annotation exists and ignores the ignored return value if the called method is annotated with CanIgnoreReturnValue.

[KengoTODA]

As I commented at #429 (comment)
existing solution is JSR305 annotation.

[bmarwell]

@KengoTODA in other issues it is being discussed that JSR305 is dead and should not be used.
Also, I doubt that Guava will switch to JSR305, as they are using errorprone annotations already.

Spotbugs should know about this Annotation and treat it likewise.

// Edit: Actually it was your own (correct) proposal for 4.0.0:
#421

So, better not introduce another dependency(?).

[BrainStone]

Support for Guava annotations would indeed be very handy.

[asnare]

Agreed; relying on JSR305 annotations is problematic. Amongst other things, the fact that JSR305 disbanded without producing an official spec means that the JAR everyone uses runs afoul of some of the Java licensing which restricts the use of the javax.* package namespace. This means that upstream libraries like Guava are likely to avoid them so as to not unnecessarily cause problems for members of their community.

[KengoTODA]

SpotBugs is FOSS, and any patches are welcome! :)

I personally think that current workaround is enough especially for Guava; it already has dependency on JSR305. As permanent solution, I want to make progress on #421.

[bhollis]

Hi folks,

This has caused some problems for us as we upgrade from FindBugs 3.0.1 to SpotBugs 3.1.1. Guava methods which didn't flag this detector now do. This is because #429 started respecting the annotation in package-info.java as explained in #429 (comment). We're going to have to suppress the whole detector now, until either this is fixed here or Guava changes their annotations. (I tried to suppress this error only for Guava classes, but it didn't work).

[dpursehouse]

In guava 23.6 they have migrated away from jsr305 to use annotations from Error Prone and Checker Framework.

https://github.com/google/guava/releases/tag/v23.6

[rschmitt]

As @bhollis hinted, there appears to be no way to suppress RV_RETURN_VALUE_IGNORED errors associated with calls to Guava, other than to suppress RV_RETURN_VALUE_IGNORED altogether. Is there anything else we can do on 3.1.0?

[iloveeclipse]

Is there anything else we can do on 3.1.0

Provide a patch which either allows the particular detector to know the particular annotation or adds the rule that @CanIgnoreReturnValue equals @CheckReturnValue(when = When.NEVER).

[rschmitt]

Is there an extensibility mechanism in SpotBugs that allows us to implement that on version 3.1.0 as a workaround, or would that just be a patch for a new version?

[philipl]

Something like this?

diff --git a/spotbugs/src/main/java/edu/umd/cs/findbugs/ba/CheckReturnAnnotationDatabase.java b/spotbugs/src/main/java/edu/umd/cs/findbugs/ba/CheckReturnAnnotationDatabase.java
index 59dec70d5..05e7fa4ce 100644
--- a/spotbugs/src/main/java/edu/umd/cs/findbugs/ba/CheckReturnAnnotationDatabase.java
+++ b/spotbugs/src/main/java/edu/umd/cs/findbugs/ba/CheckReturnAnnotationDatabase.java
@@ -292,6 +292,8 @@ public class CheckReturnAnnotationDatabase extends AnnotationDatabase<CheckRetur
             .createClassDescriptor(CheckReturnValue.class);
     private static final ClassDescriptor CHECK_RETURN_NULL_JSR305 = DescriptorFactory
             .createClassDescriptor("javax/annotation/CheckReturnValue");
+    private static final ClassDescriptor CAN_IGNORE_RETURN_VALUE = DescriptorFactory
+            .createClassDescriptor("com/google/errorprone/annotations/CanIgnoreReturnValue");

     private final Map<String, CheckReturnValueAnnotation> packageInfoCache = new HashMap<>();

@@ -338,6 +340,12 @@ public class CheckReturnAnnotationDatabase extends AnnotationDatabase<CheckRetur
                 return CheckReturnValueAnnotation.createFor(When.ALWAYS);
             }
         }
+
+        annotation = clazz.getAnnotation(CAN_IGNORE_RETURN_VALUE);
+        if (annotation != null) {
+            return CheckReturnValueAnnotation.createFor(When.NEVER);
+        }
+
         return null;
     }
 }