From b332a924531e72070b91ca219580eb0c198c28b7 Mon Sep 17 00:00:00 2001 From: Alex Burka Date: Mon, 1 Aug 2016 14:32:33 -0400 Subject: [PATCH 1/3] RFC for mem::discriminant() --- text/0000-discriminant.md | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 text/0000-discriminant.md diff --git a/text/0000-discriminant.md b/text/0000-discriminant.md new file mode 100644 index 00000000000..a45c6110e58 --- /dev/null +++ b/text/0000-discriminant.md @@ -0,0 +1,36 @@ +- Feature Name: (fill me in with a unique ident, my_awesome_feature) +- Start Date: (fill me in with today's date, YYYY-MM-DD) +- RFC PR: (leave this empty) +- Rust Issue: (leave this empty) + +# Summary +[summary]: #summary + +One para explanation of the feature. + +# Motivation +[motivation]: #motivation + +Why are we doing this? What use cases does it support? What is the expected outcome? + +# Detailed design +[design]: #detailed-design + +This is the bulk of the RFC. Explain the design in enough detail for somebody familiar +with the language to understand, and for somebody familiar with the compiler to implement. +This should get into specifics and corner-cases, and include examples of how the feature is used. + +# Drawbacks +[drawbacks]: #drawbacks + +Why should we *not* do this? + +# Alternatives +[alternatives]: #alternatives + +What other designs have been considered? What is the impact of not doing this? + +# Unresolved questions +[unresolved]: #unresolved-questions + +What parts of the design are still TBD? From 597d1e59e4fa649476d735c568c408e6324d0077 Mon Sep 17 00:00:00 2001 From: Alex Burka Date: Mon, 1 Aug 2016 14:36:19 -0400 Subject: [PATCH 2/3] write RFC --- text/0000-discriminant.md | 104 ++++++++++++++++++++++++++++++++++---- 1 file changed, 93 insertions(+), 11 deletions(-) diff --git a/text/0000-discriminant.md b/text/0000-discriminant.md index a45c6110e58..ba3983a248a 100644 --- a/text/0000-discriminant.md +++ b/text/0000-discriminant.md @@ -1,36 +1,118 @@ -- Feature Name: (fill me in with a unique ident, my_awesome_feature) -- Start Date: (fill me in with today's date, YYYY-MM-DD) +- Feature Name: discriminant +- Start Date: 2016-08-01 - RFC PR: (leave this empty) -- Rust Issue: (leave this empty) +- Rust Issue: [#24263](https://github.com/rust-lang/rust/pull/24263), [#34785](https://github.com/rust-lang/rust/pull/34785) # Summary [summary]: #summary -One para explanation of the feature. +Add a function that extracts the discriminant from an enum variant as a comparable, hashable, printable, but (for now) opaque and unorderable type. # Motivation [motivation]: #motivation -Why are we doing this? What use cases does it support? What is the expected outcome? +When using an ADT enum that contains data in some of the variants, it is sometimes desirable to know the variant but ignore the data, in order to compare two values by variant or store variants in a hash map when the data is either unhashable or unimportant. + +The motivation for this is mostly identical to [RFC 639](https://github.com/rust-lang/rfcs/blob/master/text/0639-discriminant-intrinsic.md#motivation). # Detailed design [design]: #detailed-design -This is the bulk of the RFC. Explain the design in enough detail for somebody familiar -with the language to understand, and for somebody familiar with the compiler to implement. -This should get into specifics and corner-cases, and include examples of how the feature is used. +The proposed design has been implemented at [#34785](https://github.com/rust-lang/rust/pull/34785) (after some back-and-forth). That implementation is copied at the end of this section for reference. + +A struct `Discriminant` and a free function `fn discriminant(v: &T) -> Discriminant` are added to `std::mem` (for lack of a better home, and noting that `std::mem` already contains similar parametricity escape hatches such as `size_of`). For now, the `Discriminant` struct is simply a newtype over `u64`, because that's what the `discriminant_value` intrinsic returns, and a `PhantomData` to allow it to be generic over `T`. + +Making `Discriminant` generic provides several benefits: + +- `discriminant(&EnumA::Variant) == discriminant(&EnumB::Variant)` is statically prevented. +- In the future, we can implement different behavior for different kinds of enums. For example, if we add a way to distinguish C-like enums at the type level, then we can add a method like `Discriminant::into_inner` for only those enums. Or enums with certain kinds of discriminants could become orderable. + +The function requires a `Reflect` bound on its argument because discriminant extraction is a partial violation of parametricity, in that a generic function with no bounds on its type parameters can nonetheless find out some information about the input types, or perform a "partial equality" comparison. This restriction is debatable (open question #2), especially in light of specialization. The situation is comparable to `TypeId::of` (which requires the bound) and `mem::size_of_val` (which does not). Note that including a bound is the conservative decision, because it can be backwards-compatibly removed. + +```rust +/// Returns a value uniquely identifying the enum variant in `v`. +/// +/// If `T` is not an enum, calling this function will not result in undefined behavior, but the +/// return value is unspecified. +/// +/// # Stability +/// +/// Discriminants can change if enum variants are reordered, if a new variant is added +/// in the middle, or (in the case of a C-like enum) if explicitly set discriminants are changed. +/// Therefore, relying on the discriminants of enums outside of your crate may be a poor decision. +/// However, discriminants of an identical enum should not change between minor versions of the +/// same compiler. +/// +/// # Examples +/// +/// This can be used to compare enums that carry data, while disregarding +/// the actual data: +/// +/// ``` +/// #![feature(discriminant_value)] +/// use std::mem; +/// +/// enum Foo { A(&'static str), B(i32), C(i32) } +/// +/// assert!(mem::discriminant(&Foo::A("bar")) == mem::discriminant(&Foo::A("baz"))); +/// assert!(mem::discriminant(&Foo::B(1)) == mem::discriminant(&Foo::B(2))); +/// assert!(mem::discriminant(&Foo::B(3)) != mem::discriminant(&Foo::C(3))); +/// ``` +pub fn discriminant(v: &T) -> Discriminant { + unsafe { + Discriminant(intrinsics::discriminant_value(v), PhantomData) + } +} + +/// Opaque type representing the discriminant of an enum. +/// +/// See the `discriminant` function in this module for more information. +pub struct Discriminant(u64, PhantomData<*const T>); + +impl Copy for Discriminant {} + +impl clone::Clone for Discriminant { + fn clone(&self) -> Self { + *self + } +} + +impl cmp::PartialEq for Discriminant { + fn eq(&self, rhs: &Self) -> bool { + self.0 == rhs.0 + } +} + +impl cmp::Eq for Discriminant {} + +impl hash::Hash for Discriminant { + fn hash(&self, state: &mut H) { + self.0.hash(state); + } +} + +impl fmt::Debug for Discriminant { + fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { + self.0.fmt(fmt) + } +} +``` # Drawbacks [drawbacks]: #drawbacks -Why should we *not* do this? +1. Anytime we reveal more details about the memory representation of a `repr(rust)` type, we add back-compat guarantees. The author is of the opinion that the proposed `Discriminant` newtype still hides enough to mitigate this drawback. (But see open question #1.) +2. Adding another function and type to core implies an additional maintenance burden, especially when more enum layout optimizations come around (however, there is hardly any burden on top of that associated with the extant `discriminant_value` intrinsic). # Alternatives [alternatives]: #alternatives -What other designs have been considered? What is the impact of not doing this? +1. Do nothing: there is no stable way to extract the discriminant from an enum variant. Users who need such a feature will need to write (or generate) big match statements and hope they optimize well (this has been servo's approach). +2. Directly stabilize the `discriminant_value` intrinsic, or a wrapper that doesn't use an opaque newtype. This more drastically precludes future enum representation optimizations, and won't be able to take advantage of future type system improvements that would let `discriminant` return a type dependent on the enum. # Unresolved questions [unresolved]: #unresolved-questions -What parts of the design are still TBD? +1. Can the return value of `discriminant(&x)` be considered stable between subsequent compilations of the same code? How about if the enum in question is changed by modifying a variant's name? by adding a variant? +2. Is the `T: Reflect` bound necessary? +3. Can `Discriminant` implement `PartialOrd`? From e5c9852cda73c0c6e163b73a90d0c13ca2275a1e Mon Sep 17 00:00:00 2001 From: Alex Burka Date: Mon, 1 Aug 2016 15:06:16 -0400 Subject: [PATCH 3/3] remove Reflect bound --- text/0000-discriminant.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/0000-discriminant.md b/text/0000-discriminant.md index ba3983a248a..4d1ccdaed0b 100644 --- a/text/0000-discriminant.md +++ b/text/0000-discriminant.md @@ -27,7 +27,7 @@ Making `Discriminant` generic provides several benefits: - `discriminant(&EnumA::Variant) == discriminant(&EnumB::Variant)` is statically prevented. - In the future, we can implement different behavior for different kinds of enums. For example, if we add a way to distinguish C-like enums at the type level, then we can add a method like `Discriminant::into_inner` for only those enums. Or enums with certain kinds of discriminants could become orderable. -The function requires a `Reflect` bound on its argument because discriminant extraction is a partial violation of parametricity, in that a generic function with no bounds on its type parameters can nonetheless find out some information about the input types, or perform a "partial equality" comparison. This restriction is debatable (open question #2), especially in light of specialization. The situation is comparable to `TypeId::of` (which requires the bound) and `mem::size_of_val` (which does not). Note that including a bound is the conservative decision, because it can be backwards-compatibly removed. +The function no longer requires a `Reflect` bound on its argument even though discriminant extraction is a partial violation of parametricity, in that a generic function with no bounds on its type parameters can nonetheless find out some information about the input types, or perform a "partial equality" comparison. This is debatable (see [this comment](https://github.com/rust-lang/rfcs/pull/639#issuecomment-86441840), [this comment](https://github.com/rust-lang/rfcs/pull/1696#issuecomment-236669066) and open question #2), especially in light of specialization. The situation is comparable to `TypeId::of` (which requires the bound) and `mem::size_of_val` (which does not). Note that including a bound is the conservative decision, because it can be backwards-compatibly removed. ```rust /// Returns a value uniquely identifying the enum variant in `v`. @@ -58,7 +58,7 @@ The function requires a `Reflect` bound on its argument because discriminant ext /// assert!(mem::discriminant(&Foo::B(1)) == mem::discriminant(&Foo::B(2))); /// assert!(mem::discriminant(&Foo::B(3)) != mem::discriminant(&Foo::C(3))); /// ``` -pub fn discriminant(v: &T) -> Discriminant { +pub fn discriminant(v: &T) -> Discriminant { unsafe { Discriminant(intrinsics::discriminant_value(v), PhantomData) }