is(Class) and is(Object) both are Matcher<Object>, which makes Java sense

[GoogleCodeExporter]

Purpose of code changes on this branch:

By removing the type parameter from is(T) and is(Class<T>), more uses of
allOf, anyOf, both/and, and either/or compile without warning, and little
seems to be lost, since equals and instanceof both operate on Object.

However, this might be considered a big change.  I'm more than happy to see
examples of "I wish that X didn't compile, but with this change, it does!"

After the review, I'll merge this branch into:
/trunk

Original issue reported on code.google.com by david.s...@gmail.com on 28 May 2009 at 7:57

[GoogleCodeExporter]

Well, I wondered why Matcher even took a type parameter, since all Matchers can
operate on any Object, however, I assumed that the reason was so that the types 
would
be available for uses like jMock argument matchers. Otherwise, they seem to be 
more
of a hassle than they're worth (and I'm still not convinced that the <? super X>
construction is right, at least in some cases -- but once you've started, you 
have to
do it everywhere, it seems).

Perhaps one of the developers can explain the rationale behind the type 
parameter and
where it is useful. But it seems to me that this change would break such uses, 
if
they exist.

Original comment by mhack...@kanayo.com on 29 May 2009 at 1:27

[GoogleCodeExporter]

A comment from the dev team would be indeed interresting!

Original comment by brice.du...@gmail.com on 25 Sep 2009 at 12:21

[GoogleCodeExporter]

I've added a helper class to my project to support mixed types in the 
assertThat(..),
it doesn't appear to break anything:

public static <N, M> void assertThat(N n, Matcher<M> matcher)

and simply executes:

assertTrue("Expected " + matcher + "\n\tgot: " + n, matcher.matches(n));

any reason why this isn't a reliable solution?  Very rudimentary tests pass 
against
this syntax.

Original comment by bit.str...@verizon.net on 10 Feb 2010 at 3:26

[GoogleCodeExporter]

bit.str's patch basically removes all type checking from assertThat, which I 
now 
believe is the right thing.  Unfortunately, it doesn't address the deeper 
issues with 
combinatoric matchers like allOf and anyOf.

Original comment by s...@google.com on 19 Feb 2010 at 7:30

[GoogleCodeExporter]

is(Class) should be a Matcher<Object> but take an optional matcher argument 
that is parameterised by the type of the class.

E.g. is(String.class, containsString("foo"))

The signature would be:

    <T> Matcher<Object> is(Class<T> type, Matcher<? super T> downcastValueMatcher)

Original comment by nat.pr...@gmail.com on 1 Feb 2011 at 3:27

[GoogleCodeExporter]

Nat,

Based on previous conversations and attempts, I think if any further changes 
are made to the type rules for hamcrest, we first need to augment the javadoc 
for Matcher to include what T _means_, and then patch the types to better fit 
that meaning.  Otherwise, there's no way to evaluate whether "is(Class) 
_should_ be X".

Alternatively, if there were a promise that we would never change typing rules 
of matchers, and T just means "whatever T happened to be in hamcrest as of date 
X", that would also be possible.

But we can't just go arbitrarily changing type signatures yet again because it 
happens to sound good.  That seems to be the status quo, which is why if 
there's ever a JUnit 5.0, it will not include hamcrest.

Original comment by s...@google.com on 3 Feb 2011 at 3:04

[GoogleCodeExporter]

Nat,

While not a shorthand like you're proposing, you could accomplish the same with 
the following:  assertThat("bar", 
both(is(String.class)).and(containsString("foo")));

Original comment by yottabit...@gmail.com on 8 Feb 2011 at 4:06

[GoogleCodeExporter]

We're not changing type signatures because we want them to "sound good".  We're 
changing them because they are incorrect and we're fixing the bug.  Likewise, 
we can't leave incorrect upper & lower bounds in the code because they stop 
Hamcrest working in libraries that rely on static typing (JMock, Mockito, 
etc.).  JUnit's assertions can be untyped if you wish (although you won't get 
early warnings of type errors from the IDE).  Everyone else has to suffer with 
the limitations and complications of the Java 5 type system and it's awkward 
intersection with reflection. We want to avoid adding to that misery.

