Support sealed classes #629

now · 2021-07-23T21:12:38Z

This implements support for JEP 409: Sealed Classes, see
https://openjdk.java.net/jeps/409.

The implementation handles the sealed and non-sealed keywords and sorts them
according to their JLS order. This requires us to look at the actual text of
the Tok, as there’s no TokenKind for sealed and non-sealed.

The optional permits clause is handled in the same manner as the implements
clause. A new private method, classDeclarationTypeList, has been added to
facilitate this.

This fixes #603.

I implemented this in a manner that I deemed fit with the surrounding code, but if I missed something, don’t hesitate to get back to me and I’ll adjust this PR to suit.

This implements support for JEP 409: Sealed Classes, see https://openjdk.java.net/jeps/409. The implementation handles the sealed and non-sealed keywords and sorts them according to their JLS order. This requires us to look at the actual text of the Tok, as there’s no TokenKind for sealed and non-sealed. The optional permits clause is handled in the same manner as the implements clause. A new private method, classDeclarationTypeList, has been added to facilitate this. This fixes google#603.

now · 2021-07-24T06:49:44Z

I just realized that I didn’t consider backwards compatibility at all. I’ll add a Java15InputAstVisitor and handle it there.

The previous commit didn’t consider backwards compatibility with regards to compilation on a pre-Java 15 JDK. This patch breaks out all Java 15-specific code to the com.google.googlejavaformat.java.java15 package. This change requires us to make ModifierOrderer non-final and some of its methods non-static so that we can override asModifier to handle sealed and non-sealed in the new Java15ModifierOrderer. The new Java15InputAstVisitor overrides a new protected method, getPermitsClause, that reflectively invokes getPermitsClause. Extracting this method seems to make the most sense, as visitClassDeclaration is quite complex and overriding it as a whole didn’t seem like a good idea. Finally, creation of the InputAstVisitor was simplified, as creating it via reflection for Java14 and Java15 isn’t necessary, as Java14InputAstVisitor and Java15InputAstVisitor are built on earlier JDKs anyway.

cushon

Thanks for the contribution!

cushon · 2021-07-25T18:26:44Z

core/src/main/java/com/google/googlejavaformat/java/Formatter.java

-    } else {
-      visitor = new JavaInputAstVisitor(builder, options.indentationMultiplier());
-    }
+    if (getMajor() >= 15)


