Introduce `ConflictDetection` utility class #478

eric-picnic · 2023-01-27T15:53:04Z

Suggested commit message:

Introduce support for detecting conflicts when renaming methods (#478)

By moving reusable conflict detection code from `JUnitMethodDeclaration` to a
new `ConflictDetection` class.

github-actions · 2023-01-27T15:55:23Z

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

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.util.ConflictDetection`	1	19

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

rickie

Added a commit with some suggestions.

Suggested commit message;

Introduce `ConflictDetection` utility class (#478)

rickie · 2023-01-30T09:22:48Z

...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetection.java

+import java.util.Optional;
+
+/**
+ * A set of helper methods for finding conflicts which would be caused by applying certain fixes.


Hmm, not a huge fan of the "certain fixes".

I'm suggesting an alternative,feel free to revert or tweak :).

rickie · 2023-01-30T09:25:27Z

...one-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetectionTest.java

+
+  /**
+   * A {@link BugChecker} that flags method rename blockers found by {@link
+   * ConflictDetection#findMethodRenameBlocker(Symbol.MethodSymbol, String, VisitorState)}.


Suggested change

* ConflictDetection#findMethodRenameBlocker(Symbol.MethodSymbol, String, VisitorState)}.

* ConflictDetection#findMethodRenameBlocker(MethodSymbol, String, VisitorState)}.

rickie · 2023-01-30T09:26:32Z

...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitMethodDeclaration.java

@@ -8,8 +8,8 @@
 import static com.google.errorprone.matchers.Matchers.hasModifier;
 import static com.google.errorprone.matchers.Matchers.not;
 import static java.util.function.Predicate.not;
+import static tech.picnic.errorprone.bugpatterns.util.ConflictDetection.findMethodRenameBlocker;


Let's not statically import. That may read a bit better.

github-actions · 2023-01-30T09:29:44Z

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

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.util.ConflictDetection`	1	19

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

Stephan202

Rebased and added a commit. Didn't call out all changes.

Stephan202 · 2023-02-11T15:27:29Z

...one-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetectionTest.java

+  void matcher() {
+    CompilationTestHelper.newInstance(RenameBlockerFlagger.class, getClass())
+        .addSourceLines(
+            "/A.java",


Suggested change

"/A.java",

"A.java",

Stephan202 · 2023-02-11T15:32:16Z

...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetection.java

+   *   <li>Whether the rename would merely introduce a method overload, rather than clashing with an
+   *       existing method declaration.
+   *   <li>Whether the rename would cause a method in a superclass to be overridden.


We do check superclasses; let's rephrase this.

Stephan202 · 2023-02-12T15:51:22Z

...one-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetectionTest.java

+    @Override
+    public Description matchMethod(MethodTree tree, VisitorState state) {
+      return ConflictDetection.findMethodRenameBlocker(
+              ASTHelpers.getSymbol(tree), tree.getName() + "t", state)


Clearer (or some variation hereof):

Suggested change

ASTHelpers.getSymbol(tree), tree.getName() + "t", state)

ASTHelpers.getSymbol(tree), tree.getName() + "WithSuffix", state)

Yes but then indeed the in -> int case is not tested nicely. Couldn't figure out a nicer way to do that. (Based on the changes not including this change I suspect you went through the same train of thoughts there 😉).

Exactly 😄. Forgot to remove this comment 👍

Stephan202 · 2023-02-12T16:15:32Z

...r-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetection.java

+import java.util.Optional;
+
+/** A set of helper methods for detecting conflicts that would be caused when applying fixes. */
+public final class ConflictDetection {


Fine for now, but I suspect that eventually we'll want a more generic name. Time will tell :)

github-actions · 2023-02-12T16:20:45Z

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

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.util.ConflictDetection`	1	19

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

github-actions · 2023-02-12T16:38:22Z

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

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.util.ConflictDetection`	1	19

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

Stephan202 · 2023-02-12T16:46:18Z

I see that one mutant survived; will have a look at that in a bit.

github-actions · 2023-02-12T19:35:42Z

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

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.util.ConflictDetection`	0	20

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

rickie

Changes LGTM!

rickie · 2023-02-13T10:47:05Z

...one-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetectionTest.java

+    @Override
+    public Description matchMethod(MethodTree tree, VisitorState state) {
+      return ConflictDetection.findMethodRenameBlocker(
+              ASTHelpers.getSymbol(tree), tree.getName() + "t", state)


Yes but then indeed the in -> int case is not tested nicely. Couldn't figure out a nicer way to do that. (Based on the changes not including this change I suspect you went through the same train of thoughts there 😉).

by moving reusable conflict detection code from `JUnitMethodDeclaration` to a new `ConflictDetection` class.

rickie · 2023-02-13T10:48:05Z

Rebased, will merge once 🍏.

github-actions · 2023-02-13T10:50:08Z

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

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.util.ConflictDetection`	0	20

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

eric-picnic requested a review from rickie January 27, 2023 15:53

rickie approved these changes Jan 30, 2023

View reviewed changes

rickie requested a review from Stephan202 January 30, 2023 09:27

Stephan202 force-pushed the eric-picnic/conflict-detection branch from 1f246e1 to 85dba89 Compare February 12, 2023 16:18

Stephan202 approved these changes Feb 12, 2023

View reviewed changes

Stephan202 added this to the 0.9.0 milestone Feb 12, 2023

rickie reviewed Feb 13, 2023

View reviewed changes

eric-picnic and others added 4 commits February 13, 2023 11:47

Introduce support for detecting conflicts when renaming methods

60eaff8

by moving reusable conflict detection code from `JUnitMethodDeclaration` to a new `ConflictDetection` class.

Suggestions

517770d

Restore JDK 11 compatibility

e757463

Squash the final mutant

9dfb06b

rickie force-pushed the eric-picnic/conflict-detection branch from 0df7030 to 9dfb06b Compare February 13, 2023 10:47

rickie changed the title ~~Introduce support for detecting conflicts when renaming methods~~ Introduce ConflictDetection utility class Feb 13, 2023

rickie merged commit 29469cb into master Feb 13, 2023

rickie deleted the eric-picnic/conflict-detection branch February 13, 2023 11:43

rickie added the improvement label Feb 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `ConflictDetection` utility class #478

Introduce `ConflictDetection` utility class #478

eric-picnic commented Jan 27, 2023 •

edited by rickie

github-actions bot commented Jan 27, 2023

rickie left a comment

rickie Jan 30, 2023

rickie Jan 30, 2023

rickie Jan 30, 2023

github-actions bot commented Jan 30, 2023

Stephan202 left a comment

Stephan202 Feb 11, 2023

Stephan202 Feb 11, 2023

Stephan202 Feb 12, 2023

rickie Feb 13, 2023

Stephan202 Feb 13, 2023

Stephan202 Feb 12, 2023

github-actions bot commented Feb 12, 2023

github-actions bot commented Feb 12, 2023

Stephan202 commented Feb 12, 2023

github-actions bot commented Feb 12, 2023

rickie left a comment

rickie Feb 13, 2023

rickie commented Feb 13, 2023

github-actions bot commented Feb 13, 2023

	* ConflictDetection#findMethodRenameBlocker(Symbol.MethodSymbol, String, VisitorState)}.
	* ConflictDetection#findMethodRenameBlocker(MethodSymbol, String, VisitorState)}.

	ASTHelpers.getSymbol(tree), tree.getName() + "t", state)
	ASTHelpers.getSymbol(tree), tree.getName() + "WithSuffix", state)

Introduce ConflictDetection utility class #478

Introduce ConflictDetection utility class #478

Conversation

eric-picnic commented Jan 27, 2023 • edited by rickie

github-actions bot commented Jan 27, 2023

rickie 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

github-actions bot commented Jan 30, 2023

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

github-actions bot commented Feb 12, 2023

github-actions bot commented Feb 12, 2023

Stephan202 commented Feb 12, 2023

github-actions bot commented Feb 12, 2023

rickie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickie commented Feb 13, 2023

github-actions bot commented Feb 13, 2023

Introduce `ConflictDetection` utility class #478

Introduce `ConflictDetection` utility class #478

eric-picnic commented Jan 27, 2023 •

edited by rickie