Skip to content

Commit 0298dbd

Browse files
authored
Recognize the value modifier of Valhalla value classes (#141)
* Recognize the value modifier of Valhalla value classes Port of google-java-format 1959806; its first attempt, 5f7a569, was rolled back the same day. A modifier is no longer told by its text alone: nextIsModifier checks the token against the flags of the ModifiersTree being visited, so `value`, `sealed` and `non` stay identifiers where they are names. Before this a lambda parameter named `sealed` or `non` failed with "expected token: ')'". The orderer looks VALUE and NON_* up by name with Enums.getIfPresent, since neither VALUE nor NON_VALUE exists on JDK 21. The value class and record tests are skipped on a JDK without Modifier.VALUE, which is every GA release up to 27; they need a Valhalla early-access build. Closes #131 * Run the tests on the Valhalla early-access build The value class tests of the previous commit are skipped on every JDK without Modifier.VALUE, which is every release up to 27. A fourth jdk leg runs them on the JEP 401 early-access build from jdk.java.net, based on JDK 27 for now. No setup-java distribution serves project builds, so the leg downloads the archive pinned by URL and checksum and installs it as a jdkfile; both move by hand when a newer build appears. * Expect the modifier order of the pinned Valhalla build The first run of the Valhalla leg sorted `value final public` to `public value final`: the JEP 401 early-access builds declare VALUE between NON_SEALED and FINAL in javax.lang.model.element.Modifier, whose order ModifierOrderer follows. Upstream expects `final value`, the order of Valhalla's lworld branch, where VALUE comes last. The tests now expect what the pinned build does; re-check them when CI moves to a newer build. * Run the value class tests on JDK 28 early access JEP 401 was integrated into JDK 28 on 2026-07-31, so its early-access builds are the JDK with value classes, and Temurin's come through actions/setup-java as 28-ea. The leg replaces the jdk.java.net Valhalla build pinned two commits ago: that line, based on JDK 27, declares VALUE between NON_SEALED and FINAL in javax.lang.model.element.Modifier and so sorted `value final public` to `public value final`. Mainline declares VALUE last, after STRICTFP, and sorts it to `public final value`, the order upstream's tests expect; the tests expect it again. * Spell the early-access leg as setup-java does The matrix entry is `28-ea`, the version actions/setup-java takes, and the Build step strips the suffix for -PjavaRuntime, which wants the number alone; the expression that appended `-ea` from an extra `ea` flag goes. * Bump gradle-wrapper from 9.8.1 to 9.8.2 Run with `./gradlew wrapper --gradle-version 9.8.2`, twice so that 9.8.2 wrote its own files: it adds the retries and retryBackOffMs properties for the distribution download, at their defaults. * Build the plugins' test projects with the oldest Gradle each JDK runs The Gradle plugins' tests built their projects with Gradle 9.3.1, the oldest the plugins support, on the JVM of the test, so the JDK 27 leg left them out: 9.3.1 does not run on Java 27 (Gradle's compatibility table names 9.8.0 as the first that does), and every TestKit build failed with "Unsupported class file major version 71". The reason in ci.yml blamed the wrapper's Gradle, which never mattered. GradleTestProject now reads the version from testkit.gradleVersion, which the build sets from -PtestkitGradleVersion, and CI's jdk legs name the oldest Gradle that runs on their JDK: 9.3.1 on 25, 9.4.0 on 26, 9.8.0 on 27. Only 28-ea keeps the exclusion, since no Gradle release runs on Java 28 yet. * Try the plugins' tests on 28-ea with Gradle 9.8.2 Gradle 9.8.2 claims Java 27, not 28, so the leg finds out whether its TestKit builds run on 28-ea regardless. If they do not, the leg goes back to leaving them out. * Leave the plugins' tests out on 28-ea again The try with Gradle 9.8.2 failed as its release notes suggested: it compiles no build script on Java 28, "Unsupported class file major version 72". The leg excludes the Gradle plugins' tests again, with that as the reason, until a Gradle release runs on Java 28.
1 parent ed9c1b3 commit 0298dbd

9 files changed

Lines changed: 186 additions & 37 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -55,19 +55,33 @@ jobs:
5555
strategy:
5656
fail-fast: false
5757
matrix:
58+
# `gradle` is what the Gradle plugins' tests build their projects with: the oldest release that runs on
59+
# the leg's JDK (https://docs.gradle.org/current/userguide/compatibility.html#java_runtime), or 9.3.1,
60+
# the oldest the plugins support, where that is older. Each leg thus tests the far end of what it can.
61+
# 28-ea has none that runs on it yet.
5862
include:
5963
- jdk: 25
64+
gradle: 9.3.1
6065
gradle_args: ''
6166
gradle_args_reason: ''
6267
- jdk: 26
68+
gradle: 9.4.0
6369
gradle_args: ''
6470
gradle_args_reason: ''
6571
- jdk: 27
72+
gradle: 9.8.0
73+
gradle_args: ''
74+
gradle_args_reason: ''
75+
# Value classes (JEP 401) are a preview feature of JDK 28, so until it is released the `value` modifier
76+
# tests (#131) run on its early-access builds and are skipped everywhere else. The version is spelled
77+
# as setup-java spells it; Gradle gets the number alone.
78+
- jdk: 28-ea
79+
gradle: ''
6680
gradle_args: -x :gradle-open-java-format:test
6781
gradle_args_reason: >-
68-
Gradle 9.7.1 does not run on Java 27 yet, and the Gradle plugin's tests run their TestKit builds on
69-
the test JVM, where they fail with "Unsupported class file major version 71". Those tests are left
70-
out until the wrapper's Gradle supports Java 27.
82+
No Gradle release runs on Java 28 yet: 9.8.2, the newest, fails to compile a build script there with
83+
"Unsupported class file major version 72". The Gradle plugins' tests build their projects with Gradle
84+
on the test JVM, so they are left out until a release runs on it.
7185
steps:
7286
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
7387
with:
@@ -99,11 +113,14 @@ jobs:
99113

100114
# Gradle does not look into the runner's tool cache, so it is told where the test JDK is. The path is
101115
# a step's output, so it reaches the shell as a variable rather than being pasted into the script.
116+
# -PjavaRuntime takes a number, so an early-access leg loses its -ea suffix here.
102117
- name: Build
103118
env:
104119
TEST_JDK: ${{ steps.test-jdk.outputs.path }}
120+
JDK: ${{ matrix.jdk }}
121+
TESTKIT_GRADLE: ${{ matrix.gradle }}
105122
run: >-
106-
./gradlew test -PjavaRuntime=${{ matrix.jdk }} ${{ matrix.gradle_args }}
123+
./gradlew test -PjavaRuntime="${JDK%-ea}" -PtestkitGradleVersion="$TESTKIT_GRADLE" ${{ matrix.gradle_args }}
107124
-Porg.gradle.java.installations.paths="$TEST_JDK"
108125
109126
- name: Publish Test Report

‎README.md‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -137,9 +137,14 @@ mise trust && mise install
137137
./gradlew test # what the CI build job runs
138138
```
139139

140-
`-PjavaRuntime=25` runs the same tests on JDK 25, as CI's `jdk` jobs do for 25, 26 and 27. The code
141-
is compiled for Java 21 either way. Gradle has to find that JDK: installed with mise, or named with
142-
`-Porg.gradle.java.installations.paths=/path/to/jdk`.
140+
`-PjavaRuntime=25` runs the same tests on JDK 25, as CI's `jdk` jobs do for 25, 26 and 27, and for
141+
the early-access builds of 28, where value classes are a preview feature and the `value` modifier
142+
tests run. The code is compiled for Java 21 either way. Gradle has to find that JDK: installed with
143+
mise, or named with `-Porg.gradle.java.installations.paths=/path/to/jdk`.
144+
145+
The Gradle plugins' tests build their projects with Gradle 9.3.1, the oldest the plugins support.
146+
`-PtestkitGradleVersion=9.8.0` builds them with another, as CI's JDK 27 leg does: 9.3.1 does not run
147+
on Java 27, and 9.8.0 is the oldest release that does.
143148

144149
Nothing inside the build downloads a JDK. `gradle.properties` turns toolchain auto-download off and
145150
reads the installations from `JDK21_HOME` and `GRAALVM_HOME`, so a missing JDK is an error you can

‎gradle-open-java-format/build.gradle‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,13 @@ def writeNativeImagePath = tasks.register("writeNativeImagePath") {
147147
tasks.named("test").configure {
148148
dependsOn(writeImplClasspath)
149149

150+
// The Gradle the TestKit builds run on, read by GradleTestProject, which defaults to the oldest Gradle 9
151+
// the plugins support. CI's jdk legs pass the oldest release that runs on their JDK; empty means none.
152+
def testkitGradleVersion = providers.gradleProperty('testkitGradleVersion').filter { !it.isEmpty() }
153+
if (testkitGradleVersion.isPresent()) {
154+
systemProperty 'testkit.gradleVersion', testkitGradleVersion.get()
155+
}
156+
150157
if (rootProject.ext.nativeImageRequested) {
151158
dependsOn(writeNativeImagePath)
152159
} else {

‎gradle-open-java-format/src/test/java/com/palantir/javaformat/gradle/testing/GradleTestProject.java‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -45,10 +45,12 @@
4545
public final class GradleTestProject {
4646

4747
/**
48-
* The oldest Gradle 9 we claim to support — the interesting end of the range to test against. There is no version
49-
* matrix any more: Gradle 8 was dropped when the build itself moved to 9.
48+
* The Gradle the generated projects build with: the oldest Gradle 9 we claim to support, the interesting end of
49+
* the range to test against. There is no version matrix any more: Gradle 8 was dropped when the build itself moved
50+
* to 9. On a JDK that old a Gradle cannot run on, the build names the oldest one that can in
51+
* {@code testkit.gradleVersion}; CI's jdk legs take it from Gradle's compatibility table.
5052
*/
51-
private static final String GRADLE_VERSION = "9.3.1";
53+
private static final String GRADLE_VERSION = System.getProperty("testkit.gradleVersion", "9.3.1");
5254

5355
/** Written by java-gradle-plugin: the classpath {@link GradleRunner#withPluginClasspath()} injects. */
5456
private static final String PLUGIN_METADATA = "plugin-under-test-metadata.properties";
Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
distributionBase=GRADLE_USER_HOME
22
distributionPath=wrapper/dists
3-
distributionUrl=https\://services.gradle.org/distributions/gradle-9.8.1-bin.zip
3+
distributionUrl=https\://services.gradle.org/distributions/gradle-9.8.2-bin.zip
44
networkTimeout=10000
5+
retries=0
6+
retryBackOffMs=500
57
validateDistributionUrl=true
68
zipStoreBase=GRADLE_USER_HOME
79
zipStorePath=wrapper/dists

‎open-java-format/src/main/java/com/palantir/javaformat/java/JavaInputAstVisitor.java‎

Lines changed: 52 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
import static com.sun.source.tree.Tree.Kind.STRING_LITERAL;
3535
import static java.util.stream.Collectors.toList;
3636

37+
import com.google.common.base.Enums;
3738
import com.google.common.base.MoreObjects;
3839
import com.google.common.base.Predicate;
3940
import com.google.common.base.Throwables;
@@ -146,6 +147,7 @@
146147
import java.util.regex.Pattern;
147148
import java.util.stream.Stream;
148149
import javax.annotation.Nullable;
150+
import javax.lang.model.element.Modifier;
149151
import javax.lang.model.element.Name;
150152

151153
/** An AST visitor that builds a stream of {@link Op}s to format from the given {@link CompilationUnitTree}. */
@@ -1420,8 +1422,11 @@ public Void visitMethod(MethodTree node, Void unused) {
14201422
}
14211423
}
14221424
}
1423-
builder.addAll(
1424-
visitModifiers(annotations, Direction.VERTICAL, /* declarationAnnotationBreak= */ Optional.empty()));
1425+
builder.addAll(visitModifiers(
1426+
annotations,
1427+
node.getModifiers().getFlags(),
1428+
Direction.VERTICAL,
1429+
/* declarationAnnotationBreak= */ Optional.empty()));
14251430

14261431
Tree baseReturnType = null;
14271432
Deque<List<? extends AnnotationTree>> dims = null;
@@ -2236,30 +2241,36 @@ protected List<Op> visitModifiers(
22362241
Optional<BreakTag> declarationAnnotationBreak) {
22372242
boolean isRecordParameter = modifiersTree instanceof JCTree.JCModifiers && isInRecord(modifiersTree);
22382243
return visitModifiers(
2239-
modifiersTree.getAnnotations(), annotationsDirection, declarationAnnotationBreak, isRecordParameter);
2244+
modifiersTree.getAnnotations(),
2245+
modifiersTree.getFlags(),
2246+
annotationsDirection,
2247+
declarationAnnotationBreak,
2248+
isRecordParameter);
22402249
}
22412250

22422251
private List<Op> visitModifiers(
22432252
List<? extends AnnotationTree> annotationTrees,
2253+
Set<Modifier> flags,
22442254
Direction annotationsDirection,
22452255
Optional<BreakTag> declarationAnnotationBreak) {
2246-
return visitModifiers(annotationTrees, annotationsDirection, declarationAnnotationBreak, false);
2256+
return visitModifiers(annotationTrees, flags, annotationsDirection, declarationAnnotationBreak, false);
22472257
}
22482258

22492259
private List<Op> visitModifiers(
22502260
List<? extends AnnotationTree> annotationTrees,
2261+
Set<Modifier> flags,
22512262
Direction annotationsDirection,
22522263
Optional<BreakTag> declarationAnnotationBreak,
22532264
boolean isRecordParameter) {
2254-
if (annotationTrees.isEmpty() && !nextIsModifier()) {
2265+
if (annotationTrees.isEmpty() && !nextIsModifier(flags)) {
22552266
return EMPTY_LIST;
22562267
}
22572268
Deque<AnnotationTree> annotations = new ArrayDeque<>(annotationTrees);
22582269
builder.open(ZERO);
22592270
boolean first = true;
22602271
boolean lastWasAnnotation = false;
22612272
while (!annotations.isEmpty()) {
2262-
if (nextIsModifier()) {
2273+
if (nextIsModifier(flags)) {
22632274
break;
22642275
}
22652276
if (!first) {
@@ -2286,7 +2297,7 @@ private List<Op> visitModifiers(
22862297
: annotationsDirection.isVertical()
22872298
? forceBreakList(declarationAnnotationBreak)
22882299
: breakList(declarationAnnotationBreak);
2289-
if (annotations.isEmpty() && !nextIsModifier()) {
2300+
if (annotations.isEmpty() && !nextIsModifier(flags)) {
22902301
return trailingBreak;
22912302
}
22922303
if (lastWasAnnotation) {
@@ -2295,11 +2306,11 @@ private List<Op> visitModifiers(
22952306

22962307
builder.open(ZERO);
22972308
first = true;
2298-
while (nextIsModifier() || !annotations.isEmpty()) {
2309+
while (nextIsModifier(flags) || !annotations.isEmpty()) {
22992310
if (!first) {
23002311
builder.addAll(breakFillList(Optional.empty()));
23012312
}
2302-
if (nextIsModifier()) {
2313+
if (nextIsModifier(flags)) {
23032314
String currentToken = builder.peekToken().get();
23042315
token(currentToken);
23052316
if (currentToken.equals("non")) {
@@ -2316,23 +2327,39 @@ private List<Op> visitModifiers(
23162327
return breakFillList(Optional.empty());
23172328
}
23182329

2319-
boolean nextIsModifier() {
2330+
/**
2331+
* The {@code value} modifier of Project Valhalla's value classes, on a JDK that has it. It is looked up by name so
2332+
* that the formatter still compiles for JDK 21.
2333+
*/
2334+
@Nullable
2335+
private static final Modifier VALUE_MODIFIER =
2336+
Enums.getIfPresent(Modifier.class, "VALUE").orNull();
2337+
2338+
/**
2339+
* Whether the next token is one of {@code flags}, the modifiers of the declaration being visited. The text alone
2340+
* does not tell the contextual modifiers {@code sealed} and {@code value} from identifiers of that name, and a
2341+
* lambda parameter named so starts its declaration with it.
2342+
*/
2343+
boolean nextIsModifier(Set<Modifier> flags) {
2344+
if (flags.isEmpty()) {
2345+
return false;
2346+
}
23202347
return switch (builder.peekToken().get()) {
2321-
case "public",
2322-
"protected",
2323-
"private",
2324-
"abstract",
2325-
"static",
2326-
"final",
2327-
"transient",
2328-
"volatile",
2329-
"synchronized",
2330-
"native",
2331-
"strictfp",
2332-
"default",
2333-
"sealed",
2334-
"non",
2335-
"-" -> true;
2348+
case "public" -> flags.contains(Modifier.PUBLIC);
2349+
case "protected" -> flags.contains(Modifier.PROTECTED);
2350+
case "private" -> flags.contains(Modifier.PRIVATE);
2351+
case "abstract" -> flags.contains(Modifier.ABSTRACT);
2352+
case "static" -> flags.contains(Modifier.STATIC);
2353+
case "final" -> flags.contains(Modifier.FINAL);
2354+
case "transient" -> flags.contains(Modifier.TRANSIENT);
2355+
case "volatile" -> flags.contains(Modifier.VOLATILE);
2356+
case "synchronized" -> flags.contains(Modifier.SYNCHRONIZED);
2357+
case "native" -> flags.contains(Modifier.NATIVE);
2358+
case "strictfp" -> flags.contains(Modifier.STRICTFP);
2359+
case "default" -> flags.contains(Modifier.DEFAULT);
2360+
case "sealed" -> flags.contains(Modifier.SEALED) || flags.contains(Modifier.NON_SEALED);
2361+
case "non", "-" -> flags.contains(Modifier.NON_SEALED);
2362+
case "value" -> VALUE_MODIFIER != null && flags.contains(VALUE_MODIFIER);
23362363
default -> false;
23372364
};
23382365
}

‎open-java-format/src/main/java/com/palantir/javaformat/java/ModifierOrderer.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import static com.google.common.base.Preconditions.checkState;
2020
import static com.google.common.collect.Iterables.getLast;
2121

22+
import com.google.common.base.Enums;
2223
import com.google.common.collect.ImmutableList;
2324
import com.google.common.collect.Ordering;
2425
import com.google.common.collect.Range;
@@ -201,7 +202,7 @@ private static Modifier asModifier(ImmutableList<Token> tokens) {
201202
if (modifier == null) {
202203
return null;
203204
}
204-
return Modifier.valueOf("NON_" + modifier.name());
205+
return Enums.getIfPresent(Modifier.class, "NON_" + modifier.name()).orNull();
205206
}
206207

207208
/**
@@ -254,6 +255,7 @@ private static Modifier asModifier(Token token) {
254255
}
255256
return switch (token.getTok().getText()) {
256257
case "sealed" -> Modifier.valueOf("SEALED");
258+
case "value" -> Enums.getIfPresent(Modifier.class, "VALUE").orNull();
257259
default -> null;
258260
};
259261
}

‎open-java-format/src/test/java/com/palantir/javaformat/java/FormatterTest.java‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,12 +34,16 @@
3434
import java.nio.file.Files;
3535
import java.nio.file.Path;
3636
import java.time.Duration;
37+
import java.util.Arrays;
3738
import java.util.List;
39+
import javax.lang.model.element.Modifier;
3840
import org.junit.jupiter.api.Assumptions;
3941
import org.junit.jupiter.api.Test;
4042
import org.junit.jupiter.api.io.TempDir;
4143
import org.junit.jupiter.api.parallel.Execution;
4244
import org.junit.jupiter.api.parallel.ExecutionMode;
45+
import org.junit.jupiter.params.ParameterizedTest;
46+
import org.junit.jupiter.params.provider.ValueSource;
4347

4448
@Execution(ExecutionMode.CONCURRENT)
4549
final class FormatterTest {
@@ -568,4 +572,68 @@ void removeTrailingTabsInComments() throws FormatterException {
568572

569573
assertThat(formatter.formatSource(input)).isEqualTo(expected);
570574
}
575+
576+
@Test
577+
void valueClassAndRecord() throws FormatterException {
578+
Assumptions.assumeTrue(
579+
Arrays.stream(Modifier.values())
580+
.anyMatch(modifier -> modifier.name().equals("VALUE")),
581+
"`value` is a modifier only on a JDK with Project Valhalla's value classes");
582+
String input = """
583+
value final public class PointValueClass {
584+
final int x;
585+
final int y;
586+
587+
PointValueClass(int x, int y) {
588+
this.x = x;
589+
this.y = y;
590+
}
591+
}
592+
593+
value public record PointValueRecord(int x, int y) {}
594+
""";
595+
String expected = """
596+
public final value class PointValueClass {
597+
final int x;
598+
final int y;
599+
600+
PointValueClass(int x, int y) {
601+
this.x = x;
602+
this.y = y;
603+
}
604+
}
605+
606+
public value record PointValueRecord(int x, int y) {}
607+
""";
608+
Formatter formatter = Formatter.createFormatter(
609+
JavaFormatterOptions.builder().style(Style.OJF).build());
610+
611+
assertThat(formatter.formatSource(input)).isEqualTo(expected);
612+
}
613+
614+
@ParameterizedTest
615+
@ValueSource(strings = {"non", "sealed", "value"})
616+
void contextualKeywordAsIdentifier(String name) throws FormatterException {
617+
// `value`, `sealed` and `non-sealed` are modifiers only in front of a declaration that carries them. Anywhere
618+
// else they are identifiers, and a lambda parameter is the one declaration that starts with its name.
619+
String input = """
620+
import java.util.Map;
621+
622+
class T {
623+
private final int NAME;
624+
625+
T(int NAME) {
626+
this.NAME = NAME;
627+
}
628+
629+
void f(Map<String, Integer> map) {
630+
map.forEach((key, NAME) -> System.err.println(key + NAME));
631+
}
632+
}
633+
""".replace("NAME", name);
634+
Formatter formatter = Formatter.createFormatter(
635+
JavaFormatterOptions.builder().style(Style.OJF).build());
636+
637+
assertThat(formatter.formatSource(input)).isEqualTo(input);
638+
}
571639
}

0 commit comments

Comments
 (0)