Introduce `MockitoMockClassReference` check #454

Badbond · 2023-01-07T14:39:11Z

~~Opening PR as draft already to get some feedback on the best approach for the unimplemented test cases regarding type arguments and generics (see corresponding test).~~

Suggested commit message:

Introduce `MockitoMockClassReference` check (#454)

Flags Mockito mock and spy creation expressions that explicitly specify the
type of mock or spy to create, while this information can also be inferred from
context.

Description

Adds a bug check that flags usages of Mockito#mock(class) and Mockito#spy(Class) which can be replaced with the no-arg overload, as introduced in Mockito 4.10.0 (mockito/mockito#2779). This is possible when the type this mock is assigned to is known and equal to the class passed to this method.

Some simple cases:

-    Foo foo = mock(Foo.class);
+    Foo foo = mock();

  Foo getFoo() {
-    return mock(Foo.class);
+    return mock(); 
  }

Edge-cases

Subtypes

We don't want to replace cases where the class passed is a subtype of the class that the mock is assigned to. For example (where Foo extends|implements Bar):

Bar bar = mock(Foo.class);
assertThat(bar).isInstanceOf(Foo.class);

Changing this would otherwise make the assertion fail.

Classes with generic type arguments

It is safe to mock classes with generic type parameters (e.g. List<E>) as the mock should be programmed either way based on the type of the type parameter. Therefore these are currently rewritten. For example:

-    List<Foo> foos = mock(List.class);
-    List bars = mock(List.class);
+    List<Foo> foos = mock(); 
+    List bars = mock();

Variably typed mocks

When the type of the mock is variable, especially when encapsulated in a method parameter, there can be problems when removing the argument. Therefore, currently, these cases are not supported. A simple case that could be rewritten is:

-Class<BigInteger> clazz = BigInteger.class;
-BigInteger mock = mock(clazz);
+BigInteger mock = mock();
}

However, when the variable is dynamic, it could lead to problems. For example:

-<T> T getMock(class<T> clazz) {
-  return mock(clazz);
+<T> T getMock() {
+  return mock();
}


@Test
void mockType() {
-  assertThat(getMock(BigInteger.class)).isInstanceOf(BigInteger.class);
+  assertThat((BigInteger) getMock()).isInstanceOf(BigInteger.class);
}

Non-explicitly typed mocks

When the mock is assigned to a non-explicitly typed variable (i.e. when using var), we can not simply drop the mock argument as otherwise Mockito could not derive its type. Thus we will not rewrite:

var foo = mock(Foo.class);

Badbond · 2023-01-07T14:39:51Z

...-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MockitoMockClassReferenceTest.java

+  }
+
+  @Test
+  void unimplementedCases() {


I'd like some input as to how we wish to behave in these edge-cases and what would be the ideal way to detect those cases.

Badbond · 2023-01-07T14:42:31Z

...-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MockitoMockClassReferenceTest.java

+            "  <T extends Number> T getGenericMock(Class<T> clazz) {",
+            "    // No idea how to detect type T as part of generics usage.",
+            "    return mock(clazz);",
+            "  }",


Coming to think of it, if it were not such a complex case where T extends Number, but where it was just T, then we could likely rewrite it:

<T> T getGenericMock(Class<T> clazz) { - return mock(clazz); + return mock(); }

Perhaps this case is just similar as variableTypedMock above but instead with a generic result type. So we might be able to simplify these test cases a bit and indeed follow the ASTHelpers.isSameType approach where it compared the erasure of both types.

Badbond · 2023-01-07T14:43:51Z

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

+  // XXX: Replace `var` usage with explicit type instead.
+  private static final Matcher<VariableTree> INCOMPATIBLE_VARIABLE_TREE =
+      anyOf(ASTHelpers::hasNoExplicitType, MockitoMockClassReference::hasTypeDifference);


As discussed with @Stephan202 offline. It might be cool to also rewrite the following:

- var foo = mock(Foo.class); + Foo foo = mock();

Didn't have the brain juice to go after this as it consists of multiple fixes. Feel free to have a crack at it. If someone wants to pair up, I'm open to that too. Otherwise, I'd favor leaving the XXX and revisiting this at a later time (and creating a GH issue for it).

Badbond · 2023-01-07T14:45:51Z

...-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MockitoMockClassReferenceTest.java

+            "    // This case is arguable as it is unsafe in any case.",
+            "    // This currently is not identified as the `erasure` is the same.",
+            "    List<String> genericallyTypedMock = mock(List.class);",


As pointed out, this is an unsafe cast in both before and after case. As such, at least in our own codebases, we sometimes have casts with SuppressWarnings. If we are to rewrite these and we can circumvent these warnings, it might be nice to also remove existing suppressions.

Hmm, I would have expected dropping List.class to be safe here. And if we can drop a suppression in this case: great!

I don't think we should rewrite this case, if we would, it'd require a suppression as you mention. That is not an improvement IMO, so I wouldn't bother for now.

The same holds for the getGenericMock so I'm not sure if we would gain anything from that. Also, if we apply that change, then the parameter would be unused 🤔. So all in all, not sure if it would be an improvement.

I filed mockito/mockito#2866; with those changes dropping List.class would be fine.

Stephan202

Very nice @Badbond! I'll try to find some time to play with the code for proper advice; not sure I'll have time for that this weekend, though. But let's see 🤞

Stephan202 · 2023-01-07T15:24:15Z

...-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MockitoMockClassReferenceTest.java

+            "    // This case is arguable as it is unsafe in any case.",
+            "    // This currently is not identified as the `erasure` is the same.",
+            "    List<String> genericallyTypedMock = mock(List.class);",


Hmm, I would have expected dropping List.class to be safe here. And if we can drop a suppression in this case: great!

rickie

Added a commit with some suggestions. Also some opinions on open stuff :).

