Rust: discriminant_value intrinsic -- tracking issue for 639

[nikomatsakis]

Tracking issue for rust-lang/rfcs#639.

Some things to nail down before stabilization:

• [x] prototype implementation
• [ ] signature needs to have a <T:Reflect> bound
• [x] documentation should reflect the inherent instability in this value and describe valid uses
• [ ] perhaps signature should require input T to actually be an enum instance?
• [ ] do we want to adjust the return type in any way to enforce the above rules?

[pnkfelix]

The prototype implementation at #20907 is very easy to rebase (I have already done so). I plan to land that prototype as part of work towards #15523

[pyfisch]

How is the progress here? I want to use it in a package, but I don't think it will be 1.0, right? Will it be included in 1.1?

[briansmith]

RE the documentation issue: The value wouldn't be unstable for the case of a C-like enum, so the statement about the value being unstable should be qualified as such. And, it would also be useful to document that this feature can/should be used instead of as for safer conversions of C-like enums. A new lint that does that would be great.

However, the RFC says that the type is always u64 which means that I'd have to use as in my code anyway, as I need u8-typed discriminant values (e.g. for parsing ASN.1). Accordingly, I think that the part of the design where the return type is fixed at u64 should be fixed.

[durka]

I previously suggested a way to solve the return type problem: introduce a trait instead of (or in addition to) the intrinsic function.

trait MagicEnumTrait {
    type Repr;
    fn value(&self) -> Self::Repr;
}

This trait would be auto-implemented for every enum and Repr defined to be the type of the discriminant (in case the discriminant is packed away, Repr is the smallest integer type with enough bits). Also, Repr corresponds to the signedness of the enum in the case of things like #[repr(i8)] which makes it easier to compare discriminants for ordering.

[SimonSapin]