Original comment by nat.pr...@gmail.com on 10 Feb 2011 at 7:57

[GoogleCodeExporter]

Nat,

Sorry to be combative in my last comment.  Here's the history.  This change:

http://code.google.com/p/hamcrest/source/detail?spec=svn353&r=353

broke many type signatures in JUnit, leaving us with the decision of whether to 
support Hamcrest 1.1 or Hamcrest 1.2, but no option to support both.  

Which was the "correct" type system?  That in 1.1, or in 1.2?  Did 353 create a 
bug, or remove one?

I recommend a spec in the javadoc of Matcher<T> that allows us to determine 
whether a subtype of Matcher is correctly or incorrectly typed without 
reference to "it compiles in the current version of JUnit" or "it compiles in 
the current version of jMock" (which appears to have been the driving reason 
behind change 353).  Yes, it's unfortunate that Java's type system will force 
us to guarantee much less in that spec than we wish, but we should do what we 
can.

At the very least, we could have a set of compilation tests, statements that 
should compile or shouldn't, if the typing is right.

Once we have that spec or test suite, we can fix any deviations in hamcrest 
itself, JUnit, jMock, Mockito, etc, and assure our users that if they 
themselves pay attention to the spec, all will be well.

If a representative acceptable to the hamcrest committers can commit to speedy 
review, I can try (again) to draft such a spec.  Does that make sense?

Original comment by s...@google.com on 10 Feb 2011 at 3:09

[GoogleCodeExporter]

1.1 had many incorrect generics and wildcards.  1.2 attempted to fix them but 
mis-defined wildcards for collection matchers.  I went through of the 
signatures of collection matchers as thoroughly as I could and have been 
implementing fixes in the 1.3-RCx releases.  A few mistakes slipped through in 
the last RC release.  Also I've found a few other signatures that do not make 
sense in the is(...) matchers (which I may just deprecate and remove since they 
do not actually add any functionality, just syntactic sugar).  However, in 
general the 1.3 release is much better.  I know of a large project that easily 
converted from 1.1 to 1.3RC3 without any major issues (just fixing some of 
their own matcher signatures) and it works fine with jMock 2.5.1.

Good documentation in the JavaDoc (and/or project Wiki) is a very good idea. It 
deserves its own issue.  I'd be happy to give some time to review/collaborate 
on any documentation effort.

Original comment by nat.pr...@gmail.com on 10 Feb 2011 at 11:18

• Changed state: Started

[GoogleCodeExporter]

OK, I'll give the documentation a shot.  Should the discussion thereon
continue here, or just on the mailing list?

My main job in the documentation is threading the needle between two
different type questions that can be asked of the same matcher.  The
difference between is(Class) and any(Class) illustrates the
difference.  What the assertThat method wants to know about a Matcher
is:

1) What's the most specific type that will match all inputs for which
this Matcher _is a "valid question"_?

What jMock's with() method wants to know is:

2) What's the most specific type that will match all inputs for which
this Matcher _will return true_?

In many cases, like StringMatchers.contains, type #1 and type #2 are
both String.  However, for new InstanceOf(String.class), #1 is Object,
and #2 is String.  Same with new IsEqual("myString").

It seems that it would be possible to build a framework very similar
to hamcrest in which both #1 and #2 are explicit:

Matcher<Object, String> m = new InstanceOf(String.class);

I think this would allow for more useful types in allOf, assertThat,
with, and anyOf, just for starters.

So, the questions:

1) In Matcher<T>, across all uses of hamcrest, is T type #1, or type #2?
2) What would be the costs and benefits of moving to a system where
both type #1 and type #2 are explicit?

