No way to suppress CS8655 (switch is not exhaustive on null)

[TessenR]

Version Used:

Branch main (4 Sep 2021)
Latest commit 9525fa5 by Tomáš Matoušek:
Options refactoring (#55686)

Steps to Reproduce:

Compile the following code:

#nullable enable
class C
{
  void Test(string? x)
  {
    x ??= "";
    _ = (x! switch
    {
      { Length: > 10 } => 1,
      {  } => 2,
    })!;
  }
}

Expected Behavior:
No warnings. The input is not nullable.
Even with nullable input the switch input has a nullable warning suppression expression.
The switch expression itself has a nullable warning suppression.

Actual Behavior:
warning CS8655: The switch expression does not handle some null inputs (it is not exhaustive). For example, the pattern 'null' is not covered. is reported with no way to suppress it.
I believe Roslyn should treat suppressed governing expressions as non-nullable inputs. Alternatively a suppression on the switch itself should suppress warnings about exhaustiveness on null values.
I believe both options would be a great addition to each other since CS8655/CS8847 can be reported for properties' checks as well so the second option would be the only way to suppress this warning. However the first one would be a more natural approach for cases where the compiler doesn't pick up that the input value itself is guranteed to be non-nullable.

[Youssef1313]

A similar issue: #40759

[jcouv]

Feels like there are two problems:

1. x should be considered not-null after x ??= "", so no suppression should be necessary in the first place The switch doesn't properly account for the null-state of x
2. if x were maybe-null when reaching the switch then x! should be considered not-null for purpose of the switch

[Youssef1313]

@jcouv The second problem looks by design to me.

The pattern { } is an explicit null check, which gets the null state back to nullable.

[jcouv]

Ah! Good point. There's a pure null test in there. Indeed, that behavior is by-design.

[Note: when fixing the issue, we should test the same scenario with a tuple as input (1, x!)]

[TessenR]

@jcouv Could you please explain what exactly is Roslyn considering nullable after the pure null check in the switch expression? Because it can't be 'x' since Roslyn doesn't learn from pure null checks when a governing expression is suppressed. E.g.:

#nullable enable
class C
{
  void Test(string? x)
  {
    x ??= "";
    _ = x! switch
    {
      { Length: > 10 } => 1,
      {  } => 2,
      null => 3
    };
    x.ToString(); // no warning; Roslyn doesn't learn anything about 'x' because it's suppressed
                  // there will be a warning if you remove the suppression
  }
}

2. Shouldn't there be a way to suppress this diagnostic except rewriting the patterns in a switch expression? E.g. with a suppression on the switch expression itself. If nullable warnings are lifted to errors it might be hard to figure out why Roslyn complains here and how to suppress it.

3. Together these two points make it look very confusing. I tried to minify the example in the original issue but please check the following example. Roslyn knows that both c and c.Prop are not nullable before the switch. Roslyn knows that they are not nullable after the switch. The governing expression is suppressed. The switch expression is suppressed. Yet the warning is still there. How is a developer supposed to deal with it?

#nullable enable
class C
{
  public string Prop { get; set; } = "";
  void Test(C c)
  {
    c.Prop.ToString(); // no warnings
    _ = (c! switch // CS8655 the pattern 'null' is not covered.
    {
      { Prop: { } } => 0,
    })!;
    c.Prop.ToString(); // no warnings
  }
}

Actually the last example makes me think that there's a bug in learning from pure null check patterns. Why the warning tells me that null is not covered in the switch? There's no pure null check pattern for c now. Is this by design as well? Should I open a new issue for this one?

[RikkiGibson]

x is { } is considered a pure null test. There's basically a null test in the bound nodes for the DAG that represents this. After evaluating this test we end up with a branch where x was null but no explicit switch arm handling it.

I think if a correspondence akin to the following is present, the behavior is likely expected: SharpLab

#nullable enable

public class C {
    public void M1(string x) {
        _ = x switch { { } => 0 }; // warning
    }
    public void M2(string? x) {
        _ = x! switch { { } => 0 }; // warning
    }
}

The behavior is perhaps inconvenient but does fall out from all our existing rules. It does seem like we should be able to suppress this warning with ! somewhere, though.

[Youssef1313]

For some reason, assigning to a temp variable suppresses the warning:

https://sharplab.io/#v2:EYLgtghglgdgNAExAagD4GIYFcA2OLA4CmABETAcQLABQtAAgMwn0BMJAwiQN60n8tm9ACwkAsgEYAFPQkAGEgA8AlDz4CNAfRIBeJSQDOAdygAXAMYALHjYC+JU0TAAHXQD4SC2wG4SAej8SADkAeRIjCAAnGFgAcwB+RPV+W2TBFlExVhl5eKVVXhoNLV0lAEJDEwtrbjt3TxIff0CI6Li01JpbIA=

I'm not sure if that's expected or not.

[RikkiGibson]

It is expected. The guideline is that once a pattern is doing more than just null testing an input, it is no longer a pure null test.

[TessenR]

x is { } is considered a pure null test. There's basically a null test in the bound nodes for the DAG that represents this. After evaluating this test we end up with a branch where x was null but no explicit switch arm handling it.

@RikkiGibson I understand what happens there from the compiler's perspective, but it looks like separating dag temp variables from their original variables is an implementation detail that causes the confusion here.

E.g. in the following example according to this rule there are pure null tests for dag temp variables associated with c and c.Prop. Therefore branches where both these variables are null are reachable and fall into the discard pattern branch.

However, neither in this branch nor after the switch expression these variables are considered nullable.

#nullable enable
class C
{
  public string Prop { get; set; } = "";
  void Test(C c)
  {
    c.Prop.ToString(); // no warnings
    _ = (c! switch
    {
      { Prop: { } } => 0,
      { } => 1,
      _ => c.Prop.Length // no warnings
    })!;
    c.Prop.ToString(); // no warnings
  }
}

This behavior is here because there's an explicit check that prevents the compiler from learning from pure null checks.
Roslyn only learns from pure null checks if the governing expression is not using nullable warnings suppression:

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

Line 195 in 315c2e1

int slot = node.Expression.IsSuppressed ? GetOrCreatePlaceholderSlot(node.Expression) : MakeSlot(node.Expression);

Essentially the compiler says here:

• there are no pure null checks for c in the switch expression because the governing expression is suppressed
• there's a pure null check for a dag temp variable initialized with the governing expression i.e. c which is considered reachable with 'null' value because that's the semantics of pure null checks
• learning from the pure null check on the dag temp variable cannot be suppressed because it doesn't exist in the source code
• hence you cannot remove the discard pattern branch as otherwise the null dag temp variable value will not be covered by the switch
• but the original variable c is not nullable because the pure null check is only done on the dag temp variable within the switch, but not for the variable c itself

I'd argue that from a developer's perspective c is the variable which is being tested in the { } => 1 pattern, therefore suppression on the governing expression should also suppress all the null checks on dag temp variables originating from it. This is why I expected the suppression on the governing expression to work.

Either way, the main point of the issue is the inability to suppress the warning anywhere, in the code. Is it really by design as the label says?

[RikkiGibson]

I think that the inability to suppress the warning is perhaps an unintended consequence. I do believe this issue should remain open to track resolution of that. I haven't been been able to carve out time to dig into the concerns in more detail to decide if anything about the flow analysis of the DAG itself should change, but I am intending to, and appreciate your in depth writeup on the issue.