New check request: handle @Override on record component declarations somehow

[kevinb9n]

Because @Override is a @Target(METHOD) annotation, it's automatically permitted on record component declarations. It would normally cause the annotation to be copied to the generated accessor method, but in this case it's a SOURCE-retention annotation so there's nothing to do.

But it misses out on the normal javac enforcement of @Override, and the annotation ends up being meaningless.

It could have a reasonable meaning: it could ensure that the generated accessor method is overriding something.

From what I understand so far it would be reasonable for JLS to have required this enforcement (personal opinion only), but it's not something we'd be likely to fix any time soon. So, Error Prone might want to provide this enforcement, maybe?

And if so, then I guess the @Override suggester might also want to address this case. But it would be bad to suggest adding @Override to a record component if it wasn't being enforced. (I assume the suggester doesn't already do that?)

As a side note, a hand-written accessor method in a record class can also always use @Override regardless of supertype methods (JLS 9.6.4.4 explains). That should be unrelated to this though.

[kevinb9n]

Note that nothing in the JLS explicitly supports that @Override is meant to be interpreted this way. I'm arguing only that it would at least be a sensible and useful interpretation.

As it is, an organization that follows a "use @O wherever you can" policy can delete an internal interface method and use compilation errors to find all the stuff they can clean up (not being needed anymore, unless it has usages of its own). But as they do this they will miss record components.

[cushon]

I took a quick look at this and it seems like javac is dropping @Override annotations on record components before Error Prone sees them, e.g. for

public record R(@Override int foo, @Override String bar) implements I {
}

the AST we get is

public <init>(int foo, String bar) {
    super();
}

I'll dig into that a bit, I wonder if there's attribution logic that's propagating record component annotations and sneakily dropping those ones or something. If so it might require changes to how javac's modeling this for Error Prone to be able to do anything.

[kevinb9n]

That's interesting. At a reflection level I would expect those to be invisible because the annotation interface doesn't have @Target(RECORD_COMPONENT). But at the source-analysis level, it sure seems like they should be coming through. Does it do the same for any METHOD-only annotation?

[cushon]

Sorry false alarm, it's possible to get them through ClassSymbol#getRecordComponents -> RecordComponent.accessorMeth

[eamonnmcmanus]

We've had an internal Google issue about this for some time (for Googlers, b/378708626). The situation is fairly complicated. I'm going to try to summarize it here, and express some opinions.

Terminology

Suppose we have a record like this:

record Foo(int bar, String baz) {
  public int bar() {
    return this.bar;
  }
}

The record Foo has two components, bar and baz, specified by the corresponding component declarations int bar and String baz. Each component has an accessor method of the same name, so bar() and baz() here. The component bar has an explicit accessor method that appears in the source code. The component baz has an implicit accessor method generated by the compiler. Client code with an instance Foo foo accesses the components using the accessor methods, foo.bar() and foo.baz().

Each component also has an implicit private final field of the same name and type in the record.

@Override on accessor methods

A method in a record can be annotated with @Override, which can mean that the method overrides a method from a supertype; or that the method is an explicit accessor method; or both.

interface Quux {
  String baz();
  String frob(String name);
}

record Foo(int bar, String baz) implements Quux {
  @Override // explicit accessor method
  public int bar() {
    return this.bar;
  }
  
  @Override // override of inherited method
  public String frob(String name) {...}
  
  @Override // explicit accessor method AND override of inherited method
  public String baz() {
    return this.baz;
  }
}

It is of course a compile error if @Override is applied to a method that is neither an explicit accessor nor an override of an inherited method.

Annotations on record components

A record component declaration can have annotations. Since a component declaration corresponds to a constructor parameter, a private field, and an accessor method of the record class, an annotation is allowed there if it would be allowed in any of those places, or if it applies to record components themselves. The annotation is effectively copied to each place where it is applicable: if it is applicable to fields, then it's copied to the implicit field for the component; if it applicable to parameters, then it's copied to the constructor parameter for the component; and if it's applicable to methods, then it's copied to the accessor method. But it is only copied to an implicit accessor method, not an explicit one.

