hardayal / hamcrest

Automatically exported from code.google.com/p/hamcrest
0 stars 0 forks source link

generic map matchers broken #47

Closed GoogleCodeExporter closed 8 years ago

GoogleCodeExporter commented 8 years ago
The following should both compile:
        assertThat(new HashMap<X,Y>(), hasKey(new X()))
        assertThat(new HashMap(), hasKey(new X()))

They do not. Similarly for hasValue. The hasKey methods in Matchers should
be parametrised only the key type K and should return
        Matcher<? super Map<K, ?>>
instead of
        Matcher<Map<K, V>>

See the following thread for more details, and also for a more general fix
proposal.
http://groups.google.com/group/hamcrest-dev/browse_thread/thread/b3e1b118bcc6f22
0

Original issue reported on code.google.com by joe.kear...@gtempaccount.com on 9 Sep 2008 at 10:09

GoogleCodeExporter commented 8 years ago
Since posting this issue I've tested against the head revision. Some of the 
problems
are fixed. In particular, changing signatures from
    <K,V> Matcher<Map<K,V>> hasKey(K key)
to
    <K> Matcher<Map<K,?>> hasKey(K key)
removing the unbound V parameter allows the compiler to infer the map value 
type from
the first parameter in the assertThat statement. This allows hasKey to work in a
typed map. (The same holds for hasValue, though I'll stick to hasKey in this 
post for
clarity.)

Unfortunately, the more I look at this problem, the more it seems that to solve 
the
more general problem we need a wider solution than just in the method signatures
relating to map matchers. I hoped that just by widening the bounds on the 
parameter
type of the matcher would allow this all to fall out, but I believe we need 
something
more, closer to what I suggested in the message I posted to the group, link 
above.

The next part of the problem comes when, for a map with parameters K, V, we try 
to
match a key with type J <: K. One contrived example: J = Integer, K = Number.
    Map<Number, V> map = getMap();
    assertThat(map, hasKey((Number) Integer.valueOf(1))); // compiles against trunk
    assertThat(map, hasKey(Integer.valueOf(1))); // fails to compile
Clearly more realistic use cases will come up, for example consider 
parametrising the
key as an interface.

It turns out that there is a simple solution to all of this. The reason it's
difficult and requires a lot of generic magic to get the above statements to be 
valid
is because we have this single-sided restriction on the type parameter of the
Matcher, that the actual value being matched is of a type bounded above by the
parameter type of the Matcher. There is no a priori reason that this should be 
so, as
opposed to the converse.

In my post to the mailing list previously I asked for ideas about how to get 
around
some of these generic issues, and my suggestion at the time was to remove the
parameter from the Matcher interface. I understand now that this isn't possible 
due
to JMock integrations.

It seems to me that the other suggestion I gave (and disregarded) might be an
approach to consider. In the signature for assertThat, rather than associate 
the type
of the "actual" value and the parameter type of the matcher (either by 
identifying
them or stipulating that one bounds the other above/below) we should 
disassociate
them entirely. We can still have the parameter on the Matcher interface and it 
can
still have the same interpretation as currently, but we're also able to make 
more
assertions that are reasonable to make, for example
    assertThat(3, greaterThan(2.0))
    assertThat(3.0, greaterThan(2))
Note that with this change to assertThat we can even do away with all of the 
extra
generic magic.

So my proposal is to rewrite the assertThat methods with the following 
signatures
(requiring no other code changes):
    <T, M> void assertThat(T actual, Matcher<M> matcher)
    <T, M> void assertThat(String reason, T actual, Matcher<M> matcher)

All of the tests pass, again with no other code changes.

For extra context I've attached some discussion of the generic magic I tried to 
go
through to get this to work without changing assertThat - I'm almost certain it 
won't
work, due in part because Java's type inference is not strong enough. I've also
attached some test cases. They don't compile against current trunk (r318), but
compile and pass with these changes.

Original comment by joe.kear...@gtempaccount.com on 15 Oct 2008 at 8:58

Attachments:

