No Delegate Subtraction

ID

csharp.no_delegate_subtraction

Severity

high

Remediation Complexity

medium

Remediation Risk

low

Remediation Effort

medium

Resource

Code Smell

Language

CSharp

Tags

code_smell, delegate, event, suspicious-operator

Description

Reports - and -= that subtract a composed chain from a delegate — a subtrahend that is itself several delegates combined with +, rather than a reference to one delegate.

Subtracting a single delegate is well defined and is not reported, whether written as -= or as -, and however many single subtrahends the chain has. Unsubscribing from an event with myEvent -= handler is likewise the correct idiom and is not reported.

Rationale

Removal from a delegate chain is defined as removal of a contiguous run, not of individual members. The right-hand chain is searched for inside the left-hand one as a whole sequence; if that exact sequence appears, it is removed, and if it does not, nothing is removed and the result equals the left operand.

So subtracting a two-member chain a + c from a + b + c gives back all three members untouched, because a and c are not adjacent. Removal also starts from the end of the chain, which surprises anyone expecting the first match to go. Code that builds a chain dynamically and then subtracts another chain from it is therefore very likely to keep invoking handlers it believed were gone.

A single-member subtrahend has none of that ambiguity: the last occurrence of that one delegate is removed, which is exactly what the code reads as.

using System;

public delegate void Notify(string message);

public class Publisher
{
    private Notify handlers;
    private event EventHandler Changed;

    public Notify Prune(Notify all, Notify a, Notify b)
    {
        return all - (a + b);           // FLAW — removes a contiguous run, or nothing at all
    }

    public void DropChain(Notify a, Notify b)
    {
        handlers -= a + b;              // FLAW — same rule applies to the compound form
    }

    public void DropOne(Notify one)
    {
        handlers -= one;                // OK, removing a single delegate is well defined
    }

    public Notify Trim(Notify chain, Notify a, Notify b)
    {
        return chain - a - b;           // OK, each subtrahend is a single delegate
    }

    public void Unsubscribe(EventHandler handler)
    {
        Changed -= handler;             // OK, the documented way to unsubscribe from an event
    }
}

Remediation

Remove the members one at a time, so each removal is the well-defined single-delegate case. When the set of handlers is genuinely dynamic, keep them in a collection you control — a List<Notify> — and add or remove entries explicitly, building the chain only when it is time to invoke. For the single-handler case, prefer an event and its -= accessor, whose semantics are the ones every reader expects.

References