Add @CheckReturnValue to detect common mistake through static analysis

[jodzga]

trafficstars

A common mistake among new to ParSeq is to treat Task as if it's creation caused is execution e.g.:

Task<RelevanceItems> getRelevanceItemsTask = _relevanceBackendClient.get(keys)
  .map(relevanceItems -> {
    if (Sets.difference(userItems, relevanceItems).size() > 0) {
      // Doesn’t actually execute since this is a Task but isn’t connected to parent task
      _userItemsClient.update(userId, userItems.addAll(relevanceItems));
    }
    return _userItems;
  });

Using jsr305 @CheckReturnValue on methods that return Task would allow detecting this mistake through static analysis.

[linjer]

This would be an excellent feature!

I might suggest considering jsr308 and Checker Framework over new jsr305 usage.

jsr305 is incompatible with Java 9 and has been inactive for a long time now. This has spurred a number of recent proposals to migrate off js305. One of the most popular options has been jsr308 and the Checker Framework.

See https://github.com/google/guava/issues/2960 for discussion around Guava's recent migration. They have chosen to already migrate to the Checker Framework for nullability checks, and I believe they are still deciding on other annotations.

Currently, a lot of tooling support is not quite there, but I suspect with the upcoming Java 9 breakage and Guava's migration, we should see broader support follow very soon.

[skylarbpayne]

As I'm looking at this more, I'm not sure that a change like this would catch a majority of the cases I've seen.

If we put @CheckReturnValue annotations on all core methods in ParSeq returning tasks, then we will catch cases like:

Task<RelevanceItems> getRelevanceItemsTask = _relevanceBackendClient.get(keys)
  .map(relevanceItems -> {
    if (Sets.difference(userItems, relevanceItems).size() > 0) {
      // Doesn’t actually execute since this is a Task but isn’t connected to parent task
      Task.callable(() -> doSomeStuff());
    }
    return _userItems;
  });

But unless _userItemsClient.update also has this annotation, we will not detect an issue with:

Task<RelevanceItems> getRelevanceItemsTask = _relevanceBackendClient.get(keys)
  .map(relevanceItems -> {
    if (Sets.difference(userItems, relevanceItems).size() > 0) {
      // Doesn’t actually execute since this is a Task but isn’t connected to parent task
      _userItemsClient.update(userId, userItems.addAll(relevanceItems));
    }
    return _userItems;
  });

Ideally, we'd want to have some type of static check to ensure that all results of type Task<T> are not ignored. At the moment, I'm not sure whether that's feasible, but I'd like to find better ideas to catch this case.

[jodzga]

One possibility is to create a custom FindBugs rule. It should not be too hard: refactor rule that validates @CheckReturnValue annotation with a twist that it applies to all methods (annotation is not necessary) which return Task type. This could be a tiny contrib project.

[skylarbpayne]

That's a good idea. It looks like methods with the @CheckReturnValue annotation are parsed into a database [1] in the format of CheckReturnValueAnnotation [2]. Then the use of return values is verified in MethodReturnCheck [3]. I'm not too sure how much I understand here yet, but it seems doable.

[1] https://github.com/findbugsproject/findbugs/blob/d1e60f8dbeda0a454f2d497ef8dcb878fa8e3852/findbugs/src/java/edu/umd/cs/findbugs/detect/BuildCheckReturnAnnotationDatabase.java

[2] https://github.com/findbugsproject/findbugs/blob/d1e60f8dbeda0a454f2d497ef8dcb878fa8e3852/findbugs/src/java/edu/umd/cs/findbugs/ba/CheckReturnValueAnnotation.java

[3] https://github.com/findbugsproject/findbugs/blob/d1e60f8dbeda0a454f2d497ef8dcb878fa8e3852/findbugs/src/java/edu/umd/cs/findbugs/detect/MethodReturnCheck.java#L237

[linjer]

If this turns out to be high effort, I want to note that findbugs community is moving towards the spotbugs project.

Some alternatives could include

• Adding this as a feature in spotbugs instead
• Use Google ErrorProne