From 6282d1244f199a1827d2bced36df9d56c5b13a52 Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Thu, 24 Sep 2026 22:57:31 +0300 Subject: [PATCH] Reorder non-sealed as one modifier "non-sealed private interface B" came out as text that no longer parsed, so the whole file failed with " expected". javac lexes non-sealed as three tokens, "non", "-" and "sealed", and the modifier sorting looked at one token at a time: it never saw "non-sealed", took the trailing "sealed" as the modifier to move in front of "private", and left "non-" behind. This ports google/google-java-format#1107 by Liam Miller-Cushon: the sorter now reads the three tokens as one modifier and writes it back whole. The golden Sealed and the sealedClass test come from upstream, with the expected output in this project's style; a second unit test pins the shape that failed. The 15,747 files of the JDK 21 sources format exactly as before; none of them writes non-sealed out of order. --- .../javaformat/java/ModifierOrderer.java | 149 +++++++++++++++--- .../javaformat/java/FileBasedTests.java | 2 +- .../javaformat/java/ModifierOrdererTest.java | 13 ++ .../javaformat/java/testdata/Sealed.input | 6 + .../javaformat/java/testdata/Sealed.output | 9 ++ 5 files changed, 153 insertions(+), 26 deletions(-) create mode 100644 open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/Sealed.input create mode 100644 open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/Sealed.output diff --git a/open-java-format/src/main/java/com/palantir/javaformat/java/ModifierOrderer.java b/open-java-format/src/main/java/com/palantir/javaformat/java/ModifierOrderer.java index 26e83f130..e59bf115c 100644 --- a/open-java-format/src/main/java/com/palantir/javaformat/java/ModifierOrderer.java +++ b/open-java-format/src/main/java/com/palantir/javaformat/java/ModifierOrderer.java @@ -16,6 +16,9 @@ package com.palantir.javaformat.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,10 +29,10 @@ 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 javax.annotation.Nullable; import javax.lang.model.element.Modifier; /** Fixes sequences of modifiers to be in JLS order. */ @@ -40,6 +43,74 @@ static JavaInput reorderModifiers(String text) throws FormatterException { return reorderModifiers(new JavaInput(text), ImmutableList.of(Range.closedOpen(0, text.length()))); } + /** + * The tokens that make up one modifier. Usually a single token (e.g. for {@code public}), but a modifier that + * contains a {@code -} (e.g. {@code non-sealed}) is lexed as three tokens. + */ + static final class ModifierTokens implements Comparable { + private final ImmutableList tokens; + + @Nullable + private final Modifier modifier; + + static ModifierTokens create(ImmutableList tokens) { + return new ModifierTokens(tokens, asModifier(tokens)); + } + + static ModifierTokens empty() { + return new ModifierTokens(ImmutableList.of(), null); + } + + private ModifierTokens(ImmutableList tokens, @Nullable Modifier modifier) { + this.tokens = tokens; + this.modifier = modifier; + } + + boolean isEmpty() { + return tokens.isEmpty() || modifier == null; + } + + @SuppressWarnings("for-rollout:NullAway") + Modifier modifier() { + return modifier; + } + + ImmutableList 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().length(); + } + + ImmutableList getToksBefore() { + return first().getToksBefore(); + } + + ImmutableList getToksAfter() { + return last().getToksAfter(); + } + + @Override + @SuppressWarnings("for-rollout:NullAway") + 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. */ static JavaInput reorderModifiers(JavaInput javaInput, Collection> characterRanges) throws FormatterException { @@ -52,43 +123,37 @@ static JavaInput reorderModifiers(JavaInput javaInput, Collection Iterator it = javaInput.getTokens().iterator(); TreeRangeMap 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 modifierTokens = new ArrayList<>(); - List mods = new ArrayList<>(); + List 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 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).toString()); - if (i < (modifierTokens.size() - 1)) { + replacement.append(sorted.get(i).modifier()); + if (i < (sorted.size() - 1)) { addTrivia(replacement, modifierTokens.get(i).getToksAfter()); } } @@ -104,9 +169,45 @@ private static void addTrivia(StringBuilder replacement, ImmutableList it) { + Token token = it.next(); + ImmutableList.Builder 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()); + } + + @Nullable + private static Modifier asModifier(ImmutableList 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. */ + @Nullable @SuppressWarnings("for-rollout:NullAway") private static Modifier asModifier(Token token) { TokenKind kind = ((JavaInput.Tok) token.getTok()).kind(); @@ -140,8 +241,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/open-java-format/src/test/java/com/palantir/javaformat/java/FileBasedTests.java b/open-java-format/src/test/java/com/palantir/javaformat/java/FileBasedTests.java index 5305d7c47..026265920 100644 --- a/open-java-format/src/test/java/com/palantir/javaformat/java/FileBasedTests.java +++ b/open-java-format/src/test/java/com/palantir/javaformat/java/FileBasedTests.java @@ -49,7 +49,7 @@ public final class FileBasedTests { ImmutableMultimap.builder() .putAll(14, "Records", "RSL", "Var", "ExpressionSwitch", "I574", "I594") .putAll(15, "I603") - .putAll(16, "I588") + .putAll(16, "I588", "Sealed") .putAll(17, "I683", "I684", "I696") .putAll( 21, diff --git a/open-java-format/src/test/java/com/palantir/javaformat/java/ModifierOrdererTest.java b/open-java-format/src/test/java/com/palantir/javaformat/java/ModifierOrdererTest.java index 6a660a7a0..dc3ecc8f0 100644 --- a/open-java-format/src/test/java/com/palantir/javaformat/java/ModifierOrdererTest.java +++ b/open-java-format/src/test/java/com/palantir/javaformat/java/ModifierOrdererTest.java @@ -99,4 +99,17 @@ public void whitespace() throws FormatterException { .getText(); assertThat(output).contains("public\n static int a;"); } + + @Test + public void sealedClass() throws FormatterException { + assertThat(ModifierOrderer.reorderModifiers("non-sealed sealed public").getText()) + .isEqualTo("public sealed non-sealed"); + } + + @Test + public void nonSealedBeforeAccessModifier() throws FormatterException { + assertThat(ModifierOrderer.reorderModifiers("non-sealed private interface B extends I {}") + .getText()) + .isEqualTo("private non-sealed interface B extends I {}"); + } } diff --git a/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/Sealed.input b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/Sealed.input new file mode 100644 index 000000000..f19165843 --- /dev/null +++ b/open-java-format/src/test/resources/com/palantir/javaformat/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/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/Sealed.output b/open-java-format/src/test/resources/com/palantir/javaformat/java/testdata/Sealed.output new file mode 100644 index 000000000..0d5d0b377 --- /dev/null +++ b/open-java-format/src/test/resources/com/palantir/javaformat/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 {} +}