Fix handling of annotations in compact record constructors

#574. PiperOrigin-RevId: 367902654
google · Apr 12, 2021 · 4c0c23b · 4c0c23b
1 parent 5defb58
commit 4c0c23b
Show file tree

Hide file tree

Showing 7 changed files with 48 additions and 23 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -29,19 +29,19 @@ jobs:
       fail-fast: false
       matrix:
         os: [ ubuntu-latest ]
-        java: [ 15, 11 ]
+        java: [ 16, 14, 11 ]
         experimental: [ false ]
         include:
           # Only test on macos and windows with a single recent JDK to avoid a
           # combinatorial explosion of test configurations.
           - os: macos-latest
-            java: 15
+            java: 16
             experimental: false
           - os: windows-latest
-            java: 15
+            java: 16
             experimental: false
           - os: ubuntu-latest
-            java: 16-ea
+            java: 17-ea
             experimental: true
     runs-on: ${{ matrix.os }}
     continue-on-error: ${{ matrix.experimental }}

diff --git a/core/src/main/java/com/google/googlejavaformat/java/java14/Java14InputAstVisitor.java b/core/src/main/java/com/google/googlejavaformat/java/java14/Java14InputAstVisitor.java
@@ -15,7 +15,6 @@
 package com.google.googlejavaformat.java.java14;
 
 import static com.google.common.collect.ImmutableList.toImmutableList;
-import static com.google.common.collect.MoreCollectors.toOptional;
 
 import com.google.common.base.Verify;
 import com.google.common.collect.ImmutableList;
@@ -27,13 +26,13 @@
 import com.sun.source.tree.ClassTree;
 import com.sun.source.tree.ExpressionTree;
 import com.sun.source.tree.InstanceOfTree;
+import com.sun.source.tree.ModifiersTree;
 import com.sun.source.tree.SwitchExpressionTree;
 import com.sun.source.tree.Tree;
 import com.sun.source.tree.VariableTree;
 import com.sun.source.tree.YieldTree;
 import com.sun.tools.javac.code.Flags;
 import com.sun.tools.javac.tree.JCTree;
-import com.sun.tools.javac.tree.JCTree.JCMethodDecl;
 import com.sun.tools.javac.tree.JCTree.JCVariableDecl;
 import com.sun.tools.javac.tree.TreeInfo;
 import java.util.List;
