Investigate why S2259 does not always trigger on C# 8 code

[andrei-epure-sonarsource]

Description

C#8 introduces the concept of non-nullable reference types. By default, the reference types are considered non-nullable. See details in Nullable reference types.

I analyzed andrei-epure-sonarsource/CSharp8Demo on SonarCloud. Note that the csproj has the NullableContextOptions enabled.

Rule S2259 doesn't behave normally with easy-to-find cases.

Repro steps

Look at this line on sonarcloud. It is clearly a NRE (and it does thrown at runtime).

Expected behavior

The analyzer should raise S2259 at line 17, 20, 25 and 42.

Actual behavior

Only line 42 has a raised issue.

Known workarounds

None.

Related information

• SonarC# Version 7.16

[christophe-zurn-sonarsource]

While digging into the reasons the issues are not raised for C#8, I found 2 causes:

1. CFG generation fails when it encounters a null-forgiving operator (ex: str!.Length), as code will go inside CSharpControlFlowGraphBuilder#BuildExpression(ExpressionSyntax expression, Block currentBlock), and the expression has type SuppressNullableWarningExpression, which is not known => method will throw a NotSupportedException. To fix this, we should use an updated Shim layer that defines correctly the SuppressNullableWarningExpression kind inside theSyntaxKindEx class, as well as update the logic inside BuildExpression to handle this type of syntax node correctly.

2. Unrelated to C#8, but SE engine exploration stops on the first null dereference found (due to this line here). So for the analyzer to raise an issue on each of the dereference found in the example code referenced above, we should either put them in separate methods, or continue the SE after the first issue found.

[pavel-mikula-sonarsource]

We need to add test cases and then we'll see what the behavior of the new engine is

[ckasabula]

Our team is seeing this issue. I am unable to view the sources referenced in this thread.

This hides many potential issues in our codebase.

Any update on this?

See the following example.

public static class Example
{
    public class Test
    {
        public string Uri { get; set; }
    }

    public static void Method()
    {
        // change just one of these member accesses on the lines commented w/ S2259
        // to use null conditional operator and it suppresses all other occurrences of S2259
        // warning in the method 
        int[] ia = null;
        Console.WriteLine(ia.Length); // S2259
        Test[] ta = Array.Empty<Test>();
        var test1 = ta.FirstOrDefault();
        Console.WriteLine(test1.Uri); // S2259
        Test test2 = null;
        Console.WriteLine(test2.Uri); // S2259

        // A single null coalescing will suppress all S2259 errors in the method
        // Uncomment the following
        // var test3 = test2 ?? new ();
        // Console.WriteLine(test3.Uri);
    }
}

[ckasabula]

A single null coalescing will suppress all S2259 warnings in a method too.

[ckasabula]

We are dotnet 6.0 / C# 10.

[pavel-mikula-sonarsource]

We're in process of rewriting our Symbolic Execution engine. We'll revisit this once the effort is done. That should happen later this year.

[martin-strecker-sonarsource]

Related issue #4250 for the new engine.

[sebastien-marichal]

@andrei-epure-sonarsource Do you think this issue can be closed?