build(errorprone): promote the ten checks that have no sites left - #3419
Open
juherr wants to merge 6 commits into
Open
build(errorprone): promote the ten checks that have no sites left#3419juherr wants to merge 6 commits into
juherr wants to merge 6 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThis change tightens Error Prone configuration and updates production and test code to satisfy the enabled checks. It also makes string normalization locale-independent and removes unused code, redundant syntax, and ambiguous imports. ChangesError Prone cleanup
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The change promotes cleaned Error Prone checks and adjusts test-code handling, with the reported build completing successfully and no actionable merge-blocking risk remaining beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
juherr
force-pushed
the
juherr/errorprone-promote-clean-checks-to-error
branch
from
August 25, 2026 16:43
f8dfb0d to
db6c1b6
Compare
The gate that chose how NullAway and SelfAssertion read a compile was
`name.contains("Test")`, so it classified code by a coincidence in the task
name. testng-test-kit is test code living in a main source set: its task is
`compileJava`, so it received neither HandleTestAssertionLibraries nor the
SelfAssertion opt-out, even though its org.testng.xml half is null-marked by
the package-info.class on its compile classpath and checked today.
The Error Prone plugin already models this. `compilingTestOnlyCode` takes its
convention from the source set name and a module can override it, so the
module declares what it is instead of being guessed at.
Proven with a throwaway probe in the marked package of testng-test-kit --
`assertThat(s).isNotNull()` followed by a dereference -- which fails to compile
before the change and passes after. A green build alone could not show this:
the change has no other visible effect.
Each of these is a rewrite javac itself suggests, with no change in behaviour: - empty parentheses on annotations in the test.enable fixtures. @test and @test() are the same annotation by definition (JLS 9.7.2) and TestNG reads annotations reflectively, so the two forms it paired were never distinct. - Boolean.FALSE/Boolean.TRUE where a primitive literal is meant. - the nested ConfigMethodArguments.Builder import, qualified at its uses. - parameters differing from the field they assign only by capitalisation. - a javadoc block sitting between @OverRide and the method it documents, which is a comment rather than javadoc where it stood. - toLowerCase without a locale. Not a bug fix: the suffixes Parser.canParse receives contain neither i nor I, so a Turkish default locale cannot reach them. It is insurance, and cheaper than the argument about whether it bites. - a field that was only ever written, with the writes that fed it. Only InconsistentCapitalization is emptied here. The counts the others were scoped against came from truncated compiler output -- javac stops reporting after 100 warnings per compile task -- so the real inventory is several times larger and the rest is worked through in the commits that follow.
Machine output, kept in its own commit so it can be re-derived rather than
read hunk by hunk. Produced by Error Prone's own patcher:
-XepPatchChecks:MissingOverride,UnnecessaryParentheses,BadImport,BooleanLiteral,NotJavadoc
-XepPatchLocation:IN_PLACE
followed by autostyleApply, which is what puts the inserted annotations on
their own line.
MissingOverride, UnnecessaryParentheses, BadImport and BooleanLiteral are now
empty. NotJavadoc has one site the patcher does not fix; it is handled by hand
in the next commit.
The parenthesis removals all keep their grouping, and the one Boolean.TRUE in
a data provider boxes to the same cached instance the literal does.
The Error Prone patcher has no fix for these, or its fix would be wrong. Fixed: - toLowerCase without a locale, at every remaining site. All eight compare identifiers -- file suffixes, a protocol name, a scripting language name, a class name -- so Locale.ROOT is what they meant. - a stray "/////" separator at the end of a test file. javac parses a run of three or more slashes as a markdown documentation comment (JEP 467), so it parsed as documentation attached to nothing. - YamlSchema.mapKey now casts the setter through the same helper that key and listKey already use, which removes the single-call-site uncheckedCast the check was pointing at rather than suppressing it. Suppressed, with the reason at the site: - both finalizers in the github1461 leak test. One decrements the counter the test spins on until it reaches zero -- without it the wait never ends and the test fails on its timeOut -- and the other is the observation the test was written to make, so neither can go. - the String overload of newInstance, on the interface, the implementation and the override in SuiteRunner. The class is named rather than passed, so the type variable cannot appear in the formals, and the signature is public API.
The Error Prone patcher was tried here first and rejected: it deletes the initialiser along with the variable, so it removed a parser.parse call that was the subject of its own test, the factory-parameter rows from the emailable report, createXmlInclude calls that build the XML the test then runs, and a static field initialiser that throws on purpose. All of that stayed green. These are by hand instead. Where the value really was dead it is gone. Where only the variable was dead the call stays: dumpParametersInfo, parser.parse and createXmlInclude are all kept and only their unread results dropped. EmailableReporter2's dead store was the one site this branch reported rather than fixed, as GITHUB-3418; it has since been fixed upstream by accumulating into hasRows instead of overwriting it. All that is left here is reading the parameters local already in hand rather than calling getParameters again. Two sites are named rather than annotated, because Error Prone reads an "unused" prefix as deliberate and a lambda parameter cannot carry an annotation at all: the Comparator lambda in TestHTMLReporterTest, and the local whose division by zero is what makes a @BeforeClass fail. Elsewhere the reason goes at the site with @SuppressWarnings("unused"), the spelling already used in this tree: the Comparator body on MethodSorting.NONE, the Guice-injected constructor parameters whose injection is what the sample proves, and the field whose evaluation is what makes class initialisation fail. Test2's constructor names UnusedVariable and UnusedMethod instead of that alias. It is never called and its parameter is never read, both deliberately, and the alias would have hidden the second one -- whose suggested fix is the no-arg constructor the comment at the site says must not exist.
Ten checks now have no occurrence anywhere in the build, so they can be errors instead of warnings. Until now none of them failed anything: they printed into an output long enough that nobody read it, which is the same as not running. Promotion is what makes the preceding commits hold. Verified by putting one violation back and watching the compile fail on it, rather than by reading the configuration. Checks that still have sites are untouched and stay warnings, MissingSummary among them.
juherr
force-pushed
the
juherr/errorprone-promote-clean-checks-to-error
branch
from
August 26, 2026 09:27
db6c1b6 to
0cb61e2
Compare
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
1 participant
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.
Ten Error Prone checks have no unsuppressed site left in the build, so they are now errors
instead of warnings. Until now none of them failed anything: they printed into an output long
enough that nobody read it, which is the same as not running them.
The measurement was wrong before this branch, and that is the main finding
javac reports at most 100 warnings per compile task. Four tasks here were past that, so
every warning count previously quoted for this repository was truncated — including the one
this work was scoped against. Fixing sites of one check made unrelated checks appear from
behind the cut-off, which is how it was noticed.
MissingSummaryEvery number in this description comes from
--rerun-taskswith-Xmaxwarnsraised throughan untracked init script, with the javac error count published beside it — a plain javac
error silences the whole Error Prone pass, so a low warning count can mean the pass never
ran.
Result
MissingOverrideUnnecessaryParenthesesUnusedVariableBooleanLiteralBadImportStringCaseLocaleUsageTypeParameterUnusedInFormalsFinalizeNotJavadocInconsistentCapitalizationThe whole 1002 → 775 drop is accounted for: 222 from the checks above, 3
MissingSummarythatstop applying once
testng-test-kitis correctly treated as test code, 1JavaUtilDateon aDatethat nothing read, and 1UnusedMethodon the constructor whose suppression names itexplicitly. No check anywhere in the build gained a warning.
Both changes that are not sweeps
The test-code gate no longer keys on the task name.
name.contains("Test")decided howNullAway reads a compile, so
testng-test-kit— test code living in a main source set — gotneither
HandleTestAssertionLibrariesnor theSelfAssertionopt-out, while itsorg.testng.xmlhalf was null-marked and checked. The Error Prone plugin already modelsthis:
compilingTestOnlyCodetakes its convention from the source set name and a modulecan override it.
This has no other visible effect, so a green build could not prove it. A throwaway probe in
the marked package of
testng-test-kitdid:[NullAway]dereferenced expression 's' is @NullableThe promotion was verified by breaking it, not by reading the config. One
@Test()putback into
test/enable/A.java:Both probes were removed before committing.
Where the sweep needed judgement
UnnecessaryParentheseslooked like it would cost fixture coverage: thetest.enablesamplesdeliberately pair
@Testwith@Test(), andEnableTestasserts both run. They cannotdiffer — JLS 9.7.2 defines a marker annotation as shorthand for the empty-parenthesis form, and
TestNG reads annotations reflectively — so the pair was a source-form duplicate and the
parentheses went. The now-visible duplication between those method pairs is left alone.
Error Prone's patcher was used for
MissingOverride,UnnecessaryParentheses,BadImport,BooleanLiteralandNotJavadoc, and rejected forUnusedVariable. It deletes the initialiser alongwith the variable, so on this codebase it removed a
parser.parsecall that was the subject ofits own test, the factory-parameter rows from the emailable report,
createXmlIncludecallsthat build the XML the test then runs, and a static initialiser that throws on purpose. All of
it stayed green. Those 36 sites are by hand: where the value was dead it is gone, where only
the variable was dead the call stays.
The sweep also surfaced a real bug:
EmailableReporter2overwrotehasRowsinstead ofaccumulating into it, so a result carrying only factory parameters got a placeholder row it did
not need. That was out of scope for an inert sweep, so it was reported as #3418 rather than
fixed here — and it has since been fixed upstream by #3427, which this branch is rebased onto.
Ten sites are suppressed rather than fixed, each with the reason at the site — the two
finalizers the leak test spins on, two Guice-injected constructor parameters whose injection is
what the sample proves, a field whose evaluation is what makes class initialisation fail, a
constructor parameter whose absence would change what a
PackageTestassertion is exposed to,Comparator.compareon the enum constant that compares nothing, and the three<T> T newInstance(String, ...)declarations — interface, implementation and override, one ofthem public API — where the class is named rather than passed so the type variable cannot reach
the formals.
Finalizeis the one check that cleaned nothing: both its sites are suppressed, so it ispromoted on the bet that a new violation is worth stopping rather than on cleanup it bought.
The build config says so.
TypeParameterUnusedInFormalsdid clean one —YamlSchema.uncheckedCasthad a single call site and a sibling helper that already did the job,so the method went instead of gaining a suppression.
Two further sites are named rather than annotated, which is what Error Prone's
unusedprefixmeans: a
Comparatorlambda, where a parameter cannot carry an annotation at all, and the localwhose division by zero is what makes a
@BeforeClassfail. The annotated sites are spelled"unused"to match what is already in the tree — exceptTest2's constructor, which namesUnusedVariableandUnusedMethodexplicitly: it is never called and its parameter is neverread, both deliberately, and the alias would have hidden the second one, whose suggested fix is
the no-arg constructor the comment at that site says must not exist.
StringCaseLocaleUsageis not a bug fix at any of its sites, and that was checked ratherthan assumed. Turkish lowercasing only moves an uppercase
I. The entry namesParser.canParsematches carry none (
.xml,.yml,.yaml), and the two script engines on the test classpathreport
GroovyandBeanShell— identical undertr_TR, as is every spelling a suite filecould use for them.
Locale.ROOTis insurance against an engine or a suffix that does containone; no behaviour changes today.
What this deliberately does not do
MissingSummaryis 358 of the remaining 775 andstays a warning; Error Prone MissingSummary is the bulk of the build's warning output and is not enforced #3417 tracks what to do about it. What the branch buys is that these ten
checks cannot regrow.
keep the printed output at its current size. Correctness does not depend on it — a promoted check
emits errors, and any error fails the build whether or not it is among the first hundred
reported — but anyone counting warnings from an ordinary build will undercount, and the
configuration says so at the point where checks are promoted.
ReferenceEquality,EqualsGetClass,JdkObsolete,JavaUtilDate,StringSplitter,MixedMutabilityReturnType) are untouched,as is the javac
deprecation/removalbacklog.Verification
./gradlew build: 0 failures, 0 errors. Six commits, each formatted and each compilingwith both counters at zero.
Summary by CodeRabbit
Bug Fixes
Refactor
Chores