Handle type annotations #5

cushon · 2015-05-16T03:20:28Z

We always want to format type annotations in-line:

@Nullable Object foo() {}

This is tricky in contexts where type annotations could be mixed together with declaration annotations, such as field, method, and variable declarations. There's no syntactic information to we can use to distinguish between @NullableType Object foo() {} and @Deprecated Object foo() {}, but we want to output the first annotation in-line, and the second vertically.

Currently, we output ambiguous annotations as declaration annotations. This is fine for Java 7, and has the advantage of never incorrectly formatting a declaration annotation as a type annotation.

Here are some possible heuristics we could use to support this case for Java 8:

If the annotation occurs before a regular modifier, it's probably a declaration annotation:

@Deprecated
static Object foo() {}

If the annotation occurs after regular modifiers, and it isn't on a void-returning method, it's probably a type annotation:

static @Nullable Object foo() {}

If the declaration isn't a void-returning method, and it has no regular modifiers, consider the existing formatting: type annotations are probably on the same line as the type, declaration annotations are probably vertical. (The risk here is that we'd preserve things like @Override Object foo() {}.)
If the declaration is named @Override, it's probably a declaration annotation. This would offset the disadvantage of (3), but it could be confusing if @Override behaves differently than other annotations.
An annotation with parameters is unlikely to be a type annotation, and should probably be formatted vertically.

The text was updated successfully, but these errors were encountered:

cpovirk · 2015-05-20T14:38:46Z

An annotation with parameters is unlikely to be a type annotation, and
should probably be formatted vertically.

Will DI binding annotations like @Named("foo") ever be considered to be type annotations? (I assume that they're not today, since Guice+Dagger don't require Java 8).

kevinb9n · 2015-05-20T15:09:47Z

imho, they could be, but it seems awfully dodgy.

cgdecker · 2015-05-20T15:15:59Z

If nothing else, changing binding annotations to type annotations seems unlikely/impossible since it would be an incompatible change to the JSR-330 spec. That said, I don't see any particular reason to think that type annotations would be especially likely to not have parameters.

mernst · 2016-09-13T23:24:27Z

The proposed heuristics are generally reasonable. However, I am concerned about this one:

An annotation with parameters is unlikely to be a type annotation

There are type annotations that take parameters; for instance, the Nullness Checker of the Checker Framework contains six of them (@AssertNonNullIfNonNull, @Covariant, @EnsuresNonNull, @EnsuresNonNullIf, @KeyFor, and @RequiresNonNull), and so do other type systems.

When using the heuristics, I think google-java-format should examine as much code as possible -- at least the entire compilation unit or file, but preferably all the files that were passed to google-java-format. All the occurrences could vote on whether to treat the annotation as a declaration annotation or a type annotation. This will lead to correct results more often, and it will make the codebase more internally consistent. google-java-format can also look for a definition of the annotation definition in the files being formatted, which would give exact information.

Here is an alternate approach that could make even fewer errors: tell google-java-format which annotations are type annotations. google-java-format could read a .annotation-disambiguation file on the classpath (for example, in the google-java-format.jar file), in the user's home directory, and the current directory. The one in google-java-format.jar would contain information about every annotation defined in the JDK or in selected libraries such as Guava. This approach works well for me in post-processing scripts that I already use to fix up formatting of type annotations after google-java-format runs. This approach could be combined with the heuristics.

mernst · 2018-05-12T18:50:26Z

As a datapoint, this problem currently affects only 50 cases of @Nullable in Guava, or 4% of its @Nullable annotations. Guava is not yet fully annotated with @Nullable annotations.

This problem affects over 100 @Nullable annotations in Daikon, or 14% of its @Nullable annotations. Daikon is fully annotated with @Nullable annotations.

I work around the problem by running a wrapper around google-java-format: https://github.com/plume-lib/run-google-java-format. It contains a lst of annotations that should be treated as type annotations. That strategy is simple and has worked well for me. Perhaps google-java-format could adopt it. google-java-format's list could be fully-qualified annotation names, which would avoid any possibility of ambiguity.

cushon · 2018-05-16T23:04:24Z

