walkercoderanger / exhaustivematching Goto Github PK

The error message should change if the type is not a subtype at all to indicate that rather than that it isn't a direct subtype.

Hi,
I am missing maybe the most obvious part in the readme -- how do I actually run this analyzer? I added it to the project -- is it supposed to automatically run in my IDE now (I use Jetbrains Rider)?

Is there a way to automatically run it in the continuous integration workflows? Just running MSBuild does not produce any errors.

Thanks for your help! (Once I know how to run it, I'll make a PR to document that in the readme since other users might also be lost.)

It would be helpful to detect the closed hierarchy of a class with private constructor and nested subclasses having either also a private constructor or a sealed modifier. This is a way to simulate the desparately missing discriminated unions safely in C#.

public abstract class Result<TSuccess, TError> {
    private Result() { }

    public sealed class Success : Result<TSuccess, TError> {
        public TSuccess Value { get; }
        public Success(TSuccess value) { Value = value; }
    }

    public sealed class Error : Result<TSuccess, TError> {
        public TError Value { get; }
        public Error(TError value) { Value = value; }
    }
}

A switch expression like this should provide the error:

var x = result switch
{
    Result<string, string>.Error error => ""Error: "" + error,
    _ => throw ExhaustiveMatch.Failed(result),
};

This would be o.k.:

var x = result ◊1⟦switch⟧
{
    Result<string, string>.Error error => ""Error: "" + error,
    Result<string, string>.Success success => ""Success: "" + success,
    _ => throw ExhaustiveMatch.Failed(result),
}

Publish on NuGet.org, but unlist old versions

The not a direct subtype error "EM012" should highlight the actual argument to the ClosedAttribute that is the problem.

With v0.3.0 a bug was introduced that incorrectly reports EM0014 for multi-level type hierarchies.

Currently, the subtypes of a closed type are called member types or union of types. There should be a better name for them. Perhaps case types? Change to use the new name consistently.

If you create a hierarchy of interface case types three deep, you can't use the middle one as a switch case. That is given IRoot, ICase, and IValue each of which implements the previous with IRoot and ICase being closed types, you can't switch on ICase. An error is reported that it "is not a case type inheriting from type being matched".

It appears the issue is that it isn't a concrete type or a leaf type, so it doesn't get put into the list of concrete case types, but it should still be a valid case type. These concepts need to be split.

GetTypeByMetadataName returns null, and causes null reference exception.
Please ban Microsoft.CodeAnalysis.Compilation.GetTypeByMetadataName for the entire repository.
See this: Compilation.GetTypeByMetadataName returns null even though the other ObsoleteAttribute is internal

Create a quick fix to add a case clause for a missing case.

Hello

In my use case, some of the classes cannot be abstract. They are basic models used for deserialization

example

public enum Kingdom
{
    Animalia,
    Plantae,
}

public enum DogBreed 
{
    Pug,
    Maltese,
}

public enum CatBreed 
{
    Pug,
    Maltese,
}

public class Organism
{
     public Kingdom Kingdom {get; set;}
}

public class Dog : Organism
{
    public DogBreed Breed {get; set; }
}


public class Cat: Organism
{
       public CatBreed Breed {get; set; }
}

public class OrganismHandler
{
    public void Handle(Organism organism)
    {
        switch(organism)
        {
            case Dog dog:
                Handle(dog);
                break;

            case Cat cat:
                Handle(cat)
                break;
            
           // if I do not add a case for class Organism the analyzer complains
           // if I do, it does not complain if I do not add Cat or Dog
           // what I want is to not need to handle organism, would even be ok to get analysis erros if I did
            default: 
                throw ExhaustiveMatch.Failed(regionSpecificForm);
        }

        private void Handle(Dog dog)
        {
                 ....
        }
        
        private void Handle(Cat cat)
        {
                 ....
        }
    }
}

I don't entirely remember what this was supposed to be about. However, it is referencing the unit tests MirrorHierarchy and MirrorHierarchyMustBeCovered, and the example MirrorExample in MirrorHierarchy.cs. I think the point is that using such mirror hierarchies can be confusing and the rules need to be documented. However, at the moment I don't remember the rules I had in mind.

See issue number #26

Whenever I install the ExhaustiveMatching NuGet package to a .NET Framework 4.7.2 project I get compiler/MSBuild warnings that Microsoft.CodeAnalysis version 3.3.0 was not found - unfortunately that version doesn't exist on NuGet, it jumps from 3.3.0-beta3-final to 3.3.1 ( https://www.nuget.org/packages/microsoft.codeanalysis )

I can reproduce this consistently at my end.

Having the same type repeated in the case type list of the closed attribute should be an error.

Report an error for all case clauses trying to match a type that is not a subtype. (Or is this enforced by the compiler?

I had a nasty runtime error just now in a strong-named project targeting .NET Framework 4.8 that uses this NuGet package, as strong-named assemblies transitively require all referenced assemblies to also have strong-names.

I can work-around it with the StrongNamer hack package, but I'd prefer it if this assembly were strong-named properly in the first place.

Microsoft explicitly recommend that libraries distributed as NuGet packages should be strong-named:

You should strong name your open-source .NET libraries. Strong naming an assembly ensures the most people can use it, and strict assembly loading only affects .NET Framework.

If a closed type declares itself to be one of its cases, this causes an infinite loop in the analyzer. The analyzer will correctly find an error that the listed type isn't a direct subtype, but other code can't handle this error case.

Partial types mean that a type may be closed but some cases are in one file and other cases are in another file. To support this, the Closed attribute and the code should support multiple closed attributes per type.

The following sitautions are valid switches on a shape.

The type names should be handled correctly by the exhaustive checker.

switch (shape)
{
    case Square square:
        Console.WriteLine(""Square: "" + square);
        break;
    case Circle circle:
        Console.WriteLine(""Circle: "" + circle);
        break;
    case Triangle: // label syntax
        Console.WriteLine(""Triangle!"");
        break;
    default:
        throw ExhaustiveMatch.Failed(shape);
}";

var result = shape switch
{
    Square square => ""Square: "" + square,
    Circle circle => ""Circle: "" + circle,
    Triangle => ""Triangle!"", // type name
    _ => throw ExhaustiveMatch.Failed(shape),
};";

Currently, this error highlights the whole case clause, it should highlight only the when clause.

It is possible to get exceptions from the analyzer when analyzer invalid code. In particular, I believe that an unrecognized ClosedAttribute can cause it to assume the constructor parameters are not params but individual types.

The ExhaustiveMatching.Analyzer.Enums project doesn't seem to be very useful by itself, and the solution and repo could be simplifeid by rolling it into the ExhaustiveMatching.Analyzer project.

Thoughts?

Create a readme that leads with an example. Then goes on to how to install from nuget?

• Coin flip is a good example of a standard enum
• IPv6 and IPv4 could be a good object example
• Need good hierarchy example

If you create an inheritance hierarchy of interfaces that are closed and a matching hierarchy of classes that implement those interfaces, you'll get errors that the classes aren't case-types of the closed interfaces. However, sometimes this should be allowed. For example:

[Closed(typeof(ISquare), typeof(ICircle))]
interface IShape { }
interface ISquare : IShape { }
interface ICircle : IShape { }

// Gives an error, but is fine as long as all subclasses implement one of the interfaces covered by the case types
abstract class Shape : IShape { }
class Square : Shape, ISquare { }
class Circle : Shape, ICircle { }

An empty typeof() as an argument to ClosedAttribute causes EM002 "Subtype not handled by switch:" with no type listed.

Currently, a number of diagnostic errors highlight the entire switch statement, including the body. They should only highlight the switch head. Meaning they should highlight just the switch keyword. The switch expression is excluded because it could be long and may have other errors.

The analyzer still throws exceptions sometimes which show up as warnings in VS. I think it is because of invalid code, but I'm not sure. Things to try:

• Unknown Type in Case Clause
• Unknown Type in Case Class List

Switch expressions are not being checked. That is a real issue when a suggested refactoring is changing to switch expressions.

Add a quick fix for error EM011 that a type is not in its parent's list of cases, that adds it.

Hello!

First of all, I absolutely love this library. I really appreciate your work on it :)

I ran into a case today where I had refactored some code that previously had not allowed null to be a valid value, but now I have switch cases throwing ExhaustiveMatch.Failed. I realized it was because I had not been handling the null case in the switch.

In the documentation, I see you have the following:

Since C# reference types are always nullable, but may be intended to never be null, exhaustiveness checking does not require a case for null. If a null value is expected it can be handled by a case null:. The analyzer will ignore this case for its analysis.

For nullable enum types, the analyzer requires that there be a case null: to handle the null value.

I understand that this might not be desired as a default behavior, but I would really personally like to be forced to handle null on reference types.

Is there any way that we can see this as an opt-in thing in the future? Perhap something we can decorate our assembly with as an attribute, or similar.

Thank you!
Anthony

If you create an exhaustive matching switch statement on a flags enum an error should be reported because that doesn't make sense.

Currently, these report a list of cases not covered, they should report a single one and the error should be repeated as many times as there are missing cases.

Currently, every subtype of a closed type must be closed, sealed, or struct. Open type hierarchies should be supported, meaning that subtypes don't need to be any of those things. Instead, when matching against the closed type, only its immediate subtype and subtypes of a chain of closed types can be used to match against. That is because exhaustiveness can only be checked for closed types, however it is fine to use an open type to catch all possible subtypes of it.

Note: this adds new error(s) for switch cases trying to match subtypes whose immediate parent aren't closed.

This is an awesome package and I really appreciate your effort.

The readme was planned to have a "Why?" section but it was dropped. Write one. Discussing the expression problem would be good.

I'd love to use this ExhaustiveMatching analyzer with a number of NuGet projects that I work on but I understand that even though this is a "private" dependency, it still causes my project to have a reference to the ExhaustiveMatching.dll assembly which then must be referenced by my output NuGet package - this isn't ideal.

It should be possible for the ExhaustiveMatching code-analysis to work by detecting the use of built-in exception types (e.g. ArgumentOutOfRangeException) in a switch block provided they're annotated with a known magic const string for the message: ctor parameter - or allow use to redefine the ExhaustiveMatching exceptions in our own projects - and the analyzer could then look for that without us needing to reference the ExhaustiveMatching.dll assembly.

On a related note, I'd like to have two separate exceptions to : one derived from ArgumentOutOfRangeException for when switching over an enum parameter argument, and another derived from InvalidOperationException when swiching over an enum that isn't a parameter argument.

The analyzer does not handle record classes.
for example :

    [Closed(typeof(UserNotFound))]
    public abstract record Response
    {
        public record UserNotFound : Response;

        public record Success : Response;
    }

    class Controller
    {
        public int Get(Response response)
        {
            return response switch
            {
                Response.UserNotFound _ => -1,
                _ => throw ExhaustiveMatch.Failed(response)
            };
        }
    }

The analyzer doesn't detect that the "Success" class is needed to be added to the Closed attribute.
The analyzer doesn't detect that the "Success" clause needs to be handled in the Get() method

Thanks for this library, really appreciate the effort !

The ClosedAttribute is set AllowMultiple=true because we want to allow partial classes to have a closed attribute on each declaration of the class. However, we don't really want to allow multiple closed attributes on a single class declaration. That is on one of the partial class declarations. Issue an error from the analyzer for multiple closed attributes on a declaration.

Expand error numbers to 4 digits to match compiler errors

If nullable reference types are enabled, we should enforce requirements for a case null: if that isn't already enforced by the compiler.

Based on the work for #42 it would be nice to have the same feature also für discriminated unions implemented the C# record syntax.

public abstract record Result<TSuccess, TError> {
    private Result() { }

    public sealed record Success(TSuccess Value) : Result<TSuccess, TError>;

    public sealed record Error(TError value) : Result<TSuccess, TError>;
}

A switch with missing arms could also giving the analyzer error however there's a slight source of errors:

It is technically possible to subclass from the Result record outside of the scope because the C# record syntax for non-sealed records always creates a protected copy constructor. Anyway I would argue that the developer intention is clear as there are some intrinsics about what the copy constructor does and where it is used.

This line of code compiles even outside of the Result record but providing a concrete instance (Error in this case) does not make real sense as only the properties of the base record would be considered in the copy constructor:

public record OtherResult(string Value) : Result<string, string>(new Error(Value))

See also this StackOverflow question to see there are also other developers interested in such a feature.

As I understand it however, to really detect that situation, we must switch to Roslyn version 3.9.0 as the IsRecord property was added to the type symbol interface.