Cleaner code for Mockito users by better integration with static code checkers, more @CheckReturnValue

[Stephan202]

Builds on #1130, #1228.

[Stephan202]

Note that almost all non-void org.mockito.Mockito methods are now annotated @CheckReturnValue. You could consider going the other way by annotating the class as a whole @CheckReturnValue and adding @CanIgnoreReturnValue to the non-void methods that form the complement.

[codecov-io]

Codecov Report

Merging #1270 into release/2.x will not change coverage.
The diff coverage is n/a.

@@              Coverage Diff              @@
##             release/2.x   #1270   +/-   ##
=============================================
  Coverage           88.2%   88.2%           
  Complexity          2355    2355           
=============================================
  Files                291     291           
  Lines               5945    5945           
  Branches             709     709           
=============================================
  Hits                5244    5244           
  Misses               521     521           
  Partials             180     180

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/org/mockito/Mockito.java	96.55% <ø> (ø)	39 <0> (ø)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d926aa...ae3aac6. Read the comment docs.

[Stephan202]

Context for this change: within a company-internal PR I found an unused mock(Some.class) statement and thought "it'd be nice if Mockito supported @CheckReturnValue". Then I found #1228, but noticed that the #mock methods weren't annotated. In fixing that I decided to go over the whole class. For each method annotated in this PR I traced its logic; most are side-effect free, while the remainder throw an exception in case of misuse. The only non-void method in org.mockito.Mockito I did not annotate is #ignoreStubs, because it returns its input.

[TimvdLippe]

The reason I did not include these methods in #1228 was because there were no side-effects. Checking them would therefore not required, as it would be perfectly okay (from a Mockito-perspective) that these methods are invoked and then discarded. The methods I annotated do have side-effects and thus are harmful if you discard the return value.

Therefore I am hesitant to accept this PR, as (to my knowledge) these methods are pure and are not harmful for Mockito to be invoked a lot of times.

[Stephan202]

Invoking these methods many times may not be harmful in a functional sense, but IMHO @CheckReturnValue is more about establishing correctness. Ignoring the return value of any of these methods indicates a human error, leading to cluttered tests in the best case and a false sense of security in the worst case.

Looking at Guava, that seems to be the way how they use the annotation. For example, it is applied to the whole package com.google.common.collect, with some methods within that package annotated @CanIgnoreReturnValue; none of those are pure.

[TimvdLippe]

I do understand that point-of-view. However, in that case we would need to update every public API that is non-void to have this annotation, which seems counter-intuitive.

Anyways, these are my 2 cents. Paging @mockito/core to give their opinion 😄

[Stephan202]

[..] in that case we would need to update every public API that is non-void to have this annotation [..]

Well a single annotation in package-info.java takes care of that (example) :D.

But indeed, let's see how others feel about it.

[mockitoguy]

Interesting discussion! Here's how I would summarize the decision point:

• a) benefit: a chance of making our customer's code cleaner because they could identify and remove dead code.
• b) downside: hard to apply this strategy consistently. We would have to review all public API and always remember to use those annotations when adding new API.

Given that we already started using the annotations, the downside b) already applies. Merging the PR does not make it any worse. Hence, the downside can be discounted and we only have the benefit :)

+1 I'm in favor of merging the PR.

Thank you @Stephan202 for tracing the public API methods and coming up with the PR!

[TimvdLippe]

Please rebase on current master as we incorporated ErrorProne, which will probably complain about some of these additions.

[Stephan202]

@TimvdLippe the current master's last commit is e51a051, but git grep -i prone doesn't show an Error Prone integration. Can't find another applicable branch using git log -S prone -i --all either. Am I looking cross-eyed, or...? :)

[TimvdLippe]

Ah my brain was derping. I was working on #1339 and thought I merged it, but I did not. So we have to rebase after that PR is merged. Sorry for the confusion.

[Stephan202]

No worries 😄. I see #1339 also targets the release/2.x branch, so I won't need to switch the target branch for this PR.

(And indeed, that PR contains a bunch of @SuppressWarnings("CheckReturnValue"); this PR will almost certainly force the addition of a bunch more of those.)

I've subscribed to the other PR and will rebase again once it's merged.

[Stephan202]

@TimvdLippe I saw that #1228 was merged, so I rebased this PR and fixed the tests. PTAL :)

[Stephan202]

I dropped this statement instead of adding @SuppressWarnings({"CheckReturnValue", "MockitoUsage"}), but maybe I just don't understand how this statement is relevant to the test. (I would have expected an interaction with the @Mocked field.) Let me know if I should revert this.

[TimvdLippe]

Yeah this seems fine to me 👍

[TimvdLippe]

Awesome thanks!