Introduce `AssociativeMethodInvocation` check #560

Stephan202 · 2023-04-01T13:15:41Z

~~❗ This PR is on top of #561. ❗~~

Suggested commit message:

Introduce `AssociativeMethodInvocation` check (#560)

This is in preparation for a bug checker that collapses multiple @BeforeTemplate methods into a single Refaster.anyOf method; such code changes may require flatting of nested Refaster.anyOf expressions; something taken care of by this check.

github-actions · 2023-04-01T13:17:53Z

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

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.AssociativeMethodInvocation`	0	19

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

github-actions · 2023-04-01T13:22:33Z

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

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.AssociativeMethodInvocation`	0	19

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

github-actions · 2023-04-02T14:50:12Z

Surviving mutants in this change: 6
Killed mutants in this change: 34

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.util.SourceCode`	6	17
🎉`tech.picnic.errorprone.bugpatterns.AssociativeMethodInvocation`	0	17

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

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssociativeMethodInvocation.java

Stephan202

Rebased; PR now targets master. Tnx for the review @oxkitsune 🙇

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssociativeMethodInvocation.java

github-actions · 2023-04-07T18:35:12Z

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

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.AssociativeMethodInvocation`	0	17

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

sonarqubecloud · 2023-04-08T12:40:27Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication

rickie

Added a commit and some thoughts. PTAL 😄

rickie · 2023-04-18T06:10:33Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssociativeMethodInvocation.java

+@BugPattern(
+    summary =
+        "These methods implement an associative operation, so you can flatten the list of operands",
+    link = BUG_PATTERNS_BASE_URL + "AssociativeOperatorUsage",


Suggested change

link = BUG_PATTERNS_BASE_URL + "AssociativeOperatorUsage",

link = BUG_PATTERNS_BASE_URL + "AssociativeMethodInvocation",

AssociativeMethodInvocation sounds like the better one indeed :).

rickie · 2023-04-18T06:34:07Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssociativeMethodInvocation.java

+    implements MethodInvocationTreeMatcher {
+  private static final long serialVersionUID = 1L;
+  private static final Supplier<Type> ITERABLE = Suppliers.typeFromClass(Iterable.class);
+  private static final ImmutableList<Matcher<ExpressionTree>> ASSOCIATIVE_OPERATIONS =


Technically speaking this could also be a ImmutableSet right? Any reason for not doing so?

Not really. Perhaps because these Matchers don't implement a properly equality test anyway. But will change :)

rickie · 2023-04-18T06:39:10Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssociativeMethodInvocation.java

+
+  @Override
+  public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
+    for (Matcher<ExpressionTree> matcher : ASSOCIATIVE_OPERATIONS) {


Should we start with the following if statement for fast return (tests still pass)?

if (tree.getArguments().isEmpty()) { return Description.NO_MATCH; }

Pitest will flag it, but I agree 👍

rickie · 2023-04-18T06:50:53Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssociativeMethodInvocation.java

+ * A {@link BugChecker} that flags unnecessarily nested usage of methods that implement an
+ * associative operation.
+ *
+ * <p>The arguments to such methods can be flattened without affecting semantics, while making the


Suggested change

* <p>The arguments to such methods can be flattened without affecting semantics, while making the

* <p>The arguments of such methods can be flattened without affecting semantics, while making the

Why is it "to" in this context, that doesn't sound right 🤔.

I know that both forms are commonly used (with "of" being more prevalent), but IMHO "to" is better here: we're passing arguments to a method, and they aren't "owned" by the method.

rickie · 2023-04-18T07:01:48Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssociativeMethodInvocation.java

+@AutoService(BugChecker.class)
+@BugPattern(
+    summary =
+        "These methods implement an associative operation, so you can flatten the list of operands",


I'm not really a fan of the "you" in this sentence. What about:

These methods implement an associative operation, so the list of operands can be flattened

It doesn't read as nice, but it avoids the term you 😋.

github-actions · 2023-04-18T07:05:23Z

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

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.AssociativeMethodInvocation`	0	17

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

Stephan202

Rebased and added a commit. Tnx for the review!

Stephan202 · 2023-04-18T08:51:45Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssociativeMethodInvocation.java

+ * A {@link BugChecker} that flags unnecessarily nested usage of methods that implement an
+ * associative operation.
+ *
+ * <p>The arguments to such methods can be flattened without affecting semantics, while making the


I know that both forms are commonly used (with "of" being more prevalent), but IMHO "to" is better here: we're passing arguments to a method, and they aren't "owned" by the method.

Stephan202 · 2023-04-18T08:53:59Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssociativeMethodInvocation.java

+    implements MethodInvocationTreeMatcher {
+  private static final long serialVersionUID = 1L;
+  private static final Supplier<Type> ITERABLE = Suppliers.typeFromClass(Iterable.class);
+  private static final ImmutableList<Matcher<ExpressionTree>> ASSOCIATIVE_OPERATIONS =


Not really. Perhaps because these Matchers don't implement a properly equality test anyway. But will change :)

Stephan202 · 2023-04-18T08:54:12Z

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssociativeMethodInvocation.java

+
+  @Override
+  public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
+    for (Matcher<ExpressionTree> matcher : ASSOCIATIVE_OPERATIONS) {


Pitest will flag it, but I agree 👍

github-actions · 2023-04-18T09:00:03Z

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

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

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

Stephan202 added this to the 0.10.0 milestone Apr 1, 2023

Stephan202 requested a review from rickie April 1, 2023 13:15

Stephan202 mentioned this pull request Apr 2, 2023

Introduce SourceCode#unwrapMethodInvocation utility method #561

Merged

Stephan202 force-pushed the sschroevers/associative-operators branch from d99f65c to fc6263d Compare April 2, 2023 14:48

Stephan202 changed the base branch from master to sschroevers/introduce-unwrap-method-invocation-utility April 2, 2023 14:49

rickie requested a review from oxkitsune April 6, 2023 07:31

oxkitsune approved these changes Apr 6, 2023

View reviewed changes

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssociativeMethodInvocation.java Show resolved Hide resolved

rickie force-pushed the sschroevers/introduce-unwrap-method-invocation-utility branch from c731b57 to 5ae92ab Compare April 7, 2023 12:48

Base automatically changed from sschroevers/introduce-unwrap-method-invocation-utility to master April 7, 2023 13:05

Stephan202 force-pushed the sschroevers/associative-operators branch from fc6263d to 485594e Compare April 7, 2023 18:29

Stephan202 commented Apr 7, 2023

View reviewed changes

...ne-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/AssociativeMethodInvocation.java Show resolved Hide resolved

rickie reviewed Apr 18, 2023

View reviewed changes

Stephan202 and others added 5 commits April 18, 2023 10:37

Introduce AssociativeMethodInvocation check

9ecd478

Doh

b25e8b4

Post-rebase cleanup

fed00d1

Update URL name

1153b4d

Address feedback

e5c8d06

Stephan202 force-pushed the sschroevers/associative-operators branch from 5147ccd to e5c8d06 Compare April 18, 2023 08:57

Stephan202 commented Apr 18, 2023

View reviewed changes

rickie approved these changes Apr 19, 2023

View reviewed changes

rickie added the new feature label Apr 19, 2023

rickie merged commit ee0884e into master Apr 19, 2023

rickie deleted the sschroevers/associative-operators branch April 19, 2023 06:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `AssociativeMethodInvocation` check #560

Introduce `AssociativeMethodInvocation` check #560

Stephan202 commented Apr 1, 2023 •

edited

Loading

github-actions bot commented Apr 1, 2023

github-actions bot commented Apr 1, 2023

github-actions bot commented Apr 2, 2023

Stephan202 left a comment

github-actions bot commented Apr 7, 2023

sonarqubecloud bot commented Apr 8, 2023

rickie left a comment

rickie Apr 18, 2023

rickie Apr 18, 2023

Stephan202 Apr 18, 2023

rickie Apr 18, 2023

Stephan202 Apr 18, 2023

rickie Apr 18, 2023

Stephan202 Apr 18, 2023

rickie Apr 18, 2023

github-actions bot commented Apr 18, 2023

Stephan202 left a comment

Stephan202 Apr 18, 2023

Stephan202 Apr 18, 2023

Stephan202 Apr 18, 2023

github-actions bot commented Apr 18, 2023

	link = BUG_PATTERNS_BASE_URL + "AssociativeOperatorUsage",
	link = BUG_PATTERNS_BASE_URL + "AssociativeMethodInvocation",

	* <p>The arguments to such methods can be flattened without affecting semantics, while making the
	* <p>The arguments of such methods can be flattened without affecting semantics, while making the

Introduce AssociativeMethodInvocation check #560

Introduce AssociativeMethodInvocation check #560

Conversation

Stephan202 commented Apr 1, 2023 • edited Loading

github-actions bot commented Apr 1, 2023

github-actions bot commented Apr 1, 2023

github-actions bot commented Apr 2, 2023

Stephan202 left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 7, 2023

sonarqubecloud bot commented Apr 8, 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

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 Apr 18, 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

github-actions bot commented Apr 18, 2023

Introduce `AssociativeMethodInvocation` check #560

Introduce `AssociativeMethodInvocation` check #560

Stephan202 commented Apr 1, 2023 •

edited

Loading