For Stylo we want to build with a stable Rust compiler. For Servo’s style crate, the only thing missing is discriminant_value. (https://github.com/servo/servo/pull/11816)

We use it with an enum that represents a property declaration: there’s one variant per supported CSS property, and each variants has a field with the property value. (The Rust types for values can be different for each property.) During the cascade, we get an sequence of declarations ordered by precedence, and we want to only keep one per property. We use discriminant_value to determine which property a given enum value represents.

There’s already some code generation going on in this crate, so we could write a function or method that behaves similarly to discriminant_value and uses match internally, but I don’t know if this would optimize as well. (This is used in a hot loop.)

[ranma42]

@SimonSapin if the results of the internal function are exactly the same as discriminant_value, LLVM should be able to optimise away the match completely and replace it with the same memory load that would be performed by discriminant_value. This has worked for me with several C-like enums (hand-checking the assembly output), so this might be a viable alternative if you do not want to wait for the stabilisation. Nonetheless I would recommend to verify if LLVM optimisations also work as intended in your case.

[SimonSapin]

These enums are not C-like though, otherwise I could use enum_value as usize. I’ll try it anyway.

[pnkfelix]

My questions at this point:

• has the signature of the intrinsic gotten a bound, as listed as a work item in the description by Niko?
  
  
  
  • my understanding is that the answer here is "no."
  
  
• should we provide a stable function that is merely a shim over the existing intrinsic, or should we hold off stabilization in order to attempt to implement some way to vary the return type based on the particular enum (which has been discussed previously, including in the comments here).
  
  
  
  • I'm personally ambivalent here; I can see the motivation to stabilize the thing we have, but I also respect people concerned about prematurely stabilizing before exploring the alternatives.
  
  
• if we decide to hold off stabilization to explore an alternative API: do such alternatives require a new RFC (or at least RFC amendment)?
  
  
  
  • I think the answer here is "yes."
  
  

[aturon]

Given that we can always deprecate an API in favor of an improved version later, and that the functionality is working fine (and is something we definitely want to provide), I see little downside to stabilizing a wrapper function right now. (Lang team discussed this and all feel the same way.)

[brson]

OK, so the concrete next step here is to write a wrapper function with the same signature and tag it stable. What module is it exported from? What's the process? Does it need an FCP period?

[briansmith]

Also, Repr corresponds to the signedness of the enum in the case of things like #[repr(i8)]

In that case, I would expect Repr to be exactly the time mentioned in #[repr(...)]. For example, #[repr(i16)] would have Repr = u16 even if a smaller representation could be used. This would provide some level of ABI stability in the case the enum is to be shared with non-Rust code and the enum is expanded in the future (e.g. from 250 to 300 choices).

[durka]

@briansmith agreed. Does rustc sometimes shrink enums even if you specify
the repr with the attribute? I would expect it not to.
On Jun 23, 2016 19:48, "Brian Smith" [email protected] wrote:

Also, Repr corresponds to the signedness of the enum in the case of things
like #[repr(i8)]

In that case, I would expect Repr to be exactly the time mentioned in

[repr(...)]. For example, #[repr(i16)] would have Repr = u16 even if a

smaller representation could be used. This would provide some level of ABI
stability in the case the enum is to be shared with non-Rust code and the
enum is expanded in the future (e.g. from 250 to 300 choices).

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/rust-lang/rust/issues/24263#issuecomment-228218760,
or mute the thread
https://github.com/notifications/unsubscribe/AAC3n2un5WmfnGg9JMv_eyuD44-yOmxAks5qOxthgaJpZM4D96D5
.

[briansmith]

@briansmith agreed. Does rustc sometimes shrink enums even if you specify
the repr with the attribute? I would expect it not to.

I don't know. I just read your comment above as suggesting that the smallest type should be used, insted of the type mentioned in #[repr(...)], and I just wanted to clarify that the smallest type shouldn't be used if the type wa explicitly given in #[repr(...)].

[nikomatsakis]

@brson

What's the process? Does it need an FCP period?

Usually unstable things undergo a "release cycle"-long FCP period, yes.

[nikomatsakis]

has the signature of the intrinsic gotten a bound, as listed as a work item in the description by Niko?

This seems like an important point. Now, in the meantime since we added this intrinsic, we also adopted specialization, which may make the need for such a bound moot. Do we all agree we "just don't care" about a bound?

[durka]

Well, if there's no bound, what happens when you call the function on a
value that isn't an enum variant? Is it UB, unspecified, defined to return
0 or -1 or...? Currently it seems you get 0: https://is.gd/odCsAO

I'll try to write an RFC for my trait idea because I think being able to
find out the repr type of an enum is valuable.

On Mon, Jun 27, 2016 at 2:15 PM, Niko Matsakis [email protected]
wrote:

has the signature of the intrinsic gotten a bound, as listed as a work
item in the description by Niko?

This seems like an important point. Now, in the meantime since we added
this intrinsic, we also adopted specialization, which may make the need for
such a bound moot. Do we all agree we "just don't care" about a bound?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/rust-lang/rust/issues/24263#issuecomment-228829059,
or mute the thread
https://github.com/notifications/unsubscribe/AAC3n6rGfeK1Aa0IppTD5U7CSK1nGpCfks5qQBNHgaJpZM4D96D5
.

[SimonSapin]

Servo update: TL;DR we’re not blocked on stabilization anymore, but it’d still be nice to have it eventually for debug build performance.

---

In https://github.com/servo/servo/pull/11884 I’ve worked around this being unavailable on stable with generated code like this:

impl PropertyDeclaration {
    fn discriminant_value(&self) -> usize {
        match *self {
            PropertyDeclaration::AlignContent(..) => 0,
            PropertyDeclaration::AlignItems(..) => 1,
            PropertyDeclaration::AlignSelf(..) => 2,
            // ...
            PropertyDeclaration::ZIndex(..) => 147,
        }
    }
}

And made a test crate to look at the generated code:

#![feature(core_intrinsics)]
extern crate style;
use std::intrinsics::discriminant_value;

pub fn with_match(decl: &style::properties::PropertyDeclaration) -> usize {
    decl.discriminant_value()
}

pub fn with_intrinsic(decl: &style::properties::PropertyDeclaration) -> usize {
    unsafe { discriminant_value(decl) as usize }
}

With cargo rustc --release -- --emit=llvm-ir, this giant match compiles to something very close to discriminant_value:

; ModuleID = 'style_stable.0.rs'
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

%"11.style::properties::PropertyDeclaration" = type { i64, [0 x i64], [60 x i64] }

; Function Attrs: norecurse nounwind readonly uwtable
define i64 @_ZN12style_stable10with_match17h5ee51730fa1d4ed4E(%"11.style::properties::PropertyDeclaration"* noalias nocapture readonly dereferenceable(488)) unnamed_addr #0 {
entry-block:
  %.idx = getelementptr %"11.style::properties::PropertyDeclaration", %"11.style::properties::PropertyDeclaration"* %0, i64 0, i32 0
  %.idx.val = load i64, i64* %.idx, align 8
  ret i64 %.idx.val
}

