Update sources to satisfy a new Error Prone check.#2887
Merged
eamonnmcmanus merged 2 commits intogoogle:mainfrom Aug 1, 2025
Merged
Update sources to satisfy a new Error Prone check.#2887eamonnmcmanus merged 2 commits intogoogle:mainfrom
eamonnmcmanus merged 2 commits intogoogle:mainfrom
Conversation
The latest Error Prone release includes a new check `EffectivelyPrivate`, which flags class members that are public or protected when the enclosing class isn't. This is only a warning in Error Prone, but because we treat warnings as errors it currently breaks the build. Nearly all cases were in private nested classes in tests. For the most part the members didn't need to be public. In a couple of places they were accessed through reflection in the test, and it was enough to change `getField` to `getDeclaredField` (etc). (`getField` only retrieves public fields.) I don't believe any of these tests depended on the public members for the correctness of what they were testing. Although `getField` works when the target field is public, regardless of the visibility of the containing class, actually reading or writing a field requires both the field and its containing class to be public unless `Field.setAccessible(true)` is called, and likewise for constructors and methods.
cpovirk
approved these changes
Aug 1, 2025
|
|
||
| private static class Foo { | ||
| @SuppressWarnings("unused") | ||
| @SuppressWarnings({"unused", "EffectivelyPrivate"}) |
Member
There was a problem hiding this comment.
Another option is @Keep, but I don't have much reason to prefer one over the other:
@Keepwould also satisfy Error Prone's unused-field checking, though AFAIK not any other tools' similar checking. (So it's unclear whether we'd be wise to remove"unused"at that point.) It also prevents Error Prone from suggesting things like making fieldsfinal, which may be good or bad in various cases.@Keephas runtime retention, which is likely irrelevant here.@Keepisn't applicable to method parameters, but that is likely irrelevant here, too.@Keepcan be used as a meta-annotation, but that's not relevant here.
So this is fine as it is as far as I'm concerned, as are the other suppressions.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
The latest Error Prone release includes a new check
EffectivelyPrivate, which flags class members that are public or protected when the enclosing class isn't. This is only a warning in Error Prone, but because we treat warnings as errors it currently breaks the build.Nearly all cases were in private nested classes in tests. For the most part the members didn't need to be public. In a couple of places they were accessed through reflection in the test, and it was enough to change
getFieldtogetDeclaredField(etc). (getFieldonly retrieves public fields.) I don't believe any of these tests depended on the public members for the correctness of what they were testing. AlthoughgetFieldworks when the target field is public, regardless of the visibility of the containing class, actually reading or writing a field requires both the field and its containing class to be public unlessField.setAccessible(true)is called, and likewise for constructors and methods.