hasItem matcher factory has wrong generic wildcard

[GoogleCodeExporter]

Should be ? super T not ? extends T.

Original issue reported on code.google.com by nat.pr...@gmail.com on 14 Oct 2009 at 8:26

[GoogleCodeExporter]

Can you check either the report or your version of the code? 
The current version says Matcher<Iterable<? super T>> hasItem(Matcher<? super 
T> elementMatcher)

Original comment by smgfree...@gmail.com on 15 Oct 2009 at 12:45

[GoogleCodeExporter]

Ah... don't think that the return type should be Matcher<Iterable<? super T>> 
but
Matcher<Iterable<T>>

Original comment by nat.pr...@gmail.com on 15 Oct 2009 at 4:34

[GoogleCodeExporter]

Nat, note that that signature disallows the following:
    assertThat(new HashSet<Number>(), hasItem(1));

Looking at what's in trunk for MatcherAssert#assertThat, first glance at this it
looks to me that the signatures required are
    Matcher<Iterable<? super T>> hasItem(Matcher<? super T> matcher)
    Matcher<Iterable<? super T>> hasItem(T t)
    void assertThat(T actual, Matcher<? super T> matcher)

Original comment by joe.kear...@gtempaccount.com on 15 Oct 2009 at 4:50

[GoogleCodeExporter]

I don't think that the current signature allows:
     assertThat(new HashSet<Number>(), hasItem(1));

I think it should be: Matcher<Iterable<? extends T>> hasItem(Matcher<? super T> 
matcher)

that would allow hasItem(1) to match a HashSet<Number>  and also allow hasItem
matchers to be combined as expected.  E.g. and(hasItem(1),hasItem(1.0)) would 
be a
Matcher<Iterable<? extends Number>>

I think that makes sense.

Original comment by nat.pr...@gmail.com on 15 Oct 2009 at 7:30

[GoogleCodeExporter]

"note that that signature disallows the following: assertThat(new 
HashSet<Number>(),
hasItem(1));"

From a static-typing point of view, that *should* be disallowed because the set 
may
contain numbers that are not integers, but you've created a matcher that is 
typed by
an Integer. If you want to match any kind of Number you need a matcher that is 
typed
by Number.  E.g. hasItem((Number)Integer.valueOf(1)).

Original comment by nat.pr...@gmail.com on 15 Oct 2009 at 7:56

[GoogleCodeExporter]

No, my comment about combining them is wrong.  I think Matcher<Iterable<? super 
T>>
is right.  Goddamn generics... they make everything so ridiculously 
complicated.  I
wish I was programming in Haskell where the compiler just worked this 
bureaucratic
trivia out for me.

Original comment by nat.pr...@gmail.com on 15 Oct 2009 at 8:13

[GoogleCodeExporter]

so this issue can be closed?

Original comment by smgfree...@gmail.com on 16 Oct 2009 at 9:57

[GoogleCodeExporter]

Hi,

I'm currently writing some test, related to collections and I wanted to use 
hasItem/IsCollectionContaining. While doing this, I ran into the same 
generics-issue, related to super/extended modificator, discussed here. But 
according to my test, I'd vote for the hasItem-Method signature, using 
"extends"...
Matcher<Iterable<? extends T>> hasItem(Matcher<? super T> matcher)

Let me explain why...
The "super" style, will not work if you don't know the exact type of the list. 
In my case, I wanted to test a List<? extends ZipEntry>, but I couldn't get it 
working, with the given "hasItem" matcher. The only way was to copy the 
IsCollectionContaining class, and to modify it to use extends instead of super 
and than it worked. 

I know that this makes it a bit harder to match sub-class objects of the type, 
given by the list (like finding an Integer object in a List<Number>), but it is 
still possible [with <Number>hasItem(new Integer(2)) ]; while hasItem is/would 
be useless, with Lists, you don't know the exact type of.

I attached a little sample with 4 different Lists, each with an hasItem 
assertion. Try to get them all working, once with
"Matcher<Iterable<? super T>> hasItem(Matcher<? super T> matcher)" and than 
with 
"Matcher<Iterable<? extends T>> hasItem(Matcher<? super T> matcher)"

Best regard, Daniel

Original comment by drothma...@googlemail.com on 27 Jan 2011 at 9:59

Attachments:

• IsCollectionContainingTest.java

[GoogleCodeExporter]

BTW: this issue should be linked to #24 and maybe also to #103...

Original comment by drothma...@googlemail.com on 27 Jan 2011 at 10:06

[GoogleCodeExporter]

Which version of Hamcrest are you using?

Original comment by nat.pr...@gmail.com on 28 Jan 2011 at 10:07

[GoogleCodeExporter]