GoogleCodeExporter commented 8 years ago
So the patch for http://code.google.com/p/hamcrest/issues/detail?id=43  was 
super simple :) - this one was 
much harder!

I managed to create a patch to fix this issue- but it turned out to be a 
massive change :) Basically ensuring 
consistency in the use of Matcher<? super T> rather than Matcher<T> along with 
some other Map related 
changes.

It was one of those patches that turned up more and more compile errors the 
more I tried to fix - I nearly 
stopped several times - but kept on going until the end :)

So all things build & compile plus I've added some more Map related tests to 
check things work both with and 
without generics for containing, key and value tests.

I'm sorry this is such a big patch! 

Original comment by james.st...@gmail.com on 17 Oct 2008 at 3:16

Attachments:

GoogleCodeExporter commented 8 years ago
Added another patch with more test cases including JoeK's recent examples

Original comment by james.st...@gmail.com on 17 Oct 2008 at 5:27

Attachments:

GoogleCodeExporter commented 8 years ago
here's another patch with a modified assertThat() which compiles JoeK's test 
case

Original comment by james.st...@gmail.com on 17 Oct 2008 at 5:36

Attachments:

GoogleCodeExporter commented 8 years ago
Thanks James. I'll take a look and apply the patch.

Original comment by smgfree...@gmail.com on 18 Oct 2008 at 8:14

GoogleCodeExporter commented 8 years ago
I'd advise using David Saff's suggestion for a simpler signature:
    void assertThat(Object actual, Matcher<?> matcher)

instead of
    <T,K extends T,V extends T> void assertThat(K actual, Matcher<V> matcher)

I think these are equivalent anyway, since, as David points out, T=Object is a
solution for this in all cases.

Original comment by joe.kear...@gtempaccount.com on 20 Oct 2008 at 9:09

GoogleCodeExporter commented 8 years ago
Agreed! I'm now kinda coming around to the same conclusion - unless we can 
figure out some overloaded 
assertThat() method calls which take the MapMatcher stuff as parameters? (As it 
mostly just seems to be Map 
related assertions fail)

But I agree that having a mismatch in the types of the matcher & actual values 
doesn't matter that much - as the 
test is gonna fail if you mismatch stuff anyway

Original comment by james.st...@gmail.com on 21 Oct 2008 at 10:49

GoogleCodeExporter commented 8 years ago
I /think/ this is done. I applied James' patch and a couple of other tweaks. 
Fingers crossed.
Closed until someone reopens it.

Original comment by smgfree...@gmail.com on 20 Nov 2008 at 8:44

GoogleCodeExporter commented 8 years ago

Original comment by smgfree...@gmail.com on 22 Nov 2008 at 12:16

GoogleCodeExporter commented 8 years ago
Could I ask that this be reopened please? The TypeInferenceTest I attached 
above has
a few compile errors, in cases that seem reasonable:

assertThat(new HashMap(), not(hasKey(new X()))); // raw map
assertThat(new HashMap(), not(hasValue(new Y()))); // raw map
assertThat(new X(), is(new SubX())); // works is SubX is up-cast to X

Thanks.

Original comment by joe.kear...@gtempaccount.com on 26 Mar 2009 at 5:29

GoogleCodeExporter commented 8 years ago
I thought we decided at some point not to support raw collections?

Original comment by googlech...@m3p.co.uk on 26 Mar 2009 at 6:50

GoogleCodeExporter commented 8 years ago
Oh, I'm sorry I missed that conversation. I would object strongly to that. I 
have no
particular love for legacy code, but when you don't have a choice but to use 
older
code it's all the more disappointing to have to put trivial casts in all over 
the
place to be able to make assertions about them.

A particular case that I hit was with Spring transactions. Spring currently 
(pre-3.0)
targets Java 4 for the most part. I couldn't assert that a map, returned raw 
from a
Spring method, contained my objects without casting to Map<Object, Object> and
suppressing the warning.

Worse is the last example above. Perhaps this equivalent version is clearer:
    SubX subx = new SubX();
    X x = subx;
    assertThat(x, is(subx));

Original comment by joe.kear...@gtempaccount.com on 26 Mar 2009 at 7:00