Export to GitHub

hamcrest - issue #100

hasItem matcher factory has wrong generic wildcard


Posted on Oct 14, 2009 by Quick Hippo

Should be ? super T not ? extends T.

Comment #1

Posted on Oct 15, 2009 by Swift Cat

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

Comment #2

Posted on Oct 15, 2009 by Quick Hippo

Ah... don't think that the return type should be Matcher> but Matcher>

Comment #3

Posted on Oct 15, 2009 by Happy Bird

Nat, note that that signature disallows the following: assertThat(new HashSet(), 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> hasItem(Matcher matcher) Matcher> hasItem(T t) void assertThat(T actual, Matcher matcher)

Comment #4

Posted on Oct 15, 2009 by Quick Hippo

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

I think it should be: Matcher> hasItem(Matcher matcher)

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

I think that makes sense.

Comment #5

Posted on Oct 15, 2009 by Quick Hippo

"note that that signature disallows the following: assertThat(new HashSet(), 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)).

Comment #6

Posted on Oct 15, 2009 by Quick Hippo

No, my comment about combining them is wrong. I think Matcher> 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.

Comment #7

Posted on Oct 16, 2009 by Swift Cat

so this issue can be closed?

Comment #8

Posted on Jan 27, 2011 by Quick Rabbit

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> hasItem(Matcher 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, 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), but it is still possible [with 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> hasItem(Matcher matcher)" and than with "Matcher> hasItem(Matcher matcher)"

Best regard, Daniel

Attachments

Comment #9

Posted on Jan 27, 2011 by Quick Rabbit

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

Comment #10

Posted on Jan 28, 2011 by Quick Hippo

Which version of Hamcrest are you using?

Comment #11

Posted on Jan 29, 2011 by Quick Rabbit

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

Comment #12

Posted on Feb 15, 2011 by Happy Monkey

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

Attachments

Comment #13

Posted on Mar 23, 2011 by Happy Kangaroo

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.

Comment #14

Posted on May 12, 2012 by Massive Hippo

tagging as Java

Comment #15

Posted on Aug 29, 2012 by Helpful Wombat

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

Set numbers = new HashSet(); 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 hasItem>(Matcher matcher)

ie, a raw matcher.

Comment #16

Posted on Dec 12, 2012 by Happy Dog

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, and the second works out to Matcher> from Hamcrest 1.2 onward.

List can be cast to Iterable.

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 I will still get back a Matcher>.

Comment #17

Posted on Dec 12, 2012 by Happy Dog

Whoops. :)

s/SupperWarnings/SuppressWarnings/

Comment #18

Posted on Dec 12, 2012 by Happy Dog

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

Comment #19

Posted on Dec 12, 2012 by Happy Dog

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 I will still get back a Matcher>.

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

Comment #20

Posted on Apr 10, 2013 by Swift Dog

I might be missing something but Matcher> seems like a reasonable return type for hasItems(Matcher). To preserve backward compatibility, could we have a new matcher: public static Matcher> contains(final Matcher matcher) { return new TypeSafeMatcher>() { @Override protected boolean matchesSafely(Iterable 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);
        }
    };
}

Comment #21

Posted on Apr 11, 2013 by Happy Dog

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.

Comment #22

Posted on Apr 11, 2013 by Happy Dog

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

Given the following:

interface Foo {} interface Bar {} interface Baz {}

Foo> a = new Foo>() {};

The following compiles:

Foo> b = a; Foo> c = a;

While these do not:

Foo> d = (Foo>) a; Foo> e = (Foo>) 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.

Comment #23

Posted on Apr 11, 2013 by Swift Dog

thanks for the info quali,

As far as I can tell, Iterable just never makes sense while Iterable usually does because Iterables produce rather than consume. I've also noticed that Matchers.contains returns Matcher> 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> and it does seem to work as expected in that I can do: assertThat(List, containsItem(Matcher))

Comment #24

Posted on Apr 11, 2013 by Happy Dog

@richard, are you testing this in Java 7?

Comment #25

Posted on Apr 11, 2013 by Swift Dog

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> hasItem(Matcher matcher)

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

Comment #26

Posted on Apr 11, 2013 by Swift Dog

@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.

Comment #27

Posted on Apr 1, 2014 by Happy Bear

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.

Status: Accepted

Labels:
Type-Defect Priority-Critical Java