No Is Check On This

ID

csharp.no_is_check_on_this

Severity

critical

Remediation Complexity

medium

Remediation Risk

low

Remediation Effort

medium

Resource

Code Smell

Language

CSharp

Tags

api-design, code_smell, inheritance

Description

Reports code that asks which of its own subtypes this happens to be: an is test against a pattern that names a runtime type — this is Circle, this is Circle c, this is not Circle, this is Circle or Square, this is Circle { Radius: > 10 } c — and a switch statement or switch expression whose governing expression is this. Code guarded that way is subclass-specific behaviour that has been written in the base type.

Rationale

When a type asks which of its own descendants it happens to be, the branch it takes is knowledge that belongs to the descendant. Two costs follow. Every new subclass forces an edit to the base type, so the class that should have been closed to modification never is. And a subclass added by someone who never reads this method quietly falls into the fallback branch, with no compiler diagnostic and no test failure to reveal it.

Declaring a virtual member and overriding it per subclass moves the decision to the type system: the dispatch is automatic and an unimplemented case is visible at the declaration site.

The spelling does not change the argument, so every modern form of the same test is reported: the negated pattern, the or / and combinators, the recursive pattern that adds property constraints to a type, and both switch forms. What matters is whether the pattern tests a runtime type somewhere inside it — this is not null does not, and is left alone, as are constant patterns, a property pattern with no type, the as conversion and exact-type checks written with GetType().

Type tests on any other operand are perfectly ordinary and are not reported — narrowing a parameter, a field or a collection element is how is is meant to be used.

public abstract class Shape
{
    public double Area()
    {
        if (this is Circle c)              // FLAW — Circle-specific maths in the base type
        {
            return 3.14159 * c.Radius * c.Radius;
        }
        return 0;
    }

    public bool IsFlat() => this is not Circle;      // FLAW — the same test, negated

    public string Kind() => this switch              // FLAW — the same test, as a switch
    {
        Circle => "circle",
        _ => "shape"
    };

    public bool Exists() => this is not null;        // OK — no runtime type is tested

    public string Name(object other)
    {
        if (other is Circle)               // OK — narrowing a value from outside
        {
            return "circle";
        }
        return "shape";
    }
}

public class Circle : Shape
{
    public double Radius;
}

Remediation

Declare a virtual (or abstract) member on the base type and move each branch body — or each switch arm — into the matching subclass override. Where the branch is a one-off and inheritance is not available, consider passing the behaviour in rather than inspecting the runtime type.

References