dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.11k stars 4.04k forks source link

Use of possibly unassigned field '_dummyPrimitive' (with sharplab) #67212

Open AnorZaken opened 1 year ago

AnorZaken commented 1 year ago

Version Used: All versions available on sharplab

Steps to Reproduce:

  1. write (non-async) Task method
  2. write if expression on some bool + resolving a nullable value, e.g. "msg.isFoo && msg.NullableId is not Guid id".
  3. write local (async) implementation method inside the first method that uses the non-nullable result (binds this+id) and call it. ...Error manifests if the result of the nullable expression is used inside the local method, e.g. "Console.Write(id);"

A minimal repro, with source-code provided, is ideal. Using sharplab is preferred for compiler/language issues whenever possible: https://sharplab.io/#v2:C4LgTgrgdgNAJiA1AHwAICYAMBYAUBgRj1UwAJUCBWAbj2IGZz1yCB2PAbz1J/MYoBs5IQFkAhgEsoACgCU3Xl1y8VpAG5iwpALYBTAM76xAc12kAvKSi6A7qREGjpgCLAA9nNrLVPBT4kAZqTSeoYmugB0EvoAYm5upABkiTqO4VFw4gCeAEZm0VZuwKQA4hAScKQV8t4+Sj4+FACc0gBEgDwbgCD7rbJeDaqorMIRAMJu2gAOADa6wLpwqAJ9DQC+fqrrKoOkAIL6WVAAxgCSk1Oem7yXPKgAHMK7+0en03LXpPX9vIHBoU6R0TibhqX0U736zWk1WWoLWtVUcJUcLhxGYDjCLncnHWqEYOXiUyqsXiH1MwGoiN4uNK5TgAH4qpkxLkzBwyRS8CsgA

Diagnostic Id: CS0170

Expected Behavior: No error.

Actual Behavior: Error on hidden compiler variable.

AnorZaken commented 1 year ago

Complete repo code (same as sharplab link above):

using System;
using System.Threading.Tasks;

public class Program
{
    public static Task Main()
    {
        var message = new MessageDto();

        if (message.isFoo && message.idMaybe is not Guid id)
        {
            Console.WriteLine("🌄");
            return Task.CompletedTask;
        }

        return AsyncImpl(); // <--- error CS0170 on this line

        async Task AsyncImpl()
        {
            if (message.isFoo)
            {
                Console.WriteLine(id);
            }
        }
    }
}

class MessageDto
{
    public bool isFoo {get;}
    public Guid? idMaybe {get;}
}
RikkiGibson commented 1 year ago

It's correct to have a definite assignment error here. However, it would be more useful for the message to refer to id instead of _dummyPrimitive.

Compare to what happens when the local function body is inlined at the usage. SharpLab

using System;
using System.Threading.Tasks;

public class Program
{
    public static Task Main()
    {
        var message = new MessageDto();

        if (message.isFoo && message.idMaybe is not Guid id)
        {
            Console.WriteLine("🌄");
            return Task.CompletedTask;
        }

        if (message.isFoo)
        {
            Console.WriteLine(id); // error CS0165: Use of unassigned local variable 'id'
        }

        return null!;
    }
}

class MessageDto
{
    public bool isFoo {get;}
    public Guid? idMaybe {get;}
}

Let's keep the issue open to track fixing the diagnostic message.

AnorZaken commented 1 year ago

It's correct to have a definite assignment error here. However, it would be more useful for the message to refer to id instead of _dummyPrimitive.

Compare to what happens when the local function body is inlined at the usage. SharpLab

using System;
using System.Threading.Tasks;

public class Program
{
    public static Task Main()
    {
        var message = new MessageDto();

        if (message.isFoo && message.idMaybe is not Guid id)
        {
            Console.WriteLine("🌄");
            return Task.CompletedTask;
        }

        if (message.isFoo)
        {
            Console.WriteLine(id); // error CS0165: Use of unassigned local variable 'id'
        }

        return null!;
    }
}

class MessageDto
{
    public bool isFoo {get;}
    public Guid? idMaybe {get;}
}

Let's keep the issue open to track fixing the diagnostic message.

I thought you meant there could be multi-threading issues, because isFoo could change between the two if-statements, so I stored the values in local variables, but it still gives that unassigned error (on id). It is not correct that it should do so.

