Disallow nullable suppression in patterns

[alrz]

Fixes #54071

[jcouv]

LGTM Thanks (iteration 1)

[alrz]

Looks like suppression operator is allowed but only in some cases, presumably only a coincidence because in those other cases parser couldn't handle it, we didn't have an explicit check. How should we approach this?

        _= o is 1!; // ok (error with this change)
        const int i = 1;
        _= o is i!; // error

[jcouv]

Looks like suppression operator is allowed but only in some cases [...]How should we approach this?

Given that a suppression in such place is nonsense, I think we can just disallow and treat the old behavior as a bug. We'd document the (minor) break just in case.

[alrz]

I've seen is null! usages but it's just wrong and misleading. I suppose we need compat council sign off on this.
Where should we document the breaking change, post VS2019? And what version we're targeting for the fix?

[jcouv]

Will start email thread with compat council.
This would be documented in a new file for DotNet 6, similar to this one from DotNet 5: https://github.com/dotnet/roslyn/blob/main/docs/compilers/CSharp/Compiler%20Breaking%20Changes%20-%20DotNet%205.md

[jcouv]

Sorry for the delay. Before I send the email out, let's clarify what is the proposed scope of break. Is it just on number literals and null?

[alrz]

All literal expressions can be suppressed right now, wherever they appear in a constant/relational pattern. Only identifiers gave syntax errors previously because we couldn't parse ! after a type-like.

Note this PR doesn't cover the case where a subexpression is suppressed, we may need to descend into the expression more deeply.

[alrz]

Note this PR doesn't cover the case where a subexpression is suppressed, we may need to descend into the expression more deeply.

@jcouv This would probably have even less impact than the top-level case but I'm not sure if we want it.

I think we could flag the pattern binder and report on any suppression instead of walking down the whole thing again here.

[jcouv]

case where a subexpression is suppressed

@alrz I'm not sure where we stand on subexpressions. Could we start with a couple of examples to clarify the discussion?

[alrz]

Both of these checks are not covered for subexpressions.

public class C {
    bool b;
    public void M() {
        
        if (this is { b: default }) {} // error
        if (this is { b: true! }) {} // proposed to flag as error
        
        if (this is { b: 0 == default }) {} // no error
        if (this is { b: 1 == 1! }) {} // no error

    }
}

[jcouv]

I think it's fine to leave those last two line as "no error". Let's stick with the PR as scoped :-)

[jcouv]

@dotnet/roslyn-compiler for a second review. Thanks

[jcouv]

@dotnet/roslyn-compiler for a second review. Thanks

[RikkiGibson]

I feel like the particular control flow here is a little weird. i.e. this could be return e and the continues could be breaks. However this seems subjective enough that I won't block the PR about it.

[jcouv]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[jcouv]

CI integration legs won't pass until #55681 is merged.

[jcouv]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).