rickie · 2023-01-09T08:27:56Z

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

+          argument(0, isSameType(Class.class.getName())),
+          staticMethod().onClass("org.mockito.Mockito").namedAnyOf("mock", "spy"));
+  // XXX: Replace `var` usage with explicit type instead.
+  private static final Matcher<VariableTree> INCOMPATIBLE_VARIABLE_TREE =


Instead of using the tree type in the name, we generally give it a name that describes what it flags :). Pushing a proposal.

I think dropping TREE already makes it clear here :).

rickie · 2023-01-09T09:37:58Z

...-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MockitoMockClassReferenceTest.java

+            "    // This case is arguable as it is unsafe in any case.",
+            "    // This currently is not identified as the `erasure` is the same.",
+            "    List<String> genericallyTypedMock = mock(List.class);",


I don't think we should rewrite this case, if we would, it'd require a suppression as you mention. That is not an improvement IMO, so I wouldn't bother for now.

rickie · 2023-01-09T09:52:09Z

...-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MockitoMockClassReferenceTest.java

+            "    // This case is arguable as it is unsafe in any case.",
+            "    // This currently is not identified as the `erasure` is the same.",
+            "    List<String> genericallyTypedMock = mock(List.class);",


The same holds for the getGenericMock so I'm not sure if we would gain anything from that. Also, if we apply that change, then the parameter would be unused 🤔. So all in all, not sure if it would be an improvement.

