Add support for java 14 language features #365

fawind · 2020-11-16T12:02:10Z

After this PR

Closes #347

Cherry-picking relevant commits from google-java-format and adapting them for pjf.

Changes:

Use build in jdk javac instead of errorprone javac.
- Because of this, the source and target compatibility is increased to 11.
- That's also the reason for all the import changes.
Add initial support for switch-statements, var, yield, records, text blocks.

==COMMIT_MSG==
Add support for java 14 language features
==COMMIT_MSG==

Possible downsides?

Dropping jdk 8 support.

changelog-app · 2020-11-16T12:02:15Z

Generate changelog in `changelog/@unreleased`

Type

Description

Add support for java 14 language features.
Break: Increase minimum source compatibility from java 8 to java 11.
- To use version 2.x of the formatter, you need to run on java 11 or higher.
- If you require the formatter to run on java 8, you won't be able to upgrade past version 1.x. Note that you can still produce java 8 jars from your repo but need to use at least java 11 to run the formatter.

Check the box to generate changelog(s)

Generate changelog entry

debugger/package-lock.json

iamdanfox · 2020-11-16T14:13:18Z

palantir-java-format/build.gradle

+    ]
+
+    if (JavaVersion.current() < JavaVersion.VERSION_14) {
+        excludes = ['**/Java14InputAstVisitor.java']


oooh nice, i was wondering how painful it was gonna be to do some kinda of dual compilation here, or if were going to have to do some multi-release jar thing!

One caveat with this approach is I think gradle has some new functionality where you can specify the JDK to use for these compile tasks independently from the JDK used to evaluate gradle, and JavaVersion.current() will grab the gradle evaluating one

palantir-java-format/src/test/java/com/palantir/javaformat/java/FileBasedTests.java

iamdanfox · 2020-11-16T14:16:36Z

...java-format/src/test/resources/com/palantir/javaformat/java/testdata/ExpressionSwitch.output

+            case 0:
+                yield 0;
+            default:
+                yield n / i;


woah this is a funky keyword... ~~based on the reading of https://www.baeldung.com/java-thread-yield it sounds like not something that we expect palantir codebases to contain!~~

nvm Thread.yield and yield keyword are totally different... looks like this one is just a siwtch-expression specific return!

palantir-java-format/src/main/java/com/palantir/javaformat/java/Formatter.java

build.gradle

iamdanfox · 2020-11-16T14:31:10Z

...tir-java-format/src/main/java/com/palantir/javaformat/java/java14/Java14InputAstVisitor.java

+/**
+ * Extends {@link JavaInputAstVisitor} with support for AST nodes that were added or modified for Java 14.
+ */
+public class Java14InputAstVisitor extends JavaInputAstVisitor {


I'm wondering what the longer-term implications of this pattern are - e.g. java 15 introduces a teeny bit more syntax right (specifically the sealed keyword I think)... would the idea be to have one of these classes for each of the java versions we support compiling with? and then at some point take a hard stance and increment our required min version, collapsing some of these classes into one?

iamdanfox · 2020-11-16T23:06:54Z

.circleci/template.sh

-export PRIMARY_BRANCH=develop
+export ONLY_11=true
+export UNIT_TEST_15=true
+export UNIT_TEST_14=false


I think for the final publish step, it's important the the java compiler is java 14 to ensure we don't hit the exclusion below, as we definitely want that class to appear in the final jar!

if (JavaVersion.current() < JavaVersion.VERSION_14) { excludes = ['**/Java14InputAstVisitor.java'] }

Maybe we have to diverge from the CircleCI template?

Or maybe JDK=14 / JDK=15 would be enough? Seems like we do want. the entirely independent full ./gradlew build pipelines on both java11, java14 and maybe other higher versions too? (i.e. effectively our unit-test-11/unit-test-15 thingies I think).

iamdanfox · 2020-11-16T23:15:12Z

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

@@ -3396,7 +3412,10 @@ int declareOne(
            if (modifiers.isPresent()) {
                visitAndBreakModifiers(modifiers.get(), annotationsDirection, Optional.of(verticalAnnotationBreak));
            }
-            builder.open(type != null ? plusFour : ZERO);
+            boolean isVar = builder.peekToken().get().equals("var")


ok so this is the first block where we diverge from the google-java-format version? google/google-java-format@4ddb914#diff-0c133fd389bc6f207e146ba08dbc50dd78aa088ff31cdc56cb14ebb31e97ec49R158

EDIT nvm seems like we're up to date with their current impl, not their initial PR

https://github.com/google/google-java-format/blob/93d649d0fbc07fc604f7b47b3098cf6e1df521de/core/src/main/java/com/google/googlejavaformat/java/JavaInputAstVisitor.java#L3287-L3291.

It's kinda interesting because they're using the peekToken and doing a string comparison to get away with supporting java13+ features here in this plain JavaInputAstVisitor, rather than making another verbose Java13InputAstVisitor.

I put a debugger on this, and it seems like the call stack of visitEnhancedForLoop -> visitToDeclare -> declareOne, we lost the node variable which could give us a first-class, typed way of detecting this rather than just relying on string comparisons.

I believe it would just be a matter of calling the JCVariableDecl#isImplicitlyTyped() method, and that would tell us we're looking at a var declaration. Not 100% sure if it's worth diverging from their implementation in order to plumb this down though, as it might make future maintenance more annoying, despite slightly cleaner code

Happy to make the change but I would probably stick to the original impl if possible. That way we can cherry-pick new features and fixes instead of reimplementing them which will likely save us quite a bit of work down the road.

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

iamdanfox · 2020-11-16T23:30:15Z

...tir-java-format/src/main/java/com/palantir/javaformat/java/java14/Java14InputAstVisitor.java

+        scan(node.getValue(), null);
+        token(";");
+        return null;
+    }


amusing that they missed the yield keyword in their original g-j-f PR: google/google-java-format@4ddb914#diff-8dbec381ea929cc5907d0ae86ac47ce873aa4ed4ad9337ce85db28642b1dd67fR59-R63. Confirming this now matches the current state! https://github.com/google/google-java-format/blob/93d649d0fbc07fc604f7b47b3098cf6e1df521de/core/src/main/java/com/google/googlejavaformat/java/java14/Java14InputAstVisitor.java#L60-L68

iamdanfox · 2020-11-16T23:33:17Z

...tir-java-format/src/main/java/com/palantir/javaformat/java/java14/Java14InputAstVisitor.java

+        return null;
+    }
+
+    public void visitRecordDeclaration(ClassTree node) {


this is pretty hard to read, but it looks like at least it matches the current g-j-f impl, so at least we'll be able to stay in sync. https://github.com/google/google-java-format/blob/93d649d0fbc07fc604f7b47b3098cf6e1df521de/core/src/main/java/com/google/googlejavaformat/java/java14/Java14InputAstVisitor.java#L99-L160

iamdanfox · 2020-11-16T23:36:06Z

...tir-java-format/src/main/java/com/palantir/javaformat/java/java14/Java14InputAstVisitor.java

+            }
+        }
+        switch (node.getCaseKind()) {
+            case STATEMENT:


looks like this branch diverges slightly - what was the motivation here?

https://github.com/google/google-java-format/blob/93d649d0fbc07fc604f7b47b3098cf6e1df521de/core/src/main/java/com/google/googlejavaformat/java/java14/Java14InputAstVisitor.java#L218-L223

The pjf visitStatement method as a different signature to support inlining statements:

pfj:

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

Line 2173 in 8552645

protected void visitStatements(List<? extends StatementTree> statements, boolean inlineFirst) {

gjf: https://github.com/google/google-java-format/blob/6bde01e0880586518efad4cc5fd5a00916f41bb5/core/src/main/java/com/google/googlejavaformat/java/JavaInputAstVisitor.java#L2148

The code used here is consistent to how we handle case statements in the non-j14 visitor:

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

Lines 1832 to 1838 in 8552645

boolean isBlock =

node.getStatements().size() == 1 && node.getStatements().get(0).getKind() == BLOCK;

builder.open(isBlock ? ZERO : plusTwo);

if (isBlock) {

builder.space();

}

visitStatements(node.getStatements(), isBlock);

iamdanfox · 2020-11-16T23:59:41Z

Thanks for burning through all the thorny gradle bits to make this actually work in our setup, I'm pretty psyched to actually use these new preview features :)

I think there's the publishing thing to fix (needs a jdk14 in the docker container to compile with), then I think you should probably cut a release candidate here, and just see if there are any additional changes needed in baseline at all for this to be useable. Probably worth making your own version of that devx/template-witchcraft#1414 PR, and seeing how many hacks are strictly necessary!

* WIP * cleanup * Make IntelliJ work out of the box * Fix npe on jdk 11 Co-authored-by: fwindheuser <fwindheuser@palantir.com>

iamdanfox · 2020-11-23T15:08:12Z

.circleci/config.yml

      - save_cache:
          key: 'trial-publish-gradle-cache-v2-{{ checksum "versions.props" }}-{{ checksum "build.gradle" }}'
          paths: [ ~/.gradle/caches ]
      - store_test_results: { path: ~/junit }
      - store_artifacts: { path: ~/artifacts }

  publish:
-    docker: [{ image: 'circleci/openjdk:8u222-stretch-node' }]
+    docker: [{ image: 'circleci/openjdk:15-jdk-buster-node' }]


nice, so we're publishing with java 15, but unit-test-11 should ensure that older codepaths still work :)

iamdanfox

Looks great to me! Might be worth fleshing out the implications of the sourceCompat 11 thing a little bit more in the changelog though, kinda to explain that palantir-java-format 2.X can only run on java11+ now (which is LTS). It's OK run the formatter on java11, but still produce java8 jars from your repo, but if you want to run the formatter on java8 you won't be able to upgrade past 1.X

fawind · 2020-11-24T12:36:34Z

👍

fawind added 17 commits November 11, 2020 22:48

Use build in jdk 11 javac

7ae557d

Update circle template to java 11

ac6f8c8

Use latest supported language level

f96c518

Use option file for javadoc

cd8e734

Set compiler args for idea

6dc3a13

<WIP> java 14 support

2d0a63d

Update spotless

afef12f

Add initial support for java 14 language features

895fe2a

Fix switch statement format

af12de4

Exclude java 14 ast visitor from compilation when not run with j14

0dfe621

Fix spotless integration test

450bf09

Add java 15 tests

2c05467

Fix formatting records without an explicit constructor

8552645

Add support for yield statement

20bf0f5

Gradle clean up

da05d7e

Merge remote-tracking branch 'origin/develop' into fw/java14

ae87333

Add comments

d0051fe

fawind requested a review from iamdanfox November 16, 2020 12:02