Skip to content

Commit af2cdea

Browse files
RadikalJinRadikalJin
authored
Merge pull request #159 from alfasoftware/feature/s1124-modifiers-order
Add ModifiersOrderOperation (java:S1124)
2 parents cfea0eb + 3807c73 commit af2cdea

6 files changed

Lines changed: 357 additions & 0 deletions

File tree

‎astra-core/src/main/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s1124/ModifiersOrderOperation.java‎

Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,158 @@
1+
package org.alfasoftware.astra.core.refactoring.operations.sonar.s1124;
2+
3+
import java.io.IOException;
4+
import java.util.ArrayList;
5+
import java.util.Comparator;
6+
import java.util.List;
7+
import java.util.stream.Collectors;
8+
9+
import org.alfasoftware.astra.core.utils.ASTOperation;
10+
import org.alfasoftware.astra.core.utils.AstraUtils;
11+
import org.eclipse.jdt.core.dom.AST;
12+
import org.eclipse.jdt.core.dom.ASTNode;
13+
import org.eclipse.jdt.core.dom.BodyDeclaration;
14+
import org.eclipse.jdt.core.dom.ChildListPropertyDescriptor;
15+
import org.eclipse.jdt.core.dom.CompilationUnit;
16+
import org.eclipse.jdt.core.dom.IExtendedModifier;
17+
import org.eclipse.jdt.core.dom.Modifier;
18+
import org.eclipse.jdt.core.dom.StructuralPropertyDescriptor;
19+
import org.eclipse.jdt.core.dom.rewrite.ASTRewrite;
20+
import org.eclipse.jdt.core.dom.rewrite.ListRewrite;
21+
import org.eclipse.jface.text.BadLocationException;
22+
import org.eclipse.text.edits.MalformedTreeException;
23+
import org.slf4j.Logger;
24+
import org.slf4j.LoggerFactory;
25+
26+
/**
27+
* Refactoring operation implementing SonarQube rule java:S1124
28+
* "Modifiers should be declared in the correct order".
29+
*
30+
* <p>The Java Language Specification recommends the following canonical modifier order:
31+
* <ol>
32+
* <li>Annotations</li>
33+
* <li>{@code public}</li>
34+
* <li>{@code protected}</li>
35+
* <li>{@code private}</li>
36+
* <li>{@code abstract}</li>
37+
* <li>{@code default}</li>
38+
* <li>{@code static}</li>
39+
* <li>{@code final}</li>
40+
* <li>{@code transient}</li>
41+
* <li>{@code volatile}</li>
42+
* <li>{@code synchronized}</li>
43+
* <li>{@code native}</li>
44+
* <li>{@code strictfp}</li>
45+
* </ol>
46+
*
47+
* <p>When keyword modifiers appear out of this order on any body declaration (type, method, field,
48+
* etc.) they are reordered in-place. Annotations are left at their original positions.
49+
*
50+
* <p>Examples:
51+
* <ul>
52+
* <li>{@code static public void foo()} &rarr; {@code public static void foo()}</li>
53+
* <li>{@code final static int X = 1} &rarr; {@code static final int X = 1}</li>
54+
* <li>{@code final public static String S} &rarr; {@code public static final String S}</li>
55+
* <li>{@code synchronized static void bar()} &rarr; {@code static synchronized void bar()}</li>
56+
* </ul>
57+
*
58+
* <p>Mirrors the detection logic of SonarJava's {@code ModifiersOrderCheck}.
59+
*/
60+
public class ModifiersOrderOperation implements ASTOperation {
61+
62+
private static final Logger log = LoggerFactory.getLogger(ModifiersOrderOperation.class);
63+
64+
/**
65+
* Canonical modifier order per the Java Language Specification (JLS 17).
66+
* Modifiers not present in this list are placed at the end in their original relative order.
67+
*/
68+
static final List<Modifier.ModifierKeyword> MODIFIER_ORDER = List.of(
69+
Modifier.ModifierKeyword.PUBLIC_KEYWORD,
70+
Modifier.ModifierKeyword.PROTECTED_KEYWORD,
71+
Modifier.ModifierKeyword.PRIVATE_KEYWORD,
72+
Modifier.ModifierKeyword.ABSTRACT_KEYWORD,
73+
Modifier.ModifierKeyword.DEFAULT_KEYWORD,
74+
Modifier.ModifierKeyword.STATIC_KEYWORD,
75+
Modifier.ModifierKeyword.FINAL_KEYWORD,
76+
Modifier.ModifierKeyword.TRANSIENT_KEYWORD,
77+
Modifier.ModifierKeyword.VOLATILE_KEYWORD,
78+
Modifier.ModifierKeyword.SYNCHRONIZED_KEYWORD,
79+
Modifier.ModifierKeyword.NATIVE_KEYWORD,
80+
Modifier.ModifierKeyword.STRICTFP_KEYWORD
81+
);
82+
83+
@Override
84+
public void run(CompilationUnit compilationUnit, ASTNode node, ASTRewrite rewriter)
85+
throws IOException, MalformedTreeException, BadLocationException {
86+
87+
if (!(node instanceof BodyDeclaration)) {
88+
return;
89+
}
90+
91+
BodyDeclaration bodyDecl = (BodyDeclaration) node;
92+
93+
@SuppressWarnings("unchecked")
94+
List<IExtendedModifier> allModifiers = bodyDecl.modifiers();
95+
96+
List<Modifier> keywordModifiers = allModifiers.stream()
97+
.filter(m -> m instanceof Modifier)
98+
.map(m -> (Modifier) m)
99+
.collect(Collectors.toList());
100+
101+
if (keywordModifiers.size() < 2) {
102+
return;
103+
}
104+
105+
List<Modifier.ModifierKeyword> currentOrder = keywordModifiers.stream()
106+
.map(Modifier::getKeyword)
107+
.collect(Collectors.toList());
108+
109+
List<Modifier.ModifierKeyword> sortedOrder = new ArrayList<>(currentOrder);
110+
sortedOrder.sort(Comparator.comparingInt(k -> {
111+
int idx = MODIFIER_ORDER.indexOf(k);
112+
return idx == -1 ? MODIFIER_ORDER.size() : idx;
113+
}));
114+
115+
if (currentOrder.equals(sortedOrder)) {
116+
return;
117+
}
118+
119+
log.info("Reordering modifiers in [{}]: {} -> {}",
120+
AstraUtils.getNameForCompilationUnit(compilationUnit),
121+
currentOrder.stream().map(k -> k.toString()).collect(Collectors.joining(" ")),
122+
sortedOrder.stream().map(k -> k.toString()).collect(Collectors.joining(" ")));
123+
124+
ChildListPropertyDescriptor modifiersProperty = findModifiersProperty(bodyDecl);
125+
if (modifiersProperty == null) {
126+
return;
127+
}
128+
129+
AST ast = compilationUnit.getAST();
130+
ListRewrite listRewrite = rewriter.getListRewrite(bodyDecl, modifiersProperty);
131+
132+
for (int i = 0; i < keywordModifiers.size(); i++) {
133+
Modifier.ModifierKeyword target = sortedOrder.get(i);
134+
if (!currentOrder.get(i).equals(target)) {
135+
listRewrite.replace(keywordModifiers.get(i), ast.newModifier(target), null);
136+
}
137+
}
138+
}
139+
140+
/**
141+
* Finds the "modifiers" ChildListPropertyDescriptor for the given node by inspecting
142+
* its structural properties. This avoids hard-coding per-subclass property constants
143+
* (MethodDeclaration.MODIFIERS2_PROPERTY, FieldDeclaration.MODIFIERS2_PROPERTY, etc.)
144+
* which are distinct objects even though they share the same semantics.
145+
*/
146+
@SuppressWarnings("rawtypes")
147+
private static ChildListPropertyDescriptor findModifiersProperty(ASTNode node) {
148+
for (Object prop : node.structuralPropertiesForType()) {
149+
if (prop instanceof ChildListPropertyDescriptor) {
150+
ChildListPropertyDescriptor clpd = (ChildListPropertyDescriptor) prop;
151+
if ("modifiers".equals(clpd.getId())) {
152+
return clpd;
153+
}
154+
}
155+
}
156+
return null;
157+
}
158+
}

