Fix modifier order handling for non-sealed

`non-sealed` is tokenized as three tokens, the modifier sorting logic was assuming it would show up as a single token. PiperOrigin-RevId: 640284683
google · Jun 5, 2024 · 1330568 · 1330568
1 parent db08589
commit 1330568
Show file tree

Hide file tree

Showing 5 changed files with 140 additions and 27 deletions.
diff --git a/core/src/main/java/com/google/googlejavaformat/java/ModifierOrderer.java b/core/src/main/java/com/google/googlejavaformat/java/ModifierOrderer.java
@@ -16,6 +16,9 @@
 
 package com.google.googlejavaformat.java;
 
+import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.collect.Iterables.getLast;
+
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Ordering;
 import com.google.common.collect.Range;
@@ -26,12 +29,12 @@
 import com.sun.tools.javac.parser.Tokens.TokenKind;
 import java.util.ArrayList;
 import java.util.Collection;
-import java.util.Collections;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
 import javax.lang.model.element.Modifier;
+import org.jspecify.annotations.Nullable;
 
 /** Fixes sequences of modifiers to be in JLS order. */
 final class ModifierOrderer {
@@ -42,6 +45,71 @@ static JavaInput reorderModifiers(String text) throws FormatterException {
         new JavaInput(text), ImmutableList.of(Range.closedOpen(0, text.length())));
   }
 
+  /**
+   * A class that contains the tokens corresponding to a modifier. This is usually a single token
+   * (e.g. for {@code public}), but may be multiple tokens for modifiers containing {@code -} (e.g.
+   * {@code non-sealed}).
+   */
+  static class ModifierTokens implements Comparable<ModifierTokens> {
+    private final ImmutableList<Token> tokens;
+    private final Modifier modifier;
+
+    static ModifierTokens create(ImmutableList<Token> tokens) {
+      return new ModifierTokens(tokens, asModifier(tokens));
+    }
+
+    static ModifierTokens empty() {
+      return new ModifierTokens(ImmutableList.of(), null);
+    }
+
+    ModifierTokens(ImmutableList<Token> tokens, Modifier modifier) {
+      this.tokens = tokens;
+      this.modifier = modifier;
+    }
+
+    boolean isEmpty() {
+      return tokens.isEmpty() || modifier == null;
+    }
+
+    Modifier modifier() {
+      return modifier;
+    }
+
+    ImmutableList<Token> tokens() {
+      return tokens;
+    }
+
+    private Token first() {
+      return tokens.get(0);
+    }
+
+    private Token last() {
+      return getLast(tokens);
+    }
+
+    int startPosition() {
+      return first().getTok().getPosition();
+    }
+
+    int endPosition() {
+      return last().getTok().getPosition() + last().getTok().getText().length();
+    }
+
+    ImmutableList<? extends Tok> getToksBefore() {
+      return first().getToksBefore();
+    }
+
+    ImmutableList<? extends Tok> getToksAfter() {
+      return last().getToksAfter();
+    }
+
+    @Override
+    public int compareTo(ModifierTokens o) {
+      checkState(!isEmpty()); // empty ModifierTokens are filtered out prior to sorting
+      return modifier.compareTo(o.modifier);
+    }
+  }
+
   /**
    * Reorders all modifiers in the given text and within the given character ranges to be in JLS
    * order.
@@ -57,43 +125,37 @@ static JavaInput reorderModifiers(JavaInput javaInput, Collection<Range<Integer>
     Iterator<? extends Token> it = javaInput.getTokens().iterator();
     TreeRangeMap<Integer, String> replacements = TreeRangeMap.create();
     while (it.hasNext()) {
-      Token token = it.next();
-      if (!tokenRanges.contains(token.getTok().getIndex())) {
-        continue;
-      }
-      Modifier mod = asModifier(token);
-      if (mod == null) {
+      ModifierTokens tokens = getModifierTokens(it);
+      if (tokens.isEmpty()
+          || !tokens.tokens().stream()
+              .allMatch(token -> tokenRanges.contains(token.getTok().getIndex()))) {
         continue;
       }
 
-      List<Token> modifierTokens = new ArrayList<>();
-      List<Modifier> mods = new ArrayList<>();
+      List<ModifierTokens> modifierTokens = new ArrayList<>();
 
-      int begin = token.getTok().getPosition();
-      mods.add(mod);
-      modifierTokens.add(token);
+      int begin = tokens.startPosition();
+      modifierTokens.add(tokens);
 
       int end = -1;
       while (it.hasNext()) {
-        token = it.next();
-        mod = asModifier(token);
-        if (mod == null) {
+        tokens = getModifierTokens(it);
+        if (tokens.isEmpty()) {
           break;
         }
-        mods.add(mod);
-        modifierTokens.add(token);
-        end = token.getTok().getPosition() + token.getTok().length();
+        modifierTokens.add(tokens);
+        end = tokens.endPosition();
       }
 
-      if (!Ordering.natural().isOrdered(mods)) {
-        Collections.sort(mods);
+      if (!Ordering.natural().isOrdered(modifierTokens)) {
+        List<ModifierTokens> sorted = Ordering.natural().sortedCopy(modifierTokens);
         StringBuilder replacement = new StringBuilder();
-        for (int i = 0; i < mods.size(); i++) {
+        for (int i = 0; i < sorted.size(); i++) {
           if (i > 0) {
             addTrivia(replacement, modifierTokens.get(i).getToksBefore());
           }
-          replacement.append(mods.get(i));
-          if (i < (modifierTokens.size() - 1)) {
+          replacement.append(sorted.get(i).modifier());
+          if (i < (sorted.size() - 1)) {
             addTrivia(replacement, modifierTokens.get(i).getToksAfter());
           }
         }
@@ -109,11 +171,41 @@ private static void addTrivia(StringBuilder replacement, ImmutableList<? extends
     }
   }
 
+  private static @Nullable ModifierTokens getModifierTokens(Iterator<? extends Token> it) {
+    Token token = it.next();
+    ImmutableList.Builder<Token> result = ImmutableList.builder();
+    result.add(token);
+    if (!token.getTok().getText().equals("non")) {
+      return ModifierTokens.create(result.build());
+    }
+    if (!it.hasNext()) {
+      return ModifierTokens.empty();
+    }
+    Token dash = it.next();
+    result.add(dash);
+    if (!dash.getTok().getText().equals("-") || !it.hasNext()) {
+      return ModifierTokens.empty();
+    }
+    result.add(it.next());
+    return ModifierTokens.create(result.build());
+  }
+
+  private static @Nullable Modifier asModifier(ImmutableList<Token> tokens) {
+    if (tokens.size() == 1) {
+      return asModifier(tokens.get(0));
+    }
+    Modifier modifier = asModifier(getLast(tokens));
+    if (modifier == null) {
+      return null;
+    }
+    return Modifier.valueOf("NON_" + modifier.name());
+  }
+
   /**
    * Returns the given token as a {@link javax.lang.model.element.Modifier}, or {@code null} if it
    * is not a modifier.
    */