Our style guide requires optional braces (https://google.github.io/styleguide/javaguide.html#s4.1-braces), e.g.:

-if (getMajor() >= 15) +if (getMajor() >= 15) {

Do you mind adding those, here and below? I can take care of it when merging the PR if that's easier.

I can fix it, but I must first ask why google-java-format doesn’t format the source like this. I ran all sources through google-java-format 1.10.0 before committing them. Is it intended behavior to skip braces when there’s only one statement, or is that a bug in google-java-format?

(I see after running some tests of this that braces aren’t added, but will be retained if they’re there. So this is intended behavior? But I still wonder why the braces aren’t added if they’re missing.)

I just realized that I messed this up in another way. I forgot to check for any exclusion rules in core/pom.xml and I see that Java14InputAstVisitor.java is being excluded there. I’ll restore the code to load the visitor via reflection and make sure to set up the proper profile in core/pom.xml to handle Java15InputAstVisitor.java as well.

I must first ask why google-java-format doesn’t format the source like this

See #51. The formatter's job is primarily to fix issues with whitespace, there are more rules in the style guide than just whitespace

I hadn’t realized that. I’ll look into fixing #51 next ;-P.

cushon · 2021-07-25T18:30:36Z

core/src/main/java/com/google/googlejavaformat/java/java15/Java15ModifierOrderer.java

+  protected Modifier asModifier(Token token) {
+    Modifier m = super.asModifier(token);
+    if (m != null) return m;
+    else if (token.getTok().getText().equals("sealed")) return Modifier.valueOf("SEALED");


What do you think about putting this directly in ModifierOrderer? Checking the text of the modifier token and using Modifier.valueOf should still compile with earlier language levels. The main reason to have separate classes like Java14InputAstVisitor is to allow references APIs only available at the language level, like new AST nodes.

Sorry, I should have mentioned my reasoning in my commit. The thing that speaks against doing it that way is that Modifier.valueOf will throw IllegalArgumentException if given an unknown name, which will be the case for Java 14 and earlier. I figured that this was a bit cleaner, as it avoids handling the potential IllegalArgumentException and also avoids handling (and dismissing) an exception for Java 15 and later, should there have been a typo in the source, "SELAED", for example.

If there's an invalid modifier it should fail to parse and this code will never be executed, so I think it's fine to have it always try to handle sealed and non-sealed

Ah, of course.

cushon · 2021-07-25T18:31:23Z

core/src/test/java/com/google/googlejavaformat/java/FormatterIntegrationTest.java

@@ -50,7 +50,7 @@
  private static final ImmutableSet<String> JAVA14_TESTS =
      ImmutableSet.of("I477", "Records", "RSLs", "Var", "ExpressionSwitch", "I574", "I594");

-  private static final ImmutableSet<String> JAVA16_TESTS = ImmutableSet.of("I588");
+  private static final ImmutableSet<String> JAVA16_TESTS = ImmutableSet.of("I588", "I603");


Can you add a JAVA15_TESTS entry for the new tests?

That makes sense, I’ll fix that.

cushon · 2021-07-25T18:32:47Z

core/src/test/resources/com/google/googlejavaformat/java/testdata/I603.input

@@ -0,0 +1 @@
+sealed abstract class T permits D, E {}


Maybe add a couple more test cases, like one that has both a permits clause and extends or implements, and one where the signature doesn't all fit on one line?

Yes, that makes sense. It just struck me now that these tests will fail if not run on a JDK 15 or later. Should I handle this in core/pom.xml and exclude them if that’s the case?

The logic in FormatterIntegrationTest.java will ensure these tests are only run on JDK >= 15. They aren't compiled by maven it just sees them as resources. I don't think an exclusion in the pom is necessary.

Yes, that’s right, my mistake.

Exclude Java15InputAstVisitor if we’re running on a JDK earlier than 15. Invoke getPermitsClause() directly. Load JAva15InputAstVisitor via reflection. Handle sealed and non-sealed modifers in ModifierOrderer directly. Add JAVA15_TESTS that consists of I603 for now. Add a couple more tests that checks that long lines are handled correctly, along with multiple permitted classes over multiple lines following implemented interfaces.

now · 2021-07-26T14:01:19Z

It’s unfortunate that excludePackageNames is a String and not a String[]. The profile stuff gets a bit messy because of it.

I think I got everything set up correctly now, but if there’s still improvements to be made, I’ll gladly make them too :-).

If everything is A-OK, how would you like the commits? Should I squash everything and rewrite the commit message? Would you prefer that I squash it and break it up into smaller commits afterwards?

cushon

Thanks for the changes! This LGTM aside from two more nits

If everything is A-OK, how would you like the commits? Should I squash everything and rewrite the commit message? Would you prefer that I squash it and break it up into smaller commits afterwards?

It'll get squashed when I merge it, the description in the PR message looks good as a commit message, I'll plan on using that as the message for the squashed commit

cushon · 2021-07-26T17:37:46Z

core/src/main/java/com/google/googlejavaformat/java/java15/Java15InputAstVisitor.java

+  @Override
+  protected List<? extends Tree> getPermitsClause(ClassTree node) {
+    return node.getPermitsClause();
+  }


One more thought here: we're going to need to add more version-specific visitors eventually, especially if there are new visit* methods for new AST nodes. However for cases where there's a new getter on an existing node, we're already using reflection in a couple of places, e.g. here:

google-java-format/core/src/main/java/com/google/googlejavaformat/java/java14/Java14InputAstVisitor.java

Line 60 in bf5ef1c

(ModuleTree) CompilationUnitTree.class.getMethod("getModule").invoke(node);

I wonder if it'd be simpler to call getPermitsClause reflectively and defer adding a new visitor until there are new AST nodes, and to also clean up some of the reflection in Java14InputAstVisitor when that happens?

This is fairly subjective and I don't mind making that change when I import it, but let me know if you have any input on that.

I didn’t quite understand why these methods were being invoked reflectively in Java14InputAsVisitor to begin with. The code is only compiled in a version that should have it. Is it to handle the potential case of a preview feature getting changed/dropped in a future Java release? Regarding getPermitsClause, that’s the final API in Java 17, as far as I can tell. I don’t mind having it invoked reflectively in Java15InputAstVisitor, but I’d like to know the reason behind it (sorry, I should have asked earlier).

Or do you mean that we should perhaps drop Java15InputAstVisitor altogether and do the reflective call in JavaInputAstVisitor instead? I’m fine with that too. That’d simplify pom.xml and Formatter. I implemented it this way to keep version-specific code out of JavaInputAstVisitor as much as possible, but perhaps this minor thing isn’t worth the effort.

Or do you mean that we should perhaps drop Java15InputAstVisitor altogether and do the reflective call in JavaInputAstVisitor instead?

Thanks, yes, I think I'd slightly prefer to drop Java15InputAstVisitor and use reflection in either JavaInputAstVisitor or Java14InputAstVisitor.

getModule is invoked reflectively because that method was added to CompilationUnitTree in JDK 17. In general the uses of reflection in Java14InputAstVisitor should all be for APIs that aren't the same for all versions >= 14 (because they were preview APIs that changed, or they were added in versions after 14).

My tentative plan for supporting future language versions is to introduce new JavaNInputAstVisitor classes for major versions that add new AST nodes and visit* methods, but to not necessarily add a new Visitor class for major versions if the only changes were in getters and it's not too unwieldy to use reflection.

cushon · 2021-07-26T17:40:27Z

core/src/main/java/com/google/googlejavaformat/java/ModifierOrderer.java

@@ -152,7 +152,15 @@ private static void addTrivia(StringBuilder replacement, ImmutableList<? extends
   * is not a modifier.
   */
  private static Modifier asModifier(Token token) {
-    return getModifier(((JavaInput.Tok) token.getTok()).kind());


WDYT about putting this in the default: in getModifier? This is currently the only call of getModifier, but putting the logic there would prevent a bug if another call of getModifier is added.

Also I'd consider using a switch instead of the if/else chain:

switch (token.getTok().getText()) { case "sealed": return Modifier.valueOf("SEALED");

I didn’t do it like that initially, as getModifier takes a TokenKind (and returns early if that’s null) and I didn’t want to change the signature or the null check, but I agree that it’s not optimal. Would it be OK if I inline getModifier in asModifier and handle both cases using switches there? (I agree that a switch is much better than an if/else chain.)

Inlining getModifier SGTM

This commit moves the relevant code in Java15InputAstVisitor to Java14InputAstVisitor for possible future cleanup. This avoids a lot of seremony in pom.xml. ModifierOrderer is cleaned up a bit by inlining getModifier() in asModifier() and using a switch instead for checking for sealed and non-sealed.

now · 2021-07-27T10:56:15Z

I think that that should do it.

One more question: It seems that there’s always an empty line after a class or interface declaration in this project, but I can’t find that in the style guide. I don’t mind it being there, I’m only wondering what the reasoning for having it is.

cushon · 2021-07-27T17:57:30Z

One more question: It seems that there’s always an empty line after a class or interface declaration in this project, but I can’t find that in the style guide. I don’t mind it being there, I’m only wondering what the reasoning for having it is.

The style guide doesn't have a rule for that, there's some related discussion in #318 (comment) and #476 (comment)

This implements support for JEP 409: Sealed Classes, see https://openjdk.java.net/jeps/409. The implementation handles the sealed and non-sealed keywords and sorts them according to their JLS order. This requires us to look at the actual text of the Tok, as there’s no TokenKind for sealed and non-sealed. The optional permits clause is handled in the same manner as the implements clause. A new private method, classDeclarationTypeList, has been added to facilitate this. This fixes #603. I implemented this in a manner that I deemed fit with the surrounding code, but if I missed something, don’t hesitate to get back to me and I’ll adjust this PR to suit. Fixes #629 COPYBARA_INTEGRATE_REVIEW=google/google-java-format#629 from now:I603 4825e3310e00a1145304d64b4b73e9d3d03d08b3 PiperOrigin-RevId: 387160750

google-cla bot added the cla: yes label Jul 23, 2021

cushon requested changes Jul 25, 2021

View reviewed changes

cushon reviewed Jul 26, 2021

View reviewed changes

cushon approved these changes Jul 27, 2021

View reviewed changes

copybara-service bot closed this in cc8b96f Jul 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support sealed classes #629

Support sealed classes #629

now commented Jul 23, 2021

now commented Jul 24, 2021

cushon left a comment

cushon Jul 25, 2021

now Jul 25, 2021

now Jul 25, 2021

cushon Jul 25, 2021

now Jul 26, 2021

cushon Jul 25, 2021

now Jul 25, 2021

cushon Jul 25, 2021

now Jul 26, 2021

cushon Jul 25, 2021

now Jul 25, 2021

cushon Jul 25, 2021

now Jul 25, 2021

cushon Jul 25, 2021

now Jul 26, 2021

now commented Jul 26, 2021 •

edited

Loading

cushon left a comment

cushon Jul 26, 2021

now Jul 26, 2021

cushon Jul 26, 2021

cushon Jul 26, 2021

now Jul 26, 2021 •

edited

Loading

cushon Jul 26, 2021

now commented Jul 27, 2021

cushon commented Jul 27, 2021

Support sealed classes #629

Support sealed classes #629

Conversation

now commented Jul 23, 2021

now commented Jul 24, 2021

cushon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

now commented Jul 26, 2021 • edited Loading

cushon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

now Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

now commented Jul 27, 2021

cushon commented Jul 27, 2021

now commented Jul 26, 2021 •

edited

Loading

now Jul 26, 2021 •

edited

Loading