Implement PartialOrd and Ord for Discriminant

[EFanZh]

I found it difficult to implement PartialOrd or Ord for enums without the ability to compare two Discriminant values. Is it OK to implement PartialOrd and Ord for Discriminant?

[rustbot]

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[clubby789]

@rustbot label +T-libs-api -T-libs

[joshtriplett]

Seems reasonable.

@rfcbot merge

[rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se

Concerns:

• exposes-ordering resolved by Implement PartialOrd and Ord for Discriminant #106418 (comment)
• renominate for T-lang when libs concerns are resolved resolved by Implement PartialOrd and Ord for Discriminant #106418 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[Mark-Simulacrum]

Do we want to say anything about the ordering here being unstable? Does this imply that an enum that isn't itself implementing partialord now needs to worry about not reordering the variants?

[BurntSushi]

@Mark-Simulacrum I thought about that while reviewing this and felt like this part of the docs covered that aspect. But I think adding more clarifying docs to cover this explicitly would not hurt.

[Mark-Simulacrum]

Hm, I think that covers the standard library / compiler implementation, but I'm not sure it does much for library authors. That is, we can use a change in order to return different variants, but it doesn't necessarily mean that a library author defining an enum isn't now implicitly promising that they'll keep the same order. (since, in general, changing the enum definition would be a breaking change). To some extent the language is exposing a "new" way to learn about which order the variants are in here.

[EFanZh]

@Mark-Simulacrum: The PartialOrd derive macro specified that the orders of enum variants depend on their discriminants: https://doc.rust-lang.org/stable/std/cmp/trait.PartialOrd.html#derivable, so changing the discriminants is already a breaking change.

[daxpedda]

Related: #51561, which was an old issue proposing this.

[Mark-Simulacrum]

@Mark-Simulacrum: The PartialOrd derive macro specified that the orders of enum variants depend on their discriminants: https://doc.rust-lang.org/stable/std/cmp/trait.PartialOrd.html#derivable, so changing the discriminants is already a breaking change.

Right, but this is exposing that order on things that don't derive PartialOrd. If you're already exposing the order through PartialOrd, it's not a difference in behavior.

[Mark-Simulacrum]

@scottmcm seems to have felt similarly on that thread - #51561 (comment) - thanks for finding it.

[scottmcm]

Thanks for the ping here, Mark!

I remain strongly opposed to anything that makes it impossible for a library author to leave open the door to reorder their enum variants later.

If they want to opt-in to exposing the order, such as if these impls were impl<T> cmp::Ord for Discriminant<T> where T: ExposedVariantOrder with a #[derive(ExposedVariantOrder)] or something, then sure, having this would be fine.

But, for example, ControlFlow is intentionally not exposing the order of its variants right now

rust/library/core/src/ops/control_flow.rs

Lines 82 to 83 in aa5dbee

	// ControlFlow should not implement PartialOrd or Ord, per RFC 3058:
	// https://rust-lang.github.io/rfcs/3058-try-trait-v2.html#traits-for-controlflow

and I'd like to keep it that way, but that's impossible if this PR lands. This is not just a PR for two impls, this is a PR for "no public enum can ever be reordered later".

(As another example here, there's been work in the past to see whether we could reorder the variants in Result to make ? more efficient, which would have to include manually writing the Ord impl to match, but would be possible so long as this PR doesn't land, but if this does land, that becomes impossible.)

Even derive(PartialOrd) I could at least change from deriving it to implementing it manually in order to continue to provide consistent behaviour after reordering, but there's no way to fixup a Discriminant ordering later.

(Another direction that I'd like to see and would help the motivation here is better ways of accessing discriminants, like the derive in rust-lang/rfcs#3046 )

[daxpedda]

#106418 (comment) makes sense to me, Discriminant can simply say that the order is unstable.

I actually always assumed that this comment already says that:

Stability

The discriminant of an enum variant may change if the enum definition changes. A discriminant of some variant will not change between compilations with the same compiler.

But it's actually not a 100% clear, but I would argue that this should be improved. "A discriminant of some variant will not change between compilations with the same compiler." implies that Discriminant is already unstable between compiler versions to me. In any case, if we implement PartialOrd and Ord for Discriminant, this section has to be extended.

The main motivation of this PR is to allow implementers of PartialOrd and Ord to use what the standard library is using internally already, to allow for efficient code generation:

rust/compiler/rustc_builtin_macros/src/deriving/generic/mod.rs

Lines 1098 to 1131 in aa5dbee

	/// ```
	/// #[derive(PartialEq)]
	/// # struct Dummy;
	/// enum A {
	/// A1,
	/// A2(i32)
	/// }
	/// ```
	/// is equivalent to:
	/// ```
	/// #![feature(core_intrinsics)]
	/// enum A {
	/// A1,
	/// A2(i32)
	/// }
	/// impl ::core::cmp::PartialEq for A {
	/// #[inline]
	/// fn eq(&self, other: &A) -> bool {
	/// let __self_tag = ::core::intrinsics::discriminant_value(self);
	/// let __arg1_tag = ::core::intrinsics::discriminant_value(other);
	/// __self_tag == __arg1_tag &&
	/// match (self, other) {
	/// (A::A2(__self_0), A::A2(__arg1_0)) =>
	/// *__self_0 == *__arg1_0,
	/// _ => true,
	/// }
	/// }
	/// }
	/// ```
	/// Creates a tag check combined with a match for a tuple of all
	/// `selflike_args`, with an arm for each variant with fields, possibly an
	/// arm for each fieldless variant (if `unify_fieldless_variants` is not
	/// `Unify`), and possibly a default arm.
	fn expand_enum_method_body<'b>(

(used in PartialOrd/Ord as well, see sample output here)

So adding another trait, e.g. ExposedVariantOrder, doesn't make sense to me here unless it remains private, which traits don't currently support, see RFC 3323. Any other purpose should be filled by rust-lang/rfcs#3046 imo.

[dtolnay]

@rfcbot cancel

[rfcbot]

@dtolnay proposal cancelled.

[Mark-Simulacrum]

It sounds like the position is that the discriminant order is public but not considered breaking without explicit documentation on the relevant type indicating it's stable? In other words, for a tool checking library semver (e.g. obi1kenobi/cargo-semver-checks#305) this PR is not intended to make it a breaking change for e.g. the standard library to reorder an enum that doesn't derive PartialOrd itself, and so the tool shouldn't return an error on such a change?

That matches the transmute case and size_of, etc., so that would seem ok to me.

[dtolnay]

Relaunching #106418 (comment) with libs-api + lang.

@rfcbot fcp merge

[rfcbot]

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @tmandry

Concerns:

• overly-broad (Implement PartialOrd and Ord for Discriminant #106418 (comment))
• rfc-3607-instead (Implement PartialOrd and Ord for Discriminant #106418 (comment))
• stability wrt compiler version resolved by Implement PartialOrd and Ord for Discriminant #106418 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[dtolnay]

@Mark-Simulacrum yes: it's in the same bucket as transmute and size_of, or even more closely related, discriminant values on C-like enums, or const values in general.

pub enum Enum {
    Foo,
    Bar,
}

Even disregarding Discriminant<Enum>, is it a breaking change to reorder the above variants? Usually no, despite the fact someone could have written let _: [(); Enum::Bar as usize - Enum::Foo as usize]; which would then break. The baseline contract is "don't do that". Individual libraries can make a stronger contract; for example if the enum is HttpStatusCode with NotFound = 404, there's an indication the library would consider it a breaking change to change those values, especially if it's documented showing status_code as usize as intended usage.

Or consider unicode_xid::UNICODE_VERSION. The constant can and does change values across patch versions of the same library: 0.2.2 and 0.2.3 and 0.2.4 have a different value. The baseline API contract for constants is that it has a value of the declared type; changing the type, or name, or existence of the constant, are breaking changes. Individual libraries, implicitly or explicitly, supply a stronger contract for specific constants.

[tmandry]

Is it important that the discriminant order remain the same between different versions of the compiler? If not I'd like to document that it could change, and left a suggestion to that effect.

@rfcbot concern stability wrt compiler version

Otherwise this proposal looks good to me. Thanks for pushing it through.

@rfcbot reviewed

[daxpedda]

Is it important that the discriminant order remain the same between different versions of the compiler? If not I'd like to document that it could change, and left a suggestion to that effect.

The current proposal still specifies the Discriminant as an opaque wrapper around the enum discriminant, which according to the reference is well defined and would probably have some very large breaking changes throughout the ecosystem if changed.

So it should remain the same between compiler versions.

[Amanieu]

Is it important that the discriminant order remain the same between different versions of the compiler? If not I'd like to document that it could change, and left a suggestion to that effect.

The discriminant is guaranteed to be in the order in which variants are defined in the source code. This is necessary for many use cases such as using the discriminant ordering to implement Ord/PartialOrd for enums.

[RalfJung]

The comparison with transmute is a bit off since that is an unsafe operation, whereas comparing discriminants would be possible entirely in safe code. So it's more like size_of, though it exposes higher-fidelity information than size_of does.

[traviscross]

We discussed this in the lang meeting today without resolution.

[scottmcm]

I remain concerned about exposing this with no opt-out on an unrestricted generic type
@rfcbot concern overly-broad

I'm committing to making an alternative proposal because I shouldn't block without one. Please hold my feet to the fire if that's no up in a week.

Basically, I have an idea for how we might be able to do this, from #106418 (comment)

2. Expose the variant ordering privately, only accessible by the type owner/module.

Solution 2. is obviously more desirable, but AFAIK Rust can't do that and there is no proposal to add a feature like that.

---

Just to confirm, the primary use here is the

I found it difficult to implement PartialOrd or Ord for enums without the ability to compare two Discriminant values.

from the OP, right? (And similarly the derive-more cases from #106418 (comment) )

If there's anything else that people are wanting to use this for, please mention it.

From a quick scan of other posts here, the only other thing brought up was #106418 (comment) which to me sounds like it wants a separate TreeOrd derive, since it mentions "should there be something else?"

[AaronKutch]

My comment was just a variation on it being difficult to implement PartialOrd for large enums. Now that I think about it, I could just associate each variant with a different integer and avoid a O(n^2) nested match that way, but really what I want is that it readily optimizes down internally into a single discriminant comparison and I don't have to write a macro that deals with explicit discriminants or other edge cases. The "other" thing I was mentioning was in case the discriminant could end up being different from what #[derive(PartialOrd)] compares against, but from the tail end of this discussion it appears that the ordering will correspond always according to source code ordering (or to explicit discriminants).

[tmandry]

It sounds like my original concern has been resolved in favor of "we have to keep the ordering stable", so marking it as such. I'm interested to see @scottmcm's proposal, though.

@rfcbot resolve stability wrt compiler version

[daxpedda]

I'm committing to making an alternative proposal because I shouldn't block without one. Please hold my feet to the fire if that's no up in a week.

@scottmcm friendly ping!

[scottmcm]

Thanks for the poke, @daxpedda ! I've posted it as rust-lang/rfcs#3607

[joshtriplett]

@rfcbot concern rfc-3607-instead

[obi1kenobi]

It sounds like the position is that the discriminant order is public but not considered breaking without explicit documentation on the relevant type indicating it's stable? In other words, for a tool checking library semver (e.g. obi1kenobi/cargo-semver-checks#305) this PR is not intended to make it a breaking change for e.g. the standard library to reorder an enum that doesn't derive PartialOrd itself, and so the tool shouldn't return an error on such a change?

That matches the transmute case and size_of, etc., so that would seem ok to me.

Just wanted to quickly check my understanding of the stance w.r.t SemVer here:

Does implementing PartialOrd on the enum constitute an explicitly-documented guarantee of order stability between the variants?

The other ways of exposing a discriminant (#[repr(i8)] or unit-only enums) do not constitute such a guarantee by themselves, if my understanding of this thread is accurate. I wasn't sure about PartialOrd, hence my question.

[the8472]

In my opinion inferring whether the discriminant values are promised to be stable from PartialOrd is a poor approach. An implementer could always derive PartialOrd today but switch to a manual implementation tomorrow and then reorder the fields.