-  private static Modifier asModifier(Token token) {
+  private static @Nullable Modifier asModifier(Token token) {
     TokenKind kind = ((JavaInput.Tok) token.getTok()).kind();
     if (kind != null) {
       switch (kind) {
@@ -145,8 +237,6 @@ private static Modifier asModifier(Token token) {
       }
     }
     switch (token.getTok().getText()) {
-      case "non-sealed":
-        return Modifier.valueOf("NON_SEALED");
       case "sealed":
         return Modifier.valueOf("SEALED");
       default:

diff --git a/core/src/test/java/com/google/googlejavaformat/java/FormatterIntegrationTest.java b/core/src/test/java/com/google/googlejavaformat/java/FormatterIntegrationTest.java
@@ -50,7 +50,7 @@ public class FormatterIntegrationTest {
       ImmutableMultimap.<Integer, String>builder()
           .putAll(14, "I477", "Records", "RSLs", "Var", "ExpressionSwitch", "I574", "I594")
           .putAll(15, "I603")
-          .putAll(16, "I588")
+          .putAll(16, "I588", "Sealed")
           .putAll(17, "I683", "I684", "I696")
           .putAll(
               21,

diff --git a/core/src/test/java/com/google/googlejavaformat/java/ModifierOrdererTest.java b/core/src/test/java/com/google/googlejavaformat/java/ModifierOrdererTest.java
@@ -17,6 +17,7 @@
 package com.google.googlejavaformat.java;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.TruthJUnit.assume;
 
 import com.google.common.base.Joiner;
 import com.google.common.collect.Range;
@@ -103,4 +104,11 @@ public void whitespace() throws FormatterException {
             .getText();
     assertThat(output).contains("public\n  static int a;");
   }
+
+  @Test
+  public void sealedClass() throws FormatterException {
+    assume().that(Runtime.version().feature()).isAtLeast(16);
+    assertThat(ModifierOrderer.reorderModifiers("non-sealed sealed public").getText())
+        .isEqualTo("public sealed non-sealed");
+  }
 }
diff --git a/core/src/test/resources/com/google/googlejavaformat/java/testdata/Sealed.input b/core/src/test/resources/com/google/googlejavaformat/java/testdata/Sealed.input
@@ -0,0 +1,6 @@
+class T {
+  sealed interface I extends A permits C, B {}
+  final class C implements I {}
+  sealed private interface A permits I {}
+  non-sealed private interface B extends I {}
+}
diff --git a/core/src/test/resources/com/google/googlejavaformat/java/testdata/Sealed.output b/core/src/test/resources/com/google/googlejavaformat/java/testdata/Sealed.output
@@ -0,0 +1,9 @@
+class T {
+  sealed interface I extends A permits C, B {}
+
+  final class C implements I {}
+
+  private sealed interface A permits I {}
+
+  private non-sealed interface B extends I {}
+}