False positives for Lombok's @Data annotation

[ksiczek]

Describe the bug

• I have verified that the issue is reproducible against the latest version
  of the project.
• I have searched through existing issues to verify that this issue is not
  already known.

Minimal Reproducible Example

Given below class

import com.fasterxml.jackson.annotation.JsonProperty;
import lombok.Data;

@Data
public class ErrorProneTestObject {

    @JsonProperty("event")
    private String event;

}

we get unclear recommendations saying that we might replace @Data with @Data (see Logs section). It seems it happens only for Lombok @Data, or @Setter:

@Setter
public class ErrorProneTestObject {

    @JsonProperty("event")
    private String event;

    public String getEvent() {
        return event;
    }

} 

Logs

.../ErrorProneTestObject.java:6: Note: [CanonicalAnnotationSyntax] Omit redundant syntax from annotation declarations
@Data
^
    (see https://error-prone.picnic.tech/bugpatterns/CanonicalAnnotationSyntax)
  Did you mean '@Data'?

I tried several different combinations of the annotations and configurations and it seems that:

• it always happens if you use the ones I specified above
• it stops happening if I remove one of them
• it also stops happening if I remove JsonProperty's parameter

Expected behavior

I'm pretty sure that the error message does not help much so it is either a bug in the analysis or in the message. Does it sound familiar to you guys?

Setup

• MacOS Ventura
• openjdk version "19.0.2" 2023-01-17
• Error Prone version 2.18.0
• Error Prone Support version 0.8.0
• EDIT: I also have disableWarningsInGeneratedCode = true in the configuration

Additional context

I just started using error-prone and it might be a good opportunity for me to see how custom rules work. If you consider reported behavior a bug I think I could dive deeper into it ☺️

[rickie]

Hi @ksiczek, thanks for taking the time to report an issue!

This sure looks like a bug and should be fixed. The thing is that Lombok probably generates extra parentheses "under the hood" that are flagged by this check.

it also stops happening if I remove JsonProperty's parameter

This is interesting and didn't expect that though 🤔.

I think we should have something to exclude certain annotations from this analysis. We have this code in a different check that is probably similar to what we need here.

I'll need some more time to dive a bit deeper into this before I can give a good answer or direction. Already wanted to share this and let you know we noticed the issue 😄.

[rickie]

What version of Lombok are you using?

I tried to reproduce it in the project by using the following test (using Lombok 1.18.26):

  @Test
  void lombokWithData() {
    CompilationTestHelper.newInstance(CanonicalAnnotationSyntax.class, getClass())
        .addSourceLines(
            "A.java",
            "import lombok.Data;",
            "import com.fasterxml.jackson.annotation.JsonProperty;",
            "",
            "@Data",
            "public class A {",
            "  @JsonProperty(\"foo\")",
            "  private String foo;",
            "}")
        .doTest();
  }

It didn't fail for me, so it might be the case that this test doesn't fully use the features of Lombok?

CC: @Stephan202.

[ksiczek]

We also use the 1.18.26 of Lombok. Could it be something different in our stack that interferes? I thought that if I had created an additional class in our project then I isolated it enough, but maybe I was wrong. One thing comes to my mind - we do use lombok.addLombokGeneratedAnnotation = true and I have not tried running w/o it.

[mohamedsamehsalah]

@rickie It didn't fail for me, so it might be the case that this test doesn't fully use the features of Lombok?

I can't reproduce the issue using tests aswell. I also tried using example project from #716 wtih lombok.addLombokGeneratedAnnotation = true property set in lombok.config file.

[rickie]

Thanks for looking into this @mohamedsamehsalah. Good to know 😄.

@ksiczek Can you try running it without lombok.addLombokGeneratedAnnotation = true? If you still see the failure, can you compare your setup with this example project and try to make a reproduction case there. If we don't know the specifics that can cause this issue it's hard to figure out what the problem is.

[Stephan202]

It didn't fail for me, so it might be the case that this test doesn't fully use the features of Lombok?

That's correct; for Lombok to work in this case, it needs to be enabled explicitly. See the setArgs solution mentioned here. When I add that flag I can indeed reproduce the reported issue.

I had hoped that the solution implemented in google/error-prone#4076 would also resolve this issue, but it doesn't, because the issue at hand is that CanonicalAnnotationSyntax (incorrectly) attempts to flag the explicitly specified @JsonProperty("event") annotation, which is not itself in a @SuppressWarnings("all") scope.

That then leaves the question: why does the check attempt to flag the (already canonical) @JsonProperty("event") annotation? It turns out that, while this code is explicitly defined, Lombok does not retain it's exact source code representation, by instead mapping it, but also the implicit value = assignment, to the @Data annotation. This causes our check to think that the annotation looks like @JsonProperty(value = "event") (because the assignment has an explicit source presentation), even though it doesn't. Conversely, this issue also means that the non-canonical annotation @JsonProperty() would not be flagged.

Possible next steps (not mutually exclusive):

1. Flag this issue with Lombok.
2. Update CanonicalAnnotationSyntax to work around it.

Point (2) could involve something like was attempted in #728, but as I also mentioned there: I really prefer that also here we identify a more generic solution. (A @Data-annotated class can contain explicitly defined code, and to the extent that Lombok does not hide the details from us, we'd like to accurately process that code.)