Original comment by s...@google.com on 11 Feb 2011 at 3:34

[GoogleCodeExporter]

Nat, Steve,

Sorry, I was away on vacation, so I missed any activity.  Before I can document 
Matcher<T>, I need at least some thoughts on question #1 in the above comment:

1) In Matcher<T>, across all uses of hamcrest, is T type #1, or type #2?

Thanks,

   David

Original comment by s...@google.com on 4 Mar 2011 at 6:12

[GoogleCodeExporter]

The type T what we know at compile time about about the type of values that 
will be successfully matched.

This static type lets us catch errors at compile time.  This static type 
information is also used to force Java's type "semi-inference" to let us 
synthesise parameter values during imposterised method capture in jMock.  (I 
guess Mockito works the same way, but I've not looked).  This is an artefact of 
the broken type system in Java 5 and 6.  I believe that Java 7 will fix the 
type inference, and we would not then need the type parameter on Matcher 
(hooray!).  However, it'll be some time before Java 7 is widely adopted, 
assuming there are still some Java programmers left who have not migrated to 
Scala or Clojure.

Anyway... here's an example of how the type parameter is used:

Assuming Apple, Orange and Banana are subtypes of Fruit...

When you combine matchers (with the allOf function, for example), it should be 
impossible to combine a Matcher<Apple> and a Matcher<Orange> into a 
Matcher<Fruit>, because a Matcher<Fruit> could also match a Banana, which is 
neither an Apple nor an Orange and so we know at compile time that we should 
not pass a Banana to a Matcher<Apple> or Matcher<Orange>.  But it is valid to 
combine a Matcher<Apple> and a Matcher<Fruit> into a Matcher<Apple> because any 
kind of fruit, including an Apple, can be passed to a Matcher<Fruit>.

How does this apply to isA?  An instanceof check can be applied to any object. 
So isA(Class<T>) should return a Matcher<Object> that matches any object if 
it's dynamic type is T or a subtype of T.  However, when using isA, you usually 
want to combine it with matchers of type Matcher<T>.  E.g. you'd like to say 
something like: allOf(isA(String.class), startsWith("foo")), to mean "check 
that it's a string and if it is, that it should start with 'foo'".  However, 
that can really confuse Java's type system if you need to combine lots of 
matchers.  

Instead, I suggest that the isA factory should have an overload: 
isA(String.class, startsWith("foo")), which takes Class<T> and Matcher<T> and 
returns a Matcher<Object>.  I've implemented this on a client's project and 
found that Java is much happier composing Matcher<T>'s where the static type 
information is given by the Class<T>.

Original comment by nat.pr...@gmail.com on 10 Mar 2011 at 11:07

[GoogleCodeExporter]

Regarding comment 11: the T in Matcher<T> has meaning #1.  There's no way for 
the type parameter to have meaning #2, because (as far as I understand what 
you've written) that depends on the *value*, not just the type.

Original comment by nat.pr...@gmail.com on 10 Mar 2011 at 11:12

[GoogleCodeExporter]

I assume you meant to say (a) "The type T [is] what we know at compile time 
about about the type of values that will be successfully matched."  On reading 
this, I internally think about isA(String.class).  Since "we know at compile 
time that" only values with type String "will be successfully matched", I 
assume you would take this to mean that T should be String, that is, this 
should compile:

Matcher<String> m = isA(String.class);  // This would be an instantiation of my 
"meaning #2"

But then you say (b) "An instanceof check can be applied to any object. So 
isA(Class<T>) should return a Matcher<Object> that matches any object"

That is, this should (instead) compile:

Matcher<Object> m = isA(String.class); // This would be an instantiation of my 
"meaning #1"

So I'm left confused.  I think we may be confusing each other, since comment 14 
seems to indicate that you believe that there's no way for us to use meaning 
#2, and I believe that there could be two hamcrest-like libraries, one of which 
uses meaning #1, and one of which uses meaning #2, each with a well-defined 
semantic that differs from the other.

Moving forward, you said "When you combine matchers..."  it is true that in an 
ideal world, this would not compile:

Matcher<Apple> a;
Matcher<Orange> o;
Matcher<Fruit> m = allOf(a, o);

However, this is perfectly sensible, and should compile:

Matcher<Apple> a;
Matcher<Orange> o;
Matcher<Fruit> m = anyOf(a, o);

So the sensibility of combining depends on what kind of combining is being 
done.  The fact that anyOf and allOf currently have the same typing in hamcrest 
shows to me that there is internal confusion about what T should mean.  Do you 
believe that allOf and anyOf currently have the "correct" types?

Again, I understand that we are constrained to perhaps have to say "We wish 
that we could type this as Matcher<XXX>, but because we have to support the 
Eclipse 3.5 compiler, which is broken, and its idiosyncratic interpretation of 
the Java typing rules, which are broken, we can't."  If this is the case, we 
should document it.

I completely understand your examples of is(String.class, 
containsString("foo")) (from comment 5), and isA(String.class, 
startsWith("foo")) (from comment 13).  I think that such an API addition would 
be a usability win, regardless of the typing, but it still leaves open the spec 
for the T in Matcher<T>.

Am I making progress toward understanding here?  Does continuing this 
conversation make sense, or are my opinions about the importance and 
feasibility of a more consistent typing for Matcher so far out of step from the 
maintainers and other hamcrest users that should I just build a JUnit-specific, 
renamed fork of hamcrest, and save the hamcrest maintainers time to think about 
other things?

Original comment by s...@google.com on 11 Mar 2011 at 3:35

[GoogleCodeExporter]

Arggh.  Again I let my java type-safety frustrations spill into grumpiness at 
people.  I do want to be sure that continuing this conversation has value for 
all involved.  If it would really be easier for me to just do what I need to 
keep JUnit working without hassling the hamcrest team, I am happy to go that 
direction, with malice toward none, and charity toward all.  :-)