https://sharplab.io/#v2:EYLgtghglgdgNAFxAJwK7wCYgNQB8ACATAAwCwAUEQIwX7EAE+VArANwW0DMjhjVA7BQDeFemMbcmANkYyAstBgAKAJSjxI8uO30AbhGT0wAUwDOpiAHNj9ALz0YxgO705Zi9YAiCAPar2Wjpi6kHAPj4ANvRQpgBi4XZG7lbGAHQx8T4BQWIA4qhQGAD80RgAcokm5inpGAoAnsDG2TkhOlAAZvRKGQkAZH2lFTEOPgj0+YWlaoFBmjk6TACcSgBEgDwbgCD7qyotC2L4/LKpAMI+YAAOEcYIxhj4Uns6AL5t2m/ind29PjML8/sDlQVoVdvQAPTg+jGZDIHyGE4AZWIVCkzBA9AAqqYbD4uugIOYoJZHBh6BEfABjCBRfTIKAQYDXegAckKLI+Ylesx0nMYRxgqAiEQAhE9udzaLw3NUvL5hG18NwwpFonEEkJrAhWNztEqJgViqUGk16JqbjqKM8gA===

There is zero possibility of id being unassigned on that line. The compiler is doing a poor job analyzing the flow.

It should be evident enough by the fact that you can inline it further...

https://sharplab.io/#v2:EYLgtghglgdgNAFxAJwK7wCYgNQB8ACATAAwCwAUEQIwX7EAE+VArANwW0DMjhjVA7BQDeFemMbcmANkYyAstBgAKAJSjxI8uO30AbhGT0wAUwDOpiAHNj9ALz0YxgO705Zi9YAiCAPar2Wjpi6kHAPj4ANvRQpgBi4XZG7lbGAHQx8T4BQWIA4qhQGAD80RgAcokm5inpGAoAnsDG2TkhOlAAZvRKGeFqgUGaOe1dPeXRpg4+CPT5haX9w9pDS0FMAJxKAESAPBuAIPtbKi2r2vj8sqkAwj5gAA4RxgjGGPhSx0sAvm3DG2NH9AB6AH0YzIZA+QyWHyOegAWlKxhgCCgAGMIFFIpZUeiIvVvuIvgMdPixGcLtc7g8ni83m1CYTaLw3NUvL5hG18NwwpEJpl6EJrAhWITTtw5sVSg0mvzBcKKB8gA===

The real issue is of course that it doesn't properly narrow "type" possibilities...

https://sharplab.io/#v2:EYLgtghglgdgNAExAagD4AEBMAGAsAKCwEYD1sACdIgVgG4DSBmSzSogdgIG8Dy/LmVAGyURAWWgwAFAEpe/Hvn7LyANwgAncmACmAZz0QA5jvIBecjB0B3cmP2GTAEQAuAe1n0lKvvJ/A3NwAbcig9ADFA820HYx0AOjDIty8fPgBxAFcoBAB+UIQAOWjdAzjEhAkAT2AdVLS/FSgAM3IpJKiAMk6C4rDLTKCguW8fRTSVKgBOKQAiQB4NwBB92Zl6ib50dlF4gGE3MAAHIJ0XHQR0ITWVAF9G5Tv+FraOtxGJ8fWM7IQC6JzC+IANQgQUydQePmm7QQq3IAHo4eQdBoNG4tDsAMrYIhCaggcgAVT0pjcrUyMAgBigRisPyCbgAxiC1JooBBgMdyAByHJciHkW6jFT8zYDIYAQiugsFpFY9jKznc3Ea6GYAWCoQiUS4JhctEFylV5CyOXyOWqtXIOpO+oI1yAA=

I do find it possibly acceptable that you do not wish to support it, but on that note, if you wrote the same code in Typescript, the compiler (and its Intellisense) would correctly inform you that the variable has been narrowed from Guid|null to Guid, and that it is impossible for it to be null inside the second if-statement.

But maybe I'm just ranting... It just bothers me because the result of the is operator should leak out into the surrounding scope. And in the surrounding scope, id is conditionally assigned iif isFoo is true.

CyrusNajmabadi commented 1 year ago

image

the compiler doesn't make assumptions about what happens to message.isFoo (it could change). Or even that there is a relationship between these two values (nothing ensures that relationship that is true at one point continues to hold later on).

CyrusNajmabadi commented 1 year ago

so I stored the values in local variables, but it still gives that unassigned error (on id). It is not correct that it should do so.

The compiler makes no connection between different variables (as per the language specification). If you would like it to do so, that would be a language request for the C# language. You can propose such language rules over at dotnet/csharplang by opening a discussion. Thanks! :)

RikkiGibson commented 1 year ago

Perhaps a better word choice than "correct" would be "expected". The compiler is behaving as designed here.

In other words, there are many many cases where a more sophisticated static analysis, or a human reader, would know the variable is always assigned in the place the compiler is giving an error here, but that's ok. The compiler's analysis isn't intended to avoid erroring on every case where we could possibly determine that things are assigned. The design is meant to be straightforward and to have low computational complexity.

ammfx commented 1 year ago
void F() {
    var t0 = DateTime.Now;
    G(); // CS0170 Use of possibly unassigned field '_dummyPrimitive'
    var t1 = DateTime.Now - t0;
    void G() { var x = t1; } // no error for t1
}

If G() captures many variables, it is hard to find what is wrong when the only error is about '_dummyPrimitive'