github-actions · 2023-01-15T10:16:00Z

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

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.MockitoMockClassReference`	0	22

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

Badbond

I am happy with the current state of the PR to continue its review 👍. Thanks for filing the Mockito PR upstream @Stephan202.

Badbond · 2023-01-15T10:26:03Z

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

@@ -45,11 +51,13 @@ public final class MockitoMockClassReference extends BugChecker
  private static final Matcher<MethodInvocationTree> MOCKITO_MOCK_OR_SPY =
      allOf(
          argumentCount(1),
-          argument(0, isSameType(Class.class.getName())),
+          argument(0, allOf(isSameType(Class.class.getName()), not(isVariable()))),


There can be caveats when the passed argument is a variable that can lead into non-compilable/working-with-Mockito code, e.g. the following is odd and won't work:

-<T> T getMock(class<T> clazz) { - return mock(clazz); +<T> T getMock() { + return mock(); } @Test void mockType() { - assertThat(getMock(BigInteger.class)).isInstanceOf(BigInteger.class); + assertThat((BigInteger) getMock()).isInstanceOf(BigInteger.class); }

There might be cases where we could still replace the variable, but I couldn't find an elegant way for it without going into the caveats. For example the trivial:

-Class<BigInteger> clazz = BigInteger.class; -BigInteger mock = mock(clazz); +BigInteger mock = mock(); }

Feel free to play around and see if you find an elegant way.

Yeah, I think it's fair to leave this out of scope.

Badbond · 2023-01-15T10:31:03Z

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

+  }
+
+  private static boolean hasTypeDifference(ReturnTree tree, VisitorState state) {
+    Tree returnTypeTree = requireNonNull(state.findEnclosing(MethodTree.class)).getReturnType();


This requireNonNull is safe because passed is a ReturnTree. Alternatively we could have passed the matching MethodInvocationTree (to prevent having to do tree.getExpression again), but then finding the enclosing MethodTree is less 'safe' in case this method gets reused. I found this to be cleaner and clearer given the similar method signatures.

Furthermore, inlining as a lambda expression instead was flagged by ErrorProne, and indeed I do find this to read better.

This will not select the correct type in case of a return statement inside a lambda expression, such as this case:

Stream.of("foo").map(x -> { return mock(Integer.class ); }).toArray();

(And if this code is part of an initializer block, then state.findEnclosing(MethodTree.class) will return null 🙃.)

Badbond · 2023-01-15T10:37:06Z

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

+  // XXX: Replace `var` usage with explicit type instead.
+  private static final Matcher<VariableTree> INCOMPATIBLE_VARIABLE_TREE =
+      anyOf(ASTHelpers::hasNoExplicitType, MockitoMockClassReference::hasTypeDifference);


Didn't have the brain juice to go after this as it consists of multiple fixes. Feel free to have a crack at it. If someone wants to pair up, I'm open to that too. Otherwise, I'd favor leaving the XXX and revisiting this at a later time (and creating a GH issue for it).

Stephan202

Had a quick look; I don't mind proposing a fix, but @Badbond if you wan to have a crack at it yourself, lemme know :)

Stephan202 · 2023-01-15T11:08:45Z

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

+    Tree parent = state.getPath().getParentPath().getLeaf();
+    if (parent.getKind() == VARIABLE
+        && INCOMPATIBLE_VARIABLE.matches((VariableTree) parent, state)) {
+      return Description.NO_MATCH;
+    } else if (parent.getKind() == RETURN
+        && INCOMPATIBLE_RETURN.matches((ReturnTree) parent, state)) {
+      return Description.NO_MATCH;
+    }
+
+    return describeMatch(tree, SuggestedFix.delete(Iterables.getOnlyElement(tree.getArguments())));


The code now assumes a match in case the parent is not a VARIABLE or RETURN statement. So the following statement is also (incorrectly) flagged:

Objects.hash(mock(Integer.class));

I found another edge case, but I'm not sure if it is a problem or not... I was just playing around and did this random thing:

String s = new String(mock(String.class));

However, if we drop String.class the new String( can't detect which overload to use. Is that something we should fix? I'm not sure how realistic / niche this is 😋.

After some rewriting and trying stuff out I found out we are strictly speaking too many cases. So the example in my previous comment is the Kind.NEW_CLASS edge case. We should only rewrite things if we are sure it will be correct, so swapping the default here.

While doing so, I changed the setup to not use Matchers and have multiple hasTypeDifference methods. PTAL and let me know what you think.

Badbond · 2023-01-15T12:03:57Z

Had a quick look; I don't mind proposing a fix, but @Badbond if you wan to have a crack at it yourself, lemme know :)

Feel free to!

Stephan202 · 2023-01-15T13:21:06Z

On the list it goes 😄

github-actions · 2023-01-27T08:03:49Z

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

class	surviving	killed
🎉`tech.picnic.errorprone.bugpatterns.MockitoMockClassReference`	0	22

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

rickie

Did a rewrite based on some (open) comments and edge cases that I spotted 😄. Let me know what you think.

Added a commit and rebased.

rickie · 2023-01-27T08:17:24Z

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

+    Tree parent = state.getPath().getParentPath().getLeaf();
+    if (parent.getKind() == VARIABLE
+        && INCOMPATIBLE_VARIABLE.matches((VariableTree) parent, state)) {
+      return Description.NO_MATCH;
+    } else if (parent.getKind() == RETURN
+        && INCOMPATIBLE_RETURN.matches((ReturnTree) parent, state)) {
+      return Description.NO_MATCH;
+    }
+
+    return describeMatch(tree, SuggestedFix.delete(Iterables.getOnlyElement(tree.getArguments())));


I found another edge case, but I'm not sure if it is a problem or not... I was just playing around and did this random thing:

String s = new String(mock(String.class));

However, if we drop String.class the new String( can't detect which overload to use. Is that something we should fix? I'm not sure how realistic / niche this is 😋.

rickie · 2023-01-27T12:06:25Z

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

+    return describeMatch(tree, SuggestedFix.delete(Iterables.getOnlyElement(tree.getArguments())));
+  }
+
+  private static boolean hasTypeDifference(VariableTree tree, VisitorState state) {


After adding some random test cases I found some more examples / cases that should be covered. I'll push a change where this is addressed.

rickie · 2023-01-27T12:08:27Z

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

+    Tree parent = state.getPath().getParentPath().getLeaf();
+    if (parent.getKind() == VARIABLE
+        && INCOMPATIBLE_VARIABLE.matches((VariableTree) parent, state)) {
+      return Description.NO_MATCH;
+    } else if (parent.getKind() == RETURN
+        && INCOMPATIBLE_RETURN.matches((ReturnTree) parent, state)) {
+      return Description.NO_MATCH;
+    }
+
+    return describeMatch(tree, SuggestedFix.delete(Iterables.getOnlyElement(tree.getArguments())));


After some rewriting and trying stuff out I found out we are strictly speaking too many cases. So the example in my previous comment is the Kind.NEW_CLASS edge case. We should only rewrite things if we are sure it will be correct, so swapping the default here.

While doing so, I changed the setup to not use Matchers and have multiple hasTypeDifference methods. PTAL and let me know what you think.

github-actions · 2023-01-27T12:11:43Z

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

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.MockitoMockClassReference`	1	28

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 finish reviewing yet :)

