Why isn't this code unreachable?

Question

I found a case where I have some code that I believe to be unreachable and is not detected. No warning is issued neither by the compiler nor by Visual Studio.

Consider this code:

enum Foo { A, B, C }
class Bar { public Foo type; }

static class Program
{
    private static void Main()
    {
        var bar = new Bar { type = Foo.A };

        if (bar.type == Foo.B)
        {
            Console.WriteLine("lol");
        }
    }
}

Obviously, the program will not print out "lol" because the condition in the if statement is false. I do not understand why a warning is not issued for the unreachable code though. My only hypothesis is that that could potentially be reachable if you have a race condition in a multi-threaded program. Is this correct?

Rosebud

It is checked at runtime. – Kristynkrock

Or maybe Foo.A could potentially be equal to Foo.B – Amnesia

Compile-time checks for reachability don't cover everything. Because of this you would usually record code-coverage during unit-tests or by manually running the program. While the race-condition would be a valid case, a much more likely one would be that the Bar class modifies the value itself. That might not be the case in your example but certainly a possibility in a real world application. – Duck

I know there are duplicates for this question here on Stack Overflow that does a good job of explaining this, but @ManfredRadlwimmer has explained it rather well above. The compiler doesn't do this particular kind of control flow analysis and thus doesn't detect that this code is never going to be executed. – Fortuitous

This kind of analysis requires the compiler to solve the infamous Halting Problem. The designers were wise to not try to tackle that. – Nelsen

Unreachable at compile time but not necessarily at runtime. You could put a breakpoint on the "if" statement and then change your bar variable. – Intellectuality

@ChrisNeve I'm pretty sure that the specs of languages don't take into account that kind of modification in a debugging session. Sure, theoretically if you have enough bad luck that some high energy particle hits the wrong transistor that might happen without using a debugger... – Thrum

Anyway, I'm pretty sure that there are tools that do some static analysis for unreachable code(and other stuff). Just be aware that, since the underlying problem is undecidable, they have to choose between correctness and completeness (i.e. either they are able to find all cases of unreachable code, but they will also incorrectly detect some reachable code as unreachable, or they will not detect some unreachable code) – Thrum

@Giacomo Alzetta That's a good point, and you're right, the specs probably don't take that case into account, but code that is completely and always unreachable (eg a return right after another return) will also be unreachable at runtime. – Intellectuality

There are plenty of cases where the compiler doesn't report unreachable code when it could. int x = M(); if (x == 123.456) { /* unreachable */ } The comparison is legal because x is convertible to double, and the comparison will always be false no matter what the value of x is. The compiler isn't smart enough to deduce that, and is not required by the specification to be that smart. If you're in the habit of writing unreachable code then get in the habit of using a code coverage tool. – Farthing

Incidentally, the compiler used to treat the consequence in int x = M(); if (x * 0 != 0) { ... } as unreachable, reasoning that any int times zero was zero, and so the condition was false. Though that reasoning was correct, that rule is found nowhere in the specification and therefore was a bug in the compiler which I fixed. Since C# 3.0 the compiler has correctly-according-to-the-specification treated the consequence of an if as unreachable only when the condition is false and involves only constant expressions. – Farthing

@EricLippert The removal of comments from JSON comes to mind. Even a (much greater) nuisance is worth it when the effect is less surprise and more interoperability. – Ardene

@ChrisNeve Is actually right. What if some code on another thread modifies Foo.type? It’s not a local variable, it’s a field in some object. – Tourane

@EricLippert maybe it was a bug in the specification that should have been fixed? – Josephson

@JensTimmerman: It was not. The specification was right, the implementation was wrong. Remember, one of the goals of the specification is to be simple and understandable and it is already 800 pages long. The rules in the spec are simple: the consequence of an if is unreachable only if the condition is constant false, and constant expressions do not contain variables. – Farthing

Answer 1

Static analysis can only do so much, and it will only mark code as unreachable if it can prove that a value cannot be changed. In your code, what happens inside Bar is out of the scope of the method flow and can't be statically reasoned about. What if Bar's constructor launches a thread that sets the value of type back to B? The compiler can't know about it, because, again, the internals of Bar aren't scoped to the method.

If your code was checking the value of a local variable, then the compiler could know if there was no way for it to change. But that's not the case here.

Jame

Static analysis can only do so much namely, most (useful) static analysis is sound but not complete - i.e. it is not guaranteed to infer all properties of programs, just that what it does infer is true. Inferring all properties of programs (including whether they halt or not or variations thereof) is proven undecidable, and we like our editor not hanging possibly indefinitely, right? – Archibold

@TobiaTesan - on the other hand, it is possible for an editor to distinguish between "no unreachable code", "possibly unreachable code", and "unreachable code". – Bravar

ReSharper, for instance, adds all sorts of analyses and warnings that the compiler doesn't. Like warnings about possibly unintended mutual recursion or possible Stack overflow exceptions. But these are distinctly marked and separate from the compiler output – Jame

Answer 2

The C# specification says,

