Parse more checkstyle rules into OpenRewrite styles.#6761
Conversation
Also correct a few defaults which were wrong according to checkstyle's documentation.
…the formatting recipes, into ImportOrderStyle. Also fix how Styles are merged. In looking at this I discovered a long-standing bug which gave earlier styles precedence over later styles. This means that "weaker" styles like those we autodetect would win over "stronger" styles like those parsed from checkstyle configuration, or explicitly specified by the user. It is perhaps a testament to teh quality of Autodetect that this wasn't a bigger problem before now. There are still some test failures relating to fixing this serious bug that I need to work through.
pstreef
left a comment
pstreef
left a comment
There was a problem hiding this comment.
Great change so far! Just a few more details for a potential follow up.
| /** | ||
| * Merge many NamedStyles into one NamedStyle. | ||
| * | ||
| * @param styles The styles to be merged. Styles earlier in the list take precedence over styles later in the list. |
There was a problem hiding this comment.
This says “Styles earlier in the list take precedence over styles later in the list” but the other merge() overload above (line 64) says the opposite: “gives precedence to non-null values from styles which appear later in the list.” Looking at the code, the new comment seems correct.
| // OK to return a style with nullable non-null fields, if Autodetections doesn't provide Style.applyDefaults() will | ||
| //noinspection DataFlowIssue | ||
| return moduleList.stream() | ||
| .map(module -> new TabsAndIndentsStyle(false, null, null, null, null)) |
There was a problem hiding this comment.
I was a little confused about this, but it seems FileTabCharacter means tabs are violations so this is correct.
Maybe a comment on the meaning of the rule would be useful though.
| inc = tokens.contains("INC"); | ||
| dec = tokens.contains("DEC"); | ||
| bnoc = tokens.contains("BNOC"); | ||
| bnoc = tokens.contains("BNOT"); |
There was a problem hiding this comment.
nit: should this also be renamed to bnot?
| parenPadStyle(conf)) | ||
| .filter(Objects::nonNull) | ||
| .flatMap(Set::stream) | ||
| .collect(toSet())); |
There was a problem hiding this comment.
Nice catch converting CustomImportOrderStyle into something actually used!
customImportOrderStyle(), avoidStarImportStyle(), and importOrderStyle() can all produce ImportLayoutStyle instances. Since they're collected into a HashSet and ImportLayoutStyle.merge() does last-wins, the result depends on iteration order — which is non-deterministic for HashSet. Should this be a LinkedHashSet to make the stream ordering deterministic?
Longer term, it might be cleaner to not emit a full ImportLayoutStyle from AvoidStarImport and instead only influence the star import thresholds on whatever layout is already configured.
Since openrewrite/rewrite#6761 the CustomImportOrder rule is parsed into the ImportLayout style which is understood and acted upon by OrderImports.
With openrewrite/rewrite#6761 changing style merging to "last wins" semantics, configured styles must be applied after auto-detected styles so they take precedence. Previously configured styles were set on the parser (applied first) and auto-detected styles were added after parsing (applied last), meaning auto-detected styles would unintentionally override user configuration. Remove the now-unnecessary styles parameter from MavenMojoProjectParser#listSourceFiles and instead apply both auto-detected and configured styles in the correct order within AbstractRewriteBaseRunMojo.
2 participants
This ended up being a larger, more impactful change than I expected.
I started off implementing support for checkstyle rules we did not previously support.
In the process I noticed that some of our existing checkstyle defaults did not match checkstyle's documented defaults, so I fixed those discrepancies.
Then I saw that an OSS contributor added a
CustomImportOrderrule... but none of our formatting recipes knew it existed so it was completely pointless. So I converted that into theImportLayoutStyle.Then I wondered if the import layout style we loaded from checkstyle configuration would correctly take precedence over the autodetected import layout style and discovered it would not.
So I dug into how Styles are loaded and merged together and found that we had inadvertently implemented "first wins" rather than "last wins" semantics. Which meant that autodetected styles would supersede configured styles, oops. Perhaps it is a testament to the usefulness of Autodetect that we didn't get more complaints about this before now.