rework enums to ensure that equality remains valid

[rcoh]

We should make it so that the Unknown variant itself cannot be directly instantiated. This ensures that all paths go through from_str so that we never end up with the case of a unknown value that matches an existing variant.

The easiest way to do this is probably add #[non_exhaustive] on the Unknown variant + docs.

[Jacco]

@rcoh Can I try this one?

Did a bit research and think this needs a change in EnumGenerator.kt.

I found the code generated by this class in the models.rs files.

[rcoh]

sounds good! I might suggest a short RFC (design/rfcs) to ensure we're all on the same page about the approach. To clarify the problem / requirements:

1. Enum variants can be added in the future.
2. Upgrading your SDK shouldn't cause your code to behave differently

The current code supports 1. but does not support 2.

Consider SDK v1:

enum Version {
  IpV4,
  IpV6,
  Unknown(value)
}

But the server has since added support for IpV8.

The SDK user on v1 worked around this like so:

match version {
  Version::IpV4 => ...,
  Version::IpV6 => ....,
  Unknown(value) if value == "IpV8" => ...,
  _ => println!("other");
}

But then the user upgrades to v2 where the enum actually includes the IpV8 variant. Their code will suddenly stop working because all of a sudden we'll start producing Version::IpV8 instead of Unknown("IpV8")!

It's not so obvious how to solve this, I'd suggest trying out a few prototypes if you're interested and seeing which feels the most ergonomic. My first hunch might be something like:

// field is private
pub struct UnknownEnumValue(String);
enum Version {
  IpV4,
  IpV6,
  Unknown(UnknownEnumValue)
}

since you can't actually get the value out of unknown, users would need to do this instead:

match version {
  Version::IpV4 => ...,
  Version::IpV6 => ....,
  other @ _ if other.as_str() == "IpV8" => ...,
}

this would be forwards-compatible because both Unknown and IpV8 would return the same as_str() value.

This is a bit more meaty than I originally realized, I think it's totally tackleable, but no worries if it seems like too much to take on.

[jdisanti]

Note: We need to apply this same enum fix to our generated error types so that if an unhandled error becomes modeled later, it won't break any existing code.

[weihanglo]

We should make it so that the Unknown variant itself cannot be directly instantiated.

Do we need to enforce this rule in defining crate? If this rule is only applied to crates other than the crate defines the Enum, then I feel like #[non_exhaustive] is sufficient.

[jdisanti]

Making Unknown #[non_exhaustive] is a good first step, but it should also be useless to match explicitly on it so that people use the _ matcher instead of it.