Original comment by s...@google.com on 11 Mar 2011 at 4:05

[GoogleCodeExporter]

Pardon my intrusion as I haven't read the full thread but only skimmed a 
portion. One thing caught my eye, though.

Matcher<Apple> a;
Matcher<Orange> o;
Matcher<Fruit> m = anyOf(a, o);

This actually doesn't make sense to me. A Matcher<Fruit> should be capable of 
matching against any Fruit or subclass thereof, the same for Matcher<Orange> 
and Matcher<Apple>. However, combining the last two matchers into a single 
Matcher<Fruit> means you may pass an Apple to a Matcher<Orange> and vice versa, 
but this is not allowed.

A concrete example should help.

Matcher<Apple> a = hasStemLength(greaterThan(1)); // oranges don't have stems
Matcher<Orange> o = hasSectionCount(16);          // apples don't have sections

If you pass an Apple to o you end up calling an invalid method 
Apple.getSectionCount(). Passing the Orange to a tries to call 
Orange.getStemLength(). These methods don't exist and thus your test won't 
compile.

Hopefully this can help bridge the gap here. :)

Original comment by dharkn...@gmail.com on 16 Mar 2011 at 2:20

[GoogleCodeExporter]

Exactly!  The signature is (or should be!):

    <T> Matcher<T> anyOf(Matcher<? super T> ...)