; Function Attrs: norecurse nounwind readonly uwtable
define i64 @_ZN12style_stable14with_intrinsic17h38553922905ee296E(%"11.style::properties::PropertyDeclaration"* noalias nocapture readonly dereferenceable(488)) unnamed_addr #0 {
entry-block:
  %1 = getelementptr inbounds %"11.style::properties::PropertyDeclaration", %"11.style::properties::PropertyDeclaration"* %0, i64 0, i32 0
  %2 = load i64, i64* %1, align 8, !range !0
  ret i64 %2
}

attributes #0 = { norecurse nounwind readonly uwtable }

!0 = !{i64 0, i64 129}

But without --release the .ll file is 6843 lines long, so I’m guessing the giant match expression takes O(n variants) time in debug mode. Since this is used in a hot function, this might still affect performance of debug builds (which are already slow).

[ranma42]

@SimonSapin your comment seems to be truncated

[brson]

I removed the easy tag since there's so much going on here and still some back-and-forth, but there seems to be momentum toward just wrapping this intrinsic and exporting it _somewhere_, if somebody wants to pick it up.

[SimonSapin]

@ranma42 Fixed.

[tbu-]

How does this interact with space-optimized enums, like in https://github.com/rust-lang/rfcs/issues/1230? It looks like this could hinder progress on these, as a discriminant_value on these might need a complicated calculation.

[SimonSapin]

You need these calculations anyway for match, right? I don’t know how it’s implemented, but today’s discriminant_value seems to work fine with space optimized Option<&T> for example.

[tbu-]

@SimonSapin As far as I understand, calculating a single discriminant value is _not_ needed for simply matching. E.g. if you have Option<bool>, as 0: false, 1: true, 2: None, then calculating a discriminant is slower than just comparing two of these for equality.

[gnzlbg]

@tbu- How does this interact with space-optimized enums, like in rust-lang/rfcs#1230?

Same question over here. Currently, std::mem::size_of::<Option<Vec<T>>>() == std::mem::size_of::<Vec<T>>(). Can someone sketch how discriminant_value works in this case?

[nagisa]

For all such optimisations the discriminant ends up being some particular value of some inner field, therefore all that’s necessary is an extra comparison operation.

In Option case (the only thing we apply such optimisation ATM) the function returns 0 for None or 1 for Some. However, whereas in non-optimised case all that’s necessary is a projection (a field load), for optimised case code ends up looking like this:

%26 = icmp ne i8* %25, null
%27 = zext i1 %26 to i64
store i64 %27, i64* %4

which in x86 assembly turns out to be 2 instructions

cmpq $0, -32(%rbp)
setne %r8b

All overhead over plain match is the store of the comparison result into a register which LLVM should have an easy time optimising out.

[durka]

Does the wrapper really need a T: Reflect bound? I would argue that extracting the discriminant does not break parametricity, as it's nothing you couldn't do with a giant match. What it actually needs is a T: Enum bound, but we don't have that.

[nagisa]

Does the wrapper really need a T: Reflect bound?

I personally argue against the bound like this, but other might feel differently.

What it actually needs is a T: Enum bound, but we don't have that.

Such check should be implemented in the compiler, similar to the transmute size match check.

[durka]

Such check should be implemented in the compiler, similar to the transmute size match check.

Hmm, currently the intrinsic doesn't do anything of the sort: it simply returns 0 (for the wrapper I was going to document it as returning an unspecified value). Actually such an outside-of-typeck check would make it really hard to write a wrapper, as fn foo<T>(v: &T) -> u64 { unsafe { intrinsics::discriminant_value(v) } } would presumably fail the check, just like fn bar<T, U>(t: T) -> U { unsafe { mem::transmute(t) } } does.

[glaebhoerl]