Thanks for raising this again. We still want to do something here, it's just been on the back burner. We still don't have a huge number of type annotations, so it's a relatively minor annoyance. (Perhaps that will be changing soon, though...)

Having a hard-coded list of fully qualified names of type annotations seems like the least bad option to me. I'd like to try baking that list into the formatter instead of having a separate configuration file (like .annotation-disambiguation), to avoid having to add infrastructure for dealing with configuration.

mernst · 2018-05-17T00:30:24Z

I agree with your design (having a list of fully-qualified names of type annotations) and that it's least bad.

msridhar · 2018-12-09T18:16:19Z

We have code like the following and GJF does not reformat the code to move the annotation immediately after the documentation block:

  /**
   * Some docs
   */
  public static @Nullable Foo getFoo(String s) {

That is an instance of this issue correct? In our case it is androidx.annotation.Nullable which is not a type use annotation.

mernst · 2020-10-12T19:19:50Z

I'd like to try baking that list [of fully qualified names of type annotations] into the formatter instead of having a separate configuration file (like .annotation-disambiguation), to avoid having to add infrastructure for dealing with configuration.

Baking a list into GJF is definitely better than the current situation.

However, it doesn't seem to accommodate the use case of users who define a new type annotations. A configuration file would permit the user to notify GJF about the new type annotation. If the list of type annotations is only baked-in, then the user will have poorly-formatted code until upgrading to a new release of GJF that knows about the new annotation. Do you have thoughts about that use case?

mernst · 2020-10-26T16:02:20Z

@cushon A ping for your thoughts on handling new user-defined type annotations. Thanks!

cushon · 2020-11-10T20:47:02Z

A configuration file would permit the user to notify GJF about the new type annotation. If the list of type annotations is only baked-in, then the user will have poorly-formatted code until upgrading to a new release of GJF that knows about the new annotation. Do you have thoughts about that use case?

I understand the use-case of configuring new type annotations without having to update a hard-coded list and wait for a release. I also think avoiding configuration in the formatter has generally been a success. So I see a trade-off here.

mernst · 2020-11-10T21:05:28Z

Yes, there is a tradeoff. The question is which design you support. There would be no point in someone contributing a fix if you would reject the design. Can you give some guidance regarding the design decision of the maintainers? Thanks.

mernst · 2021-04-07T19:02:22Z

@cushon A ping for your decision about which design the maintainers prefer.

Here is another option: rather than reading a configuration file with a hard-coded name, it could be passed as a command-line argument. google-java-format already respects over half a dozen command-line arguments that change its formatting, so one more does not seem like a violation of its current design principles.

kevinb9n · 2021-06-17T04:52:53Z

I'm wondering if we've exhausted the possibilities that don't require a configuration list. What are the worst effects if all we do is implement 1-4 from comment #1? (Perhaps whether to add #5 or not is a question of numbers, but we'd take our internal numbers with a grain of salt since we aren't high CF adopters.)

item 3 seems like the key, so that even when people have to fix it it can stay fixed.

cushon · 2021-08-24T18:51:18Z

I have finally made some time to investigate having the formatter recognize type annotations and format them differently, e.g. preferring

@Deprecated
@Nullable Object foo() {}

I don't see good options for avoiding the config file. We could preserve the existing formatting, but since the formatter has been adding line breaks for annotations all along that would reduce the benefit for code bases that are already using the formatter.

The current implementation just hard codes a couple of well known type annotations.

My straw person proposal for configuration is to add a system property that takes a path to a file name containing a list of fully qualified names of annotations. So for example, you could have

$ cat typeannotations.txt
com.foo.MyTypeAnnotation

And then invoke the formatter with something like:

$ java -Dtypeannotationconfig=typeannotations.txt -jar google-java-format.jar ...

@deprecated

… list Given e.g. `@Deprecated @nullable Object foo() {}`, prefer this: ``` @deprecated @nullable Object foo() {} ``` instead of: ``` @deprecated @nullable Object foo() {} ``` The implementation is complicated by the fact that the AST doesn't store source position information for modifiers, and there's no requirement that declaration annotations, modifiers, and type annotations appear in any particular order in source. To work around this, we examine the token stream to figure out the ordering of the modifiers and annotations. #5 PiperOrigin-RevId: 392459885

@deprecated

… list Given e.g. `@Deprecated @nullable Object foo() {}`, prefer this: ``` @deprecated @nullable Object foo() {} ``` instead of: ``` @deprecated @nullable Object foo() {} ``` The implementation is complicated by the fact that the AST doesn't store source position information for modifiers, and there's no requirement that declaration annotations, modifiers, and type annotations appear in any particular order in source. To work around this, we examine the token stream to figure out the ordering of the modifiers and annotations. #5 PiperOrigin-RevId: 392459885

@deprecated

… list Given e.g. `@Deprecated @nullable Object foo() {}`, prefer this: ``` @deprecated @nullable Object foo() {} ``` instead of: ``` @deprecated @nullable Object foo() {} ``` The implementation is complicated by the fact that the AST doesn't store source position information for modifiers, and there's no requirement that declaration annotations, modifiers, and type annotations appear in any particular order in source. To work around this, we examine the token stream to figure out the ordering of the modifiers and annotations. #5 PiperOrigin-RevId: 392459885

@deprecated

… list Given e.g. `@Deprecated @nullable Object foo() {}`, prefer this: ``` @deprecated @nullable Object foo() {} ``` instead of: ``` @deprecated @nullable Object foo() {} ``` The implementation is complicated by the fact that the AST doesn't store source position information for modifiers, and there's no requirement that declaration annotations, modifiers, and type annotations appear in any particular order in source. To work around this, we examine the token stream to figure out the ordering of the modifiers and annotations. #5 PiperOrigin-RevId: 392459885

@deprecated

… list Given e.g. `@Deprecated @nullable Object foo() {}`, prefer this: ``` @deprecated @nullable Object foo() {} ``` instead of: ``` @deprecated @nullable Object foo() {} ``` The implementation is complicated by the fact that the AST doesn't store source position information for modifiers, and there's no requirement that declaration annotations, modifiers, and type annotations appear in any particular order in source. To work around this, we examine the token stream to figure out the ordering of the modifiers and annotations. #5 PiperOrigin-RevId: 392769609

@deprecated

… list Given e.g. `@Deprecated @nullable Object foo() {}`, prefer this: ``` @deprecated @nullable Object foo() {} ``` instead of: ``` @deprecated @nullable Object foo() {} ``` The implementation is complicated by the fact that the AST doesn't store source position information for modifiers, and there's no requirement that declaration annotations, modifiers, and type annotations appear in any particular order in source. To work around this, we examine the token stream to figure out the ordering of the modifiers and annotations. #5 Roll forward of 1a87579 without a use of a server-only Guava API. PiperOrigin-RevId: 392785887

@deprecated

… list Given e.g. `@Deprecated @nullable Object foo() {}`, prefer this: ``` @deprecated @nullable Object foo() {} ``` instead of: ``` @deprecated @nullable Object foo() {} ``` The implementation is complicated by the fact that the AST doesn't store source position information for modifiers, and there's no requirement that declaration annotations, modifiers, and type annotations appear in any particular order in source. To work around this, we examine the token stream to figure out the ordering of the modifiers and annotations. #5 Roll forward of 1a87579 without a use of a server-only Guava API. PiperOrigin-RevId: 392919024

mernst · 2022-07-13T18:05:06Z

@cushon Release 1.12.0 says "Format type annotation as part of the type, not part of the modifiers list", but it only handles two @Nullable annotations. Is there any chance of implementing your proposal of a -Dtypeannotationconfig=typeannotations.txt command-line argument?

cushon · 2022-07-13T20:19:01Z

It isn't a priority right now, but I think we'd take a PR that added a system property that specified a file containing additional type annotation qualified names to consider in JavaInputAstVisitor.typeAnnotations, and that included test coverage (core/src/test/java/com/google/googlejavaformat/java/MainTest.java seems like a good place, so it can exercise the system property in a subprocess.)

ViliusS · 2023-12-19T22:35:18Z

@cushon I think more explanation is needed why you think configuration file is the only option here. Are you worried about compatibility with current behaviour? Current behaviour is a bug and doesn't match the preferable style laid out in https://google.github.io/styleguide/javaguide.html . Also @mernst's proposal to add hardcoded list and a well put reasoning in #802 looks sound to me.
To have a hardcoded list to fix the bug right now, than to actually wait while someone more knowledgeable makes "proper" PR would save time for a lot of people. Now spotless, sbt, GitHub actions, even Google's IntellJ plugin doesn't agree on how type annotations should be wrapped. So we have a situation where IntellJ plugin (which uses this GJF library) rewrapps code, but CI Maven process, which uses spotless with GJF, fails. Developers spend hour battling this inconsistent behaviour between tools.
Even if system property is implemented all these tools would still have to keep their own list of annotations. IMHO, this library should provide that list. Maybe the list will not be exclusive, but it will be better than nothing.
Also, if hardcoded list is implemented nothing blocks somebody making a PR for system property. This is natural course of open source after all.

cushon mentioned this issue May 16, 2015

Extra newlines on type annotations #1

Closed

mernst mentioned this issue Jun 21, 2019

Add style check and also fix style violations typetools/templatefora-checker#3

Closed

cushon mentioned this issue Dec 19, 2019

Inconsistent annotation formatting #360

Closed

mernst mentioned this issue Oct 2, 2020

gradlew reformat takes 25 seconds even for back-to-back executions typetools/checker-framework#3735

Open

mernst mentioned this issue Oct 12, 2020

Samples formatting jspecify/jspecify#147

Closed

copybara-service bot mentioned this issue Aug 24, 2021

Format type annotation as part of the type, not part of the modifiers list #649

Merged

copybara-service bot mentioned this issue Aug 25, 2021

Format type annotation as part of the type, not part of the modifiers list #653

Merged

mernst mentioned this issue Jul 15, 2022

Expand hard-coded list of type qualifiers #802

Closed

mernst mentioned this issue Oct 11, 2022

formatAnnotations() contradicts Google Java style diffplug/spotless#1366

Closed

kevinb9n mentioned this issue Aug 29, 2024

Annotation location when mixed with modifiers #1152

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle type annotations #5

Handle type annotations #5

cushon commented May 16, 2015

cpovirk commented May 20, 2015 •

edited

Loading

kevinb9n commented May 20, 2015

cgdecker commented May 20, 2015

mernst commented Sep 13, 2016

mernst commented May 12, 2018 •

edited

Loading

cushon commented May 16, 2018

mernst commented May 17, 2018 •

edited

Loading

msridhar commented Dec 9, 2018 •

edited

Loading

mernst commented Oct 12, 2020

mernst commented Oct 26, 2020

cushon commented Nov 10, 2020

mernst commented Nov 10, 2020

mernst commented Apr 7, 2021 •

edited

Loading

kevinb9n commented Jun 17, 2021 •

edited

Loading

cushon commented Aug 24, 2021

mernst commented Jul 13, 2022

cushon commented Jul 13, 2022

ViliusS commented Dec 19, 2023

Handle type annotations #5

Handle type annotations #5

Comments

cushon commented May 16, 2015

cpovirk commented May 20, 2015 • edited Loading

kevinb9n commented May 20, 2015

cgdecker commented May 20, 2015

mernst commented Sep 13, 2016

mernst commented May 12, 2018 • edited Loading

cushon commented May 16, 2018

mernst commented May 17, 2018 • edited Loading

msridhar commented Dec 9, 2018 • edited Loading

mernst commented Oct 12, 2020

mernst commented Oct 26, 2020

cushon commented Nov 10, 2020

mernst commented Nov 10, 2020

mernst commented Apr 7, 2021 • edited Loading

kevinb9n commented Jun 17, 2021 • edited Loading

cushon commented Aug 24, 2021

mernst commented Jul 13, 2022

cushon commented Jul 13, 2022

ViliusS commented Dec 19, 2023

cpovirk commented May 20, 2015 •

edited

Loading

mernst commented May 12, 2018 •

edited

Loading

mernst commented May 17, 2018 •

edited

Loading

msridhar commented Dec 9, 2018 •

edited

Loading

mernst commented Apr 7, 2021 •

edited

Loading

kevinb9n commented Jun 17, 2021 •

edited

Loading