dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.97k stars 4.03k forks source link

Possibility to conditionally deactivate CS8509 (not exhaustive switch) #56936

Closed hugener closed 3 years ago

hugener commented 3 years ago

Brief description:

I'm playing with discriminated unions in C#. My goal is to have something that will fill-in until DUs are natively supported. So far I have a package analyzing switch statements and expressions. (Code and NuGet package)

The analyzer checks whether switches happens on a class/records marked with an DiscriminatedUnion attribute and does its checks to see if all cases are handled. Additionally, it's analyzing the cases to ensure that they are a "closed" inheritance hierarchy. In switch expressions, I get a CS8509 warning stating that the switch is not exhaustive, although it should be safe to ignore the warning in this case. Is there a way to indicate to the warning that the switch is handled by another analyzer, or could something like this be added?

Similarly, switch statement resembling switch expressions (with return) receive an CS0161 not all code path return a value.

For now I added another diagnostic that the default case should throw an UnreachableCaseException, but I would really like to get rid of that.

Languages applicable:

C# for now, should be easy to support VB.

Code example that the analyzer should report:

CS8509 is reported and SDU9999 ('Result?' is a discriminated union and should throw UnreachableCaseException in default case)

Result? result = Compute("Success");
var message = result switch
{
    Result.Success => "Great",
    Result.Warning { Message: "Tough warning" } => "Not good",
    Result.Warning warning => warning.Message,
    Result.Error { Code: > 70 } error => $"High Error code: {error.Code}",
    Result.Error error => $"Error code: {error.Code}",
    null => "Nothing",
};

SDU9999 is only there to force adding the default case like so:

_ => throw new UnreachableCaseException(typeof(Result)),
RikkiGibson commented 3 years ago

Are you looking for a diagnostic suppressor?

hugener commented 3 years ago

@RikkiGibson thanks, that's solves it for CS8509.

RikkiGibson commented 3 years ago

Great to hear! Please feel free to follow up in this thread if you have any further questions/concerns.

hugener commented 3 years ago

Implemented the suppressor and there is no error in the Error List reported and it compiles/runs, but I get a red squiggly line (TreatWarningsAsErrors) under the switch keyword. It seems the suppressor did it's job. Although the message is still there the CS8509 is gone. I wonder if that is the expected behavior? I guess that might be a VS issue. Where can I post that? image

RikkiGibson commented 3 years ago

It's possible that some bug or configuration issue prevents the suppressor from functioning in the IDE. If you have a minimal reproducer it would be great if you could file a new issue with it.

hugener commented 3 years ago

Hi @RikkiGibson

I reproduced two issues here:

  1. The red squiggly line (VS problem).
  2. Codeanalysis appears to report false FlowState, when a switch handles null (CodeAnalysis problem).

DUTester.zip

var option = Compute(args.FirstOrDefault() ?? "test");
var message = option switch // Red squiggly lines (The 'switch' expression does not handle all...), no compiler error, CS8509 suppressed by analyzer
{
    Option<int>.Some some => $"The value is {some.Value}",
    Option<int>.None => "There is no value",
    null => "Null", // option appears to be null annotated because of this null case.
                    // Does that make sense?
                    // Compute returns a non-nullable Option<int>, so the analyzer should have reported the null case as unreachable.
                    // It does not because CodeAnalysis Nullability on the TypeInfo reports its FlowState as "MaybeNull". Removing the null case changes the FlowState to NotNull.
                    // Reproduced here: https://github.com/sundews/Sundew.DiscriminatedUnions/blob/main/Source/Sundew.DiscriminatedUnions.Test/GenericSwitchExpressionAnalyzerTests.cs
                    // Test: Given_SwitchExpressionInEnabledNullableContext_When_ValueIsNotNullAndAllCasesAndNullCaseAreHandled_Then_HasUnreachableNullCaseIsReported2
};

Should I create separate issues for them both in this repository?