Assert.IsAssignableFrom is confusingly named

[jcracknell]

The method name clearly originates from the reflection API method Type.IsAssignableFrom(Type) used to implement it - and to be fair, this API is also confusing in the sense that I generally have to think about the order of operands.

IsAssignableFrom works as an infix operator (which it technically is as an instance method), however the assertion:

Assert.IsAssignableFrom<TExpected>(actual);

asserts that the actual value is a subtype of TExpected (typeof(TExpected).IsAssignableFrom(actual.GetType()) - a reasonable translation); however it reads as "is assignable from the expected type to the type of the actual value", which actually suggests the opposite type ordering relationship.

IsAssignableTo would actually technically work in this scenario, however I would suggest abandoning the entire IsAssignable naming scheme in favor of a clearer assertion called IsSubtype (to match IsType). You could also consider adding IsProperSubtype, as well as IsSupertype and IsProperSupertype (although the utility of supertype assertions feels much lower).

Edit: perhaps the most succinct explanation of the issue is that the assertion implictly reads as follows:

Assert.IsAssignableFrom<TExpected>(to: actual);

[bradwilson]

The method name clearly originates from the reflection API method Type.IsAssignableFrom(Type) used to implement it - and to be fair, this API is also confusing in the sense that I generally have to think about the order of operands.

The naming feels more confusing when there are two types than in our usage, which is a type and an object. We could add Assert.IsAssignableTo<T>(object) that does the exact same thing but then we have two identical functions with different names. 😂 It's just an unfortunate situation that our assertion came at a time when the framework only offered IsAssignableFrom; the introduction of IsAssignableTo is relatively new.

I dunno, if people want it, maybe we'll consider adding the new name and silently deprecating the old one by excluding it from Intellisense. I'd be reluctant to just outright [Obsolete] it because there's really no benefit to changing from one to the other except that the name might be slightly clearer. A better solution would be an Info-level analyzer, as that would be much simpler to turn off and much less intrusive.

IsAssignableTo would actually technically work in this scenario, however I would suggest abandoning the entire IsAssignable naming scheme in favor of a clearer assertion called IsSubtype (to match IsType).

That's an interesting alternative.

You could also consider adding IsProperSubtype, as well as IsSupertype and IsProperSupertype (although the utility of supertype assertions feels much lower).

I definitely do not want to do that. These aren't sets being compared to one another.

Edit: perhaps the most succinct explanation of the issue is that the assertion implictly reads as follows:

Assert.IsAssignableFrom<TExpected>(to: actual);

I disagree. I believe it reads like this:

Assert.IsAssignableFrom<to: TExpected>(from: actual);

Invalid syntax aside, this says "actual is a value that can be assigned to a variable of type TExpected". It's just historically unfortunate that the name is IsAssignableFrom, though it does syntactically match:

typeof(TExpected).IsAssignableFrom(actual.GetType());

Even the parameters are in the same order as the original API.

[bradwilson]

There is a third option: create overloads of Assert.IsType that allow you to specify whether you want exact matches or sub-type matches.

[jcracknell]

It's up to you how you want to deal with the feedback - I just wanted to raise the issue that when I went to find the subtype assertion I did a double take and had to check the docs/implementation to verify how it worked. Most of the problem likely stems from the word "from" in the method name which gives an implied direction to the expected (type) parameter, but IsAssignable also feels off when stood side-by-side with IsType.

[bradwilson]

I put up a poll on the xUnit.net Mastodon account: https://dotnet.social/@xunit/113364104373221985

[Jawvig]

I came here after the poll as I thought there must be more poll options! 😊

Is this an option? IsArgumentAssignableTo<T>(object arg). I'm sure you'll all hate it!

Method names that look like they're supposed to be fluent feel wrong to me when the subject of the method name is as argument that comes afterwards (be that a generic type or a parameter). I would definitely prefer using Shouldly's someVar.ShouldBeAssignableTo<Expected> for this reason. But I think using Assert.IsAssignable... is always going to risk being confusing when two arguments (type + value) follow it.

[bradwilson]

But I think using Assert.IsAssignable... is always going to risk being confusing when two arguments (type + value) follow it.

I agree, both versions of IsAssignable suffer from the same problem, especially since the method comes from Assert and not as a method on the type itself (like the traditional usage). That's why I personally think that IsType overloads are probably the best option, but I'm certainly willing to entertain something else...I just don't think IsAssignableXxx for any value of Xxx is a fix. 😄

[Jawvig]

I think you're right. IsType overloads make sense.

[bradwilson]

The poll has closed, and I've added the IsType overload.

Assertions: xunit/assert.xunit@3222c4c?w=1
Tests: 22c6849?w=1

I'll be creating a new Info-level analyzer which suggests moving from IsAssignableFrom to IsType, as well as updating all the affected Type-related analyzers, soon.

Available in v2: 2.9.3-pre.4
Available in v3: 0.5.0-pre.31

https://xunit.net/docs/using-ci-builds