Remove #[non_exhaustive] from all enums

[heaths]

We should remove #[non_exhaustive] from all enums whether they are used for DUs or not.

While discussing #785 we came up with a design like so:

model Dog {
    kind: DogKind;
    weight?: integer;
}
union DogKind {
    string,
    'Golden',
    'Lab',
}
model Golden extends Dog {
    kind: 'Golden';
    short_hair?: bool;
}
model Lab extends Dog {
    kind: 'Lab';
    color?: string;
}

With an extensible enum for a discriminator, we end up in Rust with:

#[non_exhaustive]
enum DogKind {
    Dog(Dog),
    Golden(Golden),
    Lab(Lab),
}
struct Dog {
    pub kind: Option<String>,
    pub weight: Option<i32>,
}
struct Golden {
    pub weight: Option<i32>,
    pub short_hair: Option<bool>,
}
struct Lab {
    pub weight: Option<i32>,
    pub color: Option<String>,
}

Without "base types" in Rust, all properties of the base discriminant are defined in the variants. The discriminator is only defined in the variant representing the base type.

If DogKind was fixed - an enum in TypeSpec - we'd instead have:

#[non_exhaustive]
enum DogKind {
    Golden(Golden),
    Lab(Lab),
}
struct Golden {
    pub weight: Option<i32>,
    pub short_hair: Option<bool>,
}
struct Lab {
    pub weight: Option<i32>,
    pub color: Option<String>,
}

Effectively, we invert the discriminator and discriminant since this is idiomatic of enums in Rust.

The problem we run into, though, is that if we add #[non_exhaustive] to these enums, we force users to define a catch all e.g.,

let dog: DogKind = client.get_dog(&name, None).await?.into_model()?;
match dog {
    DogKind::Golden(v) => {
        let desc = if matches!(v.short_hair, Some(b) if b) {
            "short"
        } else {
            "long"
        };
        println!("Golden retriever with {desc} hair");
    }
    DogKind::Lab(v) => println!("Labrador with {} hair", v.color.unwrap_or_else("black")),
    DogKind::Dog(v) => println!("{} dog", v.kind),
    _ => panic!("unexpected"),
}

Even with a fixed enum, they still need a catch all. The point was to avoid breaking code when a new variant is added; however, I believe this only masks the issue and could lead to data loss e.g., in a polymorphic collection. The DogKind::Dog already captures the "unsupported value" scenario (albeit without capturing additional data, which is a separate issue). If in a later version we do define, say, a DogKind::Pug variant, it won't be captured by DogKind::Dog anymore and a collection update won't include it. At the very least, with #[non_exhaustive] we're effectively forcing customers to ignore it rather than break their code if they update the client to include the DogKind::Pug variant.

Talking with @johanste, @jhendrixMSFT, and @LarryOsterman this seems bad. I know we want to avoid breaking changes, but I don't think it should come at the risk of causing data loss or, at the very least, a failure of business logic. If the service makes a breaking change 🔐 forcing the customer to ignore it with #[non_exhaustive] that forces them to add a catch all is a bad decision.

And that got me to thinking about TypeSpec enums and unions (extensible enums) used in other places as well e.g., model property types:

model Temperature {
    temp: float32;
    kind: 'celsius' | 'fahrenheit';
}

If we generate a TemperatureKind like so:

#[non_exhaustive]
enum TemperatureKind {
    Celsius,
    Fahrenheit,
}

It will force customers to ignore anything else:

let c = match m.kind? {
    TemperatureKind::Celcius => m.temp?,
    TemperatureKind::Fahrenheit => (m.temp? - 32f32) * 5/9,
    _ => panic!("unexpected"),
};

// Or potentially worse...
let c = match m.kind? {
    TemperatureKind::Fahrenheit => (m.temp? - 32f32) * 5/9,
    _ => m.temp?,
}

If we added, say, Kelvin, at runtime we'll panic in the first case or return a temperature 273.15 degrees too cold.

I maintain that breaking at compile time is not only idiomatic to Rust (mantra: "if it compiles, it works"), but the safe choice for any language in similar circumstances.

Without #[non_exhaustive], customers can still add a catch-all but it's their choice and not forced upon them. And in many cases that's probably safe. Adding Kelvin when we already have Celsius above seems absurd and adding any other temperatures that are used commonly is incredibly unlikely. But should we, breaking is safer.

It we compare this to other languages like Azure SDK for .NET that I used to work on, the same potential catastrophe exists:

enum TemperatureKind
{
    Celsius,
    Fahrenheit,
}

var c = m.Temp;
if (m.Kind == TemperatureKind::Farhenheit)
{
    c = (m.Temp - 32f) * 5/9;
}

An extensible enum using a struct wouldn't be any better: same problem as with Rust exists - in the best case, there may be a business logic failure.

Thus, I propose we get rid of #[non_exhaustive] from all enums in Rust. Most other language SDKs already consider this a breaking change anyway.

[heaths]

/cc @johanste @KrzysztofCwalina @JeffreyRichter @JonathanGiles @bterlson @LarryOsterman

[heaths]

To note, cargo - Rust's package manager - does utilize semver as intended. Much like npm/pnpm/yarn, the default version requirement post-1.0.0 will take the latest minor version bump (or the latest patch minor version bump if < 1.0.0) unless a Cargo.lock lock file is checked in that locks versions. Nuget, for example, does not do this by default and will take the latest version regardless of semver unless you explicitly change your version spec to something like *. This is basically akin to ^1.0.0 in npm (or other similar variations).

So even if we were to make a breaking change, because we have cargo-semver-checks in place to make sure we correctly bump the major or minor versions based on API changes, any breaking changes to the crate source isn't implicitly breaking because developers have to opt into taking a new major version. I think that's what really sets it apart from .NET and potentially some other Azure SDK languages.

It's important to note, however, that adding #[non_exhaustive] at some future date will be breaking because customers will have to add the catch all...but I still maintain that's bad anyway.

[jhendrixMSFT]

Definitely agree that removing non_exhaustive from fixed enums makes sense as they are by definition exhaustive.

Also agree that removing it from extensible enums makes sense (and is safe) since customers already must handle the unknown variant, making the catch-all unnecessary (and unsafe in some cases as you point out).

[RickWinter]

I'm in favor of removing non-exhaustive. catch alls are a bad practice. Heath is right that they only mask the problem

[jhendrixMSFT]

Regarding inverting the discriminator/discriminant mentioned in #809 (comment), this doesn't work when the defined enum is used outside of the DU context. To expand on the example (note that it's been cleaned up a bit to follow best practices, specifically using the enum values for the values of kind in the derived types).

@discriminator("kind")
model Dog {
    kind: DogKind;
    weight?: integer;
}
union DogKind {
    string,
    'Golden',
    'Lab',
}
model Golden extends Dog {
    kind: DogKind.Golden;
    short_hair?: bool;
}
model Lab extends Dog {
    kind: DogKind.Lab;
    color?: string;
}
model DogOwner {
    favorite: DogKind;
}

Here, we have model DogOwner with the expectation to send {"favorite":"Lab"} over the wire.

let dog_owner = DogOwner {
    favorite: DogKind::Lab,
};

Admittedly contrived, but completely possible.