Proposal: Track subtype exhaustiveness for classes with only private constructors

[agocke]

Private class constructor exhaustiveness

• Proposed
• Prototype: Not Started
• Implementation: Not Started
• Specification: Not Started

Summary

The switch expression already has a notion of exhaustiveness, and there are a variety of cases where switch arms can be confirmed to be exhaustiveness, as defined by reachability of the decision tree.

Right now the only state for reference types tracked for reachability is nullability, e.g. the following switch will not provide a warning about non-exhaustion:

int M(string? s) => s switch 
{
    string s2 => 0,
    null => 1
};

This proposal is to extend tracking to subtypes of classes with only private constructors. For example,

abstract class C
{
    private C() { }
    public class A : C { }
    public class B : C { }
}
int M(C c) => s switch
{
    A a => 0,
    B b => 1
};

The above should also produce no warnings, as the type checks in the switch expression exhaust all possible values of the input.

Unfortunately, the improvement as specified does have one backwards compatibility problem: it is an error if a switch arm is subsumed by a previous case. This would mean code which currently has a "catch all" pattern to avoid a warning in the above code would now be an error if the change were applied naively. The proposed fix for this is to not provide any warning or error if a case is unreachable only when arbitrary subclasses are not considered reachable.

This proposal should be an unalloyed benefit to programmers -- the compiler is simply smart enough to not provide incorrect warnings.

Design Meetings

• https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-09-21.md#track-subtype-exhaustiveness-for-classes-with-only-private-constructors
• https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-09-28.md#discriminated-unions

[333fred]

Seems like this would roll in with the general sealed type hierarchy proposals.

[agocke]

Same idea, but also recognizing an existing pattern

[CyrusNajmabadi]

int M(C c) => s switch
{
    A a => 0,
    B b => 1
};

Does this still need a null check?

--

Strongly in favor of this general area. Though on the fence if i think this special case is worth having support for, or if we should have the more general way to mark any hierarchy such that we can enumerate all the cases for it to do exhaustiveness checking

[agocke]

Does this still need a null check?

Imagine it's prefixed with #nullable enable and no, that doesn't, because c is not nullable.

[agocke]

Though on the fence if i think this special case is worth having support for

A different way of looking about it: this is a bug fix. It is simply true that any warning provided in the above switch expression is incorrect. The warning is that the switch expression is not exhaustive, but it is, in fact, exhaustive.

[YairHalberstadt]

A different way of looking about it: this is a bug fix.

Not quite - this makes the number of subtypes part of the public contract, and adding new subtypes is now a breaking change where it wasn't before. I still think it's worth it, but it does have costs.

[agocke]

this makes the number of subtypes part of the public contract

I don't think this is really true, because it's a broader language design opinion at the level of the system, as opposed to a pure consideration of the warning itself.

The warning provided in this case is simply incorrect. The warning says that the switch is incomplete, but it isn't. The fact that a user changing their code can produce different behavior on the consumers part is true, but in the language we've never considered that as a "breaking change."

[YairHalberstadt]

I'm not saying that this is a breaking change. What I'm saying is that till now, a library author can freely add subtypes to a class without introducing new warnings to consuming code. Now he can't.

In general if a library author cannot change something without introducing new warnings, that means this is a public contract, and in general it's best if public contracts are explicit, so authors don't accidentally tie themselves down. In this case I think authors probably would want to introduce new warnings in such a case, so it's worth it.

[agocke]

@YairHalberstadt I see, so you're talking about the broader, design-level question. As to the question of whether this would count as an "explicit" opt-in to the behavior, I would guess so.

But I don't think this negates from my fundamental point that 1) the current warning is incorrect and 2) it's clearly detectable by the compiler.

[Richiban]

Doesn't the same exhaustiveness check apply to internal classes too (or to public types with internal constructors)? All their inheritors will be known at compile time, unless I'm missing something.

[FaustVX]

@Richiban I don't think it's possible because of InternalsVisibleToAttribute

[Richiban]

Ahh, good point. It might be possible to detect that and treat any internal type defined in such an assembly as 'open' but it complicates the feature a fair bit. Sticking to private classes might be a good idea to start with.

[alrz]

This proposal is to extend tracking to subtypes of classes with only private constructors

The compiler already do something like this when it's immediately obvious:

interface I {
  sealed class A {
    void M(A a) {
      if (a is I) {} // won't warn if A is not sealed, but that would be still correct wrt effective accessibility
    } 
  }
}

