Use the Java idioms error-prone asks for: instanceof patterns, arrow switches, text blocks - #86
Merged
Merged
Conversation
error-prone's PatternMatchingInstanceof warned at 33 places in the formatter and the SPI where an instanceof check is followed by a cast of the same value. The check now declares the variable itself, and the cast goes. Names follow error-prone's suggestions, except classDecl, space and methodInvocation, which read better than jCClassDecl, nonBreakingSpace and methodInvocationTree. Nothing else changes: ./gradlew test passes, and so does :open-java-format:test on JDK 25, which also runs the import module tests that JDK 21 skips.
error-prone's StatementSwitchToExpressionSwitch warned at 33 switch statements in the formatter. They now use arrow cases: labels that shared a body are one case, and the break statements go. Where every case returned a value, the method returns a switch expression instead: ImportOrderer.isJava, nextIsModifier, the sealed check in ModifierOrderer, and TypeNameClassifier's three states, whose switches cover all four JavaCaseFormat constants and so need no default; the IllegalStateException after them goes. This is error-prone's own fix, applied with -PerrorProneApply=StatementSwitchToExpressionSwitch and formatted with formatDiff. Nothing else changes: ./gradlew test passes, and so does :open-java-format:test on JDK 25; the 15,747 files of the JDK 21 sources format exactly as before.
error-prone's StringConcatToTextBlock warned at 16 concatenations of string literals: the inputs and expected outputs in ModuleImportTest, and the template DebugRenderer writes to output.js. They are text blocks now, from error-prone's own fix formatted with formatDiff. The strings are the same: both classes hold the same string constants before and after, ./gradlew test passes, and so does :open-java-format:test on JDK 25, which runs the ModuleImportTest cases that JDK 21 skips.
error-prone's StreamFlatMapOptional warned about .flatMap(Optional::stream) in the IntelliJ plugin's settings page test, where findCheckBox walks the component tree for the first check box. It now uses the replacement error-prone suggests, .<JCheckBox>mapMulti(Optional::ifPresent), which builds no stream per element; findFirst still stops at the first check box. The three tests that call findCheckBox pass, as do the plugin's other tests.
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.
Before this PR
A clean compile printed 91 error-prone warnings, and 83 of them came from four checks that ask for Java 16–21 idioms the code did not use yet:
PatternMatchingInstanceof(33),StatementSwitchToExpressionSwitch(33),StringConcatToTextBlock(16) andStreamFlatMapOptional(1).After this PR
==COMMIT_MSG==
The code uses the idioms error-prone asks for, one commit per check:
instanceofchecks bind a pattern variable instead of casting afterwards (33 places);ModuleImportTestand theoutput.jstemplate inDebugRenderer(16);Optionals withmapMulti(Optional::ifPresent)instead offlatMap(Optional::stream)(1).==COMMIT_MSG==
The switch, text block and
mapMultichanges are error-prone's own fixes (./gradlew compileJava compileTestJava -PerrorProneApply=<check>), formatted withformatDiff, and every hunk was read. A clean compile (--rerun-tasks) now prints none of these four warnings and no new ones; 8 remain (NullAway5,UnusedMethod,PreferSafeLogger,AnnotateFormatMethod).Behaviour is unchanged:
./gradlew testpasses, and so does:open-java-format:teston JDK 25, which also runs theimport moduletests that JDK 21 skips;instanceofand switch changes, the 15,747 files of the JDK 21 sources format exactly as with main; the later commits touch no formatting code;Possible downsides?
None for users. For maintainers: the switches and
instanceofchecks now differ in shape from palantir-java-format and google-java-format, so a fix ported from upstream that touches one of these lines will need its hunk adapted by hand rather than applying cleanly.