Introduce `DirectReturn` check #513

Stephan202 · 2023-02-26T16:09:17Z

This PR replaces #1. I decided to pick this task up because the topic was raised after @rickie's recent Amsterdam JUG presentation. CC @SimonBaars.

Suggested commit message:

Introduce `DirectReturn` check (#513)

github-actions · 2023-02-26T16:11:50Z

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

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.DirectReturn`	0	26

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

Stephan202 · 2023-02-26T16:11:56Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java

+    linkType = CUSTOM,
+    severity = SUGGESTION,
+    tags = SIMPLIFICATION)
+public final class DirectReturn extends BugChecker implements BlockTreeMatcher {


Naming-wise, "something something unnecessary assignment" may be nicer. But (a) we're not performing a flow analysis that flags all unnecessary variable assignments and (b) a name that accurately describes the subset covered would be quite unwieldy. Unless I'm just not creative enough, of course 🙃. Suggestions welcome.

Stephan202 · 2023-03-06T05:28:02Z

Just realized: ideally we do not suggest inlining if (a) the variable assigned to is of a different type than the method's return type and (b) this difference may influence the return value. Prime example here is Mockito's nullary mock() method (see also #454). I'll come back to this PR to see how we can best cover this case.

Stephan202 · 2023-03-11T20:03:01Z

Rebased and added a commit to cover this case. Did make it Mockito-specifc, because attempts at generalization became too contrived.

github-actions · 2023-03-11T20:05:33Z

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

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.util.MoreASTHelpers`	0	5
🎉`tech.picnic.errorprone.bugpatterns.DirectReturn`	0	34
🎉`tech.picnic.errorprone.bugpatterns.MockitoMockClassReference`	0	12

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

Stephan202

Added one more commit.

Stephan202 · 2023-03-12T09:27:43Z

...rone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MockitoMockClassReference.java

@@ -49,7 +48,7 @@
 public final class MockitoMockClassReference extends BugChecker


CC @Badbond: I renamed/moved some logic in this class.

Stephan202 · 2023-03-12T09:28:52Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreASTHelpers.java

+  public static Optional<MethodTree> findMethodExitedOnReturn(VisitorState state) {
+    return Optional.ofNullable(state.findEnclosing(LambdaExpressionTree.class, MethodTree.class))
+        .filter(MethodTree.class::isInstance)
+        .map(MethodTree.class::cast);
+  }


I decided to move this out because it handles quite a gotcha; better to not duplicate this logic.

github-actions · 2023-03-12T10:03:37Z

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

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.util.MoreASTHelpers`	0	5
🎉`tech.picnic.errorprone.bugpatterns.DirectReturn`	0	34
🎉`tech.picnic.errorprone.bugpatterns.MockitoMockClassReference`	0	12

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

rickie

Flushing one comment I had. Will continue later today.

rickie · 2023-03-27T18:59:18Z

...-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/MoreASTHelpersTest.java

+        .addSourceLines(
+            "A.java",
+            "class A {",
+            "  void negative1(String a, Integer b) {}",


Might be good to add a test where one is a subtype of the other? (Will add this in a commit)

werli

The code itself and the added test cases were rather straightforward, nice work @Stephan202 👍

One more point about the exhaustiveness of the mockito exclusion, but I wouldn't expect more changes here as part of this PR. Hence I'm approving.

werli · 2023-03-30T15:28:03Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java

+   * <p>Inlining is generally safe, but in rare cases the operation may have a functional impact.
+   * The sole case considered here is the inlining of a Mockito mock or spy construction without an
+   * explicit type. In such a case the type created depends on context, such as the method's return
+   * type.


Great to have added this exclusion. Are we sure the exclusion is exhaustive however? My guess is: probably never fully, but this is a best-effort solution that should handle the majority of EPS users.

Nope, this is not exhaustive, unfortunately. I did play with some more generic code, but it became way more complex than I could justify for this feature. So in the end gave up on that idea.

Stephan202

Tnx for the reviews!

Stephan202 · 2023-03-30T16:36:47Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/DirectReturn.java

+   * <p>Inlining is generally safe, but in rare cases the operation may have a functional impact.
+   * The sole case considered here is the inlining of a Mockito mock or spy construction without an
+   * explicit type. In such a case the type created depends on context, such as the method's return
+   * type.


Nope, this is not exhaustive, unfortunately. I did play with some more generic code, but it became way more complex than I could justify for this feature. So in the end gave up on that idea.

rickie

Really cool check 🚀.

Have some small comments and an extra test case or two. Let me know what you think 😄.

rickie · 2023-03-30T18:06:44Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DirectReturnTest.java

+            "    return variable;",
+            "  }",
+            "",
+            "  Object salienSpyTypeVariableDeclaration() {",


Suggested change

" Object salienSpyTypeVariableDeclaration() {",

" Object salientSpyTypeVariableDeclaration() {",

rickie · 2023-03-30T18:19:06Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DirectReturnTest.java

+            "      // BUG: Diagnostic contains:",
+            "      variable = \"foo\";",
+            "      return variable;",
+            "    }",


I think this is also an interesting case to add:

Supplier<String> redundantAssignmentInLambda() { return () -> { // BUG: Diagnostic contains: String variable = toString(); return variable; };

rickie · 2023-03-30T18:20:17Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreASTHelpers.java

@@ -46,4 +50,33 @@ public static ImmutableList<MethodTree> findMethods(CharSequence methodName, Vis
  public static boolean methodExistsInEnclosingClass(CharSequence methodName, VisitorState state) {
    return !findMethods(methodName, state).isEmpty();
  }
+
+  /**
+   * Returns the {@link MethodTree} from which control flow would exit if there would be a {@code


Suggested change

* Returns the {@link MethodTree} from which control flow would exit if there would be a {@code

* Returns the {@link MethodTree} from which the control flow would exit if there would be a {@code

Sounds a bit better to me 🤔.

github-actions · 2023-03-30T18:27:08Z

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

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.util.MoreASTHelpers`	0	5
🎉`tech.picnic.errorprone.bugpatterns.DirectReturn`	0	34
🎉`tech.picnic.errorprone.bugpatterns.MockitoMockClassReference`	0	12

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

rickie · 2023-03-30T18:28:43Z

Aii, misclicked and merged instead of rebased 😅.

github-actions · 2023-03-30T18:31:21Z

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

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.util.MoreASTHelpers`	0	5
🎉`tech.picnic.errorprone.bugpatterns.DirectReturn`	0	34
🎉`tech.picnic.errorprone.bugpatterns.MockitoMockClassReference`	0	12

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

Stephan202

Tnx; changes LGTM otherwise

Stephan202 · 2023-03-30T18:33:25Z

error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/MoreASTHelpers.java

@@ -46,4 +50,33 @@ public static ImmutableList<MethodTree> findMethods(CharSequence methodName, Vis
  public static boolean methodExistsInEnclosingClass(CharSequence methodName, VisitorState state) {
    return !findMethods(methodName, state).isEmpty();
  }
+
+  /**
+   * Returns the {@link MethodTree} from which the control flow would exit if there would be a


I think we should omit "the" here. (Cause this is not about a "definite" control flow.)

Stephan202 · 2023-03-30T18:34:41Z

error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/DirectReturnTest.java

+            "    }",
+            "  }",
+            "",
+            "  Supplier<String> redundantAssignmentInLambda() {",


Suggested change

" Supplier<String> redundantAssignmentInLambda() {",

" Supplier<String> redundantAssignmentInsideLambda() {",

Also matches the method above.

rickie · 2023-03-30T18:42:46Z

Applied and rebased. Will merge once 🟢, thanks for taking a look so quickly.

Stephan202 · 2023-03-30T18:43:29Z

Let's gooooo 🚀

github-actions · 2023-03-30T18:45:19Z

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

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.util.MoreASTHelpers`	0	5
🎉`tech.picnic.errorprone.bugpatterns.DirectReturn`	0	34
🎉`tech.picnic.errorprone.bugpatterns.MockitoMockClassReference`	0	12

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

Stephan202 added this to the 0.9.0 milestone Feb 26, 2023

Stephan202 requested a review from rickie February 26, 2023 16:09

Stephan202 commented Feb 26, 2023

View reviewed changes

Stephan202 mentioned this pull request Feb 26, 2023

When variables are assigned then returned, instead directly return them #1

Closed

Stephan202 force-pushed the sschroevers/inline-variable-return-statements branch from b3c41d7 to 120e418 Compare March 11, 2023 20:02

Stephan202 commented Mar 12, 2023

View reviewed changes

werli self-requested a review March 30, 2023 12:27

rickie reviewed Mar 30, 2023

View reviewed changes

werli approved these changes Mar 30, 2023

View reviewed changes

Stephan202 commented Mar 30, 2023

View reviewed changes

rickie approved these changes Mar 30, 2023

View reviewed changes

rickie added the new feature label Mar 30, 2023

Stephan202 commented Mar 30, 2023

View reviewed changes

Stephan202 and others added 5 commits March 30, 2023 20:42

Introduce DirectReturn check

4aca597

Handle mock statements

cec2ac9

Improve test

c974ea0

Suggestions

143393f

Tweaks

2507a85

rickie force-pushed the sschroevers/inline-variable-return-statements branch from 10ddbf3 to 2507a85 Compare March 30, 2023 18:42

rickie merged commit 73cf28e into master Mar 30, 2023

rickie deleted the sschroevers/inline-variable-return-statements branch March 30, 2023 18:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `DirectReturn` check #513

Introduce `DirectReturn` check #513

Stephan202 commented Feb 26, 2023 •

edited

Loading

github-actions bot commented Feb 26, 2023

Stephan202 Feb 26, 2023

Stephan202 commented Mar 6, 2023

Stephan202 commented Mar 11, 2023

github-actions bot commented Mar 11, 2023

Stephan202 left a comment

Stephan202 Mar 12, 2023

Stephan202 Mar 12, 2023

github-actions bot commented Mar 12, 2023

rickie left a comment

rickie Mar 27, 2023

werli left a comment

werli Mar 30, 2023

Stephan202 Mar 30, 2023

Stephan202 left a comment

Stephan202 Mar 30, 2023

rickie left a comment

rickie Mar 30, 2023

rickie Mar 30, 2023

rickie Mar 30, 2023

github-actions bot commented Mar 30, 2023

rickie commented Mar 30, 2023

github-actions bot commented Mar 30, 2023

Stephan202 left a comment

Stephan202 Mar 30, 2023

Stephan202 Mar 30, 2023

rickie commented Mar 30, 2023

Stephan202 commented Mar 30, 2023

github-actions bot commented Mar 30, 2023

		@@ -49,7 +48,7 @@
		public final class MockitoMockClassReference extends BugChecker

	" Object salienSpyTypeVariableDeclaration() {",
	" Object salientSpyTypeVariableDeclaration() {",

	* Returns the {@link MethodTree} from which control flow would exit if there would be a {@code
	* Returns the {@link MethodTree} from which the control flow would exit if there would be a {@code

	" Supplier<String> redundantAssignmentInLambda() {",
	" Supplier<String> redundantAssignmentInsideLambda() {",

Introduce DirectReturn check #513

Introduce DirectReturn check #513

Conversation

Stephan202 commented Feb 26, 2023 • edited Loading

github-actions bot commented Feb 26, 2023

Choose a reason for hiding this comment

Stephan202 commented Mar 6, 2023

Stephan202 commented Mar 11, 2023

github-actions bot commented Mar 11, 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

github-actions bot commented Mar 12, 2023

rickie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

werli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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 Mar 30, 2023

rickie commented Mar 30, 2023

github-actions bot commented Mar 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

rickie commented Mar 30, 2023

Stephan202 commented Mar 30, 2023

github-actions bot commented Mar 30, 2023

Introduce `DirectReturn` check #513

Introduce `DirectReturn` check #513

Stephan202 commented Feb 26, 2023 •

edited

Loading