@CanIgnoreReturnValue for blocking stub methods returning Empty

[panchenko]

Currently IntelliJ IDEA highlights blocking calls as a warning - as AbstractBlockingStub is annotated with @CheckReturnValue.

I think as a separate feature, such an annotation can be also adde via method level option, but that would need more discussions.

For the tests - instead we can change return type of an existing method.

[ejona86]

Oh, interesting. Yeah, handling this case seems valuable. Although it adds a new direct dependency on errorprone, right? I see grpc-api is exposing errorprone as an API dependency already, but I'm still a bit uncomfortable given how much of a pain dependencies in codegen have been in other cases.

I'm surprised Bazel didn't fail to build; I'd have expected //compiler:java_grpc_library_deps__do_not_reference to need updating. Maybe we don't have any Empty-returning methods being built with Bazel.

[ejona86]

Looks like it may be better to put @CanIgnoreReturnValue on AbstractBlockingStub. We'd want to make sure that AbstractStub is still checking for return value though; sometimes how the annotations mix can be surprising.

[panchenko]

The warning is reported as AbstractBlockingStub is already annotated with @CheckReturnValue.

@CanIgnoreReturnValue is documented as

This is the opposite of {@link CheckReturnValue}. It can be used inside classes or packages
annotated with {@code @CheckReturnValue} to exempt specific APIs from the default

Using both on the same type would be confusing.

[ejona86]

Using both on the same type would be confusing.

Right. Remove @CheckReturnValue from AbstractBlockingStub. We can add @CheckReturnValue to the two methods within the class.

[panchenko]

I guess the original intent of annotating AbstractBlockingStub was making sure that a result of a remote call is not discarded - focusing mostly the generated code: if a method result is not used, then might be we don't need to call that method at all.

However, if method returns Empty, then we don't have any result, but there is a warning in IDE.

Another way to avoid such a warning would be generating the blocking stub method as void. IMHO that would make things even more clear, but would need some code generation options for compatibility.
WDYT ?

[ejona86]

I guess the original intent of annotating AbstractBlockingStub was making sure that a result of a remote call is not discarded

Doh. I forgot about that perspective. I was thinking about the methods in AbstractStub and AbstractBlockingStub themselves, particularly all the with* methods which are very easy to ignore the return value. Digging through history, and following from #4437 , it seems that was the original concern.

So one view could be "it's unlikely someone will do an RPC and ignore the response if they actually need the response, but it is very likely someone wants to ignore Empty" and thus "we don't check return values for generated messages." There is harm to checking and there's very little value gained. Dunno how much that resonates with people.

Another way to avoid such a warning would be generating the blocking stub method as void.

Yeah, I considered mentioning it. I saw it as a "if we knew this from the beginning" sort of thing. ABI makes it annoying to deal with now. But I realize now we are a bit lucky here. #10318 is creating a "v2" blocking stub. We could use void there. We could also avoid the Empty request argument.

I'm not certain whether Empty→void won't cause its own problems. But it does seem we can consider it.

[ejona86]

I think the easiest thing here is to just change AbstractBlockingStub to have @CanIgnoreReturnValue. (Although we should think about this problem for blockingStubV2 that's in progress)

[panchenko]

I am not sure about if it's worth generating more classes like in blockingStubV), the corresponding improvements can be applied to the existing stub and configured via options - meaning FileOptions / ServiceOptions / MethodOptions.

For example, there could be 2 options related to Empty:

• change Empty return type to void
• omit Empty parameter