I tried 1.2.1 and 1.3RC2, from Maven Central ...
Both are using 
"public static <T> Matcher<Iterable<? super T>> hasItem(Matcher<? super T> 
elementMatcher)" 

Original comment by drothma...@googlemail.com on 29 Jan 2011 at 11:12

[GoogleCodeExporter]

I attached a patch for this. Note that the test from Issue 24 still runs.

Original comment by philippe...@gmail.com on 15 Feb 2011 at 2:21

Attachments:

• hamcrest-java.patch

[GoogleCodeExporter]

Please apply the patch before 1.3 is released. We have a large amount of tests 
that won't compile if you go with the current decision. 

Original comment by jacmo...@gmail.com on 23 Mar 2011 at 10:30

[GoogleCodeExporter]

tagging as Java

Original comment by t.denley on 12 May 2012 at 10:50

• Added labels: Java

[GoogleCodeExporter]

Philippe's patch from Feb 15 loosens the constraints on permissible matchers: 
it's now possible to write:

Set<Number> numbers = new HashSet<Number>();
assertThat(numbers, hasItem("Not a number"));

Arguably, that should be caught at compile-time. But then, it can also be 
argued it shouldn't - not just in terms of increasing the utility of the 
matchers (which it definitely does), but in terms of equivalence to 
Collection.contains(). 

numbers.contains("Not a number") is a valid expression, so assertThat(numbers, 
hasItem("Not a number")) should be a valid expression too. Sure, the matcher 
will always fail - but that's not a reason it needs to be caught at 
compile-time. More pertinently, this allows us to provide a matcher on a 
subclass of T to check for the contents of a collection of T, which is 
something that *should* be legal without requiring messy casts. 