If you want to create an anyOf from a Matcher<Orange> and Matcher<Apple>, 
giving a Matcher you can apply to fruit, I think you should be explicit:

    anyOf(isA(Apple.class, hasStemLength(greaterThan(1)), 
          isA(Orange.class, hasSectionCount(10)));

Original comment by nat.pr...@gmail.com on 16 Mar 2011 at 10:30

[GoogleCodeExporter]

However... the consusion is that the Matcher already does the downcast.  That's 
a wart in the design to support imposters that capture calls by reflection and 
forward them on to the matchers.

So, answering David's earlier question.  He's right that my description of what 
<T> means was wrong.  I'll try again...  <T> is a type that we know is not 
going to always mismatch. The type parameter should let the type system stop us 
combining matchers to create logically meaningless predicates.  

It also lets the type inference system call the right method to create 
compatible values to be passed to an imposteriser, when capturing calls 
reflectively.

Original comment by nat.pr...@gmail.com on 16 Mar 2011 at 10:35

[GoogleCodeExporter]

dharkness,

Totally right.  I slipped up in my reasoning.  Thank you for catching that!

Once we have some documentation in place, it will probably pay to back it up 
with compilation/noncompilation tests, so that we can reference what really 
works in the future.

Original comment by s...@google.com on 16 Mar 2011 at 4:59

[GoogleCodeExporter]

We're getting really close here.  Feeling good about it.  However, on my first 
pass, I'm having problems explaining both any() and instanceOf() from:

http://code.google.com/p/hamcrest/source/browse/trunk/hamcrest-java/hamcrest-cor
e/src/main/java/org/hamcrest/core/IsInstanceOf.java

It would seem that given the definition "<T> is a type that we know is not 
going to always mismatch", both any() and instanceOf() should have the same 
<T>, but they don't.

Here's a crazy idea.  What if we created a new interface.  My first cut at a 
name is usually poor, so suggestions are definitely welcome here.  For now, 
call the interface GenerativeSpec<T>.  GenerativeSpec is the interface used by 
with() in jMock, and other frameworks like Mockito that currently use Matchers 
to "create compatible values to be passed to an imposteriser, when capturing 
calls reflectively".  Then, we make:

BaseMatcher<T> implements Matcher<T>, GenerativeSpec<T>.

This represents the fact that for most matchers, the type checks we want when 
asserting are the same as what we need when generating.  However, a few 
matchers will be different.  For example, this could now compile:

Matcher<Object> m = instanceOf(String.class);
GenerativeSpec<String> m = instanceOf(String.class);

Thus, the same expression can serve either in expressions or in imposterizer 
generation, and users don't have to use two different names (any() and 
instanceOf()) for the same idea, depending on where they're using it.

Would such a move help cure the "wart in the design"?

Original comment by s...@google.com on 17 Mar 2011 at 6:39

[GoogleCodeExporter]

Nat, others,

Any thoughts on this?

Original comment by s...@google.com on 20 May 2011 at 6:43

[GoogleCodeExporter]

Hi, all.

Two months and counting since anyone but me commented on this thread.  I'm 
moving forward with a plan to deprecate hamcrest support in JUnit-core, and 
move it to JUnit-contrib, which provides many fewer compatibility guarantees.

Original comment by s...@google.com on 27 May 2011 at 5:39

[GoogleCodeExporter]

tagging Java

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

• Added labels: Java

[GoogleCodeExporter]

Without following the details in this thread, my tl;dr is that given a working 
project declaring

junit junit 4.10 test

and including (passing) test code like

import org.junit.Test; import static org.hamcrest.CoreMatchers.*; import static org.junit.Assert.assertThat; public class MyTest { @Test public void m() { Object o = 55; assertThat(o, not(is(String.class))); assertThat(o, is(Integer.class)); } }

simply upgrading to JUnit 4.11 will not work:

COMPILATION ERROR :

MyTest.java:[7,8] error: no suitable method found for assertThat(Object,Matcher)

T#1 extends Object declared in method <T#1>assertThat(T#1,Matcher<? super T#1>)
T#2 extends Object declared in method <T#2>assertThat(String,T#2,Matcher<? super T#2>)

MyTest.java:[8,8] error: no suitable method found for assertThat(Object,Matcher)

You must switch to `assertThat(…, instanceOf(….class))`. (And in the future 
avoid using libraries which make incompatible changes to their API signatures 
rather than copying-and-deprecating.)

Original comment by jgl...@cloudbees.com on 10 May 2013 at 5:54