Fix #3005 (and #3006) draft

[nineninesevenfour]

Hello @TimvdLippe ,

this is an early preview of an attempt to fix several issues that occur currently with @InjectMocks in combination with generic types. It is not ready to merge yet, but I provide it in order to possibly receive early feedback and also to signal that I am working on a solution.

It contains a (partly) reimplementation of the logic in TypeBasedCandidateFilter. The isCompatibleTypes method in this class evolved from a simple use case with matching generic parameters of collections and starts with the assumption that the field to be injected has the same number of generic parameters as generic arguments present on the mocked field. This is not necessarily the case and I found it to be the main cause for the issues around.

Instead in the code provided with this PR, I attempt to fill in generic arguments into generic parameters from the declaration of @InjectMocks down to the candidate injection field by using the type hierarchy. The main logic for it is in GenericTypeUse.findExactAncestor().

Additionally I make a (hopefully clear) distinction of collecting generic type information - and the actual comparison of that information. The collection happens in the classes GenericTypeUse and GenericTypeResolver - emitting the type MappedType while the comparison happens in class AssignmentMatcher and then reads as follows:

    public boolean matches() {
        when(bothAreClasses()).then(checkClassesAreAssignable());
        when(bothAreMappedTypes()).then(checkRawTypesAreAssignable())
                .andThen(checkDimensionsAreEqual())
                .andThen(recursivelyCheckMappedTypes()
                    .or(elseWhen(sourceIsEmptyMappedType().and(targetIsMappedType()))
                        .then(retryWithSuperTypeOfSource())));
        when(bothAreWildcards()).then(checkWildcardsHaveNoBounds());
        when(sourceIsClass().and(targetIsWildcard())).then(checkWildcardBounds());
        when(sourceIsClass().and(targetIsTypeVariable())).then(checkTypeVariableBounds());
        when(sourceIsEmptyMappedType().and(targetIsClass())).then(compareWithRawTypeOfSource());
        return success;
    }

I admit there are similarities with the already present class GenericMetadataSupport (both carry a map of type parameters to actual arguments), but I could not reuse it as is right now, because it has been crafted for return types for the case of mocking, while here we have to handle fields.

I hope that this PR goes in the right direction and if not, please let me know. Thank you!

Todos

• add Copyright header
• add more comments explaining the code
• format code according to spotless
• run Android tests (I could need some help here)
• maybe get rid of MappedTypeSimple and MappedTypeWildcard again
• try to remember prevously collected generic information for candidate fields presented earlier to the same instance of TypeBasedCandidateFilter

Checklist

• Read the contributing guide
• PR should be motivated, i.e. what does it fix, why, and if relevant how
• If possible / relevant include an example in the description, that could help all readers
  including project members to get a better picture of the change
• Avoid other runtime dependencies
• Meaningful commit history ; intention is important please rebase your commit history so that each
  commit is meaningful and help the people that will explore a change in 2 years
• The pull request follows coding style
• Mention Fixes #<issue number> in the description if relevant
• At least one commit should mention Fixes #<issue number> if relevant

[nineninesevenfour]

would fix just posted #3019, too

[TimvdLippe]

Thanks a lot for this PR and your contribution! At the moment, I am still unsure what we should do with these kind of changes. Before, I advocated for deprecating @InjectMocks (#1518) with these kind of PRs in mind: Mockito is not a dependency injection framework, nor should we become one. Dependency injection is a valid use case and there are amazing JVM libraries out there that implement it, which have become industry standard. However, Mockito is none of that.

Therefore, with a PR that adds more than 1 thousand lines to address 3 different regressions with a single feature, I am unsure if that trade-off is worth it. I know people love @InjectMocks and don't want to see it go away, but at the same time I don't think it is acceptable to maintain all of this code in Mockito and continue to pay the cost for the wider Java community.

@mockito/developers we need to figure out what to do here. In my opinion, the intention of @InjectMocks is to provide a lightweight injection mechanism, so that developers don't need to adopt a dependency framework to inject mocks. However, with regressions as the ones linked in this PR and implementations that ship thousands of lines of Java code, I don't think @InjectMocks qualifies as lightweight. Therefore, I am inclined to not incorporate these kind of changes into mockito-core and instead would like to see @InjectMocks moved into a separate artifact mockito-injection. That way, we can move the burden to a separate package, that the community can take over to maintain. WDYT?

[nineninesevenfour]

@TimvdLippe Thank you for your quick reply! I totally agree what you say. Have you also seen my alternative suggestion here?: #3011 that one could be implemented with really a few lines of codes, would address the most use cases of @InjectMocks now and takes the burden from the Mockito-user to expose private fields in production code only to make it testable. WDYT?

[raphw]

I think that this might live in a project outside of Mockito, and we can offer the API for it with a very basic implementation as a default? Any more complex version could then be loaded via a service loader like other plugins.

[julien-breda]

Hi,

Removing InjectMocks would introduce a breaking change in Mockito. Such a change may be done in a major version (6.0 ?).

For now the version 5.3.1 is broken regarding InjectMocks and it needs a fix.
If this PR isn't approved we'll have a Mockito 5.3.1 not working as expected, since version 1.X worked nice.
@nineninesevenfour wrote many regression tests, and thanks to existing tests we can hope this change would not introduce other regressions.

For me, whatever you choose to do in version 6.0 or higher, version 5.X must be fixed.

[raphw]

I would not want to take the API out, I would however make it injectable in its implementation. Is there a more simple fix than the huge income of new code?

[nineninesevenfour]

I heard the call for simplification and will present a new version with roughly half the code in the next days.

[julien-breda]

This pull request seems to have been approved last week (Github removed the warning notice) and only need to be merged.
May we expect this fix to be included in the next patch ?

[julien-breda]

@nineninesevenfour : you maybe need to remove the draft attribute on this PR to make it reviewed and merged.

[nineninesevenfour]

replaced by #3123