@Target(ElementType.METHOD)
@interface Interesting {}

record Foo(@Interesting int bar, @Interesting String baz) {
  @Override
  public int bar() {
    return this.bar;
  }
}

In this example, the @Interesting annotation is copied to the implicit accessor baz(), but not to the explicit accessor bar(). The @Interesting is applicable to methods, so it is allowed on the bar component, but since it is applicable only to methods it has no effect whatever there. (I logged a separate issue #5183 because I think this situation merits a warning.)

@Override on record components

The rules just described have a surprising consequence for the @Override annotation. Like @Interesting, it applies only to methods, but that means that it is legal on a record component. If the component has an implicit accessor method, @Override gets copied there, which is legal because @Override is allowed for an accessor. If the component has an explicit accessor, the @Override has no effect whatever. So @Override is always allowed on a record component, and it never has any effect.

I say this is surprising because I think if you told pretty much any Java developer that @Override is legal on component declarations and asked them what it meant, they would not say it meant nothing. A developer who knew that inherited abstract methods can be satisfied by component declarations would almost certainly assume that @Override meant that:

interface Quux {
  String baz();
  String frob(String name);
}

record Foo(@Override int bar, @Override String baz) implements Quux {...}

Our developer would expect that the @Override on baz is legal because it is implementing the inherited abstract method baz(), but the @Override on bar is illegal because it is not implementing anything. However, as we've seen, both @Override annotations are legal and meaningless.

@Override and Error Prone

@Override not applied to methods when it can be

Error Prone already warns you if you have a method where it would be legal to write @Override but you have not done so. This applies both to the classic case of a method overriding a supertype method, and to the more recent case of an explicit accessor method.

record Foo(int bar) {
  public int bar() {...}  // Error Prone warns about the missing @Override here
}

Google's Java Style Guide also requires @Override here.

@Override on record components

If you put @Override on a record component, you presumably didn't intend for it to be a meaningless decoration. So it would be reasonable for Error Prone to react to it in some way. I think there are a couple of possibilities:

1. Issue a warning or even an error when @Override is applied to a record component, given that it looks like it means something but doesn't.

2. Interpret it to mean what almost everybody would assume it means, that the record component implements an inherited abstract method. Issue an error if it doesn't in fact implement such a method. Possibly also issue a warning if a component implements such a method and doesn't have @Override either on the component declaration or on its explicit accessor method.

Option 1 is the safe option. It's in the same category as many existing checks that are there to tell you that you apparently wanted to do something but are not in fact doing it.

Option 2 makes me a bit uncomfortable. I can't think of other places where Error Prone reinterprets language constructs to mean something other than what the JLS says they mean. That seems like a road we probably don't want to take?

Could the Java language change?

Given that the @Override annotation is allowed but meaningless on record components, I think it's reasonable to ask whether the Java language could change in a way that would give it a meaning.

I found this compiler-dev thread from 2021 where members of Oracle's Java team confirm that the current behaviour is not a compiler bug, but I'm not aware of any discussion about whether it could be considered a language bug. Rémi Forax brought this up briefly at the end of the thread but there doesn't seem to have been further discussion.

Concretely, the language could change such that @Override is only legal on a record component if there is an inherited no-argument method of the same name. (Record components can't have the names of no-arg methods inherited from Object or Record, so this means it must be a public interface method.)

One issue is that this would technically be a source-incompatible change. If people have been using @Override on components with some intent other than saying a component overrides a supertype method, then their code will no longer compile. For example, we could imagine people wanting to put @Override on both the component and the accessor method to signal in both places that there is an explicit accessor. That doesn't seem very likely, though, and it wouldn't be the first time the language has changed in ways that potentially break source compilations (the yield keyword comes to mind).

[kevinb9n]

(Yes, I have also come to realize that just doing option 2 for Error Prone is akward/uncomfortable/anti-linguisitic.)