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

Missing override annotation on generated methods #1343

Open
FredPraca opened this issue Mar 23, 2017 · 16 comments
Open

Missing override annotation on generated methods #1343

FredPraca opened this issue Mar 23, 2017 · 16 comments
Assignees
Labels
accepted The issue/enhancement is valid, sensible, and explained in sufficient detail enhancement

Comments

@FredPraca
Copy link

At work, we used to tag missing override annotations as errors.
When using @EqualsAndHashCode, the compiler complains about the lack of the @OverRide annotation.
The case also appears when having an interface declaring specific accessors.

Would it be possible to add this missing annotation when appropriate ?

@rzwitserloot
Copy link
Collaborator

I think we originally did not add it because it wasn't a legit move in java 1.5 or 1.6 for interfaces, except this isn't even an interface.

I really see no reason NOT to gen an @Override annotation. If anyone has a good reason not to do it, please do comment on this one!

@rzwitserloot rzwitserloot self-assigned this Mar 19, 2018
@rzwitserloot rzwitserloot added enhancement accepted The issue/enhancement is valid, sensible, and explained in sufficient detail labels Mar 19, 2018
@rzwitserloot
Copy link
Collaborator

We generate @Override; lombok's always done so. Possibly this is a bug in your IDE or linter tool or whatever it is. Can you elaborate on what kind of tool you're talking about here?

Without further feedback, this issue can be closed starting on March 31st.

@rzwitserloot
Copy link
Collaborator

NB: If it's about 'canEqual', we can't add @Override on these, unfortunately.

@mmoayyed
Copy link
Contributor

One example can be noted here:

image

Where AbstractRandomStringGenerator is defined as:

@Getter
public abstract class AbstractRandomStringGenerator implements ... {
    private int defaultLength;

...
}

This is with Lombok 1.6.20 and Gradle's error-prone plugin 0.0.13 which I believe uses the latest version of error-prone at the time of this post. This may quite likely be an issue with error-prone, not sure.

@rspilker
Copy link
Collaborator

We cannot generate @Override here, since we cannot determine the methods in an interface. However, you cna have ombok generate @lombok.Generated by putting the following line in lombok.config:

lombok.addLombokGeneratedAnnotation = true

This should tell error-prone to not generate warnings on lombok generated methods.

@mmoayyed
Copy link
Contributor

Thank you for the suggestions.

I believe that setting is and was already in place when I captured the screenshot. But I must admit I don't follow your note above: The method getDefaultLength is also part of the abstract class. Do the same rules apply there?

PS: If I actually delombok the @getter annotation in the above abstract class, I see:

    public int getDefaultLength() {
        return this.defaultLength;
    }

@mmoayyed
Copy link
Contributor

mmoayyed commented Mar 27, 2018

Ah, never mind. I see what you meant. Thanks for the hint! (It is curious though why the setting does not quire suppress the warnings)

@mithuns
Copy link

mithuns commented Apr 13, 2018

yeah, that setting does not seem to suppress warning generated by error-prone.

@ob-stripe
Copy link

I don't think Error Prone cares about the @lombok.Generated annotation, but you can configure Lombok[0] with:

lombok.addJavaxGeneratedAnnotation = true

and pass the -XepDisableWarningsInGeneratedCode flag to Error Prone[1]. This will only work with Java 8 though as the @javax.annotation.Generated is broken as of Java 9.

Alternatively, for getters and setters, you can manually tell Lombok to add the @Override annotation with the onMethod parameter[2]:

@Getter(onMethod = @__({@Override})) @Setter(onMethod = @__({@Override})) String foo;

Unfortunately, at this time I don't think there's a Java 9 compatible solution for the canEqual methods generated by @EqualsAndHashCode.

@rzwitserloot Do you think it would be possible to add something like onMethod to @EqualsAndHashCode to solve this issue?

[0] https://projectlombok.org/features/configuration
[1] http://errorprone.info/docs/flags
[2] https://projectlombok.org/features/GetterSetter

@joaoe
Copy link

joaoe commented Oct 18, 2018

Hi.

I'm hitting this problem consistently

warning: [MissingOverride] canEqual overrides method in MyServiceRequestPayloadWithBody; expected @Override
@Data

I'm implementing a Rest service and have an hierarchy of objects which are mapped from JSON to POJOs using com.fasterxml.jackson.databind.ObjectMapper.

I also have some interfaces I use to abstract away each different object when implementing the business logic. Something like:

interface MyServiceObject extends Serializable {}

interface MyServiceRequest extends MyServiceObject {
  String getRequestId();
}

@Data
class MyServiceRequestPayload implements MyServiceRequest {
  private String eventId;
  private PauloadHeader header;
}

@Data
class MyServiceRequestPayloadWithBody extends MyServiceRequestPayload {
  private List<MyServiceObject > items;
}