@@ -56,12 +55,13 @@ public Void visitBindingPattern(BindingPatternTree node, Void unused) {
     try {
       VariableTree variableTree =
           (VariableTree) BindingPatternTree.class.getMethod("getVariable").invoke(node);
-      visitBindingPattern(variableTree.getType(), variableTree.getName());
+      visitBindingPattern(
+          variableTree.getModifiers(), variableTree.getType(), variableTree.getName());
     } catch (ReflectiveOperationException e1) {
       try {
         Tree type = (Tree) BindingPatternTree.class.getMethod("getType").invoke(node);
         Name name = (Name) BindingPatternTree.class.getMethod("getName").invoke(node);
-        visitBindingPattern(type, name);
+        visitBindingPattern(/* modifiers= */ null, type, name);
       } catch (ReflectiveOperationException e2) {
         e2.addSuppressed(e1);
         throw new LinkageError(e2.getMessage(), e2);
@@ -70,7 +70,10 @@ public Void visitBindingPattern(BindingPatternTree node, Void unused) {
     return null;
   }
 
-  private void visitBindingPattern(Tree type, Name name) {
+  private void visitBindingPattern(ModifiersTree modifiers, Tree type, Name name) {
+    if (modifiers != null) {
+      builder.addAll(visitModifiers(modifiers, Direction.HORIZONTAL, Optional.empty()));
+    }
     scan(type, null);
     builder.breakOp(" ");
     visit(name);
@@ -136,10 +139,7 @@ public void visitRecordDeclaration(ClassTree node) {
       if (!node.getTypeParameters().isEmpty()) {
         typeParametersRest(node.getTypeParameters(), hasSuperInterfaceTypes ? plusFour : ZERO);
       }
-      ImmutableList<JCVariableDecl> parameters =
-          compactRecordConstructor(node)
-              .map(m -> ImmutableList.copyOf(m.getParameters()))
-              .orElseGet(() -> recordVariables(node));
+      ImmutableList<JCVariableDecl> parameters = recordVariables(node);
       token("(");
       if (!parameters.isEmpty()) {
         // Break before args.
@@ -178,14 +178,6 @@ public void visitRecordDeclaration(ClassTree node) {
     dropEmptyDeclarations();
   }
 
-  private static Optional<JCMethodDecl> compactRecordConstructor(ClassTree node) {
-    return node.getMembers().stream()
-        .filter(JCMethodDecl.class::isInstance)
-        .map(JCMethodDecl.class::cast)
-        .filter(m -> (m.mods.flags & COMPACT_RECORD_CONSTRUCTOR) == COMPACT_RECORD_CONSTRUCTOR)
-        .collect(toOptional());
-  }
-
   private static ImmutableList<JCVariableDecl> recordVariables(ClassTree node) {
     return node.getMembers().stream()
         .filter(JCVariableDecl.class::isInstance)

diff --git a/core/src/test/java/com/google/googlejavaformat/java/FormatterIntegrationTest.java b/core/src/test/java/com/google/googlejavaformat/java/FormatterIntegrationTest.java
@@ -48,7 +48,9 @@
 public class FormatterIntegrationTest {
 
   private static final ImmutableSet<String> JAVA14_TESTS =
-      ImmutableSet.of("I477", "Records", "RSLs", "Var", "ExpressionSwitch");
+      ImmutableSet.of("I477", "Records", "RSLs", "Var", "ExpressionSwitch", "I574");
+
+  private static final ImmutableSet<String> JAVA16_TESTS = ImmutableSet.of("I588");
 
   @Parameters(name = "{index}: {0}")
   public static Iterable<Object[]> data() throws IOException {
@@ -76,7 +78,7 @@ public static Iterable<Object[]> data() throws IOException {
           case "output":
             outputs.put(baseName, contents);
             break;
-          default:
+          default: // fall out
         }
       }
     }
@@ -90,6 +92,9 @@ public static Iterable<Object[]> data() throws IOException {
       if (JAVA14_TESTS.contains(fileName) && getMajor() < 14) {
         continue;
       }
+      if (JAVA16_TESTS.contains(fileName) && getMajor() < 16) {
+        continue;
+      }
       testInputs.add(new Object[] {fileName, input, expectedOutput});
     }
     return testInputs;

diff --git a/core/src/test/resources/com/google/googlejavaformat/java/testdata/I574.input b/core/src/test/resources/com/google/googlejavaformat/java/testdata/I574.input
@@ -0,0 +1,6 @@
+public record Record(@NotNull Object o) {
+
+  public Record {
+    this.o = o;
+  }
+}
diff --git a/core/src/test/resources/com/google/googlejavaformat/java/testdata/I574.output b/core/src/test/resources/com/google/googlejavaformat/java/testdata/I574.output
@@ -0,0 +1,6 @@
+public record Record(@NotNull Object o) {
+
+  public Record {
+    this.o = o;
+  }
+}
diff --git a/core/src/test/resources/com/google/googlejavaformat/java/testdata/I588.input b/core/src/test/resources/com/google/googlejavaformat/java/testdata/I588.input
@@ -0,0 +1,8 @@
+class T {
+  int f(Object x) {
+    if (x instanceof final Integer i) {
+      return i;
+    }
+    return -1;
+  }
+}
diff --git a/core/src/test/resources/com/google/googlejavaformat/java/testdata/I588.output b/core/src/test/resources/com/google/googlejavaformat/java/testdata/I588.output
@@ -0,0 +1,8 @@
+class T {
+  int f(Object x) {
+    if (x instanceof final Integer i) {
+      return i;
+    }
+    return -1;
+  }
+}