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.

Sign up for GitHub

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
wants to merge 10 commits into from
Closed

DirectReturn bugpattern clash with lombok's @Data #728

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ffeeeee
Exclude classes annotated with lombok's @Data from `DirectReturn` bug…
Jul 30, 2023
747af1b
Suggestions
rickie Aug 11, 2023
7e649bd
Tweaks
rickie Aug 11, 2023
1d2d286
Tweak
rickie Aug 11, 2023
7f78ac5
Apply suggestions
rickie Aug 29, 2023
bec7cdd
Format
rickie Aug 29, 2023
0f24503
Revert to Mo s suggestion
rickie Aug 30, 2023
e98cc88
Further improvements
rickie Aug 30, 2023
245dcf9
Drop another line
rickie Aug 30, 2023
6423d39
Suggestions
Stephan202 Sep 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions error-prone-contrib/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
5 changes: 4 additions & 1 deletion error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import static com.google.errorprone.matchers.Matchers.staticMethod;
import static com.google.errorprone.matchers.Matchers.toType;
import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL;
import static tech.picnic.errorprone.bugpatterns.util.MoreMatchers.HAS_LOMBOK_DATA;

import com.google.auto.service.AutoService;
import com.google.common.collect.Streams;
Expand All @@ -25,6 +26,7 @@
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.AssignmentTree;
import com.sun.source.tree.BlockTree;
import com.sun.source.tree.ClassTree;
import com.sun.source.tree.ExpressionStatementTree;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.IdentifierTree;
Expand Down Expand Up @@ -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 😄.

return Description.NO_MATCH;
}

Expand Down
4 changes: 4 additions & 0 deletions error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import com.google.errorprone.matchers.Matchers;
import com.google.errorprone.suppliers.Supplier;
import com.sun.source.tree.AnnotationTree;
import com.sun.source.tree.ClassTree;
import com.sun.source.tree.Tree;
import com.sun.tools.javac.code.Type;

Expand All @@ -16,6 +17,9 @@
* <p>These methods are additions to the ones found in {@link Matchers}.
*/
public final class MoreMatchers {
/** Matches classes annotated with Lombok's `@Data` annotation. */
public static final Matcher<ClassTree> HAS_LOMBOK_DATA = Matchers.hasAnnotation("lombok.Data");

private MoreMatchers() {}

/**
Expand Down
15 changes: 15 additions & 0 deletions error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DirectReturnTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,4 +223,19 @@ void replacement() {
"}")
.doTest(TestMode.TEXT_MATCH);
}

@Test
void ignoreClassesAnnotatedWithLombokData() {
CompilationTestHelper.newInstance(DirectReturn.class, getClass())
.setArgs("-processor", "lombok.launch.AnnotationProcessorHider$AnnotationProcessor")
.addSourceLines(
"A.java",
"import lombok.Data;",
"",
"@Data",
"class A {",
" private String field;",
"}")
.doTest();
}
}
37 changes: 37 additions & 0 deletions ...prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchersTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,40 @@
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker;
import com.google.errorprone.bugpatterns.BugChecker.AnnotationTreeMatcher;
import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher;
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.google.errorprone.suppliers.Supplier;
import com.sun.source.tree.AnnotationTree;
import com.sun.source.tree.ClassTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.Tree;
import org.junit.jupiter.api.Test;

final class MoreMatchersTest {
@Test
void hasLombokDataAnnotation() {
CompilationTestHelper.newInstance(HasLombokDataTestChecker.class, getClass())
.addSourceLines(
"A.java",
"import lombok.Data;",
"",
"@Data",
"// BUG: Diagnostic contains:",
"public class A {",
" private String field;",
"",
" static class B {}",
"",
" @Data",
" // BUG: Diagnostic contains:",
" static class C {}",
"}")
.addSourceLines("D.java", "public class D {}")
.doTest();
}

@Test
void hasMetaAnnotation() {
CompilationTestHelper.newInstance(HasMetaAnnotationTestChecker.class, getClass())
Expand Down Expand Up @@ -104,6 +128,19 @@ void isSubTypeOfBoundTypeUnknown() {
.doTest();
}

/** 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 HasLombokDataTestChecker extends BugChecker
implements ClassTreeMatcher {
private static final long serialVersionUID = 1L;
private static final Matcher<ClassTree> DELEGATE = MoreMatchers.HAS_LOMBOK_DATA;

@Override
public Description matchClass(ClassTree tree, VisitorState state) {
return DELEGATE.matches(tree, state) ? describeMatch(tree) : Description.NO_MATCH;
}
}

/** A {@link BugChecker} that delegates to {@link MoreMatchers#hasMetaAnnotation(String)}. */
@BugPattern(summary = "Interacts with `MoreMatchers` for testing purposes", severity = ERROR)
public static final class HasMetaAnnotationTestChecker extends BugChecker
Expand Down
5 changes: 5 additions & 0 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,6 +456,11 @@
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>org.projectlombok</groupId>
<artifactId>lombok</artifactId>
<version>1.18.28</version>
</dependency>
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
Expand Down