[EFanZh]

View all comments

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.

[Dylan-DPC]

@EFanZh any updates on addressing the concerns? thanks

[EFanZh]

@Dylan-DPC: I think the current concern of this PR is that it could be superseded by rust-lang/rfcs#3607, but that hasn’t been accepeted yet, so to my understanding, currently the only thing I can do is to wait the team to make a final decision.

[clarfonthey]

Given that this change is relatively trivial in code, the PR is incredibly old (2022!), and the PR is blocked by an RFC which seems promising but isn't going to be prioritised any time soon, would it be reasonable to just close this instead of leaving it open and blocked for maybe another 2 years?

[clarfonthey]

Also: r? libs

So we have a reviewer on the rotation, in case we do want to keep this open.

[clarfonthey]

Gonna nominate for libs/lang to kinda force the issue on whether we should close this, re: #106418 (comment)

@rustbot +I-libs-nominated +I-lang-nominated

[workingjubilee]

rust-lang/libs-team#925 has arrived as a different attempt to break the deadlock here.

[clarfonthey]

We decided in the libs meeting that the lack of consensus is enough to close this, even though that does not mean we will never do this ever. So, we can close an old issue at least.