Skip to content

PragmaScope with an empty warning list emits a bare disable/restore, silently suppressing all warnings and clobbering an enclosing scope #97

Description

@matt-edmondson

What happens

Neither constructor guards against an empty warning list:

// CodeBlocker/Scopes.cs:172-187
public PragmaScope(CodeBlocker codeBlocker, IEnumerable<string> warnings)
    : this(codeBlocker, string.Join(", ", warnings ?? []))
{
}

private static void Begin(CodeBlocker codeBlocker, string warnings)
{
    Ensure.NotNull(codeBlocker);
    codeBlocker.WriteLine($"#pragma warning disable {warnings}");
}

private static void End(CodeBlocker codeBlocker, string warnings)
{
    Ensure.NotNull(codeBlocker);
    codeBlocker.WriteLine($"#pragma warning restore {warnings}");
}

With an empty sequence — or an empty string — the emitted directives carry no warning list, which in C# means every warning.

Failure scenario

A generator computing suppressions per member, where one member needs none:

using (new PragmaScope(cb, "CS1591"))
{
    using (new PragmaScope(cb, suppressions))   // suppressions is empty
    {
        cb.WriteLine("public int X;");
    }

    cb.WriteLine("public int Y;");
}

emits:

#pragma warning disable CS1591
#pragma warning disable 
public int X;
#pragma warning restore 
public int Y;               // CS1591 is no longer suppressed here
#pragma warning restore CS1591

The bare restore restores all warnings, so public int Y; warns despite sitting inside the outer scope. And inside the inner scope, every diagnostic is off rather than none of them.

Why it matters

PragmaScope's own remarks (Scopes.cs:154-158) state that it exists because "an unbalanced suppression leaks into the rest of the file and is tedious to trace back". The empty-list case produces exactly that leak, in both directions, from the type built to prevent it.

It is also the case most likely to arise from generated input rather than a literal — a hand-written new PragmaScope(cb, "") is unlikely, but a computed collection that happens to be empty is routine.

Missing coverage

ScopesTests.cs covers "CS1591" (:113) and ["CS1591", "CA1707"] (:128) only.

Suggested fix

Either throw ArgumentException on an empty or whitespace warning list, or make an empty list a no-op scope that writes no directives at all.

The no-op reading is probably the more useful one for generator code — it lets a caller pass a computed collection without branching — but either is defensible. Whichever is chosen should be documented on the warnings parameter and pinned by a test, since the current behaviour is the one option that is never what the caller wanted.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

bugSomething isn't working

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions