Remove redundant complete() call and CompletionFailure check.
#4464
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![copybara-service[bot]](https://avatars.githubusercontent.com/in/44061?s=60&v=4){kind=small}
Those operations are already [performed during our call to `getSymbolFromString`](https://github.com/google/error-prone/blob/03d15b56478a40534b2b104e21d070d0eebc0701/check_api/src/main/java/com/google/errorprone/VisitorState.java#L432), which returns `null` in case of failure. (I've found the comment that I'm removing a little confusing, too: I think it might be making the point that we don't want to let the `CompletionFailure` propagate because that would fail Error Prone entirely when in fact the check that called `annotationsAmong` might find one of the _other_ annotations it was looking for among the annotations inherited by the class? (We would already have handled any _directly_ present annotations in `annotationsAmong` as well as in `hasAnnotation`.) If so, we're fine, as we handle `null` gracefully. (Of course, this assumes that silently ignoring an annotation that should have been inherited isn't a bad problem that should really lead to a crash. But there's only so much we can do in the presence of an incomplete classpath.) Or is it actually making the point that we want to try to look for `@Inherited` even if completing some _other_ part of the annotation has failed (if that is even possible) and hence we fall through instead of immediately returning `false`? If so, that doesn't currently work because we would already have bailed after the `null` return from `getSymbolFromString`. Anyway, whatever the comment is getting at, this CL is a no-op, so I'm just trying to avoid putting on blinders as I remove the surrounding code.) PiperOrigin-RevId: 651828339
copybara-service
bot
force-pushed
the
test_651820377
branch
from
46c4d88
to
008cfb0
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Remove redundant
complete()call andCompletionFailurecheck.Those operations are already performed during our call to
getSymbolFromString, which returnsnullin case of failure.(I've found the comment that I'm removing a little confusing, too: I think it might be making the point that we don't want to let the
CompletionFailurepropagate because that would fail Error Prone entirely when in fact the check that calledannotationsAmongmight find one of the other annotations it was looking for among the annotations inherited by the class? (We would already have handled any directly present annotations inannotationsAmongas well as inhasAnnotation.) If so, we're fine, as we handlenullgracefully. (Of course, this assumes that silently ignoring an annotation that should have been inherited isn't a bad problem that should really lead to a crash. But there's only so much we can do in the presence of an incomplete classpath.) Or is it actually making the point that we want to try to look for@Inheritedeven if completing some other part of the annotation has failed (if that is even possible) and hence we fall through instead of immediately returningfalse? If so, that doesn't currently work because we would already have bailed after thenullreturn fromgetSymbolFromString. Anyway, whatever the comment is getting at, this CL is a no-op, so I'm just trying to avoid putting on blinders as I remove the surrounding code.)