Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Annotation location when mixed with modifiers #1152

Open
Zopsss opened this issue Aug 28, 2024 · 2 comments
Open

Annotation location when mixed with modifiers #1152

Zopsss opened this issue Aug 28, 2024 · 2 comments

Comments

@Zopsss
Copy link

Zopsss commented Aug 28, 2024

When annotations are mixed with modifiers, formatter formats the code in following way:

$ cat TestingModifiers.java
/** some javadoc. */
public abstract class TestingModifiers {
  abstract @MyAnnotation2 public void fooMet1();

  final strictfp synchronized protected @MyAnnotation2 void fooMethod7() {}

  synchronized final strictfp @MyAnnotation2 public void fooMethod5() {}

  @MyAnnotation2 public static @MyAnnotation4 strictfp void someMethod5() {}

  strictfp protected final @MyAnnotation2 static synchronized void fooMethod1() {}

  native synchronized protected static @MyAnnotation2 void fooMethod3();
}

$ java -jar google-java-format-1.23.0-all-deps.jar TestingModifiers.java > TestingModifiersUpdated.java 

$ cat TestingModifiersUpdated.java 
/** some javadoc. */
public abstract class TestingModifiers {
  abstract @MyAnnotation2 public void fooMet1();

  protected final synchronized strictfp @MyAnnotation2 void fooMethod7() {}

  final synchronized strictfp @MyAnnotation2 public void fooMethod5() {}

  @MyAnnotation2
  public static @MyAnnotation4 strictfp void someMethod5() {}

  protected final strictfp @MyAnnotation2 static synchronized void fooMethod1() {}

  protected static synchronized native @MyAnnotation2 void fooMethod3();
}

$ diff -u TestingModifiers.java TestingModifiersUpdated.java 
--- TestingModifiers.java       2024-08-28 11:00:02.129632600 +0530
+++ TestingModifiersUpdated.java        2024-08-28 11:01:40.748988100 +0530
@@ -2,13 +2,14 @@
 public abstract class TestingModifiers {
   abstract @MyAnnotation2 public void fooMet1();

-  final strictfp synchronized protected @MyAnnotation2 void fooMethod7() {}
+  protected final synchronized strictfp @MyAnnotation2 void fooMethod7() {}

-  synchronized final strictfp @MyAnnotation2 public void fooMethod5() {}
+  final synchronized strictfp @MyAnnotation2 public void fooMethod5() {}

-  @MyAnnotation2 public static @MyAnnotation4 strictfp void someMethod5() {}
+  @MyAnnotation2
+  public static @MyAnnotation4 strictfp void someMethod5() {}

-  strictfp protected final @MyAnnotation2 static synchronized void fooMethod1() {}
+  protected final strictfp @MyAnnotation2 static synchronized void fooMethod1() {}

-  native synchronized protected static @MyAnnotation2 void fooMethod3();
+  protected static synchronized native @MyAnnotation2 void fooMethod3();
}

From 4.8.7 Modifiers

Class and member modifiers, when present, appear in the order recommended by the Java Language Specification:

public protected private abstract default static final transient volatile synchronized native strictfp

There's not explicitly mentioned anything about position of annotations when mixed with modifiers or annotations should not be mixed with modifiers or something similar to that in above rule.

Also, in 4.8.5 Annotations, there's not explicitly mentioned that how annotations should be positioned when used with modifiers. But all sub-sections mostly refers to:

Annotations applying to a class appear immediately after the documentation block, and each annotation is listed on a line of its own (that is, one annotation per line).

Attention to:

immediately after the documentation block

This kind of implies that annotations should be placed before any modifiers. But formatter does not places annotations before modifiers as shown in above example. So if annotations should not be mixed with modifiers and placed at the beginning or in a separate line then formatter should be updated and format the code according to this.

@kevinb9n
Copy link
Contributor

I see the style guide as supporting your point pretty unambiguously, actually. Every annotation should go either before all the modifiers (declaration annotations) or after them (type-use annotations). (If an annotation is both kinds, knock yourselves out ...)

But I'm guessing that #5 is the main problem here. Without being able to know which kind of annotation it is, our moving it could just make it even "more wrong".

@cushon
Copy link
Collaborator

cushon commented Sep 10, 2024

But I'm guessing that #5 is the main problem here. Without being able to know which kind of annotation it is, our moving it could just make it even "more wrong".

That's part of it, although the formatter now has heuristics for some well-known type annotations.

The other part is that the formatter mostly doesn't make non-whitespace changes, i.e. it only adds or removes whitespace and line breaks. There are some FRs that haven't been implemented for that reason here.

The support for reordering modifiers is a special case. It looks at the existing positions of the modifiers, sorts the modifiers, and adds them back at the original positions.

There's an Error Prone check that does do this kind of refactoring (AnnotationPosition).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants
@cushon @kevinb9n @Zopsss and others