New feature: annotate all factory methods with @CheckResult

[TWiStErRob]

Description

To prevent situations like: https://twitter.com/yogurtearl/status/1347344454668062722

We could leverage a best effort annotation that tools could detect. Using androidx.annotation.CheckResult would be ideal, because (hopefully) lint would pick up on that. If lint doesn't Errorprone can, so an alternative is JSR305's javax.annotation.CheckReturnValue. Best is both, actually this already exists on Espresso.onView:

https://github.com/android/android-test/blob/41855232b54fb77af11744e446d11204749b789c/espresso/core/java/androidx/test/espresso/Espresso.java#L85-L87

Steps to Reproduce

In a sudden lapse of judgement, import the wrong thing:

import static androidx.test.espresso.action.ViewActions.closeSoftKeyboard;

@Test public void test() {
    ...
    closeSoftKeyboard();
    ...
}

of course the right one is:

import static androidx.test.espresso.Espresso.closeSoftKeyboard;

Expected Results

Some kind of warning when using Espresso wrong.

Actual Results

Silent nothing, many hours of dev time wasted.

AndroidX Test and Android OS Versions

N/A

Link to a public git repo demonstrating the problem:

N/A

Proposal

Annotate everything in:

• ViewActions.*
• androidx.test.espresso.matcher.*
• androidx.test.espresso.intent.Intents.intending
• potentially others

I would be happy to contribute the adding of annotations on the Espresso public surface, but I have a feeling internally in Google you might require some cleanup in the monorepo if this ever happened (i.e. remove side-effect-less calls, if any). Although those can be discovered on this branch, and committed before merge.

[TWiStErRob]

It's a start :) https://github.com/android/android-test/pull/1334

[TWiStErRob]

~@brettchabot I would really go for the androidx annotation, because errorprone is a third party we need to explicitly install, lint reaches more people, out of the box.~ Should've checked the source code first...

[TWiStErRob]

cc @cpovirk

[cpovirk]

Thanks for CCing me!

My understanding is that Android Lint recognizes the Error Prone @CheckReturnValue, too. (Hmm, but maybe Error Prone does not recognize the androidx CheckResult? I should look into that....) So I think we can use that one, or maybe you're aware of other considerations?

I'm putting together another change for ViewActions now. It does indeed turn up some bugs in our repo. Thanks for the list!

[cpovirk]

Hmm, but maybe Error Prone does not recognize the androidx CheckResult? I should look into that....

It does not. There is feature request for this in our internal tracker... from 6 years ago... :)

The good news is that the reason I'm here is that some other people on my team are putting a bunch of work into CheckReturnValue/CheckResult-type checks, so one of them might well be interested in picking it up.

[cpovirk]

Whoa, Intents.intending is probably the second-most misused API I've seen yet in this project :)

[TWiStErRob]

My understanding is that Android Lint recognizes the Error Prone @CheckReturnValue, too.

I just double-checked CheckResultDetector in lint and it has supported 7 aliases in AGP 3.2.1-7.1.3 for sure.

So I think we can use that one, or maybe you're aware of other considerations?

Sounds like simply adding the error prone annotation would suffice then. If lint hadn't supported it, there would be considerations, but I think this is fine. Using it would reach a very wide audience already using lint and/or errorprone as soon as they update Espresso.

I can't think of any other issues, maybe @jakewharton, @jrodbx has ideas for what needs to be considered (as library authors and community champions).

Thanks for the list!

No probs, but please review all the public methods, because I only spent the one minute writing them out here.

[JakeWharton]

These libraries already have a dependency on the AndroidX annotations artifact whose @CheckResult annotation is already honored by default in the tooling that ships with the standard build system for Android projects. I do not think it's a good idea to add error prone's annotations solely for the purpose of adding an annotation that's already present in a dependency simply because Google's custom static analysis tool originally built for their non-Android monorepo doesn't yet honor it.

So TL;DR my vote is: use the Android standard annotation and fix error-prone separately.

