From 3807c730d32589a8be108139435d027a0102550d Mon Sep 17 00:00:00 2001 From: Joseph Date: Thu, 11 Jun 2026 13:11:17 +0100 Subject: [PATCH] Add ModifiersOrderOperation (java:S1124) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements SonarQube rule java:S1124 — modifiers must appear in the JLS canonical order (public/protected/private, abstract, default, static, final, transient, volatile, synchronized, native, strictfp). Visits every BodyDeclaration node, extracts keyword modifiers, sorts them by the canonical index, and uses ASTRewrite ListRewrite to replace each out-of-order modifier in-place, leaving annotations at their original positions. Mirrors SonarJava's ModifiersOrderCheck detection logic (iterate through modifiers against the canonical enum order; report first violation). Tests cover: static/public swap on methods, final/static and final/public/static on fields, synchronized/static, native/static, volatile/transient, nested-class abstract/static/public reordering, and nested method reordering. Noop cases confirm already-correct orderings and single-modifier declarations are untouched. Co-Authored-By: Claude Sonnet 4.6 --- .../sonar/s1124/ModifiersOrderOperation.java | 158 ++++++++++++++++++ .../sonar/s1124/ModifiersOrderExample.java | 29 ++++ .../s1124/ModifiersOrderExampleAfter.java | 29 ++++ .../s1124/ModifiersOrderNoopExample.java | 47 ++++++ .../s1124/ModifiersOrderNoopExampleAfter.java | 47 ++++++ .../s1124/TestModifiersOrderOperation.java | 47 ++++++ 6 files changed, 357 insertions(+) create mode 100644 astra-core/src/main/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s1124/ModifiersOrderOperation.java create mode 100644 astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s1124/ModifiersOrderExample.java create mode 100644 astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s1124/ModifiersOrderExampleAfter.java create mode 100644 astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s1124/ModifiersOrderNoopExample.java create mode 100644 astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s1124/ModifiersOrderNoopExampleAfter.java create mode 100644 astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s1124/TestModifiersOrderOperation.java diff --git a/astra-core/src/main/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s1124/ModifiersOrderOperation.java b/astra-core/src/main/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s1124/ModifiersOrderOperation.java new file mode 100644 index 0000000..7ac3c7b --- /dev/null +++ b/astra-core/src/main/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s1124/ModifiersOrderOperation.java @@ -0,0 +1,158 @@ +package org.alfasoftware.astra.core.refactoring.operations.sonar.s1124; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.Comparator; +import java.util.List; +import java.util.stream.Collectors; + +import org.alfasoftware.astra.core.utils.ASTOperation; +import org.alfasoftware.astra.core.utils.AstraUtils; +import org.eclipse.jdt.core.dom.AST; +import org.eclipse.jdt.core.dom.ASTNode; +import org.eclipse.jdt.core.dom.BodyDeclaration; +import org.eclipse.jdt.core.dom.ChildListPropertyDescriptor; +import org.eclipse.jdt.core.dom.CompilationUnit; +import org.eclipse.jdt.core.dom.IExtendedModifier; +import org.eclipse.jdt.core.dom.Modifier; +import org.eclipse.jdt.core.dom.StructuralPropertyDescriptor; +import org.eclipse.jdt.core.dom.rewrite.ASTRewrite; +import org.eclipse.jdt.core.dom.rewrite.ListRewrite; +import org.eclipse.jface.text.BadLocationException; +import org.eclipse.text.edits.MalformedTreeException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Refactoring operation implementing SonarQube rule java:S1124 + * "Modifiers should be declared in the correct order". + * + *

The Java Language Specification recommends the following canonical modifier order: + *

    + *
  1. Annotations
    2. + *
  2. {@code public}
    3. + *
  3. {@code protected}
    4. + *
  4. {@code private}
    5. + *
  5. {@code abstract}
    6. + *
  6. {@code default}
    7. + *
  7. {@code static}
    8. + *
  8. {@code final}
    9. + *
  9. {@code transient}
    10. + *
  10. {@code volatile}
    11. + *
  11. {@code synchronized}
    12. + *
  12. {@code native}
    13. + *
  13. {@code strictfp}
    14. + *
+ * + *

When keyword modifiers appear out of this order on any body declaration (type, method, field, + * etc.) they are reordered in-place. Annotations are left at their original positions. + * + *

Examples: + *

+ * + *

