49

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?

15
  • 17
    It is checked at runtime. Commented Apr 13, 2018 at 8:08
  • 47
    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. Commented Apr 13, 2018 at 8:14
  • 57
    This kind of analysis requires the compiler to solve the infamous Halting Problem. The designers were wise to not try to tackle that. Commented Apr 13, 2018 at 8:18
  • 15
    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. Commented Apr 13, 2018 at 15:10
  • 13
    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. Commented Apr 13, 2018 at 15:17

4 Answers 4

133

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.

3
  • 29
    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? Commented Apr 13, 2018 at 15:11
  • @TobiaTesan - on the other hand, it is possible for an editor to distinguish between "no unreachable code", "possibly unreachable code", and "unreachable code".
    – TLW
    Commented Apr 14, 2018 at 20:00
  • 2
    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 Commented Apr 14, 2018 at 20:03
27

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.

2
  • 2
    For those curious, the mention of "const parameters" is a bug in the specification. C# doesn't have them.
    – Joe Sewell
    Commented Apr 13, 2018 at 15:05
  • Looks like the spec should use "potentially reachable" instead, as the analysis, as restricted by it, is as obviously sound as incomplete. Commented Apr 14, 2018 at 20:23
9

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.

2
  • 1
    It would be great to see a citation of the bit of the Standard that puts that definition on "reachable". Commented Apr 13, 2018 at 18:55
  • 2
    Alas, this is not an alternative Bar class, since you changed Bar.type from a field to a property, a whole different animal.
    – Zano
    Commented Apr 14, 2018 at 20:12
2

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.

7
  • 2
    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 ...).
    – Joey
    Commented Apr 13, 2018 at 12:40
  • 23
    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. Commented Apr 13, 2018 at 13:59
  • 2
    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. Commented Apr 13, 2018 at 14:01
  • 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. Commented Apr 13, 2018 at 14:05
  • 12
    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. Commented Apr 13, 2018 at 14:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.