From 7fd9300499228717bcfee75ff4f836507df832ee Mon Sep 17 00:00:00 2001 From: Liam Miller-Cushon Date: Wed, 5 Jun 2024 08:29:51 -0700 Subject: [PATCH] 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: 640534518 --- .../java/ModifierOrderer.java | 142 ++++++++++++++---- .../java/FormatterIntegrationTest.java | 2 +- .../java/ModifierOrdererTest.java | 8 + .../java/testdata/Sealed.input | 6 + .../java/testdata/Sealed.output | 9 ++ 5 files changed, 140 insertions(+), 27 deletions(-) create mode 100644 core/src/test/resources/com/google/googlejavaformat/java/testdata/Sealed.input create mode 100644 core/src/test/resources/com/google/googlejavaformat/java/testdata/Sealed.output diff --git a/core/src/main/java/com/google/googlejavaformat/java/ModifierOrderer.java b/core/src/main/java/com/google/googlejavaformat/java/ModifierOrderer.java index e14b29036..1b26f7d69 100644 --- 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 { + private final ImmutableList tokens; + private final Modifier modifier; + + static ModifierTokens create(ImmutableList tokens) { + return new ModifierTokens(tokens, asModifier(tokens)); + } + + static ModifierTokens empty() { + return new ModifierTokens(ImmutableList.of(), null); + } + + ModifierTokens(ImmutableList tokens, Modifier modifier) { + this.tokens = tokens; + this.modifier = modifier; + } + + boolean isEmpty() { + return tokens.isEmpty() || modifier == null; + } + + 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().getText().length(); + } + + ImmutableList getToksBefore() { + return first().getToksBefore(); + } + + ImmutableList 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 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)); - 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 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()); + } + + private static @Nullable 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. */ - 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 index 4a4cedc82..c7142b793 100644 --- 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.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 index 1f8fc1721..0f01e9d3f 100644 --- 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 new file mode 100644 index 000000000..f19165843 --- /dev/null +++ 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 new file mode 100644 index 000000000..4bafc6399 --- /dev/null +++ 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 {} +}