Applying S3267 fix introduces CS8604 possible null reference

[paulhickman-a365]

Description

Applying the code fix suggested for S3267 can introduce C# warning CS8604 "Possible null reference argument for parameter..." if the transformation moves null checks into a LINQ expression and changes the nullability of expressions in the loop. In this situation, S3267 should not be raised.

Repro steps

Consider the method Foo below:

using System.Collections.Generic;

namespace S3267Bug
{
    public class Item
    {
        public string? Prop { get; set; }
    }

    public class Class1
    {
        public void Bar(string s)
        {
            throw new System.NotImplementedException();
        }

        public void Foo(IEnumerable<Item> items)
        {
            foreach (var item in items)
            {
                if (item.Prop != null)
                {
                    Bar(item.Prop);
                }
            }
        }
    }
}

S3267 is raised on the expression items in the foreach loop, and it offers 2 code fixes . If you apply the "Convert to LINQ (call form)" code fix, it changes the code to:

        public void Foo(IEnumerable<Item> items)
        {
            foreach (var item in items.Where(item => item.Prop != null))
            {
                Bar(item.Prop);
            }
        }

However, because the compiler doesn't know that the Where statement will remove all items where item.Prop is null, it now raises compiler warning CS8604 on the call to Bar

Note: It doesn't matter which of the two forms of code fix you choose - the same issue occurs.

Expected behavior

S3267 should not be raised if the transformation to LINQ would affect the nullability of any expressions within the loop.

Note: the bug is in the analyzer, not the code fix - in the general case for the possible loop bodies after the fix, it is not possible to write a LINQ version of this code that wouldn't cause the issue.

Actual behavior

S3267 is raised.

Known workarounds

Suppress S3267

Related information

• Target netstandard2.1
• C# Language version 8
• C#/VB.NET Plugins version 8.43.0.51
• Visual Studio version 17.2.6

[mary-georgiou-sonarsource]

Hello @paulhickman-a365, and thank you for reporting this!

I would not consider this an FP from our side - IMO, the FP is that there is a null reference warning for item.Prop when a null check for it already exists after the code fix is applied.
I would suggest raising an issue for this on the Roslyn side.

Cheers
Mary

[martin-strecker-sonarsource]

Related discussion in the Roslyn repo dotnet/csharplang#8383

[martin-strecker-sonarsource]

@paulhickman-a365
As a workaround, you can use OfType<string>() to suppress the warning:

foreach (var prop in items.Select(item => item.Prop).OfType<string>())
{
    Bar(prop);
}

Or use the null forgiving operator

foreach (var item in items.Where(item => item.Prop != null))
{
    Bar(item.Prop!);
}

These solutions are not perfect, but they are the best you can do now. There were some activities to address this problem, but all were rejected, and the only remaining initiative I'm aware of is dotnet/csharplang#3951. It is championed and was in the candidates set for C#10 but hasn't seen much traction recently. I don't expect any solution for this anytime soon.