hamcrest / JavaHamcrest

Java (and original) version of Hamcrest
http://hamcrest.org/
BSD 3-Clause "New" or "Revised" License
2.11k stars 376 forks source link

Generic type bounds of IsIterableContaining are inconsistent with those of IsMapContaining #252

Open nibsi opened 5 years ago

nibsi commented 5 years ago

Hello all! Thanks a lot for the new Hamcrest 2.1 library, it's a pleasure to work with and I can also use it easily in my Java 9 modules.

The IsIterableContaining matcher extends TypeSafeDiagnosingMatcher<Iterable<? super T>>. I understand that the goal is to, for instance, match a Set<Number> when it has an Integer.

Why then does IsMapContaining extend TypeSafeMatcher<Map<? extends K, ? extends V>>? Wouldn't it be just as reasonable to match a Map<Object, Object> when it has a String key? Or a Long value?

tumbarumba commented 5 years ago

Writing library code with Java generics is a bit like (╯ರ ~ ರ)╯︵ ┻━┻

Perhaps @sf105 might have some insights as to how the current implementation came to be the way it is?

nibsi commented 5 years ago
Collection<Banana> bananas = singleton(banana);
Collection<Fruit>  fruits  = singleton(banana);

assertThat(fruits,  hasItem(banana));
assertThat(bananas, hasItem(fruit) );  // this one is currently invalid

I think this makes sense: Why would I want to check if a collection of some specific type contains some less specific type? Usually I already know the actual runtime type of the value I use to create the matcher with, because I create it myself, whereas the collection being examined is passed in and could be anything.

The same is true for maps. Therefore I would propose to make the hasKey() and hasValue() matchers contravariant.

foal commented 2 weeks ago

The IsIterableContaining matcher extends TypeSafeDiagnosingMatcher<Iterable<? super T>>.

The IsIterableContaining have to extend TypeSafeDiagnosingMatcher<Iterable<? extends T>>.
Usualy you have a collection of entities (e.g. Car). This list can contain any object that extends Car (List<? extent Car>). And you want to apply to each element the Matcher that can check the Car or it's parent (Matcher<? super Car>).

So, you want to construct Matcher that take the Iterable<? extends Car> and apply to it given Matcher<? super Car>

tumbarumba commented 1 week ago

I've been reading up on poloymorphic collections in Java, and the PECS rule (producer extends, consumer super). I've created #422 as an attempt to address this issue. There's a new test in IsIterableContainingTest that demonstrates the problem and the fix.

I'm a bit worried that this change might impact a lot of existing code. I'd appreciate if folks could check out that fork and give it a test