[cpovirk]

This is all sadly reminiscent of our recent nullness work, in which the most standard annotation is not necessarily the one that produces the best experience for users :(

This library already had the Error Prone annotations in its deps, too, so that may eliminate the main downside for users to using that. (That's what https://github.com/android/android-test/pull/1334 used, and so will the ViewActions and Intentions.intending changes I have in flight.) If there are other downsides, though, let me know. And I'm not on this particular project; we're just poking our heads into random Google codebases, as per usual :)

[cpovirk]

Sorry, I didn't conclude that last thought. I was just going to say that the actual owners may have other reasons to prefer one or the other that I wouldn't know about.

[JakeWharton]

If both dependencies are already present then yes I don't think it matters at all.

Hot take: all non-void functions should have required checking the result as enforced by the language compiler by default with an opt-out rather than an opt-in!

[TWiStErRob]

Is it possible errorprone was added by one of these random pokes in the past? There are only 3 usages, you added the 4th, @cpovirk.

Compared to 250 usages of androidx.annotation

I like your hot-take, Jake, and agree; sadly #wrongforum, is there a KT ticket for that I can vote on?

[cpovirk]

Is it possible errorprone was added by one of these random pokes in the past?

Very likely. I don't know if the other Error Prone annotations have equivalents in other artifacts. Maybe they do?

(I do see a few instances of @CheckReturnValue @CheckResult... :))

RE: Kotlin:

I don't know if there's a Kotlin ticket for changing the language default, but there's KT-12719 for building opt-in enforcement into the language.

(We are in fact trying to flip the default inside Google. It's turning up a lot of fun bugs.)

[TWiStErRob]

Well I see 4 usages:

• com.google.errorprone.annotations.MustBeClosed in Tracer A library-internal class, so consumers don't really use it.
  As far as I remember this works out of the box (in IDEA) if the return value implements Closable, which is the case here.
• com.google.errorprone.annotations.MustBeClosed in Tracing An @ExperimentalTestApi public method, which is tightly coupled with the library-internal Tracer interface, so no end-user can use it. Only useful if the consumer has errorprone... (I'm yet to actively work on a project that uses it.)
• com.google.errorprone.annotations.InlineMe in InstrumentationRegister A deprecated public class, useless without tooling support, I'd be surprised in IDEA supported this. Kotlin has an equivalent @InlineMe(replacement = "...") @Deprecated -> @Deprecated(replaceWith = ReplaceWith("...")), which could be used if the deprecated class was Konverted which looks feasible in a binary-compatible way.
• com.google.errorprone.annotations.CheckReturnValue in IntentMatchers, the one you just added

So based on this, it's removable with almost no effort/drawback. @brettchabot I think this needs your eyes.

[cpovirk]

Removing the direct dep is likely doable. We could even set things up to continue to use those annotations inside our own repo if we wanted.

Still, I'm not sure that this is going to make much difference to users: I think that the Error Prone annotations get pulled it transitively by a number of other deps -- Dagger, Flogger, Guava, Guice, Truth:

https://github.com/android/android-test/blob/41855232b54fb77af11744e446d11204749b789c/WORKSPACE#L108-L120

The dream is that someday none will need that dep, but I would expect it to be many years before that happens.

[JakeWharton]

Very few Android projects use any of those dependencies though. Dagger is the most common, and I don't recall its runtime shipping the annotation dependency.

[cpovirk]

My mistake, you're right about Dagger: https://repo1.maven.org/maven2/com/google/dagger/dagger/2.41/dagger-2.41.pom

[cpovirk]

(And yes, deps of the few androidx-test artifacts that I finally got around to spot-checking do look fairly minimal (e.g., core, junit, espresso-core).)

[sgjesse]

On the use of com.google.errorprone.annotations.InlineMe in InstrumentationRegistry, I have just filed https://issuetracker.google.com/260089912, where R8 will fail on missing classes when processing this library.