`DirectReturn` bugpattern clash with lombok's @Data #728

mohamedsamehsalah · 2023-07-23T20:10:18Z

Suggested commit message:

Have `DirectReturn` ignore classes annotated with Lombok's `@Data` (#728)

While there, introduce the `MoreMatchers#HAS_LOMBOK_DATA` matcher.

TL;DR DirectReturn bug pattern (If not all bug patterns that use replacements) detects bugs for generated resources, but replaces the suggested fixes in the original source code.

Notes 📝
This is the generated class from the project associated with this ticket. (#716 )

public class AnyClass {
    private String anything;

    @java.lang.SuppressWarnings(value = "all")
    public AnyClass() {
        super();
    }

    @java.lang.SuppressWarnings(value = "all")
    public String getAnything() {
        return this.anything;
    }

    @java.lang.SuppressWarnings(value = "all")
    public void setAnything(final String anything) {
        this.anything = anything;
    }

    @java.lang.Override
    @java.lang.SuppressWarnings(value = "all")
    public boolean equals(final java.lang.Object o) {
        if (o == this) return true;
        if (!(o instanceof AnyClass)) return false;
        final AnyClass other = (AnyClass)o;
        if (!other.canEqual((java.lang.Object)this)) return false;
        final java.lang.Object this$anything = this.getAnything();
        final java.lang.Object other$anything = other.getAnything();
        if (this$anything == null ? other$anything != null : !this$anything.equals(other$anything)) return false;
        return true;
    }

    @java.lang.SuppressWarnings(value = "all")
    protected boolean canEqual(final java.lang.Object other) {
        return other instanceof AnyClass;
    }

    @java.lang.Override
    @java.lang.SuppressWarnings(value = "all")
    public int hashCode() {
        final int PRIME = 59;
        int result = 1;
        final java.lang.Object $anything = this.getAnything();
        result = result * PRIME + ($anything == null ? 43 : $anything.hashCode());
        return result;
    }

    @java.lang.Override
    @java.lang.SuppressWarnings(value = "all")
    public java.lang.String toString() {
        return "AnyClass(anything=" + this.getAnything() + ")";
    }
}

The issue seems to be when DirectReturn tries to optimize line

        result = result * PRIME + ($anything == null ? 43 : $anything.hashCode());

in the hashCode method.

Due to the (helper?) methods used inside DirectReturn error prone parses and tries to replace the "source code" (The original annotated class) and not the generated resource.

Fix:

Ignore files that are using @Data annotation. (See this)
2. Force DirectReturn to replace the generated sources instead of source code, but:
a. Should Error prone change generated files or files of other libraries ?
b. I dont know how to do that (Any references would be helpful 😄 )

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DirectReturnTest.java

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/SourceCode.java

rickie

Thanks again Mo for picking this up and the research 🌟 💯 🥇!

I think the best way (and easiest way) for now would be to exclude classes from analysis where Lombok is involved indeed.
You can check this example where we don't want to match specific Trees that are not annotated with Scheduled. So we need to have this for all the Lombok annotatations that can have an impact on this check.
Please note that we need to use the fully qualified class name (FQCN) to match against.

The reason I'm all in for excluding Lombok classes from analysis is that it is hard to make a correct suggestion and we should not want to go down that road.

While we are at it, maybe it's nice to introduce a new Matcher for this to MoreMatchers.java that we can reuse in the CanonicalAnnotationSyntax check. That way we can fix both tickets at once, as we could introduce a usage for this ticket as well #557.

Ideally we do not have to add the Lombok annotation to EPS (in the end it is probably inevatable though). We can use the FCQN and that should be enough.

error-prone-contrib/pom.xml

github-actions · 2023-07-30T13:15:05Z

Surviving mutants in this change: 2
Killed mutants in this change: 8

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.util.MoreMatchers`	2	4
🎉`tech.picnic.errorprone.bugpatterns.DirectReturn`	0	4

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie

Made some suggestions 😄, please take a look :).

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchers.java

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DirectReturnTest.java

error-prone-contrib/pom.xml

github-actions · 2023-08-11T14:33:56Z

Surviving mutants in this change: 1
Killed mutants in this change: 3

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.DirectReturn`	1	3

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-08-11T15:01:31Z

Surviving mutants in this change: 1
Killed mutants in this change: 3

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.DirectReturn`	1	3

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie · 2023-08-11T15:07:05Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java

@@ -67,7 +69,8 @@ public DirectReturn() {}
  @Override
  public Description matchBlock(BlockTree tree, VisitorState state) {
    List<? extends StatementTree> statements = tree.getStatements();
-    if (statements.size() < 2) {
+    ClassTree enclosingNode = ASTHelpers.findEnclosingNode(state.getPath(), ClassTree.class);


One thing I considered is implementing matchClass and do this check there, but I think this makes more sense. CC @Stephan202.

github-actions · 2023-08-11T15:09:28Z

Surviving mutants in this change: 1
Killed mutants in this change: 3

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.DirectReturn`	1	3

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie

Thanks for picking this up @mohamedsamehsalah !

Stephan202

I'll need to find time later (when TBD) for a closer look at this PR.

Checking @pacbeckh's reproduction case with a debugger, it seems that all involved Tree instances have @Data as their source representation, with the full compilation unit source matching the original source code. So I'd like to better understand whether other checks may also be impacted, and thus whether generalizations of this approach apply. (For example, perhaps we should more generally identify and suppress overlapping suggestions. Not sure.)

Stephan202 · 2023-08-12T10:29:37Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java

@@ -67,7 +69,8 @@ public DirectReturn() {}
  @Override
  public Description matchBlock(BlockTree tree, VisitorState state) {
    List<? extends StatementTree> statements = tree.getStatements();
-    if (statements.size() < 2) {
+    ClassTree enclosingClass = ASTHelpers.findEnclosingNode(state.getPath(), ClassTree.class);
+    if (statements.size() < 2 || HAS_LOMBOK_DATA.matches(enclosingClass, state)) {


Didn't check the commit history, but I see a comment about having Lombok as a dependency. Personally I think that we should have a proper test that triggers the regression before the fix. Did we check whether this is possible with (perhaps with setArgs?) without enabling Lombok for the main build?

(Another (less preferred) option is to introduce a separate Maven module with a number of fixtures like Immutables does. That would be a rather indirect check, though, because it'd rely on the self-check build.)

Sorry I don't fully get what you mean with the first part.

I created a failing test with this diff:

diff --git a/error-prone-contrib/pom.xml b/error-prone-contrib/pom.xml index cd4016df..d04e1b5e 100644 --- a/error-prone-contrib/pom.xml +++ b/error-prone-contrib/pom.xml @@ -181,6 +181,11 @@ <artifactId>mongodb-driver-core</artifactId> <scope>test</scope> </dependency> + <dependency> + <groupId>org.projectlombok</groupId> + <artifactId>lombok</artifactId> + <scope>test</scope> + </dependency> <dependency> <groupId>org.reactivestreams</groupId> <artifactId>reactive-streams</artifactId> diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DirectReturnTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DirectReturnTest.java index 42e18894..d5ef8895 100644 --- a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DirectReturnTest.java +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DirectReturnTest.java @@ -223,4 +223,26 @@ final class DirectReturnTest { "}") .doTest(TestMode.TEXT_MATCH); } + + @Test + void exclude() { + BugCheckerRefactoringTestHelper.newInstance(DirectReturn.class, getClass()) + .addInputLines( + "A.java", + "import lombok.Data;", + "", + "@Data", + "public class A {", + " private String field;", + "}") + .addOutputLines( + "A.java", + "import lombok.Data;", + "", + "@Data", + "public class A {", + " private String field;", + "}") + .doTest(TestMode.TEXT_MATCH); + } } diff --git a/pom.xml b/pom.xml index a474e356..d08c940a 100644 --- a/pom.xml +++ b/pom.xml @@ -456,6 +456,11 @@ </exclusion> </exclusions> </dependency> + <dependency> + <groupId>org.projectlombok</groupId> + <artifactId>lombok</artifactId> + <version>1.18.28</version> + </dependency>

Do you mean having a failing test like this?

If that already triggers the issue (I'm surprised that Lombok is auto-enabled; it might not be when the tests are executed with JDK 11 👀): yes, something like that!

Yes that triggers the issue. It's funny because this is basically what @mohamedsamehsalah proposed and I changed it. So I'll add a commit to restore that state 😄.

Stephan202 · 2023-08-12T10:31:05Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchersTest.java

+  public static final class TestTemplateMatcher extends BugChecker
+      implements AnnotationTreeMatcher {


In #642 I renamed this type to HasMetaAnnotationTestChecker, in line with approaches taken elsewhere.

Stephan202 · 2023-08-12T10:31:49Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchersTest.java

@@ -47,9 +50,33 @@ void hasMetaAnnotation() {
        .doTest();
  }

-  /** A {@link BugChecker} that delegates to {@link MoreMatchers#hasMetaAnnotation(String)} . */
+  @Test
+  void hasLombokDataAnnotation() {


Given the the constant field is listed first, I'd move this method up (and likewise for the test checker definition below).

Stephan202 · 2023-08-12T10:32:08Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchersTest.java

+
+  /** A {@link BugChecker} that delegates to {@link MoreMatchers#HAS_LOMBOK_DATA}. */
+  @BugPattern(summary = "Interacts with `MoreMatchers` for testing purposes", severity = ERROR)
+  public static final class LombokDataAnnotationMatcher extends BugChecker


This would then become HasLombokDataTestChecker.

github-actions · 2023-08-29T08:07:17Z

Surviving mutants in this change: 1
Killed mutants in this change: 3

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.DirectReturn`	1	3

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie · 2023-08-29T09:22:38Z

Added a commit and applied the suggestions from @Stephan202.

Didn't check the commit history, but I see a comment about having Lombok as a dependency. Personally I think that we should have a proper test that triggers the regression before the fix. Did we check whether this is possible with (perhaps with setArgs?) without enabling Lombok for the main build?

Yes this makes sense. For now I was inclined to go with what we have right now. A more generic solution for Lombok is of course nice, but having fixes for the checks that are impacted right now is something I valued more for now.

Checking @pacbeckh's reproduction case with a debugger, it seems that all involved Tree instances have @DaTa as their source representation, with the full compilation unit source matching the original source code. So I'd like to better understand whether other checks may also be impacted, and thus whether generalizations of this approach apply. (For example, perhaps we should more generally identify and suppress overlapping suggestions. Not sure.)

I would like to propose to defer looking into such a bigger solution for now. Don't get me wrong, it would be good to spent time on this, but while having limited time right now there are some other EPS things I like to focus on more :).

github-actions · 2023-08-29T09:25:17Z

Surviving mutants in this change: 1
Killed mutants in this change: 3

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.DirectReturn`	1	3

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-08-30T11:07:45Z

Surviving mutants in this change: 1
Killed mutants in this change: 3

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.DirectReturn`	1	3

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

rickie · 2023-08-30T11:08:40Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DirectReturnTest.java

+            "class A {",
+            "  private String field;",
+            "}")
+        .expectNoDiagnostics()


We can drop this, but I think it makes it quite clear though 🤔. Will drop for now.

github-actions · 2023-08-30T11:10:25Z

Surviving mutants in this change: 1
Killed mutants in this change: 3

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.DirectReturn`	1	3

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-08-30T11:11:37Z

Surviving mutants in this change: 1
Killed mutants in this change: 3

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.DirectReturn`	1	3

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

github-actions · 2023-08-30T12:09:58Z

Surviving mutants in this change: 1
Killed mutants in this change: 3

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.DirectReturn`	1	3

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@DaTa

… checker While here, add a new Matcher that determines whether a class is annotated with lombok's @DaTa.

Stephan202

Rebased and added a commit. Looking at the generated AST, I notice that the methods generated by Lombok are annotated with @java.lang.SuppressWarnings(value = "all"). Suppressing based on that seems more robust (and also covers e.g. Immutables-generated code).

Stephan202 · 2023-09-07T17:06:29Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DirectReturnTest.java

+  @Test
+  void ignoreClassesAnnotatedWithLombokData() {
+    CompilationTestHelper.newInstance(DirectReturn.class, getClass())
+        .addSourceLines(
+            "A.java",
+            "import lombok.Data;",
+            "",
+            "@Data",
+            "class A {",
+            "  private String field;",
+            "}")
+        .doTest();
+  }


If I revert the fix then this test doesn't fail. Contrary to what's claimed further up, as best as I can tell Lombok is not executed here. Seems we need to explicitly enable the annotation processor:

.setArgs("-processor", "lombok.launch.AnnotationProcessorHider$AnnotationProcessor")

sonarcloud · 2023-09-07T17:19:59Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

github-actions · 2023-09-07T17:20:46Z

Looks good. All 4 mutations in this change were killed.

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.DirectReturn`	0	4

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Stephan202 · 2023-09-08T05:24:41Z

Looking at the generated AST, I notice that the methods generated by Lombok are annotated with @java.lang.SuppressWarnings(value = "all"). Suppressing based on that seems more robust (and also covers e.g. Immutables-generated code).

We might be able to handle this upstream; see google/error-prone#4065 (comment) and subsequent discussion. I'll have a look at that in the coming days.

Stephan202 · 2023-09-10T13:46:51Z

Opened a PR upstream: google/error-prone#4076.

mohamedsamehsalah · 2023-09-10T16:58:18Z

Opened a PR upstream: google/error-prone#4076.

Thanks a lot for taking care @Stephan202 🙏

Shall we close this PR / ticket or ? 🤔

Stephan202 · 2023-09-11T04:56:30Z

Shall we close this PR / ticket or ? 🤔

I verified that the unit test added to DirectReturnTest fails when running without the upstream PR, and passes with it. So indeed we can likely close this ticket. But since the upstream PR hasn't been merged/released yet, perhaps it makes sense to keep this open as a fallback. (Alternatively we can also reopen later.)

Stephan202 · 2023-09-11T04:56:59Z

NB: Either way: thanks for the effort on this one!

Stephan202 · 2023-09-14T19:21:45Z

The upstream PR has been merged; will close this PR. But again: thanks for the effort 💪

mohamedsamehsalah linked an issue Jul 23, 2023 that may be closed by this pull request

DirectReturn BugPattern crashes with Lombok's @Data annotation #716

Closed

2 tasks

mohamedsamehsalah requested review from rickie and Stephan202 July 23, 2023 20:10

mohamedsamehsalah commented Jul 23, 2023

View reviewed changes

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java Outdated Show resolved Hide resolved

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DirectReturnTest.java Outdated Show resolved Hide resolved

mohamedsamehsalah commented Jul 25, 2023

View reviewed changes

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/SourceCode.java Outdated Show resolved Hide resolved

rickie reviewed Jul 25, 2023

View reviewed changes

error-prone-contrib/pom.xml Outdated Show resolved Hide resolved

mohamedsamehsalah force-pushed the mohamedsamehsalah/direct-return-bug-pattern branch from 7c3da88 to 6fa7dbb Compare July 30, 2023 13:12

mohamedsamehsalah marked this pull request as ready for review July 30, 2023 13:12

mohamedsamehsalah added this to the 0.13.0 milestone Jul 30, 2023

mohamedsamehsalah requested a review from rickie July 30, 2023 13:21

rickie force-pushed the mohamedsamehsalah/direct-return-bug-pattern branch from 6fa7dbb to d071681 Compare August 11, 2023 14:31

rickie reviewed Aug 11, 2023

View reviewed changes

rickie added the bug fix label Aug 11, 2023

rickie reviewed Aug 11, 2023

View reviewed changes

rickie approved these changes Aug 12, 2023

View reviewed changes

Stephan202 reviewed Aug 12, 2023

View reviewed changes

rickie modified the milestones: 0.13.0, 0.14.0 Aug 25, 2023

rickie force-pushed the mohamedsamehsalah/direct-return-bug-pattern branch from c4d6ff8 to 807679d Compare August 29, 2023 08:04

rickie force-pushed the mohamedsamehsalah/direct-return-bug-pattern branch from 0624ecc to 9a7f313 Compare August 30, 2023 11:07

rickie reviewed Aug 30, 2023

View reviewed changes

mohamedsamehsalah and others added 10 commits September 7, 2023 18:46

Exclude classes annotated with lombok's @DaTa from DirectReturn bug…

ffeeeee

… checker While here, add a new Matcher that determines whether a class is annotated with lombok's @DaTa.

Suggestions

747af1b

Tweaks

7e649bd

Tweak

1d2d286

Apply suggestions

7f78ac5

Format

bec7cdd

Revert to Mo s suggestion

0f24503

Further improvements

e98cc88

Drop another line

245dcf9

Suggestions

6423d39

Stephan202 reviewed Sep 7, 2023

View reviewed changes

Stephan202 force-pushed the mohamedsamehsalah/direct-return-bug-pattern branch from 6d8c9e1 to 6423d39 Compare September 7, 2023 17:17

Stephan202 removed this from the 0.14.0 milestone Sep 14, 2023

Stephan202 closed this Sep 14, 2023

Stephan202 deleted the mohamedsamehsalah/direct-return-bug-pattern branch September 14, 2023 19:21

This was referenced Sep 14, 2023

DirectReturn BugPattern crashes with Lombok's @Data annotation #716

Closed

False positives for Lombok's @Data annotation #557

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`DirectReturn` bugpattern clash with lombok's @Data #728

`DirectReturn` bugpattern clash with lombok's @Data #728

mohamedsamehsalah commented Jul 23, 2023 •

edited by rickie

rickie left a comment

github-actions bot commented Jul 30, 2023

rickie left a comment

github-actions bot commented Aug 11, 2023

github-actions bot commented Aug 11, 2023

rickie Aug 11, 2023

github-actions bot commented Aug 11, 2023

rickie left a comment

Stephan202 left a comment

Stephan202 Aug 12, 2023

rickie Aug 29, 2023

Stephan202 Aug 29, 2023

rickie Aug 30, 2023

Stephan202 Aug 12, 2023

Stephan202 Aug 12, 2023

Stephan202 Aug 12, 2023

github-actions bot commented Aug 29, 2023

rickie commented Aug 29, 2023

github-actions bot commented Aug 29, 2023

github-actions bot commented Aug 30, 2023

rickie Aug 30, 2023

github-actions bot commented Aug 30, 2023

github-actions bot commented Aug 30, 2023

github-actions bot commented Aug 30, 2023

Stephan202 left a comment

Stephan202 Sep 7, 2023

sonarcloud bot commented Sep 7, 2023

github-actions bot commented Sep 7, 2023

Stephan202 commented Sep 8, 2023

Stephan202 commented Sep 10, 2023

mohamedsamehsalah commented Sep 10, 2023 •

edited

Stephan202 commented Sep 11, 2023

Stephan202 commented Sep 11, 2023

Stephan202 commented Sep 14, 2023

		public static final class TestTemplateMatcher extends BugChecker
		implements AnnotationTreeMatcher {

DirectReturn bugpattern clash with lombok's @Data #728

DirectReturn bugpattern clash with lombok's @Data #728

Conversation

mohamedsamehsalah commented Jul 23, 2023 • edited by rickie

rickie left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 30, 2023

rickie left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 11, 2023

github-actions bot commented Aug 11, 2023

Choose a reason for hiding this comment

github-actions bot commented Aug 11, 2023

rickie left a comment

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 29, 2023

rickie commented Aug 29, 2023

github-actions bot commented Aug 29, 2023

github-actions bot commented Aug 30, 2023

Choose a reason for hiding this comment

github-actions bot commented Aug 30, 2023

github-actions bot commented Aug 30, 2023

github-actions bot commented Aug 30, 2023

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 7, 2023

github-actions bot commented Sep 7, 2023

Stephan202 commented Sep 8, 2023

Stephan202 commented Sep 10, 2023

mohamedsamehsalah commented Sep 10, 2023 • edited

Stephan202 commented Sep 11, 2023

Stephan202 commented Sep 11, 2023

Stephan202 commented Sep 14, 2023

`DirectReturn` bugpattern clash with lombok's @Data #728

`DirectReturn` bugpattern clash with lombok's @Data #728

mohamedsamehsalah commented Jul 23, 2023 •

edited by rickie

mohamedsamehsalah commented Sep 10, 2023 •

edited