In order to also allow expressions like assertThat(numbers, hasItem(is("Not a 
number")) (for a minimal example), the signature of the other factory method 
would have to be changed to:

public static <T> hasItem<Iterable<? extends T>>(Matcher matcher)

ie, a raw matcher.

Original comment by TARJohns...@gmail.com on 29 Aug 2012 at 8:35

[GoogleCodeExporter]

The current behavior breaks our builds under Java 7.

Example:

    assertThat(Arrays.asList("foo", "bar", "baz"), hasItem(equalTo("baz")));

    [ERROR] /project/src/test/java/com/example/HamcrestTest.java:[18,4] cannot find symbol
    [ERROR] symbol  : method assertThat(java.util.List<java.lang.String>,org.hamcrest.Matcher<java.lang.Iterable<? super java.lang.Object>>)
    [ERROR] location: class com.example.HamcrestTest

This used to compile under Java 6 due to a compiler bug. However, that bug is 
now fixed in Java 7, and the types are correctly flagged as not compatible.

Given the method signatures:

    <T> void assertThat(T, Matcher<? super T>)

    <T> Matcher<Iterable<? super T>> hasItem(Matcher<? super T>)

The first argument is List<String>, and the second works out to 
Matcher<Iterable<? super T>> from Hamcrest 1.2 onward.

List<? super T> can be cast to Iterable<T>.

    List<? super String> a = Arrays.asList("foo");
    Iterable<String> b = (Iterable<String>) a;

However casting is limited to the first "layer" of generics. You cannot cast 
the nested parameter:

    Matcher<Iterable<? super String>> a = hasItem(equalTo("baz"));
    Matcher<Iterable<String>> b = (Matcher<Iterable<String>>) a; // compile error

Casting is not possible, even with @SupperWarnings.

For this method to remain usable in unit tests, the hasItem matchers must be 
changed back to its signature from 1.1:

    <T> Matcher<Iterable<T>> hasItem(Matcher<T>)

This will not prevent clients from passing in matchers with wildcards. If I 
pass in a Matcher<? super Foo> I will still get back a Matcher<Iterable<? super 
Foo>>.

Original comment by qualidaf...@gmail.com on 12 Dec 2012 at 12:13

[GoogleCodeExporter]

Whoops. :)

s/SupperWarnings/SuppressWarnings/

Original comment by qualidaf...@gmail.com on 12 Dec 2012 at 1:17

[GoogleCodeExporter]

Filed pull request on Github: https://github.com/hamcrest/JavaHamcrest/pull/16

Original comment by qualidaf...@gmail.com on 12 Dec 2012 at 1:59

[GoogleCodeExporter]

I should correct one statement I made in comment 16:

> This will not prevent clients from passing in matchers with wildcards. If I 
pass in a Matcher<? super Foo> I will still get back a Matcher<Iterable<? super 
Foo>>.

Changing the input parameter type turned out to be unnecessary. A Matcher<? 
super T> is, by definition, compatible with elements in an Iterable<T>. My 
changes in the pull request change the return type to Matcher<Iterable<T>>, but 
leaves the input parameter(s) as Matcher<? super T>.

Original comment by qualidaf...@gmail.com on 12 Dec 2012 at 2:02

[GoogleCodeExporter]

I might be missing something but Matcher<Iterable<? extends T>> seems like a 
reasonable return type for hasItems(Matcher<? super T>). To preserve backward 
compatibility, could we have a new matcher:
<code>
    public static <T> Matcher<Iterable<? extends T>> contains(final Matcher<? super T> matcher) {
        return new TypeSafeMatcher<Iterable<? extends T>>() {
            @Override
            protected boolean matchesSafely(Iterable<? extends T> item) {
                for (T t : item) {
                    if (matcher.matches(t)) {
                        return true;
                    }
                }
                return false;
            }

            @Override
            public void describeTo(Description description) {
                description
                        .appendText("a collection containing ")
                        .appendDescriptionOf(matcher);
            }
        };
    }
</code>

Original comment by richard_...@hotmail.com on 10 Apr 2013 at 11:17

[GoogleCodeExporter]

If you look at the cumulative changes in my Github pull request, you'll note 
that all the generic type signatures have been changed in a backward compatible 
way. Generic arguments are more permissive, while generic return types are more 
specific than before.

For what it's worth, my changesets compile under both Java 6 and Java 7, with 
minimal changes to unit tests.

Original comment by qualidaf...@gmail.com on 11 Apr 2013 at 12:00

[GoogleCodeExporter]

I should point out that nested generic parameters *never* work the way you want 
with wildcards.

Given the following:

  interface Foo<T> {}
  interface Bar<T> {}
  interface Baz {}

  Foo<Bar<Baz>> a = new Foo<Bar<Baz>>() {};

The following compiles:

  Foo<? super Bar<Baz>>   b = a;
  Foo<? extends Bar<Baz>> c = a;

While these do not:

  Foo<Bar<? super Baz>>   d = (Foo<Bar<? super Baz>>)   a;
  Foo<Bar<? extends Baz>> e = (Foo<Bar<? extends Baz>>) a;

The bug that existed in Java 6 would occasionally allow these latter mismatches 
in the context of a method argument. Java 7 closed that loophole, so these 
nested generics parameters are going to continue to bite us until they are 
addressed.

Original comment by qualidaf...@gmail.com on 11 Apr 2013 at 3:26

[GoogleCodeExporter]

thanks for the info quali,

As far as I can tell, Iterable<? super T> just never makes sense while 
Iterable<? extends T> usually does because Iterables produce rather than 
consume. I've also noticed that Matchers.contains returns Matcher<Iterable<? 
extends T>> which seems correct and in any case seems like it should have the 
same return type as Matchers.hasItem

I've created my own version of hasItem (I called it containsItem) that returns 
Matcher<Iterable<? extends T>> and it does seem to work as expected in that I 
can do: assertThat(List<String>, containsItem(Matcher<CharSequence>))

Original comment by richard_...@hotmail.com on 11 Apr 2013 at 4:43

[GoogleCodeExporter]

@richard, are you testing this in Java 7?

Original comment by qualidaf...@gmail.com on 11 Apr 2013 at 4:59

[GoogleCodeExporter]

I've tested my code in Java 7 and it works as I wish/expect it to. I could very 
well have messed up an important edge case. I'm curious as to why Nat Price 
rescinded his/her comment #4 saying I think it should be: Matcher<Iterable<? 
extends T>> hasItem(Matcher<? super T> matcher)

I think it should be Matcher<Iterable<? extends T>> as well but maybe Nat is 
just two steps ahead of me, in which case I very much wish to learn his/her 
reasoning.

Original comment by richard_...@hotmail.com on 11 Apr 2013 at 5:15

[GoogleCodeExporter]

@quali, I verified your analysis of code snippets in Java 7 and all is as you 
say. the code snippet I posted in #20 works as I expect and appears consistent 
with the info you give in #22.

Original comment by richard_...@hotmail.com on 11 Apr 2013 at 5:17

[GoogleCodeExporter]

I agree with above comments. The IsCollectionContaining is completely broken.
We need IsCollectionContaining to be reimplemented with generics signatures in 
the same fashion as in 
IsIterableContainingInOrder/IsIterableContainingInAnyOrder.

If backward compatibility will be broken (actualy I'm not sure that smth will 
be broken), then probably the IsCollectionContaining should marked as 
deprecated and a new matcher introduced. Possible method names could be 
containsItem(), has() or similar.

Original comment by vladimir...@gmail.com on 1 Apr 2014 at 7:34