Roslyn: Nullable references: Deconstruction of valueTuple in foreach removes nullability of valueTuple items if collection uses System.Linq

[MiYanni]

This took me some time to figure out what was going on. I create a simple class like this:

internal class Alpha
{
    public string Value { get; set; }
    public override string ToString() => Value;
}

Here is the test code:

var items = new (int? Index, Alpha? Alpha)[]
{
    (0, new Alpha { Value = "A" }),
    (1, new Alpha { Value = "B" }),
    (2, new Alpha { Value = "C" })
}.Select(p => p);
foreach (var (index, alpha) in items)
{
    Console.WriteLine(index);
    Console.WriteLine(alpha);
}

var items2 = Enumerable.Empty<(int? Index, Alpha? Alpha)>().Where(p => true);
foreach (var (index, alpha) in items2)
{
    Console.WriteLine(index);
    Console.WriteLine(alpha);
}

Basically, any nullable class references become non-nullable in the foreach. Screenshots:

It does not seem to be specific to a particular Linq extension method. That's why I included two. As soon as you remove the Linq method, it works correctly:

[maryamariyan]

cc: @safern

[Gnbrkm41]

This may not be relevant, per:

This does not appear to be caused by the LINQ method itself; instead, it seems that deconstruction that is causing the problem. If we look at the return type of the Select method, it does return the correct type:

For example, in this piece of code that does not involve any LINQ:

(int? ayy, string? lmao) tuple = (42, "ayy lmao");
var (ayy, lmao) = tuple;

results in the string variable being a non-nullable type, despite the nullability annotation.

I have no idea why this is happening, though. Using explicit types instead of implicit type does the same thing:

(int? ayy, string? lmao) tuple = (42, "ayy lmao");
(int? ayy, string? lmao) = tuple;

https://github.com/dotnet/roslyn/issues/39732#issuecomment-551402664

[CyrusNajmabadi]

@jcouv https://github.com/dotnet/corefx/issues/42463#issue-519579638 does feel like a bug to me.

[jcouv]

Moving issue to roslyn. Something seems wrong indeed. Needs some investigation.

[MiYanni]

@Gnbrkm41 Thank you for narrowing it down. I ran into once the other week in a project I'm working on. And, then again this week in a different part of the project. I time-boxed investigating it, so I didn't get to narrow it down as far as you did.
@jcouv Please transfer the issue appropriately as it causes other issues with tools like Resharper that get quite confused about what to suggest. I had one instance where compilation failed despite VS having no qualms with the code (no red squiggles).

[jcouv]

@333fred I assigned this to you as I will be OOF most of next week. Would you be able to take a look after Ignite? Thanks

[333fred]

Alright, I've dug into this. The problem is coming from the use of a generic method call: even an annotated Identity method would hit the issue. Currently, we end up using the return type of Current from initial binding where we should not. The type from initial binding is oblivious, which then converts to not annotated. I've submitted a PR to fix the single-variable declaration case, but we still need to fix the tuple case. https://github.com/dotnet/roslyn/blob/master/src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs#L6068 does not pass the updated return type of Current through to VisitTupleDeconstructionArguments, and thus the deconstruction infers the wrong thing. We'll need to, for every tuple element, check to see whether the type was inferred, and if so, use the type from the original property. Because @jcouv is updated the handling of tuple symbols in the compiler, this part will wait until he's done.

[333fred]

The first part of this fix has been merged.