Unassigned local variable and short-circuit evaluation

Question

I have two methods, both of them compiles correctly:

public int A()
{
    int i;
    if(!int.TryParse("0", out i))
    {
        return -1;
    }
    // do sth
    return i;
}

public int B()
{
    int i;
    if(true)
    {
        return -1;
    }
    return i;
}

In the second case (method B) compiler is smart enough to detect that variable i is never used so it doesn't complain about not assigning it.

However, I have another example (a combination of both) that seems to be equivalent to method B:

public int C()
{
    int i;
    if (true || !int.TryParse("0", out i))
    {
        return -1;
    }
    return i;
}

When compiling on Windows under VisualStudio 2012 (.NET Framework 4.6.01055) it throws an error: Use of unassigned local variable 'i'. The solution is to:

initialize i with any value, or
use | operator instead of ||.

Why is that so? It looks like a compiler has all the necessary data to detect unreachable code.

Side note: Example C compiles on Linux under mono 4.6.2 with warnings about unreachable code as expected.

Are you familiar with the halting problem (https://en.wikipedia.org/wiki/Halting_problem)? That is mainly why compilers cannot always be expected to know whether or not your code is unreachable... — Idos, Jan 19 '17 at 13:48
There was an old bug quite like this but it involved [dynamic](http://stackoverflow.com/a/36062697/15498) — Damien_The_Unbeliever, Jan 19 '17 at 13:59
In the first example in method B - if you wrote invalid code after the closing brace of the _"if (true)"_ statement then it would generate a compiler error despite not being reachable - so reporting the use of an unassigned local variable seems a more consistent approach. — PaulF, Jan 19 '17 at 14:07
@Idos that does not answer the question at all. In this case the compiler can very well know that `i` is not used, and, in fact, that is precisely what it does in later versions. — InBetween, Jan 19 '17 at 14:25
@InBetween The point is that there will *always* be cases that the compiler can't prove. It will catch some of the obvious cases, but you simply can't expect it to always solve the problem, even in cases where you can prove that a given bit of code is unreachable. — Servy, Jan 19 '17 at 14:28
@Servy Yes I understand the point, but generalities shouldn't be used, no matter how valid they are, when they are not applicable to a particular case. The OP is not asking about why the compiler is not able to correctly analyze *all* cases, he's asking about this specific one, and the halting problem does not apply. — InBetween, Jan 19 '17 at 14:33
An interesting question is, whether or not, according to the language spec (which has rules on definite assignment and reachability), a compiler is required to do any particular thing in this context, given that the first argument of the `||` operator is a constant. — Jeroen Mostert, Jan 19 '17 at 14:35
@InBetween The point is that the compiler's rules for "is reachable" aren't actually whether or not it is reachable. It's only capable of recognizing a portion of reachability cases correctly. Assuming that it should always recognize reachable/unreachable code correctly, as this question seems to do, simply isn't a reasonable expectation. — Servy, Jan 19 '17 at 14:36
@Servy Ok, its not worth discussing. I agree with what you are saying, its simply that the comment with no further explanation can be misleading: the behavior is normal, the compiler can't do better. — InBetween, Jan 19 '17 at 14:40
To answer my own question: the compiler is required to evaluate constant expressions at compile time, but the expression as a whole is *not* a constant because `a || b` is only considered a constant if *both* operands are. Therefore, in the published C# standard, the statement after the `if` should be treated as "possibly reachable" and the compiler must give an error. Note that later versions of the C# language (such as implemented by VS 2016) have no formally published specs. The change may be intentional, or it may not. — Jeroen Mostert, Jan 19 '17 at 14:50

score 1 · Accepted Answer · edited May 23 '17 at 12:08

This can't be considered a bug but its an improvable feature. You are correct when you say that the compiler has enough information to know that the unassigned i is never used and therefore it should omit the compiler error.

Its improvable because, as a matter of fact, it has been improved; in VS 2015 the behavior of the compiler is the expected one : no compile time error. I can't say the same thing for previous versions of the compiler because I can't test them at the moment.

Funnily enough, neither VS 2015 or VS 2017 RC report an unreachable code warning at return i which seems a bit odd. if (true) will give this warning and if (true || ....) figures out correctly that i is not used, but the warning has been omitted for reasons I dont understand.

For more insight on why the behavior was changed, check out this answer. I knew this question rung a bell...I asked a similar one a couple years ago myself ;).

score 0 · Answer 2 · answered Jan 19 '17 at 14:18

0

When compiling on Windows under Visual Studio 2012 (.NET Framework 4.6.01055) it throws an error: Use of unassigned local variable 'i'

As it should. The variable is unassigned. You may not reference it in the code. Sometimes though, depending on the compiler, it might know on beforehand the line is never executed and removes it entirely before checking variable use. Other versions might switch the steps which might result in different outcomes. Personally I would say you shouldn't rely on that: code safe.

Just for your information. Roslyn (the latest .NET compiler) does not give an error on compiling. It seems the validation just discards all || conditions.

if (true || !int.TryParse("0", out i)) // // does not give an error, first return always used, other discarded

if (true && !int.TryParse("0", out i)) // does not give an error, i is known to always be assigned

if (false && !int.TryParse("0", out i)) // gives an error, if never results in true, and parse is known not to be called

answered Jan 19 '17 at 14:18

Patrick Hofman

153,850
22
249
325

1

*As it should*. No, its not as it should be. Your own answer disqualifies that reasoning; newer compiler versions don't raise a compile time error. – InBetween Jan 19 '17 at 14:20
That they have optimized the compiler to handle this correctly doesn't matter you should have such code lingering around. If someone changes the `true` to `false`, you open up an entire box of problems. I think you should fix them sooner. – Patrick Hofman Jan 19 '17 at 14:25
1

@InBetween: as that section of code is unreachable - should all invalid code be ignored? – PaulF Jan 19 '17 at 14:27
1

@PaulF Its not what I think the compiler should or should not do. The fact is that the compiler team decided to *change* the behavior of the compiler which to me seems like they decided that not raising an error is the sensible thing to do. – InBetween Jan 19 '17 at 14:31
@PaulF And its a sensible decision too. I knew this topic rung a bell...I asked a similar question years ago and this was Eric Lippert's [answer](http://stackoverflow.com/a/9418666/767890). – InBetween Jan 19 '17 at 14:37

Unassigned local variable and short-circuit evaluation

2 Answers2