The problem is that @Data and therefore @Getter and @EqualsAndHashCode are not aware of inherited methods. In this example, there is an abstract method getRequestId which comes from an interface and an implemented method canEqual which comes from @EqualsAndHashCode and is inherited from MyServiceRequestPayload to MyServiceRequestPayloadWithBody.

When compiling I get these warnings:

MyServiceRequestPayload.java: warning: [MissingOverride] getRequestId implements method in MyServiceRequest ; expected @Override
MyServiceRequestPayloadWithBody.java: warning: [MissingOverride] canEqual overrides method in MyServiceRequestPayload ; expected @Override

I'd really like if lombok was a bit smarter with these annotations and peeked into the hierarchy to see if there is a method with the same signature and if so, add the @Override. For me it's a bad practice to litter my code with @SuppressWarnings and using @Getter(onMethod_={@Override}) is boiler plate we should be avoiding.

I'm using IntelliJ. If I delombok the files, I see that the common methods like toString and equals do not have @Override. In my project we're using SonarQube and the Sonar Intellij plugin does complain too about those methods when I use @Data in a class hierarchy.

Thank you.

@rspilker
Copy link
Collaborator

Unfortunately we cannot look at the hierarchy, it requires resolution, see https://github.com/rzwitserloot/lombok/wiki/LOMBOK-CONCEPT:-Resolution

@joaoe
Copy link

joaoe commented Oct 19, 2018

Ah, I see. I did not know that lombok pre-compiled the files.

Then perhaps just add something extra that allows the developer to easily inject that @Override where needed ?

1. Regarding the case of overriding a setter:

It's easy to add a @Getter/Setter(onMethod_={@Override}) to the property String requestId. So that's covered, but it still requires a bit of boiler-plate, both on the class and property

I'd suggest something to simplify that case like at the class level @Getter(overrides={"requestId"}) class Klass{} or at the property level @GetterOverrides String requestId;

In case of a class level annotation, also allow that info to be passed from @Data like @Data(getterOverrides={"requestId"}, getterOverrides={})

2. Regarding @EqualsAndHashCode

I can't think of anything elegant, although I think it does not make sense to add some argument to the annotation just to hint lombok to add the @Override canEqual(). Perhaps adding @SuppressWarnings("MissingOverride") to the generated canEqual would be a way to shut up the compiler.

How about renaming that method to can<MyPackageMyKlassName>Equal() ? Does that break somehow the equals check in derived classes ? This would also result in more methods per class, but I doubt people make really long hierarchies with classes and @EqualsAndHashCode.

Extra: toString() and equals() stil need @Override there. I have SonarQube complaining on my project.

@kamilgregorczyk
Copy link

@SuppressWarnings("MissingOverride") to canEqual seems like a great idea

@hinrik
Copy link

hinrik commented Oct 25, 2019

@SuppressWarnings("MissingOverride") to canEqual seems like a great idea

Would this work? If so, is there any reason not to do it?

maggieneterval added a commit to maggieneterval/clouddriver that referenced this issue Sep 1, 2020
…ationResult and KubernetesRunJobDeploymentResult

The remaining missing @OVERRIDES warnings are for the `canEqual` methods generated by Lombok's @EqualsAndHashCode. This is a [known issue](projectlombok/lombok#1343), but before we suppress the remaining warnings, let's remove unnecessary uses of @EqualsAndHashCode.

OperationResult is annotated with @DaTa, which is equivalent to the following annotations: @getter, @Setter, @requiredargsconstructor, @tostring, and @EqualsAndHashCode. This commit replaces OperationResult's @DaTa and @NoArgsConstructor with field-level @getterS and @setters and an explicit no-args constructor. As a result, KubernetesRunJobDeploymentResult no longer requires its own @EqualsAndHashCode(callSuper = true). Also replaces its @DaTa with field-level @getter and @Setter. Let's try to avoid using @DaTa in the future for classes without a notion of logical equality that would require @EqualsAndHashCode, and instead just add the constructors and @getterS and @setters we actually need.
@cdir
Copy link

cdir commented Nov 6, 2020

We also have trouble with missing @OverRide at canEqual Method and we need to disable the error for missing overrides in eclipse project settings.

I think it is safe to generate the @OverRide as long as I only subclass other objects that also have the canEqual method.

Maybe add an additional setting wether the @OverRide should alsways be generated at canEqual or not.

@barorion
Copy link

I don't think Error Prone cares about the @lombok.Generated annotation

This should be solved by google/error-prone#2125.

So now error prone should be able to properly detect Lombok's generated code (assuming lombok.addJavaxGeneratedAnnotation = true is defined).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted The issue/enhancement is valid, sensible, and explained in sufficient detail enhancement
Projects
None yet
Development

No branches or pull requests