@durka

Does the wrapper really need a T: Reflect bound? I would argue that extracting the discriminant does not break parametricity, as it's nothing you couldn't do with a giant match.

You can't do a giant match on a generic type parameter T. But without a Reflect bound, you could invoke discriminant_value on it. That's what a parametricity violation is.

What it actually needs is a T: Enum bound, but we don't have that.

That might actually be the best thing. With T: Reflect it'd be saying "figure out whether this type is an enum, and if so, give me its discriminant, otherwise just give me 0". There's not necessarily any real _harm_ in being able to call it on something that isn't an enum, but it's a bit loose. With T: Enum it'd be saying "make sure to give me a type that's actually an enum, and I'll give you its discriminant". That's a bit nicer, and it'd let us sidestep the question of whether it even makes sense to have a Reflect still. (In a world with specialization, it's almost surely doomed, but awkwardly, that's not in stable yet, while discriminant_value would be.) It would _also_ let us cleanly solve the issue of different enums having different discriminant types, by giving the Enum trait an associated type.

[durka]

But extracting the discriminant doesn't break parametricity, that's what
I'm saying. At least no more than, say, size_of...
On Jun 29, 2016 18:47, "Gábor Lehel" [email protected] wrote:

@durka https://github.com/durka

Does the wrapper really need a T: Reflect bound? I would argue that
extracting the discriminant does not break parametricity, as it's nothing
you couldn't do with a giant match.

You can't do a giant match on a generic type parameter T. But without a
Reflect bound, you could invoke discriminant_value on it. That's what a
parametricity violation is.

What it actually needs is a T: Enum bound, but we don't have that.

That might actually be the best thing. With T: Reflect it'd be saying
"figure out whether this type is an enum, and if so, give me its
discriminant, otherwise just give me 0". There's not necessarily any real
_harm_ in being able to call it on something that isn't an enum, but it's
a bit loose. With T: Enum it'd be saying "make sure to give me a type
that's actually an enum, and I'll give you its discriminant". That's a
bit nicer, and it'd let us sidestep the question of whether it even makes
sense to have a Reflect still. (In a world with specialization, it's
almost surely doomed, but awkwardly, that's not in stable yet, while
discriminant_value would be.) It would _also_ let us cleanly solve the
issue of different enums having different discriminant types, by giving
the Enum trait an associated type.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/rust-lang/rust/issues/24263#issuecomment-229511990,
or mute the thread
https://github.com/notifications/unsubscribe/AAC3n9j_jZLhNKDFn2rgnwzTkRNsgIBFks5qQvXqgaJpZM4D96D5
.

[glaebhoerl]

Yes, size_of also does.

[nagisa]

Hmm, adding marker types for sum and product types sounds nice, but is also somewhat of an involving change that would need some RFC-level discussion to happen.

[durka]

So I'm working on implementing this. There are several routes for implementation:

• Just re-export the intrinsinc.
  
  
  
  • This requires stabilizing the intrinsic, which makes it possible to import by mistake from std::intrinsics, because neither rustdoc nor rustc consider the stability of the parent module (but they are supposed to, so opinions differ on whether this really counts as stabilization).
  
  
  • This comes with the possibility of implementing a magic check that the T is an enum, even though there's no bound in the type signature. This would be similar to the magic same-size check for transmute.
  
  
  • There's no need for a Reflect bound because you will never be able to call T::discriminant_value() in a generic context anyway.
  
  
• Write a wrapper function.
  
  
  
  • This does not require stabilizing the intrinsic.
  
  
  • We can't put in a magic check because the wrapper function would need to be generic and could never pass the check. The return value for non-enums would be unspecified.
  
  
  • We could put in a Reflect bound, but I really don't see the need for it.
  
  

I'm currently planning on naming the re-export or wrapper std::raw::discriminant. This requires stabilizing std::raw (but its contents, currently only TraitObject, can be destabilized under the same feature gate).

[aturon]

Removing nomination, but putting on P-high, as we want to land the wrapper and stabilize ASAP.

[brson]

P-high bugs should have assignees. Does anybody want to take this bug and be responsible for getting it done in the next cycle?

[durka]

I'd like to work on it. But nobody expressed an opinion yet on the question I asked above :)