The first embedded statement of an if statement is reachable if the if statement is reachable and the boolean expression does not have the constant value false.

and, concerning constant expressions,

A constant expression must be the null literal or a value with one of the following types: sbyte, byte, short, ushort, int, uint, long, ulong, char, float, double, decimal, bool, object, string, or any enumeration type.

Only the following constructs are permitted in constant expressions:

• Literals (including the null literal).
• References to const members of class and struct types.
• References to members of enumeration types.
• References to const parameters or local variables
• Parenthesized sub-expressions, which are themselves constant expressions.
• Cast expressions, provided the target type is one of the types listed above. checked and unchecked expressions
• Default value expressions
• The predefined +, –, !, and ~ unary operators.
• The predefined +, –, *, /, %, <<, >>, &, |, ^, &&, ||, ==, !=, <, >, <=, and >= binary operators, provided each operand is of a type listed above.
• The ?: conditional operator.

Member access expressions are not in this list, so the boolean expression is not constant. Thus the body of the if block is reachable.

Ruttish

For those curious, the mention of "const parameters" is a bug in the specification. C# doesn't have them. – Walsh

Looks like the spec should use "potentially reachable" instead, as the analysis, as restricted by it, is as obviously sound as incomplete. – Dorsett

Answer 3

Because no such guarantee can be made at compile time. Consider this alternative Bar class

class Bar
{
   Random random = new Random();
   Array Foos = Enum.GetValues(typeof(Foo));

    private Foo _type;
    public Foo type
    {
        get { return _type; }
        set
        {
            _type = (Foo)Foos.GetValue(random.Next(3));
        }
    }
}

Please note that "reachable" is defined at function level. It is not allowed to reach outside the function that is being tested even when it is safe to do so.

Abrade

It would be great to see a citation of the bit of the Standard that puts that definition on "reachable". – Neuritis

Alas, this is not an alternative Bar class, since you changed Bar.type from a field to a property, a whole different animal. – Alimony

Answer 4

The warning you expected is not implemented because it's not a useful warning to have.

In real-world applications, the compiler is very often faced with code that it can totally prove is unreachable, maybe even something as bald-faced as

static class Program
{
    private static void Main()
    {
        if (false)
        {
            Console.WriteLine("lol");
        }
    }
}

I don't have a C# compiler on this computer but I bet you there's no warning for that, either. This is because, when you put if (false) { ... } around a block of code, you did that on purpose, perhaps to disable something temporarily for an experiment. Nagging you about it would not be helpful.

More common is that it's not a literal false, it's a compile-time constant that the build system will set to true or false depending on configuration; you want the compiler to delete the unreachable code in one build but not the other, and you don't want complaints either way.

Even more common than that is for early optimizations like inlining and constant propagation to discover that a conditional is always false; suppose you have something like

static class Program
{
    private static void Fizz(int i)
    {
        if (i % 3 == 0) {
            Console.WriteLine("fizz");
        } else {
            Console.WriteLine(i);
        }
    }

    private static void Main()
    {
        Fizz(4);
    }
}

You clearly wouldn't want to get told that one side of the conditional inside Fizz() was unreachable just because it was only ever called with argument 4 in this program.

Unaneled

Indeed. I actually find it quite annoying that Java makes some of those analyses an error. So you quickly ensure that certain code doesn't run by preceding it with a return;, only to be met with code that no longer compiles (if (false) is fine because the check isn't terribly smart ...). – Chick

This is all very nice and well, except that your core premise is simply not true. Surrounding code with if (false) will generate a compiler warning, because you do have unreachable code. The compiler shouldn't be making assumptions about why you have unreachable code, just that it's there. What if you temporarily replaced a real condition with false and forgot to change it back? The warning would have saved you. – Jame

Specifically, if you intentionally put a chunk of unreachable code, it's your responsibility to explicitly mark it for the compiler to avoid a warning using #pragma warning disable, absolutely NOT the compiler's job. – Jame

Additionally, the Fizz example doesn't quite cut it either, because the C# compiler won't issue an Unreachable Code warning for this case (again, tested) precisely because its analysis is scoped to single method, and it doesn't make the assumption that it will always be called just the one time it's called in the current program. – Jame

@AvnerShahar-Kashtan I am not terribly experienced with C# but I would consider that a defect in the compiler and/or the language specification, sufficient to render the language unsuitable for programming in the large. – Unaneled

You are, of course, entitled to your opinion about the language and its spec. But going from "the static analyzer is scoped to a method" to "the language is unusable" is... not an entirely rational argument to make. – Jame

Code that should definitively be elided at compile time, but conditionally, is properly handled by preprocessor directives; #if DONTRUN .. #endif will not trigger a warning (obviously). I'm not aware of any language that guarantees dead code removal in its spec, even if you explicitly surround it with things like if (false), simply because it doesn't pay off to spell out rules for that and have your compiler follow them. (C# lacks any kind of static metaprogramming a la C++, so that if (thingthatsprobablyconstant()) does not elide code isn't a problem.) – Avina