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

DirectReturn bugpattern clash with lombok's @Data #728

Closed

Conversation

mohamedsamehsalah
Copy link
Contributor

@mohamedsamehsalah mohamedsamehsalah commented Jul 23, 2023

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:

  1. 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 😄 )

@mohamedsamehsalah mohamedsamehsalah linked an issue Jul 23, 2023 that may be closed by this pull request
2 tasks
Copy link
Member

@rickie rickie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
@mohamedsamehsalah mohamedsamehsalah force-pushed the mohamedsamehsalah/direct-return-bug-pattern branch from 7c3da88 to 6fa7dbb Compare July 30, 2023 13:12
@mohamedsamehsalah mohamedsamehsalah marked this pull request as ready for review July 30, 2023 13:12
@mohamedsamehsalah mohamedsamehsalah added this to the 0.13.0 milestone Jul 30, 2023
@github-actions
Copy link

  • 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.

@mohamedsamehsalah mohamedsamehsalah requested a review from rickie July 30, 2023 13:21
@rickie rickie force-pushed the mohamedsamehsalah/direct-return-bug-pattern branch from 6fa7dbb to d071681 Compare August 11, 2023 14:31
Copy link
Member

@rickie rickie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@rickie rickie added the bug fix label Aug 11, 2023
@github-actions
Copy link

  • 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.

1 similar comment
@github-actions
Copy link

  • 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.

@@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@github-actions
Copy link

  • 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.

Copy link
Member

@rickie rickie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking this up @mohamedsamehsalah !

Copy link
Member

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

@@ -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)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄.

Comment on lines 78 to 147
public static final class TestTemplateMatcher extends BugChecker
implements AnnotationTreeMatcher {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/** 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would then become HasLombokDataTestChecker.

@rickie rickie modified the milestones: 0.13.0, 0.14.0 Aug 25, 2023
@rickie rickie force-pushed the mohamedsamehsalah/direct-return-bug-pattern branch from c4d6ff8 to 807679d Compare August 29, 2023 08:04
@github-actions
Copy link

  • 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
Copy link
Member

rickie commented Aug 29, 2023

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
Copy link

  • 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.

1 similar comment
@github-actions
Copy link

  • 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 rickie force-pushed the mohamedsamehsalah/direct-return-bug-pattern branch from 0624ecc to 9a7f313 Compare August 30, 2023 11:07
"class A {",
" private String field;",
"}")
.expectNoDiagnostics()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@github-actions
Copy link

  • 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
Copy link

  • 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.

1 similar comment
@github-actions
Copy link

  • 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.

Copy link
Member

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment on lines 227 to 240
@Test
void ignoreClassesAnnotatedWithLombokData() {
CompilationTestHelper.newInstance(DirectReturn.class, getClass())
.addSourceLines(
"A.java",
"import lombok.Data;",
"",
"@Data",
"class A {",
" private String field;",
"}")
.doTest();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")

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

sonarqubecloud bot commented Sep 7, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@github-actions
Copy link

github-actions bot commented Sep 7, 2023

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
Copy link
Member

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
Copy link
Member

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

@mohamedsamehsalah
Copy link
Contributor Author

mohamedsamehsalah commented Sep 10, 2023

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

Thanks a lot for taking care @Stephan202 🙏

Shall we close this PR / ticket or ? 🤔

@Stephan202
Copy link
Member

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
Copy link
Member

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

@Stephan202 Stephan202 removed this from the 0.14.0 milestone Sep 14, 2023
@Stephan202
Copy link
Member

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

@Stephan202 Stephan202 closed this Sep 14, 2023
@Stephan202 Stephan202 deleted the mohamedsamehsalah/direct-return-bug-pattern branch September 14, 2023 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

DirectReturn BugPattern crashes with Lombok's @Data annotation
3 participants