‎astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s1124/ModifiersOrderExample.java‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
package org.alfasoftware.astra.core.refactoring.operations.sonar.s1124;
2+
3+
public class ModifiersOrderExample {
4+
5+
// static before public — wrong order on a method
6+
static public void staticBeforePublic() {}
7+
8+
// final before static — wrong order on a field
9+
final static int FINAL_STATIC = 1;
10+
11+
// all three wrong: final before public before static
12+
final public static String ALL_THREE_WRONG = "x";
13+
14+
// synchronized before static — wrong order on a method
15+
synchronized static void synchronizedStatic() {}
16+
17+
// native before static — wrong order (native method has no body)
18+
native static void nativeStatic();
19+
20+
// volatile before transient — wrong order on a field
21+
volatile transient int volatileTransient;
22+
23+
// nested class: abstract static public — wrong order (public should lead, then abstract, then static)
24+
abstract static public class WrongNestedClass {
25+
26+
// synchronized before static inside nested class — wrong order
27+
synchronized static void nestedSyncStatic() {}
28+
}
29+
}

‎astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s1124/ModifiersOrderExampleAfter.java‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
package org.alfasoftware.astra.core.refactoring.operations.sonar.s1124;
2+
3+
public class ModifiersOrderExampleAfter {
4+
5+
// static before public — wrong order on a method
6+
public static void staticBeforePublic() {}
7+
8+
// final before static — wrong order on a field
9+
static final int FINAL_STATIC = 1;
10+
11+
// all three wrong: final before public before static
12+
public static final String ALL_THREE_WRONG = "x";
13+
14+
// synchronized before static — wrong order on a method
15+
static synchronized void synchronizedStatic() {}
16+
17+
// native before static — wrong order (native method has no body)
18+
static native void nativeStatic();
19+
20+
// volatile before transient — wrong order on a field
21+
transient volatile int volatileTransient;
22+
23+
// nested class: abstract static public — wrong order (public should lead, then abstract, then static)
24+
public abstract static class WrongNestedClass {
25+
26+
// synchronized before static inside nested class — wrong order
27+
static synchronized void nestedSyncStatic() {}
28+
}
29+
}

