Illegal redefinition of parameter o when using lombok together with Eclipse null check annotation analysis

[lombokissues]

Migrated from Google Code (issue 753)

[lombokissues]

👤 tomas.bucki   🕗 Nov 21, 2014 at 15:46 UTC

What steps will reproduce the problem?

1.Use @ EqualsAndHashCode on your empty class
2. Use @ org.eclipse.jdt.annotation.NonNullByDefault
3. Enable in eclipse null annotation checks

What is the expected output? What do you see instead?

Expected output is to not see compilation errors. I see 4 errors "Type
Illegal redefinition of parameter o, inherited method from Object does not constrain this parame"

What version of the product are you using? On what operating system?

STS 3.6.2, Java 8u25, Win 7 64bit, Lombok 14.4.8

Please provide any additional information below.

Problem is in generated implementation of equals(Object o) which should be equals(@ Nullable Object o) to work together with Eclipse null annotation analysis.

[lombokissues]

👤 tomas.bucki   🕗 Nov 21, 2014 at 17:16 UTC

It seems that using @ EqualsAndHashCode(onParam = @ __(@ AllowNull)) where AllowNull is the:
@ Documented
@ Retention(RetentionPolicy.RUNTIME)
@ Target({ METHOD, FIELD, ANNOTATION_TYPE, CONSTRUCTOR, PARAMETER, TYPE_USE })
public @ interface AllowNull {
// marker annotation with no members
}
Does not solve issue. It generates equals(@ AllowNull Object o) in de-lombok process but still Eclipse reports error.

[lombokissues]

End of migration

[guFalcon]

Same with Java 1.8.0_51 and Lombok 1.16.4.

@Data
public class FileImporter {

    @Nullable
    private List<String> fileEndings = new ArrayList<String>();
}

Error displayed on the @DaTa anntation (that adds the equals-method that is unconstrained in Object) is:

Illegal redefinition of parameter o, inherited method from Object does not constrain this parameter

