-
Notifications
You must be signed in to change notification settings - Fork 39
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
DirectReturn
bugpattern clash with lombok's @Data
#728
Conversation
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
error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/SourceCode.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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 Tree
s 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.
7c3da88
to
6fa7dbb
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
6fa7dbb
to
d071681
Compare
There was a problem hiding this 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 :).
error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreMatchers.java
Outdated
Show resolved
Hide resolved
error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DirectReturnTest.java
Outdated
Show resolved
Hide resolved
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
1 similar comment
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); |
There was a problem hiding this comment.
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.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
There was a problem hiding this 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 !
There was a problem hiding this 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)) { |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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 😄.
public static final class TestTemplateMatcher extends BugChecker | ||
implements AnnotationTreeMatcher { |
There was a problem hiding this comment.
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() { |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
.
c4d6ff8
to
807679d
Compare
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Added a commit and applied the suggestions from @Stephan202.
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.
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 :). |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
1 similar comment
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
0624ecc
to
9a7f313
Compare
"class A {", | ||
" private String field;", | ||
"}") | ||
.expectNoDiagnostics() |
There was a problem hiding this comment.
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.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
1 similar comment
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
… checker While here, add a new Matcher that determines whether a class is annotated with lombok's @DaTa.
There was a problem hiding this 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).
@Test | ||
void ignoreClassesAnnotatedWithLombokData() { | ||
CompilationTestHelper.newInstance(DirectReturn.class, getClass()) | ||
.addSourceLines( | ||
"A.java", | ||
"import lombok.Data;", | ||
"", | ||
"@Data", | ||
"class A {", | ||
" private String field;", | ||
"}") | ||
.doTest(); | ||
} |
There was a problem hiding this comment.
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")
6d8c9e1
to
6423d39
Compare
Kudos, SonarCloud Quality Gate passed! |
Looks good. All 4 mutations in this change were killed.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed. |
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. |
Opened a PR upstream: google/error-prone#4076. |
Thanks a lot for taking care @Stephan202 🙏 Shall we close this PR / ticket or ? 🤔 |
I verified that the unit test added to |
NB: Either way: thanks for the effort on this one! |
The upstream PR has been merged; will close this PR. But again: thanks for the effort 💪 |
Suggested commit message:
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 )
The issue seems to be when
DirectReturn
tries to optimize linein 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:
@Data
annotation. (See this)2. ForceDirectReturn
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 😄 )