No Side Effect In Assert

ID

csharp.no_side_effect_in_assert

Severity

high

Remediation Complexity

medium

Remediation Risk

low

Remediation Effort

medium

Resource

Code Smell

Language

CSharp

Tags

assertion, code_smell, diagnostics, side-effect

Description

Reports a Debug.Assert(…​) whose condition changes program state: an assignment, an increment or decrement, or a call to a member whose name means mutation.

Rationale

Debug.Assert is declared with Conditional("DEBUG"). When the DEBUG symbol is not defined, the compiler emits no call at all — and, because the whole call site disappears, no argument is evaluated either. Anything the condition did to program state simply stops happening.

The result is a program that behaves one way under the configuration the developer tested and another way in the configuration that ships. Such a difference is hard to reproduce, because the only build that shows the bug is the one without the assertions.

Assignments and increments are certain mutations. Calls are judged by name only. A member is matched on its first camel-case word against the mutating verbs — Add, Append, Clear, Delete, Dequeue, Dispose, Enqueue, Insert, Pop, Push, Put, Remove, Retain, Set, Update — plus MoveNext and Next, so AddItem, PushBack and UpdateEntry are caught along with the bare verb, while Settings and AddressOf are not. SetEquals is excluded: it reads as a mutation but is a set comparison and belongs in an assertion. Every other call is left alone so that ordinary predicates inside an assertion are not reported.

using System.Collections.Generic;
using System.Diagnostics;

public class Cart
{
    private readonly List<string> items = new List<string>();
    private int checks;

    public void Drop(string sku)
    {
        Debug.Assert(items.Remove(sku));    // FLAW — in release the item is never removed
        Debug.Assert(++checks < 100);       // FLAW — the counter stops advancing in release
    }

    public void DropChecked(string sku)
    {
        bool removed = items.Remove(sku);   // OK — the mutation happens in every build
        Debug.Assert(removed);              // OK — the condition only reads
        Debug.Assert(items.Count >= 0);     // OK — a property read is not a mutation
    }
}

A mutating call is not reported when the enclosing method is itself gated behind [Conditional(…​)] and the mutated receiver is a local variable declared inside that same method — the whole method already disappears from a non-debug build, so there is no differential behavior left to warn about. A field mutated the same way is still reported, because the field stays visible to other, non-conditional code:

[Conditional("DEBUG")]
private void ValidateLocalOnly()
{
    using var seen = new HashSet<string>();
    Debug.Assert(seen.Add("x"));        // OK — seen never leaves this Conditional-gated method
}

[Conditional("DEBUG")]
private void ValidateLeaksField()
{
    Debug.Assert(sharedSet.Add("x"));   // FLAW — sharedSet is a field, its mutation is visible elsewhere
}

Remediation

Move the state change out of the assertion and assert over the result. Assign the call’s value to a local, or perform the mutation on its own statement, then pass the plain condition to Debug.Assert. Where the check itself must survive a release build, use a normal if with an explicit throw instead of an assertion.

References