Add @CheckReturnValue to CallOptions.withX() methods

[groakley]

This would help prevent errors like the following toy example

This is incorrect

CallOptions setExecutor(CallOptions callOptions, Executor executor) {
  // @CheckReturnValue would have caused this line to be flagged
  callOptions.withExecutor(executor);
  return callOptions;
}

It should be

CallOptions setExecutor(CallOptions callOptions, Executor executor) {
  return callOptions.withExecutor(executor);
}

[carl-mastrangelo]

This is a good idea, but hard to do. Adding this annotation would likely turn the existing runtime errors into compile time errors. It would upset a lot of people, even though their code was technically wrong.

[ejona86]

We should try to do it all the same.

[carl-mastrangelo]

@ejona86 you should reconsider. It is a significant amount of work for not much payoff. I have tried a couple times, but had to abandon the changes.

What ends up happening is a few of the easy to fix methods get the annotation, but the other ones don't, making the API weird. As a similar example, Look at ClientCalls and ServerCalls, which would be difficult to add the annotation to.

[groakley]

I would much rather my code fail to compile upfront than be silently broken in production. If a method is not assigning the return values of these methods, then the code is certainly not doing what author intended and is probably a bug.

[carl-mastrangelo]

then the code is certainly not doing what author intended and is probably a bug.

This is your assumption, and it's wrong. It is very common in tests to call a method expecting it to throw an exception on bad input. Commonly the test will not do anything with the return value because it expects it to throw. The test is correct, but the compiler doesn't know it.

Causing the build to fail is an allergic reaction to an otherwise minor problem. It probably isn't worth losing a weekly release over a small thing like forgetting to save the return value from getMaxInboundMessageSize, or any of the other idempotent methods in this class.

[charlesmunger]

It is very common in tests to call a method expecting it to throw an exception on bad input. Commonly the test will not do anything with the return value because it expects it to throw. The test is correct, but the compiler doesn't know it.

Actually, the checker is aware of these expected exception idioms - it spots rules, try-fail-catch, mockito, etc.

[carl-mastrangelo]

@charlesmunger At the time I tried to add this in, that was the primary reason. I had a difficult time just upgrading our own code doing this, and decided that the check wasn't worth the pain of updating everyones code.

If this has changed since then I think it would be worth checking if everyones code can be updated.

[groakley]

I will take another look at upgrade when I get a chance. When I last checked in November it didn't look too painful; I already fixed the violations I found on that pass.