Why does C# compiler assumes default value for SqlGuid type and let me use unassigned variables?

[felipepessoto]

Version Used: .NET Core 2.1 and Visual Studio 15.8. Compiler is latest minor (the same happens with latest major)

Steps to Reproduce:

    static void Main(string[] args)
    {
        SqlGuid varX;
        Console.WriteLine(varX.ToString());//OK

        SqlString varY;
        Console.WriteLine(varY.ToString());//OK

        int varZ;
        //Error CS0165  Use of unassigned local variable 'varZ' varZ.ToString();

        MyStruct varA;
        //Error CS0165  Use of unassigned local variable 'varA' varA.ToString();
    }

Expected Behavior:
Error CS0165 Use of unassigned local variable 'varX' varX.ToString();
Error CS0165 Use of unassigned local variable 'varY' varY.ToString();

Actual Behavior:
Compiles with no error

Notes: When I create a .NET 4.6.1 project, SqlString doesn't work as well. I also tried to refer System.Data.Common from .NET Core, and then SqlString works in .NET 4.6.1.
So, I guess there is something in type's IL that allows me to use unassigned variables. But I couldn't find any relevant difference in SqlString types from .NET Core and .NET Framework using ILSpy.

When I decompile the code above, I can see that C# compile generated a default(SqlGuid) to my local variable:

[CyrusNajmabadi]

SqlGuid is likely an empty struct in whatever assembly you are referencing. An empty struct is definitely assigned at its declaration point by definition (since... well... there is nothing to assign). So you can use it immediately.

[gafter]

@CyrusNajmabadi is almost certainly correct. This is likely a bug in the reference assembly that contains SqlGuid, which makes it appear empty.

[felipepessoto]

@CyrusNajmabadi, you are right, I tried with a empty struct and it worked as well.

The reference assembly contains the field. Maybe the C# compiler is not considering it:

[svick]

@gafter @CyrusNajmabadi I'm not sure that's it. On my machine, the System.Data.Common reference assembly for .Net Core 2.1 does contain a field, but the code still compiles as if the variable was definitely assigned.

The same thing happens when I use the Roslyn 2.9.0 API:

using System;
using System.Linq;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;

class Program
{
    static void Main()
    {
        string code = @"
using System.Data.SqlTypes;

class Program
{
    static void Main()
    {
        SqlGuid guid;
        guid.ToString();
    }
}";

        var tree = SyntaxFactory.ParseSyntaxTree(code);

        var assemblies = new[] { "System.Runtime", "System.Data.Common"};

        var compilation = CSharpCompilation.Create(null)
            .AddSyntaxTrees(tree)
            .AddReferences(assemblies.Select(a => MetadataReference.CreateFromFile(
                $@"C:\Users\Svick\.nuget\packages\microsoft.netcore.app\2.1.0\ref\netcoreapp2.1\{a}.dll")));

        foreach (var diag in compilation.GetDiagnostics())
        {
            Console.WriteLine(diag);
        }

        var model = compilation.GetSemanticModel(tree);

        var variableType = tree.GetRoot().DescendantNodes().OfType<VariableDeclarationSyntax>().Single().Type;

        var symbol = (ITypeSymbol)model.GetSymbolInfo(variableType).Symbol;

        var fields = symbol.GetMembers().OfType<IFieldSymbol>().Where(f => !f.IsStatic);

        foreach (var field in fields)
        {
            Console.WriteLine(field);
        }
    }
}

The output of the above code is:

System.Data.SqlTypes.SqlGuid._dummy

This shows that the code compiles fine (no diagnostics), even though the type of the variable has an instance field.

[svick]

Turns out @fujiy was right: the compiler is ignoring the field, on purpose, because of this code:

roslyn/src/Compilers/CSharp/Portable/FlowAnalysis/EmptyStructTypeCache.cs

