Weird C# compiler issue with variable name ambiguity

Question

Let's take the following code:

class Foo
{
   string bar;

   public void Method()
   {
      if (!String.IsNullOrEmpty(this.bar))
      {
         string bar = "Hello";
         Console.Write(bar);
      }
   }
}

This will compile, and all is good. However, let's now remove the this. prefix:

class Foo
{
   string bar;

   public void Method()
   {
      if (!String.IsNullOrEmpty(bar)) // <-- Removed "this."
      {
         string bar = "Hello";
         Console.Write(bar);
      }
   }
}

In this case, I get a compiler error. I agree this is an error, however it's the location of the error that confuses me. The error happens on the line:

string bar = "Hello";

With the message:

A local variable named 'bar' cannot be declared in this scope because it would give a different meaning to 'bar', which is already used in a 'parent or current' scope to denote something else

From what I understand about the compiler, the declaration of bar gets hoisted to the top of the Method() method. However, if that's the case, the line:

if (!String.IsNullOrEmpty(bar))

Should be considered ambiguous, as bar could be a reference to the instance field or to the not-yet-declared local variable.

To me, it seems odd that removing this. can cause a compilation error on another line. In other words, declaring a local bar variable is perfectly valid, as long as no potentially ambiguous references to bar have been made previously in the scope (note, if I comment out if (!String.IsNullOrEmpty(bar)) then the error goes away).

That all seems rather pedantic, so what's your question?:

My question is why the compiler allows an ambiguous reference to a variable before it's declared in the scope, but then flags the declaration itself as redundant. Shouldn't the ambiguous reference to bar in String.IsNullOrEmpty() be a more precise location of the error? In my example, it's of course easy to spot, but when I ran across this issue in the wild, the reference was pages up and much harder to track down.

Answer 1

From what I understand about the compiler, the declaration of bar gets hoisted to the top of the Method() method.

No, that's not the case.

The error message is quite precise here:

A local variable named 'bar' cannot be declared in this scope because it would give a different meaning to 'bar', which is already used in a 'parent or current' scope to denote something else.

The part of the C# spec which is being violated is section 7.6.2.1 (C# 4 and 5 specs):

Invariant meaning in blocks
For each occurrence of a given identifier as a full simple-name (without a type argument list) in an expression or declarator, within the local variable declaration space (§3.3) immediately enclosing that occurrence, every other occurrence of the same identifier as a full simple-name in an expression or declarator must refer to the same entity. This rule ensures that the meaning of a name is always the same within a given block, switch block, for-, foreach- or using-statement, or anonymous function.

And in the annotated C# specification, this has a helpful annotation from Eric Lippert:

One of the more subtle desirable consequences of this rule is that it becomes safer to undertake refactorings that involve moving around local variable declarations. Any such refactoring that would cause a simple name to change its semantics will be caught by the compiler.

Aside from anything else though, it seems to me that this is just good for clarity. Even if the second version were allowed, the first is clearer IMO. The compiler is ensuring that you don't write pathologically unclear code, when you can very easily fix it to be obvious what you mean where.

To put it another way: do you really want to be able to write the second version?

In particular:

In my example, it's of course easy to spot, but when I ran across this issue in the wild, the reference was pages up and much harder to track down.

... and that makes it more reasonable to allow it? Quite the reverse, I'd say - and you should also treat it as strong encouragement to refactor your code so that a single method isn't "pages" long.

Answer 2

The second definition of bar is not pulled to the method level, its scope is the if block. For example this is completely valid.

class Foo
{
    private string bar;

    public void Method()
    {
        if (!String.IsNullOrEmpty(bar)) // <-- No more this.
        {
            string bar1 = "Hello";
            Console.Write(bar);
        }

        if (!String.IsNullOrEmpty(bar)) // <-- No more this.
        {
            string bar1 = "Hello";
            Console.Write(bar);
        }
    }
}

The reason is that bar is no longer ambiguous, there is a clear name distinction between the outer and inner scope of bar/bar1 - the compiler disallows overriding the outer scope bar by a local definition.