Without any kind of modifier at the declaration-site to "verify" a closed hierarchy, I think it could get tricky to "infer" such relationship without a few false negatives, unless this is only about recognizing a specific pattern (like private constructors).

[alrz]

abstract class C
{
    private C() { }
    public class A : C { }
    public class B : C { }
}
int M(C c) => s switch
{
    A a => 0,
    B b => 1
};

Actually that should still warn unless A and B both are either sealed or define a private ctor. Question is how far the compiler should go to determine all possible subtypes.

[CyrusNajmabadi]

Wouldn't that still be exhaustive @alrz ? Any further subclasses would have do drive from A or B, and both of those are already checked in the switch...

[HaloFour]

@CyrusNajmabadi

Agreed, any subtype of A or B would be handled by that switch. This is how sealed hierarchies work in Java.

[alrz]

You're right, was thinking whether or not those could be inherited at all. That doesn't affect this particular switch, but that is also not exactly a closed hierarchy?

[alrz]

Guess as long as base types are constant it's already close- Is there any other language that allows non-flat DUs? None comes to mind.

[CyrusNajmabadi]

It's closed on the sense that we can tell if they switch is exhaustive without needing a catch all clause :-)

[HaloFour]

@alrz

Is there any other language that allows non-flat DUs? None comes to mind.

Java :)

[alrz]

@alrz

Is there any other language that allows non-flat DUs? None comes to mind.

Java :)

I mean, does it support exhaustiveness like what is described here?

[HaloFour]

@alrz

I mean, does it support exhaustiveness like what is described here?

Yes, it was an explicit goal of sealed classes:

Support future directions in pattern matching by providing a foundation for the exhaustive analysis of patterns.

And the pattern matching in switch expressions is in its third preview with Java 19:

public static String shape(Shape shape) {
    // this would be a compiler error if considered non-exhaustive.
    return switch(shape) {
        case Circle x -> "Circle";
        case Triangle x -> "Triangle";
        case Quadrilateral x -> "Quadrilateral";
    };
}

private sealed interface Shape permits Circle, Triangle, Quadrilateral { }
private static final class Circle implements Shape { }
private static final class Triangle implements Shape { }
private static sealed abstract class Quadrilateral implements Shape permits Square, Rectangle { }
private static final class Square extends Quadrilateral { }
private static final class Rectangle extends Quadrilateral { }

Add in record patterns, also in preview, and I believe this is the combination of features intended to solve for DUs in the Java language.

[HaloFour]

I actually like the approach Java has taken by making the base type declare the legal subtypes rather than trying to determine it through analysis. It more clearly codifies the intent of the developer and the expected contract of the hierarchy. I would expect that if DUs came to the language with an enum-like syntax that would cover that intent, but maybe it should be considered for closed hierarchies too.

[alrz]

private sealed interface Shape permits Circle, Triangle, Quadrilateral { }

I think that depends on sealed interface there, not anything about constructors. Also it requires derived types to be final or sealed. Those are the modifiers I was referring to in my first comment.

What is proposed here is to infer subtypes from ctor accessibility which allows it to "light up" for existing code - which implies that it won't work for non-nested types. Again, Java supports that using those modifiers at the declaration-site. On the second thought, private protected should suffice. I think my point is, it feels like a hack rather than a feature since it's all implicit.

Java has taken by making the base type declare the legal subtypes rather than trying to determine it through analysis.

From the link, that seem to be optional but given the above requirements I think it's mostly predictable? Not sure if there's any restriction on where those subtypes can actually be declared.

[FaustVX]

private sealed interface Shape permits Circle, Triangle, Quadrilateral { }
I know it's not C# related, but I find very weird to have the interface coupled with the class implementing it

[HaloFour]

@alrz

From the link, that seem to be optional but given the above requirements I think it's mostly predictable?

That's true, it's optional in the cases where the subtypes are obvious, such as nested within the parent type. But you still have to explicitly specify that the parent type is a sealed/closed hierarchy and the legal subtypes are encoded in the metadata.

[alrz]

you still have to explicitly specify that the parent type is a sealed/closed

Looking closer, there's another restriction: (The subclasses may be auxiliary or nested classes.)

No such thing exists in C# so the scope is considerably broader. That should be fine, but I'd personally prefer it to be explicit rather than relying on a precise pattern to encode this semantics as a "side effect" of just having a particular accessibility on the constructor - it's too easy to accidentally break the contract.

[Richiban]

Will this fix also apply to any upcoming file visibility modifier?