rspec/rules/S2219/csharp/rule.adoc

== Why is this an issue?

To check the type of an object there are several options:

* ``++expr is SomeType++`` or ``++expr.GetType() == typeof(SomeType)++`` if the type is known at compile time,
* ``++typeInstance.IsInstanceOfType(expr)++`` if the type is calculated during runtime.

If runtime calculated ``++Type++``s need to be compared:

* ``++typeInstance1.IsAssignableFrom(typeInstance2)++``.

Depending on whether the type is returned by a ``++GetType()++`` or ``++typeof()++`` call, the ``++IsAssignableFrom()++`` and ``++IsInstanceOfType()++`` might be simplified. Similarly, if the type is ``++sealed++``, the type comparison with ``++==++`` can be converted to an ``++is++`` call. Simplifying the calls also make ``++null++`` checking unnecessary because both ``++is++`` and ``++IsInstanceOfType++`` performs it already.


Finally, utilizing the most concise language constructs for type checking makes the code more readable, so

* ``++expr as T != null++`` checks should be simplified to ``++expr is T++``, and
* ``++expr is T++`` should be converted to ``++expr != null++``, when ``++expr++`` is of type ``++T++``.

=== Noncompliant code example

[source,csharp]
----
class Fruit { }
sealed class Apple : Fruit { }

class Program
{
  static void Main()
  {
    var apple = new Apple();
    var b = apple != null && apple.GetType() == typeof (Apple); // Noncompliant
    b = typeof(Apple).IsInstanceOfType(apple); // Noncompliant
    if (apple != null)
    {
      b = typeof(Apple).IsAssignableFrom(apple.GetType()); // Noncompliant
    }
    var appleType = typeof (Apple);
    if (apple != null)
    {
      b = appleType.IsAssignableFrom(apple.GetType()); // Noncompliant
    }

    Fruit f = apple;
    if (f as Apple != null) // Noncompliant
    {
    }
    if (apple is Apple) // Noncompliant
    {
    }
  }
}
----

=== Compliant solution

[source,csharp]
----
class Fruit { }
sealed class Apple : Fruit { }

class Program
{
  static void Main()
  {
    var apple = new Apple();
    var b = apple is Apple;
    b = apple is Apple;
    b = apple is Apple;
    var appleType = typeof(Apple);
    b = appleType.IsInstanceOfType(apple);

    Fruit f = apple;
    if (f is Apple)
    {
    }
    if (apple != null)
    {
    }
  }
}
----

=== Exceptions

Calling ``++GetType++`` on an object of ``++Nullable<T>++`` type returns the underlying generic type parameter ``++T++``, thus a comparison with ``++typeof(Nullable<T>)++`` can't be simplified to use the ``++is++`` operator, which doesn't make difference between ``++T++`` and ``++T?++``.

[source,csharp]
----
int? i = 42;
bool condition = i.GetType() == typeof(int?); // false;
condition = i is int?; // true
----
No issue is reported on the following expressions:

* ``++expr is T++`` when either operand of the ``++is++`` operator is a value type. In that case CS0183 or CS0184 reports
* ``++expr is object++``, as this is a common and efficient pattern to do null checks

ifdef::env-github,rspecator-view[]

'''
== Implementation Specification
(visible only on this page)

=== Message

Use [the "is" operator|the "IsInstanceOfType()" method|a "null" check] instead.


'''
== Comments And Links
(visible only on this page)

=== on 24 May 2019, 16:47:10 Andrei Epure wrote:
\[~nicolas.harraudeau] there's an interesting remark from a user (issue https://github.com/SonarSource/sonar-dotnet/issues/2424[#2424]) who says that it's actually better to perform ``++expr is object++`` rather than ``++expr != null++`` because of performance implications

=== on 24 May 2019, 17:15:38 Nicolas Harraudeau wrote:
\[~andrei.epure] It looks to me like this is a tradeof between readability and performance. Using ``++expr is object++`` to do a null check is more performant but less intuitive than ``++expr != null++``. We could simply accept both ``++expr is object++`` and ``++expr != null++``, explaining in the description the difference. What do you think?

include::../comments-and-links.adoc[]

endif::env-github,rspecator-view[]