Roslyn: IDE0019 suggests Negative Pattern Matching; i.e more complex code

[brinko99]

VS2017 15.7.3:

The below triggers IDE0019 which when followed produces negative pattern matching which IMO is poor code. I suggest that pattern matching not be suggested in these circumstances.

It is a common policy to avoid logical negation (!) as it's easy to overlook. In fact, the MS Pattern Matching documentation explicitly suggests not using this pattern:

You could reverse the logic by saying if (!(shape is Square s)) and the variable s would be definitely assigned only in the false branch. While this is valid C#, it is not recommended because it is more confusing to follow the logic.

Before:

var result = value as string;
if (result == null)
{
    throw new InvalidOperationException();
}

After:

if (!(value is string result))
{
    throw new InvalidOperationException();
}

[CyrusNajmabadi]

which IMO is poor code. I suggest that pattern matching not be suggested in these circumstances.

I don't see why this is considered poor code :) it was actually called out as the ''guard pattern' when we designed patterns in teh first place :)

It is a common policy to avoid logical negation (!) as it's easy to overlook.

I'm really not certain where such a policy has ever been described or enforced. :)

[CyrusNajmabadi]

@brinko99 You should be able to disable this fixer by adding hte following to your .editorconfig file:

csharp_style_pattern_matching_over_as_with_null_check=false:none

This will shut off that diagnostic for you (at least in very current builds of Roslyn).

[CyrusNajmabadi]

Note: there is also a language proposal to make the parenthese optional here. So that you can soon write the guard pattern as:

c# if !(value is string result) { throw new InvalidOperationException(); }

This will be the expected idiomatic form for guard patterns here. When that comes, this fixer will also be updated to emit that new form. The championed language proposal is here: https://github.com/dotnet/csharplang/issues/882

[brinko99]

Thanks @CyrusNajmabadi, this is helpful. The language proposal does improve readability IMO; indeed, it seems that there are many who are not crazy about the current construct (e.g. the above linked documentation note).

The .editorconfig option you mention applies to both the == null and != null checks. I much prefer pattern matching for the clean != null check but not the != null check. Might Roslyn support distinguishing between these two and allow disabling pattern matching checks that result in the (!(...)) form?

[Neme12]

There is a separate proposal for not patterns https://github.com/dotnet/csharplang/issues/1350, which would allow you to write:
c# if (value is not string result) { throw new InvalidOperationException(); }
This would also make the condition more readable, although a little verbose.

[xtqqczze]

In C# 9.0, IDE0019 actually suggests the not pattern:

if (value is not string result)
{
    throw new InvalidOperationException();
}

[CyrusNajmabadi]

Yup. We emit the idiomatic versions we think are appropriate for your language version. Closing out as by design.