A viable workaround for this error message would be (at least that's what we used to do in Java 1.7):

@Data
@EqualsAndHashCode(onParam = @__(@Nullable))
public class FileImporter {

    @Nullable
    private List<String> fileEndings = new ArrayList<String>();
}

This should add the the @nullable annotation to the parameter of the equals-method generated by the @data-annotation.

But it seems that the orX-method of Lombok doesn't work in Java 8 (tested with _25, _45 and _51) since the error message now is still(now on the @EqualsAndHashCode annotation instead of the @DaTa annotation):

Illegal redefinition of parameter o, inherited method from Object does not constrain this parameter

Implying that the @nullable annotation hasn't been added to the equals-method.

When we switch to 1.7 this second error vanishes and everything works like expected.

[ghost]

Same problem with this code, using Lombok 1.16.10, Java 1.7, Eclipse 4.6.1:

@Value
@org.eclipse.jdt.annotation.NonNullByDefault
public class Entity {

    String name;
    String code;

}

Note that adding @EqualsAndHashCode(onParam = @__(@Nullable)) on the class removes the compilation error. However, it seems strange to me to have to add a @nullable annotation when you wan't non-null fields.

[rzwitserloot]

We should scan for @NNBD annotation on the class and always generate @Nullable on the equals arg. I don't think that needs to be configurable; if you put NNBD on there, you gotta do it, right?

We should optimally also manually parse package-info.java in the same dir as your source file and check if you have @NNBD in there, and also do it. We already scan dirs (for lombok.config) so we should be able to do this. If you have split packages (the same package in another source root that has a package-info.java with @NNBD: We can't really cater to that one.

We need to code into the system the following:

If the enclosing class has an annotation of type org.eclipse.jdt.annotation.NonNullByDefault:
check if it has no arguments, or, alternatively, DefaultLocation.PARAMETER is in there. If no, we're done. If yes, add the appropriate annotation.

If the enclosing class did not have the annotation, check if the package-info.java file in the same dir has it. Do the same routine for it.

The appropriate annotation is org.eclipse.jdt.annotation.Nullable.

Secondly, if class or package has javax.annotation.ParametersAreNonnullByDefault annotation, then apply annotation javax.annotation.Nullable to the argument.

Anyone know of any other annotations in this vein?

[Gaibhne]

There are the (now deprecated in lieu of javax.annotation) FindBug annotations:

edu.umd.cs.findbugs.annotations.NonNull
edu.umd.cs.findbugs.annotations.Nullable

[rzwitserloot]

Before we fix this, we should also fix NNBD effects elsewhere. Specifically:

• I think @RequiredArgsConstructor ought to consider all not-@Nullable fields as 'required', assuming that, in the eclipse case, the argslist is empty or includes DefaultLocation.FIELD.
• I do not think it is a good idea to generate null checks everywhere; lombok already decided not to do that unless you use lombok's own @NonNull annotation (whereas @RequiredArgsConstructor treats a number of well known non-null annotations as relevant), and I bet some misguided linters will start whining about the null check we gen with an error along the lines of 'paramName can't be null here'.
• I do not think it is a good idea to introduce lombok's own @NonNullByDefault annotation.

Thus, the 2 fixes to apply to address this issue would be:

• Generate the appropriate @Nullable on the param of the generated equals method, based on checking both the class and the package-info.java file (and this involves writing a parser for package-info.java, so not quite trivial, but doable), and
• using the same mechanism to discover a relevant @NNBD annotation, change definition of whether a field is 'required' or not for @RequiredArgsContructor.

[rzwitserloot]

@Gaibne: I don't think those are relevant here; is there a NonNullByDefault annotation in findbugs's annotationset which implies @NonNull on all parameters? I don't think there is, in which case, it's not relevant for this issue.

[Gaibhne]

There is, also deprecated:

@edu.umd.cs.findbugs.annotations.DefaultAnnotation(NonNull.class)

[rzwitserloot]

So for that one, the rule is: NonNull needs to be part of the argslist. If so, generate e.u.c.f.a.Nullable.

[rzwitserloot]

Additional complication: there are now 4 places you can stick an annotation on an equals method: On the method, on the method's return type, on the parameter, and on the type holding the parameter. eclipse's @Nullable MUST appear in the 'on the type holding the parameter' position or it does not work, and lombok can't yet do that. It looks like: java.lang. @org.eclipse.jdt.annotation.Nullable Object.

So, we need to generate the @Nullable there, but this only works in J8+. Can we assume that lombok users have that at this point? Possibly yes if people are fancy enough to use @NNBD.

We also need to make a separate bug/enhancement to add onParamType for @EqAHC and @Setter.

[Gaibhne]

I hope it's not poor form for me to bump this ? Just ran into it again and after googling ended up here, reading my own comments :D

I think two years later we can definitely assume J8+.

[rzwitserloot]

I'm going to go for an intermediate solution: I'm not going to write code to do a scan for package-info, that's quite complicated, but I will scan for both common variants of org.eclipse.jdt.annotation.NonNullByDefault (javax.annotation and eclipse's), and if present, add the nullable annotation to the param of the generated equals, in the right place (on the 'type', not the param).

[rzwitserloot]

HEADSUP: We've changed how this is going to work. We're not going to hunt for @NonNullByDefault annotations; these are more usually found on the package and that'd take rather a lot of time and complication to find, so now whether lombok generates the annotation or not is too 'voodoo magical' (it is hard to explain that we see it on classes but not on packages!). Also, more generally, sometimes we need to generate the 'other' annotation (non-null), in which case there's nothing we can see anywhere to tip us off that we need to do that.

Therefore, instead, there is a lombok.config key where you configure what kind of 'flavour' of nullity annotations you want, and then lombok will generate them everywhere that it is relevant (toString, equals, canEqual, withX, and plural forms of @Singular properties for a builder).

This also covers nullity annotations relevant for issue 2221.