[brson]

@durka I'd say follow the transmute precedent and just tag it stable. Since I can't assign you I assigned myself on your behalf (whatever good that'll do).

[brson]

@durka Actually I've immediately changed my opinion. I think the wrapper is the less risky approach, assuming we don't need the enum check (and from previous comments I was under the impression we don't).

[durka]

The wrapper is also certainly the easier approach :)

[durka]

Should the wrapper be insta-stable?

[brson]

@durka Yes.

[durka]

See #34785.

[brson]

Triage: making progress! Thanks @durka !

[nagisa]

Crossposting the link to an idea for more people to see.

[durka]

An alternative of sorts has emerged where the wrapper function returns an opaque newtype that derives some traits, but not all. See rust-lang/rfcs#1696.

[brson]

P-medium since there's no longer the urgent need from servo.

[nikomatsakis]

On a related note, we decided to put the Reflect trait into FCP, with the intention of deprecating and removing it. That discussion would take place on the tracking issue. I will try to copy over some relevant comments there though so that people need not repeat themselves.

[shepmaster]

Any updates on the stability of this? Comparing an enum for variant equality without checking the contained data equality comes up sporadically on Stack Overflow.

[nagisa]

I feel that it is fairly ready for stabilisation.

I took liberty to check off the

documentation should reflect the inherent instability in this value and describe valid uses

because the documentation for mem::discriminant does mention it.

I believe that for T: Reflect and T: Enum we’ve decide to not do it and just take any value.

---

As for

do we want to adjust the return type in any way to enforce the above rules?

the return type is opaque and guarantees you can only really do the equality comparison between two Discriminants, although it does not guarantee either of T: Reflect or T: Enum.

[durka]

Reflect is gone anyway.

On Fri, Jul 14, 2017 at 1:50 PM, Simonas Kazlauskas <
[email protected]> wrote:

I feel that it is fairly ready for stabilisation.

I took liberty to check off the

documentation should reflect the inherent instability in this value and
describe valid uses

because the documentation for 'mem::discriminant`
https://doc.rust-lang.org/stable/core/mem/fn.discriminant.html does
mention it.

I believe that for T: Reflect and T: Enum we’ve decide to not do it and

just take any value.

As for

do we want to adjust the return type in any way to enforce the above rules?

the return type is opaque and guarantees you can only really do the
equality comparison between two Discriminants, although it does not
guarantee either of T: Reflect or T: Enum.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/rust-lang/rust/issues/24263#issuecomment-315423326,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAC3n4DD99mE8BOOhGJ5A7vR4rOWNFxzks5sN6pmgaJpZM4D96D5
.

[shepmaster]

As a single point of feedback, I'm comfortable with the fact that mem::discriminant takes non-enums, so long as doing so doesn't crash the program, cause UB, etc.

the equality comparison between two Discriminants

Even better, it's for Discriminant<T>, so you can't accidentally cross-compare types, which was a nice touch!

[sfackler]

I agree that we should be ready to stabilize.

@rfcbot fcp merge

[durka]

I'll mention one last time my idea of an "intrinsic trait" which would
eliminate the issue with T not being an enum. This would be a magic trait
that is implemented for all enums and includes a function for getting the
discriminant. That also provides a natural "upgrade path" for outputting
specific discriminant types, because you could add an associated type to
the trait.

On Fri, Jul 14, 2017 at 3:23 PM, Steven Fackler [e-mail]
wrote:

I agree that we should be ready to stabilize.

@rfcbot https://github.com/rfcbot fcp merge

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/rust-lang/rust/issues/24263#issuecomment-315446156,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAC3n1NhUBBDnw6tCglfA8IZjg9Lo8sPks5sN8BIgaJpZM4D96D5
.

[rfcbot]

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

• [x] @BurntSushi
• [x] @Kimundi
• [x] @alexcrichton
• [x] @aturon
• [x] @brson
• [x] @dtolnay
• [x] @eddyb
• [x] @nikomatsakis
• [x] @nrc
• [x] @pnkfelix
• [x] @sfackler
• [x] @withoutboats

Concerns:

• ?Sized resolved by https://github.com/rust-lang/rust/issues/24263#issuecomment-316590395

Once these reviewers reach consensus, 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!

See this document for info about what commands tagged team members can give me.

[nikomatsakis]

@durka it seems like we can still do that if we want; I personally find it a feature, not a bug, that this works for values of any type. In particular, when you think about "algebraic data types" in a more abstract way, structs (and really any "single value" sort of type) is really just a special case of enums where there happens to be only one variant. This is why e.g. haskell calls both of them just data. So it feels natural to me that we would just return 0 in the case of a non-enum.

EDIT: Well, not 0, but some "sentinel" value.

[dtolnay]

Have we come up with a use case for having a generic type T and wanting its discriminant value whether or not T is an enum type? Or is this a case of "no compelling reason to disallow these types" as the RFC writes? Is there a compelling reason to disallow ?Sized types? I understand that enum types are always Sized for now but it seems like an odd place to draw the line (among enum types only, Sized types only, or any types).

@rfcbot concern ?Sized

Then again ?Sized is backward compatible to add in the future if necessary.

[dtolnay]

As discussed in yesterday's libs team meeting, we do not expect this to be used with generic T whether Sized or ?Sized. If a ?Sized use case comes up, we can follow up later in a backward compatible way.

@rfcbot resolved ?Sized

[shepmaster]

I'd like to point out that these two statements are subtly different, based on my reading:

for having a generic type T and wanting its discriminant value whether or not T is an enum type

we do not expect this to be used with generic T

I don't know when someone would want to get the discriminant value for a non-enum, but at least one person on Stack Overflow has asked for a generic function to compare enum variants, along the lines of:

fn variant_eq<T>(a: &T, b: &T) -> bool {
    std::mem::discriminant(a) == std::mem::discriminant(b)
}

[joshtriplett]

@shepmaster I've wanted that several times as well, though I'd be fine with it only working on enum types if there's a way to declare that.

[real-or-random]

Was there a strong reason not to implement PartialOrd for Discriminant<T>?
My use case is an ADT-enum that describes states for a state machine. It proceeds A -> B -> C or A -> B -> D (simplified, my actual example is larger and more complex). So I want the "natural" order on the enum except that C should be incomparable to D.

This will be very easy to implement when it's possible to rely on the order of the discriminants.

[aturon]

(Checking off for @pnkfelix, who is away)

[rfcbot]

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

[durka]

There's an open ICE: #43398. Possibly just an assertion in trans needs to be removed, but I don't know anything about that code.

However, as a larger point, currently Discriminant is only 64 bits wide. Assuming we want to support 128-bit discriminants, it should be expanded or made into an enum or something, which would change the size. (If we say it's an OS-specific type (sort of a stretch) then that doesn't matter.)

[sfackler]

There are like 3 types about which we provide guarantees around their size (e.g. Option<extern fn()> is pointer sized). We can totally expand the type's internal size in the future.

[rfcbot]

The final comment period is now complete.

[durka]

New summary comment

Quoting from the original checklist:

Some things to nail down before stabilization:

• [x] prototype implementation
• [ ] signature needs to have a <T:Reflect> bound

Reflect is gone.

• [x] documentation should reflect the inherent instability in this value and describe valid uses
• [ ] perhaps signature should require input T to actually be an enum instance?

We decided against this.

• [ ] do we want to adjust the return type in any way to enforce the above rules?

IMO the return type is currently "opaque enough".

---

Concerns raised during FCP:

• [ ] [Consider](https://github.com/rust-lang/rust/issues/24263#issuecomment-318618960) implementing PartialOrd
• [ ] ICE with i128 #43398 (but #[repr(i128)] is going to remain unstable)
• [ ] [Consider](https://github.com/rust-lang/rust/issues/24263#issuecomment-321141003) changing the size to accommodate #[repr(i128)] (we can just do this later though)

---

Let's get this stabilized!

[durka]

Given that we can always _add_ an impl of PartialOrd and the other two checkboxes have to do with #[repr(i128)], itself unstable, I don't see any roadblocks. Did I miss anything?

[cramertj]

I know this issue has already been closed, and the function is on track for stabilization, but I thought I should bring this up anyway since it seems odd to me: mem::discriminant returns 0 for all non-enums, _including enum constructors_. That is, the following code prints 0 and then 1:

use std::mem;

enum MyEnum {
    First,
    Second(i32)
}

fn main() {
    println!("{:?}", mem::discriminant(&MyEnum::Second)); // prints `0`
    println!("{:?}", mem::discriminant(&MyEnum::Second(2))); // prints `1`
}

IMO, it'd be nice to have the discriminant of the enum constructor match the discriminant of the enum value. I'm not sure if I've missed the boat here, but this seems like it would be really valuable for getting the discriminant without having to fully construct a value (this doesn't work for struct constructors, but that's something we could revisit).

[durka]

In theory we can change that since the docs say the return value for
non-enums is unspecified. But an enum constructor is "just" a function, so
I'm not sure it's a good idea.

On Thu, Sep 7, 2017 at 12:50 PM, Taylor Cramer [e-mail]
wrote:

I know this issue has already been closed, and the function is on track
for stabilization, but I thought I should bring this up anyway since it
seems odd to me: mem::discriminant returns 0 for all non-enums, including
enum constructors. That is, the following code prints 0 and then 1:

use std::mem;
enum MyEnum {
First,
Second(i32)
}
fn main() {
println!("{:?}", mem::discriminant(&MyEnum::Second)); // prints 0
println!("{:?}", mem::discriminant(&MyEnum::Second(2))); // prints 1
}

IMO, it'd be nice to have the discriminant of the enum constructor match
the discriminant of the enum value. I'm not sure if I've missed the boat
here, but this seems like it would be really valuable for getting the
discriminant without having to fully construct a value (this doesn't work
for struct constructors, but that's something we could revisit).

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/rust-lang/rust/issues/24263#issuecomment-327858281,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAC3n5_4DIAuM3s6HQ4nvkuUTBhPsIcKks5sgB66gaJpZM4D96D5
.

[nikomatsakis]

Hmm. I agree with @durka that at first it surprised me (because I think of them as a function), though really the type of a variant is (I believe...) a zero-sized type that uniquely identifies the enum variant. (Unless of course you coerce it into a function.) So it seems like it should be possible to make it work, and I do find @cramertj's use case compelling.

[nikomatsakis]

But what about "struct" variants?

enum Foo { ... Bar { ... } }`

[eddyb]

@nikomatsakis We employ the nuclear option (Foo::Bar is a function which you call with braces instead of parens and any function can be defined like that - named arguments!).

[cramertj]

@eddyb I suggested that exact thing on IRC in the lang channel:

fn foo { a: i32, b: i32, c: i32 } -> i32 { a + b + c }

assert_eq!(foo { a: 1, b: 2, c: 3 }, 6);

[cramertj]

We didn't ever close this conversation, but mem::discriminant is set to stabilize in 1.21. Can we at least adjust the docs to say that the return value for non-enums is unspecified?

[durka]

@cramertj I'm not sure what you're asking for -- it currently says "If T is
not an enum, calling this function will not result in undefined behavior,
but the return value is unspecified."

On Thu, Sep 21, 2017 at 1:32 PM, Taylor Cramer [e-mail]
wrote:

We didn't ever close this conversation, but mem::discriminant is set to
stabilize in 1.21. Can we at least adjust the docs to say that the return
value for non-enums is unspecified?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/rust-lang/rust/issues/24263#issuecomment-331226688,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAC3n5Lcd-nJNePXeczKjBFfG758-02eks5skp2YgaJpZM4D96D5
.

[cramertj]

@durka Ah, I was referring to your comment above where you said

In theory we can change that since the docs say the return value for
non-enums is unspecified.

[durka]

We could try to head off the mistake by writing "If T is not an enum (including tuple variant constructors!)" or including your example from above.

[joshtriplett]

@cramertj @durka Is there a fundamental reason we can't produce a compiler error if calling it on a non-enum (including an enum "constructor")?

[durka]

@joshtriplett see previous discussion.

[Twey]

The current Discriminant<T> contains a PhantomData<*const T>. This means it's non-Send and non-Sync, but I can't think of a good reason it should be. Is there one?

[sfackler]

Nope, we should impl those traits.

[omaskery]

Hullo, does anybody know if any discussion/work is ongoing in line with @cramertj 's suggestion regarding calling std::mem::discriminant on an enum "constructor"?