Lines 191 to 195 in 57cecaf

	return _dev12CompilerCompatibility && // when we're trying to be compatible with the native compiler, we ignore
	((object)member.ContainingAssembly != _sourceAssembly || // imported fields
	member.ContainingModule.Ordinal != 0) && // (an added module is imported)
	IsIgnorableType(memberType) && // of reference type (but not type parameters, looking through arrays)
	!IsAccessibleInAssembly(member, _sourceAssembly); // that are inaccessible to our assembly.

And indeed, with <Features>strict</Features> in csproj, I get the expected error.

I find it odd that .Net goes out of its way to include a dummy field in the reference assembly and Roslyn then just ignores it.

[gafter]

Right, I had forgotten about that.

We made a mistake in the past of not including any private fields in reference assemblies. We did that because such fields (of reference type) were ignored by older compilers anyway. We could not just add them to the libraries because that would have broken people who used those "broken" libraries and depended in the blank (uninitialized) declaration to give the variable its default value. Or plan to address this issue is as follows:

1. First, we made the Roslyn compiler ignore private fields of reference type in reference assemblies. This aligns the compiler with the same bug in the native compiler. This temporarily has the problematic effect you observe.
2. Now, we are fixing the reference assemblies to start including the private fields that were omitted. This has largely been done for many assemblies and more are being fixed.
3. Next, we will add a mode to the compiler where people can explicitly ask to get the arguably correct definite assignment behavior. This will likely be linked to a warning wave in C# 8. At least, that is my hope.

While you are waiting for step 3, I recommend you use the "strict" feature flag.

[alrz]

that would have broken people who used those "broken" libraries

This is understandable (wrt backward compat) but isn't that actually more harmful than useful? Instead of a hard error with a sensible message, now I must be aware of the whole history behind this and versions that are affected, etc, plus the possible fix in the future and also, remember to opt-in!

Please break my broken code.

[CyrusNajmabadi]

@alrz The idea of Warning Waves is that htey allow you to explicitly state that you are ok with your broken code breaking. Until then, it's not really acceptable to impose that idea on people who haven't said they're ok with this.

[felipepessoto]

@CyrusNajmabadi I'm not sure what is worse. Breaking back compat is very bad. But allowing to use uninitialized variable is also not desirable and can lead to bugs.

I opened this issue after facing the problem in a customer. They have a VB.NET function with a (<Out()> ByRef SqlGuid myVar), that throws when they use myVar.Value.

I thought it would never happen if the code was written in C# (unless they set the out argument to null inside the method), because VB.NET is not aware of "out" modifier and C# wouldn't even to use the out argument if you not initialize it before. But I had the same problem since the C# compiler allows me to use SqlGuid because it is "empty".

static void MyMethod(out SqlGuid varX)
{
var x = varX.Value;
}

The strict option turn on the right behavior like you said:

Maybe we could enable the Strict flag when creating new projects, at least we will be safe by default for new projects.

There is any official docs about what changes when I enable this flag?

Thanks

[jmarolf]

Maybe we could enable the Strict flag when creating new projects, at least we will be safe by default for new projects.

new project templates in 15.8 should have strict on by default. Only existing projects will not be opted into this behavior

[gafter]

new project templates in 15.8 should have strict on by default. Only existing projects will not be opted into this behavior

That's interesting, given that the flag is not documented or supported, and that we expect to add new warnings to it in the future. How did this decision occur?

[felipepessoto]

@jmarolf @gafter , I'm using VS 15.8.1 and it doesn't include the strict flag in new projects.

[jmarolf]

@gafter @fujiy

That's interesting, given that the flag is not documented or supported, and that we expect to add new warnings to it in the future. How did this decision occur?

Sorry, had I was wrong determinism is now on by default. strict is not.

[DanielSchuessler]

FWIW, I also just stumbled over this in the wild. I was very puzzled about the fact that the compiler allowed me to use a local var of my XlCellValue struct without initialization. (It's a struct with a single field readonly object value which is part of an API ensuring that the value can only be of types produced/accepted by the Excel COM API (Range.Value etc.)).

Thanks @svick for the <Features>strict</Features> suggestion!