Skip to content

Commit bb0e54c

Browse files
sambsnydsambsnyd
authored
Parse more checkstyle rules into OpenRewrite styles. (#6761)
* Parse more checkstyle rules into OpenRewrite styles. Also correct a few defaults which were wrong according to checkstyle's documentation. * This check isn't really helpful for anything * Add explanation of usage and precedence * Convert CustomImportOrderStyle, which wasn't actually used by any of 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. * Fix style precedence in autoformat
1 parent 55ea2cd commit bb0e54c

16 files changed

Lines changed: 1673 additions & 496 deletions

File tree

‎rewrite-core/src/main/java/org/openrewrite/style/NamedStyles.java‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,12 +54,27 @@ public class NamedStyles implements Marker {
5454
Set<String> tags;
5555
Collection<Style> styles;
5656

57+
/**
58+
* Returns the effective style for a given style class.
59+
* There may be multiple named styles which provide the same style class, in whole or in part.
60+
* Consider the example of a formatting recipe which want so to know whether to use tabs or spaces for indentation.
61+
* The build plugins will attach one or more NamedStyles which provide TabsAndIndents.
62+
* Autodetect and Checkstyle may both provide some or all of TabsAndBraces.
63+
* Autodetect is a weaker signal than explicit configuration so it is attached earlier in the parsing process.
64+
* Therefore, this method gives precedence to non-null values from styles which appear later in the list.
65+
*
66+
* @param styleClass the style class whose effective value is being queried for.
67+
* @param namedStyles the list of named styles to extract a specific effective style from.
68+
* @return the effective style as computed from the list of named styles.
69+
* @param <S> a style class.
70+
*/
5771
@SuppressWarnings("unchecked")
5872
public static <S extends Style> @Nullable S merge(Class<S> styleClass,
5973
Iterable<? extends NamedStyles> namedStyles) {
6074
S merged = null;
6175
for (NamedStyles namedStyle : namedStyles) {
6276
Collection<Style> styles = namedStyle.styles;
77+
//noinspection ConstantValue
6378
if (styles != null) {
6479
for (Style style : styles) {
6580
if (styleClass.isInstance(style)) {
@@ -97,6 +112,7 @@ public class NamedStyles implements Marker {
97112

98113
List<Style> mergedStyles = new ArrayList<>(styleClasses.size());
99114
for (Class<? extends Style> styleClass : styleClasses) {
115+
//noinspection DataFlowIssue
100116
mergedStyles.add(NamedStyles.merge(styleClass, styles));
101117
}
102118

‎rewrite-core/src/main/java/org/openrewrite/style/Style.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,8 @@ default String getJacksonPolymorphicTypeTag() {
3838
return getClass().getName();
3939
}
4040

41-
default Style merge(Style lowerPrecedence) {
42-
return this;
41+
default Style merge(Style higherPriority) {
42+
return StyleHelper.merge(this, higherPriority);
4343
}
4444

4545
default Style applyDefaults() { return this; }

‎rewrite-core/src/main/java/org/openrewrite/style/StyleHelper.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -91,9 +91,6 @@ protected List<FieldAccessor> computeValue(Class<?> styleClass) {
9191
*/
9292
public static <T> T merge(T left, T right) {
9393
Class<?> styleClass = left.getClass();
94-
if (right.getClass() != styleClass) {
95-
throw new RuntimeException(left.getClass().getName() + " and " + right.getClass().getName() + " should match exactly.");
96-
}
9794
for (FieldAccessor accessor : FIELD_ACCESSORS.get(styleClass)) {
9895
try {
9996
Object rightValue = accessor.getGetter().invoke(right);
@@ -118,6 +115,7 @@ public static <T> T merge(T left, T right) {
118115
return left;
119116
}
120117

118+
@SuppressWarnings("unused")
121119
public static <T extends SourceFile> T addStyleMarker(T t, List<NamedStyles> styles) {
122120
if (!styles.isEmpty()) {
123121
Set<NamedStyles> newNamedStyles = new HashSet<>(styles);
@@ -170,6 +168,7 @@ public static <S extends Style, T extends SourceFile> S getStyle(Class<S> styleC
170168
public static <S extends Style> @Nullable S getStyle(Class<S> styleClass, List<NamedStyles> styles) {
171169
S style = NamedStyles.merge(styleClass, styles);
172170
if (style != null) {
171+
//noinspection unchecked
173172
return (S) style.applyDefaults();
174173
}
175174
return null;

‎rewrite-groovy/src/main/java/org/openrewrite/groovy/format/AutoFormatVisitor.java‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,9 @@ public AutoFormatVisitor(@Nullable Tree stopAfter) {
4141

4242
@Override
4343
public J visit(@Nullable Tree tree, P p, Cursor cursor) {
44+
if (tree == null) {
45+
tree = cursor.getValue();
46+
}
4447
JavaSourceFile cu = (tree instanceof JavaSourceFile) ?
4548
(JavaSourceFile) tree :
4649
cursor.firstEnclosingOrThrow(JavaSourceFile.class);
@@ -51,14 +54,14 @@ public J visit(@Nullable Tree tree, P p, Cursor cursor) {
5154

5255
// Format the tree in multiple passes to visitors that "enlarge" the space (Eg. first spaces, then wrapping, then indents...)
5356
J t = new NormalizeFormatVisitor<>(stopAfter).visitNonNull(tree, p, cursor.fork());
54-
t = new SpacesVisitor<>(activeStyles, stopAfter).visit(t, p, cursor.fork());
57+
t = new SpacesVisitor<>(activeStyles, stopAfter).visitNonNull(t, p, cursor.fork());
5558
t = new WrappingAndBracesVisitor<>(activeStyles, stopAfter).visitNonNull(t, p, cursor.fork());
5659
t = new NormalizeTabsOrSpacesVisitor<>(activeStyles, stopAfter).visitNonNull(t, p, cursor.fork());
5760
t = new TabsAndIndentsVisitor<>(activeStyles, stopAfter).visitNonNull(t, p, cursor.fork());
5861
t = new MinimumViableSpacingVisitor<>(stopAfter).visitNonNull(t, p, cursor.fork());
5962

6063
// With the updated tree, overwrite the original space with the newly computed space
61-
tree = new MergeSpacesVisitor(activeStyles).visit(tree, t, cursor.fork());
64+
tree = new MergeSpacesVisitor(activeStyles).visitNonNull(tree, t, cursor.fork());
6265

6366
// Then apply formatting that applies on line-endings / #lines / ...
6467
tree = new BlankLinesVisitor<>(activeStyles, stopAfter).visitNonNull(tree, p, cursor.fork());

‎rewrite-java/src/main/java/org/openrewrite/java/OrderImports.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -155,11 +155,11 @@ private <T extends SourceFile> T withStyles(T cu, List<NamedStyles> parsedStyles
155155
if (allPresent) {
156156
return cu;
157157
}
158-
// New styles must appear first to take precedence
158+
// New styles must appear last to take precedence
159159
List<Marker> markers = cu.getMarkers().getMarkers();
160160
for (NamedStyles namedStyle : parsedStyles) {
161161
if (existingStyles.stream().noneMatch(es -> namedStylesEqual(namedStyle, es))) {
162-
markers = ListUtils.concat(namedStyle, markers);
162+
markers = ListUtils.concat(markers, namedStyle);
163163
}
164164
}
165165
return cu.withMarkers(Markers.build(markers));

‎rewrite-java/src/main/java/org/openrewrite/java/format/AutoFormatVisitor.java‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -57,8 +57,8 @@ public J visit(@Nullable Tree tree, P p, Cursor cursor) {
5757
if (tree == null) {
5858
tree = cursor.getValue();
5959
}
60-
List<NamedStyles> activeStyles = new ArrayList<>(styles);
61-
activeStyles.addAll(cu.getMarkers().findAll(NamedStyles.class));
60+
List<NamedStyles> activeStyles = new ArrayList<>(cu.getMarkers().findAll(NamedStyles.class));
61+
activeStyles.addAll(styles);
6262

6363
// Format the tree in multiple passes to visitors that "enlarge" the space (Eg. first spaces, then wrapping, then indents...)
6464
J t = new NormalizeFormatVisitor<>(stopAfter).visitNonNull(tree, p, cursor.fork());
@@ -69,7 +69,7 @@ public J visit(@Nullable Tree tree, P p, Cursor cursor) {
6969
t = new TabsAndIndentsVisitor<>(activeStyles, stopAfter).visitNonNull(t, p, cursor.fork());
7070

7171
// With the updated tree, overwrite the original space with the newly computed space
72-
tree = new MergeSpacesVisitor(activeStyles).visit(tree, t, cursor.fork());
72+
tree = new MergeSpacesVisitor(activeStyles).visitNonNull(tree, t, cursor.fork());
7373

7474
// Then apply formatting that applies on line-endings / #lines / ...
7575
tree = new BlankLinesVisitor<>(activeStyles, stopAfter).visitNonNull(tree, p, cursor.fork());
@@ -93,8 +93,8 @@ public J visit(@Nullable Tree tree, P p) {
9393
if (!(cu instanceof J.CompilationUnit)) {
9494
return cu;
9595
}
96-
List<NamedStyles> activeStyles = new ArrayList<>(styles);
97-
activeStyles.addAll(cu.getMarkers().findAll(NamedStyles.class));
96+
List<NamedStyles> activeStyles = new ArrayList<>(cu.getMarkers().findAll(NamedStyles.class));
97+
activeStyles.addAll(styles);
9898

9999
// Format the tree in multiple passes to visitors that "enlarge" the space (Eg. first spaces, then wrapping, then indents...)
100100
JavaSourceFile t = (JavaSourceFile) new NormalizeFormatVisitor<>(stopAfter).visitNonNull(tree, p);
@@ -105,7 +105,7 @@ public J visit(@Nullable Tree tree, P p) {
105105
t = (JavaSourceFile) new TabsAndIndentsVisitor<>(activeStyles, stopAfter).visitNonNull(t, p);
106106

107107
// With the updated tree, overwrite the original space with the newly computed space
108-
tree = new MergeSpacesVisitor(activeStyles).visit(tree, t);
108+
tree = new MergeSpacesVisitor(activeStyles).visitNonNull(tree, t);
109109

110110
// Then apply formatting that applies on line-endings / #lines / ...
111111
tree = new BlankLinesVisitor<>(activeStyles, stopAfter).visitNonNull(tree, p);
@@ -116,6 +116,7 @@ public J visit(@Nullable Tree tree, P p) {
116116
return addStyleMarker((JavaSourceFile) tree, styles);
117117
}
118118
}
119+
//noinspection DataFlowIssue
119120
return (J) tree;
120121
}
121122

‎rewrite-java/src/main/java/org/openrewrite/java/style/Checkstyle.java‎

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,6 @@ private Checkstyle() {
5454
operatorWrapStyle(),
5555
typecastParenPadStyle(),
5656
unnecessaryParentheses(),
57-
customImportOrderStyle(),
5857
unusedImportsStyle()
5958
));
6059
}
@@ -72,10 +71,11 @@ public static DefaultComesLastStyle defaultComesLast() {
7271
return new DefaultComesLastStyle(false);
7372
}
7473

75-
public static final EmptyBlockStyle.BlockPolicy defaultBlockPolicy = EmptyBlockStyle.BlockPolicy.TEXT;
74+
// https://checkstyle.sourceforge.io/checks/blocks/emptyblock.html
75+
public static final EmptyBlockStyle.BlockPolicy defaultBlockPolicy = EmptyBlockStyle.BlockPolicy.STATEMENT;
7676

7777
public static EmptyBlockStyle emptyBlock() {
78-
return new EmptyBlockStyle(EmptyBlockStyle.BlockPolicy.TEXT, true, true, true, true,
78+
return new EmptyBlockStyle(EmptyBlockStyle.BlockPolicy.STATEMENT, true, true, true, true,
7979
true, true, true, true, true, true, true, true);
8080
}
8181

@@ -107,9 +107,10 @@ public static NeedBracesStyle needBracesStyle() {
107107
return new NeedBracesStyle(false, false);
108108
}
109109

110+
// https://checkstyle.sourceforge.io/checks/whitespace/nowhitespaceafter.html
110111
public static NoWhitespaceAfterStyle noWhitespaceAfterStyle() {
111112
return new NoWhitespaceAfterStyle(true, false, false, true, true, true,
112-
true, false, true, true, true, true, true, true);
113+
true, true, true, true, true, true, true, true);
113114
}
114115

115116
public static NoWhitespaceBeforeStyle noWhitespaceBeforeStyle() {
@@ -150,13 +151,6 @@ public static MethodParamPadStyle methodParamPadStyle() {
150151
return new MethodParamPadStyle(false, false);
151152
}
152153

153-
public static CustomImportOrderStyle customImportOrderStyle() {
154-
return new CustomImportOrderStyle(Arrays.asList(new CustomImportOrderStyle.GroupWithDepth(CustomImportOrderStyle.CustomImportOrderGroup.STATIC, null),
155-
new CustomImportOrderStyle.GroupWithDepth(CustomImportOrderStyle.CustomImportOrderGroup.STANDARD_JAVA_PACKAGE, null),
156-
new CustomImportOrderStyle.GroupWithDepth(CustomImportOrderStyle.CustomImportOrderGroup.THIRD_PARTY_PACKAGE, null)),
157-
true, false, "^$", "^(java|javax)\\.", ".*");
158-
}
159-
160154
private static UnusedImportsStyle unusedImportsStyle() {
161155
return new UnusedImportsStyle(false);
162156
}

0 commit comments

Comments
 (0)
⚡