Stephan202 · 2023-02-18T19:45:11Z

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

+    return hasTypeDifference(returnTypeTree, ((ReturnTree) parent).getExpression(), state);
+  }
+
+  private static boolean hasTypeDifference(Tree treeA, Tree treeB, VisitorState state) {


Here and above: it's generally preferred to use "positive" names (like hasSameType).

Stephan202 · 2023-02-18T19:51:32Z

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

+    if (parentKind != VARIABLE && parentKind != ASSIGNMENT && parentKind != RETURN) {
+      return Description.NO_MATCH;
+    }


We can combine this check with the if/else logic below using a switch.

Stephan202 · 2023-02-18T19:59:41Z

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

+  }
+
+  private static boolean hasTypeDifference(ReturnTree tree, VisitorState state) {
+    Tree returnTypeTree = requireNonNull(state.findEnclosing(MethodTree.class)).getReturnType();


This will not select the correct type in case of a return statement inside a lambda expression, such as this case:

Stream.of("foo").map(x -> { return mock(Integer.class ); }).toArray();

(And if this code is part of an initializer block, then state.findEnclosing(MethodTree.class) will return null 🙃.)

github-actions · 2023-02-18T20:58:12Z

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

class	surviving	killed
🧟`tech.picnic.errorprone.bugpatterns.MockitoMockClassReference`	1	21

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

Stephan202

Added one more commit. Also tweaked the suggested commit messages, as we now cover more methods. Eager to see this check in use 😄

Stephan202 · 2023-02-19T13:20:59Z

...-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MockitoMockClassReferenceTest.java

+            "  // BUG: Diagnostic contains:",
+            "  String memberMock = mock(String.class);",
+            "  // BUG: Diagnostic contains:",
+            "  String memberSpy = mock(String.class);",


Suggested change

" String memberSpy = mock(String.class);",

" String memberSpy = spy(String.class);",

Stephan202 · 2023-02-19T13:29:56Z

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

+    summary =
+        "Don't unnecessarily pass the type reference to Mockito's `mock(Class)` and spy(Class)`",


Suggested change

summary =

"Don't unnecessarily pass the type reference to Mockito's `mock(Class)` and spy(Class)`",

summary = "Don't unnecessarily pass a type to Mockito's `mock(Class)` and `spy(Class)` methods",

Stephan202 · 2023-02-19T13:36:31Z

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

@@ -45,11 +51,13 @@ public final class MockitoMockClassReference extends BugChecker
  private static final Matcher<MethodInvocationTree> MOCKITO_MOCK_OR_SPY =
      allOf(
          argumentCount(1),
-          argument(0, isSameType(Class.class.getName())),
+          argument(0, allOf(isSameType(Class.class.getName()), not(isVariable()))),


Yeah, I think it's fair to leave this out of scope.

Stephan202 · 2023-02-19T13:38:32Z

...-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MockitoMockClassReferenceTest.java

+            "    // BUG: Diagnostic contains:",
+            "    variableMock = mock(Integer.class);",
+            "    // BUG: Diagnostic contains:",
+            "    List nonGenericallyTypedMock = mock(List.class);",


Suggested change

" List nonGenericallyTypedMock = mock(List.class);",

" List rawListTypeMock = mock(List.class);",

Stephan202 · 2023-02-19T13:47:43Z

...-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MockitoMockClassReferenceTest.java

+            "    var nonExplicitlyTypedMock = mock(Integer.class);",
+            "    Class<? extends Number> variableType = Integer.class;",
+            "    Number variableTypedMock = mock(variableType);",
+            "    Integer namedMock = mock(Integer.class, \"name\");",


We can actually also simplify this one and the other two-arg overloads; it's barely any extra code. Will propose something :)

Stephan202 · 2023-02-19T15:07:05Z

...-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MockitoMockClassReferenceTest.java

+  }
+
+  @Test
+  void replacement() {


We can simplify this test, since this fix isn't (so) context dependent.

github-actions · 2023-02-19T15:26:46Z

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

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

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

github-actions · 2023-03-04T15:23:00Z

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

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

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

rickie

Do we want to create an XXX to identify when we can drop a preceding suppression like here?

(🔝 not a blocker, so approving)

Haha, passing the tree instead of calling either the getInitializer or getExpression with cast is big brain move 😄.

Nice improvements and very nice check @Badbond 🚀 !

rickie · 2023-03-04T15:52:26Z

...-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/MockitoMockClassReferenceTest.java

@@ -13,17 +13,13 @@ void identification() {
            "A.java",
            "import static org.mockito.Mockito.mock;",
            "import static org.mockito.Mockito.spy;",
+            "import static org.mockito.Mockito.withSettings;",


Note to self: always check for more edge cases that we can test 😛.

Stephan202 · 2023-03-05T10:51:48Z

Do we want to create an XXX to identify when we can drop a preceding suppression like here?

Good one. For this PR we should at least (manually?) clean up the impacted files. I see three options:

Extend this check to drop @SuppressWarnigs("unchecked") when making this change. Easiest, but rather specific.
Introduce a separate (ideally non-Mockito-specifc) check that identifies such unnecessary suppressions. Complexity TBD, but more in line with our "better to compose minimal improvements" philosophy.
Introduce an off-by-default shell script or compilesWithFix bug checker that flags an type of obsolete suppression. Complexity TBD.

Haha, passing the tree instead of calling either the getInitializer or getExpression with cast is big brain move 😄.

Tnx ;)

Stephan202 · 2023-03-05T10:55:42Z

For this PR we should at least (manually?) clean up the impacted files.

Didn't notice that the diff referenced another repo 🙃. Anyway, options remain the same.

github-actions · 2023-03-06T06:49:46Z

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

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

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

rickie · 2023-03-06T06:49:59Z

Added a commit with the XXX.

I'd like to propose the following. Create a GH issue for (1) and keep (2) for later. Although that is the best way to solve it, it sounds like quite an involved check that not everyone is able to pick up.

rickie · 2023-03-06T06:50:09Z

Added a commit with the XXX.

I'd like to propose the following. Create a GH issue for (1) and keep (2) for later. Although that is the best way to solve it, it sounds like quite an involved check that not everyone is able to pick up.

Stephan202

Having slept on it, I don't think (1) makes sense: an initial version of (2) could be one that only works for mock() calls.

Stephan202 · 2023-03-06T07:04:40Z

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

+// as to disambiguate between method overloads.
+// XXX: This check currently does not flag (implicit or explicit) lambda return expressions.
+// XXX: This check currently does not drop suppressions that become obsolete after the
+// suggested fix is applied, consider adding support for this.


Suggested change

// suggested fix is applied, consider adding support for this.

// suggested fix is applied; consider adding support for this.

github-actions · 2023-03-06T07:19:59Z

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

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

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

rickie · 2023-03-06T07:25:07Z

Having slept on it, I don't think (1) makes sense: an initial version of (2) could be one that only works for mock() calls.

Let's discuss offline what the best approach would be. For now I'd propose to merge once 💚 ! 🚀

Badbond · 2023-03-06T08:37:54Z

Thanks for the tweaks @Stephan202! PR LGTM for merge. Very nice that you flag additional cases of multi-arg mocking where we can still omit the class as well as the usage within lambda expressions. Amazing polish of tests, comments and casting too 😍.

Having slept on it, I don't think (1) makes sense: an initial version of (2) could be one that only works for mock() calls.

I would also think (2) is the best option to also gain the most value. Complexity indeed to-be-seen (likely as part of implementation), but it sounds to me that this can be a generic check flagging safe casts (to subtype/to same type/raw-to-raw) annotated with the unchecked suppression.

Badbond commented Jan 7, 2023

View reviewed changes

Stephan202 reviewed Jan 7, 2023

View reviewed changes

rickie changed the title ~~Introduce MockitoMockClassReference bug check~~ Introduce MockitoMockClassReference check Jan 9, 2023

rickie added the new feature label Jan 9, 2023

rickie added this to the 0.8.0 milestone Jan 9, 2023

rickie reviewed Jan 9, 2023

View reviewed changes

Badbond force-pushed the pdsoels/PSM-1879 branch from 040d1fd to 263bc5e Compare January 15, 2023 10:13

Badbond commented Jan 15, 2023

View reviewed changes

Badbond marked this pull request as ready for review January 15, 2023 10:38

Stephan202 reviewed Jan 15, 2023

View reviewed changes

rickie force-pushed the pdsoels/PSM-1879 branch from 263bc5e to d8bffbb Compare January 27, 2023 08:01

rickie modified the milestones: 0.8.0, 0.9.0 Jan 27, 2023

rickie reviewed Jan 27, 2023

View reviewed changes

Stephan202 force-pushed the pdsoels/PSM-1879 branch from ef8f41d to f2837e5 Compare February 18, 2023 20:55

Stephan202 reviewed Feb 18, 2023

View reviewed changes

Stephan202 approved these changes Feb 19, 2023

View reviewed changes

Badbond and others added 6 commits March 4, 2023 16:20

Introduce MockitoMockClassReference bug check

d742d6c

Add check for compatible variable tree types and extend test cases

87956a2

Add test for unimplemented edge-cases

20be36d

Self-apply and add XXX

54973a6

Add another XXX

70a4cb1

Suggestions

cabec75

Badbond and others added 4 commits March 4, 2023 16:20

Implement return type matches and do not flag variable class arguments

fedd8ef

Suggestion to restructure the check

e1b5f72

Suggestions

2c1d096

Suggestions (2)

fd70908

rickie force-pushed the pdsoels/PSM-1879 branch from 3c77c53 to fd70908 Compare March 4, 2023 15:20

rickie approved these changes Mar 4, 2023

View reviewed changes

Stephan202 mentioned this pull request Mar 6, 2023

Introduce DirectReturn check #513

Merged

Add XXX

180ddf6

Stephan202 reviewed Mar 6, 2023

View reviewed changes

Suggestion

f0c7563

rickie merged commit 61c9f67 into master Mar 6, 2023

rickie deleted the pdsoels/PSM-1879 branch March 6, 2023 08:54

	" String memberSpy = mock(String.class);",
	" String memberSpy = spy(String.class);",

		summary =
		"Don't unnecessarily pass the type reference to Mockito's `mock(Class)` and spy(Class)`",

	summary =
	"Don't unnecessarily pass the type reference to Mockito's `mock(Class)` and spy(Class)`",
	summary = "Don't unnecessarily pass a type to Mockito's `mock(Class)` and `spy(Class)` methods",

	" List nonGenericallyTypedMock = mock(List.class);",
	" List rawListTypeMock = mock(List.class);",

	// suggested fix is applied, consider adding support for this.
	// suggested fix is applied; consider adding support for this.

Introduce MockitoMockClassReference check #454

Introduce MockitoMockClassReference check #454

Conversation

Badbond commented Jan 7, 2023 • edited by Stephan202

Description

Edge-cases

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

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 Jan 15, 2023

Badbond 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

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

Badbond commented Jan 15, 2023

Stephan202 commented Jan 15, 2023

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 27, 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 Feb 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

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 19, 2023

github-actions bot commented Mar 4, 2023

rickie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stephan202 commented Mar 5, 2023

Stephan202 commented Mar 5, 2023

github-actions bot commented Mar 6, 2023

rickie commented Mar 6, 2023

rickie commented Mar 6, 2023

Stephan202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 6, 2023

rickie commented Mar 6, 2023

Badbond commented Mar 6, 2023

Introduce `MockitoMockClassReference` check #454

Introduce `MockitoMockClassReference` check #454

Badbond commented Jan 7, 2023 •

edited by Stephan202