Add more Optional methods to ReturnValueIgnored

[pkoenig10]

The return values of almost all Optional instance methods, excluding ifPresent and ifPresentOrElse, should have their return values used.

For cases where you may don't care about the return value, it's better to use isPresent and isEmpty. For example, instead of:

optional.orElseThrow(() -> ...);

Do:

if (optional.isEmpty()) {
    throw ...;
}

---

As a motivating example, I had some code that was intended to look like:

if (condition) {
    return getOptional().orElseThrow(...);
}

return ...;

But I forgot the return, so the code actually looked like:

if (condition) {
    getOptional().orElseThrow(...);
}

return ...;

Detecting the unused Optional value here would have been useful.

[cushon]

Thanks for this!

We'll probably need to put this behind a flag initially to avoid making an atomic breaking change to an existing error. If you don't mind doing that there's an example in 86f8e23, otherwise I can take care of it when I import the PR.

[cushon]

FYI we have a separate check just for this one, with a fix that migrates to ifPresent: https://github.com/google/error-prone/blob/master/core/src/main/java/com/google/errorprone/bugpatterns/OptionalMapUnusedValue.java. We were planning to add map here after that cleanup was done.

[pkoenig10]

Do you want me to remove map from here for now?

[cushon]

Calling orElseThrow to assert an Optional is present and then discarding the return result appears to be more common than the other examples here. I'm not really defending that as a pattern, just saying that making this one an error maybe not be lower priority for us. I'm curious if you've run into examples of that?

[pkoenig10]

Occasionally, but rarely.

[pkoenig10]

As a reference, SonarLint requires that return values are used for all Optional methods that return non-void values.
https://github.com/SonarSource/sonar-java/blob/d59d157c852ac924f3029ddb8c0aa366501f9fda/java-checks/src/main/java/org/sonar/java/checks/IgnoredReturnValueCheck.java#L68

Maybe we should do the same here?

[pkoenig10]

What do you think about matching all non-void instance methods like we do for Stream? This would ensure we match against any methods added in the future.

[cushon]

What do you think about matching all non-void instance methods like we do for Stream? This would ensure we match against any methods added in the future.

That SGTM, maybe as a follow-up once the flag can be removed?