‎astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s1124/ModifiersOrderNoopExample.java‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
package org.alfasoftware.astra.core.refactoring.operations.sonar.s1124;
2+
3+
public class ModifiersOrderNoopExample {
4+
5+
// public static — already correct
6+
public static void publicStatic() {}
7+
8+
// public static final — already correct
9+
public static final int PUBLIC_STATIC_FINAL = 1;
10+
11+
// private static final — already correct
12+
private static final String PRIVATE_STATIC_FINAL = "x";
13+
14+
// static synchronized — already correct
15+
static synchronized void staticSynchronized() {}
16+
17+
// transient volatile — already correct (transient < volatile in JLS order)
18+
transient volatile int transientVolatile;
19+
20+
// public final — already correct
21+
public final void publicFinal() {}
22+
23+
// protected static — already correct
24+
protected static void protectedStatic() {}
25+
26+
// single modifier: cannot be out of order
27+
public void singlePublic() {}
28+
29+
// single modifier
30+
static void singleStatic() {}
31+
32+
// single modifier
33+
final int singleFinal = 0;
34+
35+
// no modifier: nothing to reorder
36+
int noModifier;
37+
38+
// nested class: public abstract static — already correct
39+
public abstract static class CorrectNestedClass {
40+
41+
// static synchronized inside nested class — already correct
42+
static synchronized void nestedStaticSync() {}
43+
44+
// public static final — already correct
45+
public static final String NESTED_CONST = "y";
46+
}
47+
}