Mirrors the detection logic of SonarJava's {@code ModifiersOrderCheck}. + */ +public class ModifiersOrderOperation implements ASTOperation { + + private static final Logger log = LoggerFactory.getLogger(ModifiersOrderOperation.class); + + /** + * Canonical modifier order per the Java Language Specification (JLS 17). + * Modifiers not present in this list are placed at the end in their original relative order. + */ + static final List MODIFIER_ORDER = List.of( + Modifier.ModifierKeyword.PUBLIC_KEYWORD, + Modifier.ModifierKeyword.PROTECTED_KEYWORD, + Modifier.ModifierKeyword.PRIVATE_KEYWORD, + Modifier.ModifierKeyword.ABSTRACT_KEYWORD, + Modifier.ModifierKeyword.DEFAULT_KEYWORD, + Modifier.ModifierKeyword.STATIC_KEYWORD, + Modifier.ModifierKeyword.FINAL_KEYWORD, + Modifier.ModifierKeyword.TRANSIENT_KEYWORD, + Modifier.ModifierKeyword.VOLATILE_KEYWORD, + Modifier.ModifierKeyword.SYNCHRONIZED_KEYWORD, + Modifier.ModifierKeyword.NATIVE_KEYWORD, + Modifier.ModifierKeyword.STRICTFP_KEYWORD + ); + + @Override + public void run(CompilationUnit compilationUnit, ASTNode node, ASTRewrite rewriter) + throws IOException, MalformedTreeException, BadLocationException { + + if (!(node instanceof BodyDeclaration)) { + return; + } + + BodyDeclaration bodyDecl = (BodyDeclaration) node; + + @SuppressWarnings("unchecked") + List allModifiers = bodyDecl.modifiers(); + + List keywordModifiers = allModifiers.stream() + .filter(m -> m instanceof Modifier) + .map(m -> (Modifier) m) + .collect(Collectors.toList()); + + if (keywordModifiers.size() < 2) { + return; + } + + List currentOrder = keywordModifiers.stream() + .map(Modifier::getKeyword) + .collect(Collectors.toList()); + + List sortedOrder = new ArrayList<>(currentOrder); + sortedOrder.sort(Comparator.comparingInt(k -> { + int idx = MODIFIER_ORDER.indexOf(k); + return idx == -1 ? MODIFIER_ORDER.size() : idx; + })); + + if (currentOrder.equals(sortedOrder)) { + return; + } + + log.info("Reordering modifiers in [{}]: {} -> {}", + AstraUtils.getNameForCompilationUnit(compilationUnit), + currentOrder.stream().map(k -> k.toString()).collect(Collectors.joining(" ")), + sortedOrder.stream().map(k -> k.toString()).collect(Collectors.joining(" "))); + + ChildListPropertyDescriptor modifiersProperty = findModifiersProperty(bodyDecl); + if (modifiersProperty == null) { + return; + } + + AST ast = compilationUnit.getAST(); + ListRewrite listRewrite = rewriter.getListRewrite(bodyDecl, modifiersProperty); + + for (int i = 0; i < keywordModifiers.size(); i++) { + Modifier.ModifierKeyword target = sortedOrder.get(i); + if (!currentOrder.get(i).equals(target)) { + listRewrite.replace(keywordModifiers.get(i), ast.newModifier(target), null); + } + } + } + + /** + * Finds the "modifiers" ChildListPropertyDescriptor for the given node by inspecting + * its structural properties. This avoids hard-coding per-subclass property constants + * (MethodDeclaration.MODIFIERS2_PROPERTY, FieldDeclaration.MODIFIERS2_PROPERTY, etc.) + * which are distinct objects even though they share the same semantics. + */ + @SuppressWarnings("rawtypes") + private static ChildListPropertyDescriptor findModifiersProperty(ASTNode node) { + for (Object prop : node.structuralPropertiesForType()) { + if (prop instanceof ChildListPropertyDescriptor) { + ChildListPropertyDescriptor clpd = (ChildListPropertyDescriptor) prop; + if ("modifiers".equals(clpd.getId())) { + return clpd; + } + } + } + return null; + } +} diff --git a/astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s1124/ModifiersOrderExample.java b/astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s1124/ModifiersOrderExample.java new file mode 100644 index 0000000..f3451be --- /dev/null +++ b/astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s1124/ModifiersOrderExample.java @@ -0,0 +1,29 @@ +package org.alfasoftware.astra.core.refactoring.operations.sonar.s1124; + +public class ModifiersOrderExample { + + // static before public — wrong order on a method + static public void staticBeforePublic() {} + + // final before static — wrong order on a field + final static int FINAL_STATIC = 1; + + // all three wrong: final before public before static + final public static String ALL_THREE_WRONG = "x"; + + // synchronized before static — wrong order on a method + synchronized static void synchronizedStatic() {} + + // native before static — wrong order (native method has no body) + native static void nativeStatic(); + + // volatile before transient — wrong order on a field + volatile transient int volatileTransient; + + // nested class: abstract static public — wrong order (public should lead, then abstract, then static) + abstract static public class WrongNestedClass { + + // synchronized before static inside nested class — wrong order + synchronized static void nestedSyncStatic() {} + } +} diff --git a/astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s1124/ModifiersOrderExampleAfter.java b/astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s1124/ModifiersOrderExampleAfter.java new file mode 100644 index 0000000..9a1afe9 --- /dev/null +++ b/astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s1124/ModifiersOrderExampleAfter.java @@ -0,0 +1,29 @@ +package org.alfasoftware.astra.core.refactoring.operations.sonar.s1124; + +public class ModifiersOrderExampleAfter { + + // static before public — wrong order on a method + public static void staticBeforePublic() {} + + // final before static — wrong order on a field + static final int FINAL_STATIC = 1; + + // all three wrong: final before public before static + public static final String ALL_THREE_WRONG = "x"; + + // synchronized before static — wrong order on a method + static synchronized void synchronizedStatic() {} + + // native before static — wrong order (native method has no body) + static native void nativeStatic(); + + // volatile before transient — wrong order on a field + transient volatile int volatileTransient; + + // nested class: abstract static public — wrong order (public should lead, then abstract, then static) + public abstract static class WrongNestedClass { + + // synchronized before static inside nested class — wrong order + static synchronized void nestedSyncStatic() {} + } +} diff --git a/astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s1124/ModifiersOrderNoopExample.java b/astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s1124/ModifiersOrderNoopExample.java new file mode 100644 index 0000000..9b80d1f --- /dev/null +++ b/astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s1124/ModifiersOrderNoopExample.java @@ -0,0 +1,47 @@ +package org.alfasoftware.astra.core.refactoring.operations.sonar.s1124; + +public class ModifiersOrderNoopExample { + + // public static — already correct + public static void publicStatic() {} + + // public static final — already correct + public static final int PUBLIC_STATIC_FINAL = 1; + + // private static final — already correct + private static final String PRIVATE_STATIC_FINAL = "x"; + + // static synchronized — already correct + static synchronized void staticSynchronized() {} + + // transient volatile — already correct (transient < volatile in JLS order) + transient volatile int transientVolatile; + + // public final — already correct + public final void publicFinal() {} + + // protected static — already correct + protected static void protectedStatic() {} + + // single modifier: cannot be out of order + public void singlePublic() {} + + // single modifier + static void singleStatic() {} + + // single modifier + final int singleFinal = 0; + + // no modifier: nothing to reorder + int noModifier; + + // nested class: public abstract static — already correct + public abstract static class CorrectNestedClass { + + // static synchronized inside nested class — already correct + static synchronized void nestedStaticSync() {} + + // public static final — already correct + public static final String NESTED_CONST = "y"; + } +} diff --git a/astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s1124/ModifiersOrderNoopExampleAfter.java b/astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s1124/ModifiersOrderNoopExampleAfter.java new file mode 100644 index 0000000..d1f2929 --- /dev/null +++ b/astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s1124/ModifiersOrderNoopExampleAfter.java @@ -0,0 +1,47 @@ +package org.alfasoftware.astra.core.refactoring.operations.sonar.s1124; + +public class ModifiersOrderNoopExampleAfter { + + // public static — already correct + public static void publicStatic() {} + + // public static final — already correct + public static final int PUBLIC_STATIC_FINAL = 1; + + // private static final — already correct + private static final String PRIVATE_STATIC_FINAL = "x"; + + // static synchronized — already correct + static synchronized void staticSynchronized() {} + + // transient volatile — already correct (transient < volatile in JLS order) + transient volatile int transientVolatile; + + // public final — already correct + public final void publicFinal() {} + + // protected static — already correct + protected static void protectedStatic() {} + + // single modifier: cannot be out of order + public void singlePublic() {} + + // single modifier + static void singleStatic() {} + + // single modifier + final int singleFinal = 0; + + // no modifier: nothing to reorder + int noModifier; + + // nested class: public abstract static — already correct + public abstract static class CorrectNestedClass { + + // static synchronized inside nested class — already correct + static synchronized void nestedStaticSync() {} + + // public static final — already correct + public static final String NESTED_CONST = "y"; + } +} diff --git a/astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s1124/TestModifiersOrderOperation.java b/astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s1124/TestModifiersOrderOperation.java new file mode 100644 index 0000000..555b19e --- /dev/null +++ b/astra-core/src/test/java/org/alfasoftware/astra/core/refactoring/operations/sonar/s1124/TestModifiersOrderOperation.java @@ -0,0 +1,47 @@ +package org.alfasoftware.astra.core.refactoring.operations.sonar.s1124; + +import java.util.Set; + +import org.alfasoftware.astra.core.refactoring.AbstractRefactorTest; +import org.junit.Test; + +public class TestModifiersOrderOperation extends AbstractRefactorTest { + + private static final Set OPERATION = + Set.of(new ModifiersOrderOperation()); + + /** + * All cases where keyword modifiers are out of the JLS canonical order and must be reordered: + *

+ */ + @Test + public void testModifiersReordered() { + assertRefactor(ModifiersOrderExample.class, OPERATION); + } + + /** + * Cases where modifier order is already correct — the operation must make no change: + * + */ + @Test + public void testNoopCases() { + assertRefactor(ModifiersOrderNoopExample.class, OPERATION); + } +}