Do we need DefaultSubject?

[cpovirk]

It seems like Subject itself serves the purpose. Well, OK, I guess that we need some values to plug in for the type parameters (Object and... what?), but this is the same problem we have with IterableSubject, and we get by there OK with our unchecked+rawtypes create method, so this is a problem that we already have and have already solved. Or we can eliminate the problem by eliminating the type parameter (#202).

(If DefaultSubject needs to stick around, should it be ObjectSubject for consistency with other subjects?)

(I tagged this as a blocker of Release 1.0, even though it's maybe not technically SPI, on the theory that writers of custom assertThat methods are likely to use DefaultSubject, just as they're likely to use true SPI. I'm not sure whether I buy my own logic here or not.)

[cgruber]

So... we don't, really, but I remember way back having some issues with returning the parent. I think Subject should be abstract.

If we're going to do surgery, I think we actually need to do this differently... having something like

class abstract Subject { ... universals } // or maybe interface, depending.
class abstract ReferenceObjectSubject<T> {}
class abstract PrimitiveSubject {}

and maybe

class abstract AbstractObjectArraySubject<T> {} // omit if unneeded, need to think.
class abstract AbstractPrimitiveArraySubject {}

And then reset everything.

At some level, the only really necessary contract between all subjects is that they hold on to a subject, they hold a failure strategy, and they expose arbitrary methods that will report failure in the context of a subject, to the failure strategy. That's actually all behavioral contract, not API surface, except insofar as we say the Constructor is part of that.

If we make Subject an interface, then we don't really have any way to enforce the contract, but then we're not running primitives through boxing, and we can have PrimitiveSubject do the cleaner primitive things.

If we don't care about the boxing (and I would argue we don't if we are consistent in thinking this is a library for use in unit tests), then there's not a lot of reason to make Subject an interface, since everything can go through protected Object getSubject() {...}

I think it might be reasonable to do a restructuring such as the above, if only because it means we can eliminate some referencey things from primitive subjects. But Whether we want Subject to take on less work is a function of whether we mind the boxing, I think.

[cgruber]

I definitely think we need to lock down DefaultSubject, regardless... and @kluever tried to, but found callers, as predicted. We need to fix those internally before we can mark it final.

[cgruber]

We've had a lot of conversation, in building out subjects internally at Square, and I think we should move on some of these separation of concern issues. Subject probably shouldn't have any assertions, it should have plumbing, and ReferenceObjectSubject and PrimitiveSubject should have the top-level. In practice, that probably means (for backwards compatibility) adding a supertype, since everyone inherits from Subject - maybe AbstractSubject because really that's what it is. It sould have actual() and the raw infrastructure. We could probably make the fail*() methods be a set of APIs on or around FailureStrategy, and the existing Subject can just implement the existing methods against those APIs.

We did a lot of preliminary work a couple of years ago, but didn't finish. Now that I'm on the outside, I can't do the cleanup work inside google, but I could implement at least a straw-man backwards-compatible flavor of this change, which could then be tested internally, and migrated.

(cc: @cpovirk)

[cpovirk]

Lots to think about here, thanks.

1. Should methods like fail* go into a SubjectProtectedMethods superclass of Subject so that Subject can present a clearer view of the Object-specific assertions (especially in its Javadoc)? SubjectProtectedMethods could be useful for Provide support for Kotlin extension methods on Subjects #536.
2. Should assertThat(Object) return a plain Subject, or do we need DefaultSubject?
3. Should we split the inheritance tree along the lines of ReferenceObjectSubject and PrimitiveSubject?

(1) might not be a breaking change, which would mean it could wait until after 1.0. Or maybe it is a breaking change if it requires a generics change on Subject.Factory? Hmm, but we probably don't want it to, since we don't want assertThat methods to return objects that lack methods like isEqualTo. If anything, we might introduce some other kind of factory.

(2) is something that I'm still hoping to look at for 1.0, since removing DefaultSubject would be a breaking change (albeit one with an easy migration, I hope).

(3)... I have forgotten the details about, sorry :( I know it came up on, uh, "Aug 2, 2016" :) But I forget the motivation and details. My suspicion is that removing any methods from the PrimitiveSubject side is going to break a lot of callers. Was removing methods part of the goal? I do like the symmetry we have now, where the same methods are available whether you happen to be calling a method that returns an int or an Integer (especially since Java doesn't have generics over primitives, so you might have wanted to return an int but couldn't). This seems to fit with what you said above ("we don't care about the boxing"), so you may have something else in mind that I've forgotten the details of.

[cpovirk]

RE: (3): @ronshapiro points out that it would be nice to eliminate:

assertThat(3).isSameInstanceAs(3);

Not to mention isNull() and isInstanceOf().

We could even tweak other methods, like giving each primitive type its own isEqualTo and isAnyOf method that accepts, e.g., long rather than Long, which might newly permit widening conversions from int? (That might be in addition to a "normal" isEqualTo method, since we might like to continue to accept null and fail cleanly rather than with an automatic NPE. On the other hand, maybe it's better to omit such an overload to let strict nullness checkers catch the mistake at compile time....)

So there's more here than I had given it credit for. Sorry about that.

Unfortunately, with 1.0 bearing down on us, we'll likely just live with what we have. We should definitely have static analysis for Error Prone users -- which also has the advantage of working well with isSameInstanceAs(primitive), which isn't exactly handled by ReferenceSubject/PrimitiveSubject split... unless we also introduce a deprecated overload of that into ReferenceSubject.... (Avoiding the split also reduces the number of types are assertThat overloads that users see, which is nice but not essential.) I thought I already had this on my list of Truth static analysis projects, but it appears not. I've added it.

[cpovirk]

(The more specific types on the primitive subjects' isEqualTo, etc. methods could also catch type mismatches at compile time (though, again, so could static analysis). (While I said "normal" isEqualTo above, what we probably actually would have done is to have both isEqualTo(Long) and isEqualTo(long). That should permit widening conversions without permitting someone to call isEqualTo(String). On the other hand, this forbids isEqualTo(Integer), which we otherwise support... unless we add an overload for that, too....))