‎astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s1124/ModifiersOrderNoopExampleAfter.java‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
package org.alfasoftware.astra.core.refactoring.operations.sonar.s1124;
2+
3+
public class ModifiersOrderNoopExampleAfter {
4+
5+
// public static — already correct
6+
public static void publicStatic() {}
7+
8+
// public static final — already correct
9+
public static final int PUBLIC_STATIC_FINAL = 1;
10+
11+
// private static final — already correct
12+
private static final String PRIVATE_STATIC_FINAL = "x";
13+
14+
// static synchronized — already correct
15+
static synchronized void staticSynchronized() {}
16+
17+
// transient volatile — already correct (transient < volatile in JLS order)
18+
transient volatile int transientVolatile;
19+
20+
// public final — already correct
21+
public final void publicFinal() {}
22+
23+
// protected static — already correct
24+
protected static void protectedStatic() {}
25+
26+
// single modifier: cannot be out of order
27+
public void singlePublic() {}
28+
29+
// single modifier
30+
static void singleStatic() {}
31+
32+
// single modifier
33+
final int singleFinal = 0;
34+
35+
// no modifier: nothing to reorder
36+
int noModifier;
37+
38+
// nested class: public abstract static — already correct
39+
public abstract static class CorrectNestedClass {
40+
41+
// static synchronized inside nested class — already correct
42+
static synchronized void nestedStaticSync() {}
43+
44+
// public static final — already correct
45+
public static final String NESTED_CONST = "y";
46+
}
47+
}

‎astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s1124/TestModifiersOrderOperation.java‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
package org.alfasoftware.astra.core.refactoring.operations.sonar.s1124;
2+
3+
import java.util.Set;
4+
5+
import org.alfasoftware.astra.core.refactoring.AbstractRefactorTest;
6+
import org.junit.Test;
7+
8+
public class TestModifiersOrderOperation extends AbstractRefactorTest {
9+
10+
private static final Set<ModifiersOrderOperation> OPERATION =
11+
Set.of(new ModifiersOrderOperation());
12+
13+
/**
14+
* All cases where keyword modifiers are out of the JLS canonical order and must be reordered:
15+
* <ul>
16+
* <li>{@code static public} &rarr; {@code public static} (method)</li>
17+
* <li>{@code final static} &rarr; {@code static final} (field)</li>
18+
* <li>{@code final public static} &rarr; {@code public static final} (field)</li>
19+
* <li>{@code synchronized static} &rarr; {@code static synchronized} (method)</li>
20+
* <li>{@code abstract static public} &rarr; {@code public abstract static} (nested class)</li>
21+
* <li>{@code volatile transient} &rarr; {@code transient volatile} (field)</li>
22+
* <li>{@code native static} &rarr; {@code static native} (method)</li>
23+
* <li>{@code protected abstract} + {@code static synchronized} inside nested class</li>
24+
* </ul>
25+
*/
26+
@Test
27+
public void testModifiersReordered() {
28+
assertRefactor(ModifiersOrderExample.class, OPERATION);
29+
}
30+
31+
/**
32+
* Cases where modifier order is already correct — the operation must make no change:
33+
* <ul>
34+
* <li>{@code public static final} field</li>
35+
* <li>{@code public abstract} method</li>
36+
* <li>{@code private static final} field</li>
37+
* <li>{@code protected static} method</li>
38+
* <li>{@code static synchronized} method</li>
39+
* <li>{@code transient volatile} field</li>
40+
* <li>Single-modifier declarations</li>
41+
* </ul>
42+
*/
43+
@Test
44+
public void testNoopCases() {
45+
assertRefactor(ModifiersOrderNoopExample.class, OPERATION);
46+
}
47+
}

0 commit comments

Comments
 (0)