-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement a 'discriminant_value' intrinsic #639
Conversation
|
||
# Alternatives | ||
|
||
* More strongly specify the values returned. This would allow for a broader range of uses, but |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also return an opaque type that implements PartialOrd
, Ord
, PartialEq
and Eq
, but doesn't expose the raw discriminant value. This gives us flexibility with the implementation, and avoids some pitfalls with returning an explicit u64
, for example, what happens if the enum is #[repr(i64)]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the return value is unrelated to ordering, it shouldn't implement (Partial)Ord. But it should implement Hash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is in fact related to ordering of variants of the enum: rust-lang/rust#15523.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean the RFC explicitly says "Does not allow for the value to be used as part of ordering.". Perhaps require impl<T: PartialOrd> PartialOrd for DiscriminantValue<T>
. (That ordering is wrong if the enum uses a custom ordering though.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. @Aatch why couldn't the discriminant be usable in part of an Ord
impl?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the RFC is currently written, the only guarantee you have is that two values of the same type produce the same value, iff the variants are the same. While you could implement a ordering with this, I deliberately restricted such that the ordering would be effectively unspecified. The restriction could be lifted, but it would require a more rigorous specification of discriminant values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we were to take the route of specifying the ordering respecting the ordering of definition or possibly the ordering of the values of C-like enums, could you elaborate on some of the cons of that strategy? e.g. do you know of some optimizations that may block us from in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 impl<T: PartialOrd> PartialOrd for DiscriminantValue<T>
. Added benefit of not being able to mix discriminant values of different types.
|
||
# Unresolved questions | ||
|
||
* Should `#[derive]` use this intrinsic to improve derived implementations of traits? While |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems pretty crucial that this is at least used by deriving, and preferably exposed with a #[stable]
wrapper function. If it's not used inside of the stdlib and not stable, it seems kind of useless.
I try to write a language tag library in Rust, that uses big (8000 variants) enums to store all languages. |
I think adding this a good idea whether or not the compile gets smarter. That said, has anyone tried telling LLVM that other possible discriminant values are all unreachable? |
Can someone explain why an intrinsic is preferable to a compiler-generated trait implementation? |
@taralx assuming you mean a compiler-generated |
No, not a generated match. I mean an actual trait: #[lang="enumtrait"]
pub trait Enum {
pub fn discriminant(&self) -> usize
} This would allow type-level assertions of the form "T: Enum" as well. |
@taralx I think using an opaque return type, e.g., I do think it would beneficial to have such a trait (or possibly a free function) that can potentially be stabilized and called from safe code, as determining whether two enums of the same type are also of the same variant doesn't seem like it should require |
@taralx That trait method can be built on top of the intrinsic, just like how the |
Would it help to start a separate RFC for it? |
I'll probably update this RFC soon to address some of the points that have come up. One important question I'd like more feedback on is how well-specified the value returned from the intrinsic is. I went for the most conservative/least specified version, but I have no problem specifying it more if people think it is reasonable. |
I think we can at least specify that the discriminant ordering can be relied on for use in |
@sfackler I'm not sure how an opaque type would work though. How do you do anything with a type you know nothing about? It's not difficult to just cast from a u64 to an i64 if you need signed comparison. This intrinsic is intended primarily for proper ADT enums, where you can't control the discriminant values anyway. C-Like enums can be casted directly to get their value. |
@Aatch it would implement |
I was hoping to use this to implement EnumSet using BitvSet. Hence the trait suggestion. With a trait you could make the type of the discriminant associated... |
@Aatch are there other revisions you're planning on making in the near future? (I haven't read the discussion carefully yet.) |
ping @pnkfelix |
I have now read the discussion. In principle I would prefer for the RFC to actually document some of the concrete trait-based alternatives that have been laid out over the course of the discussion (though I personally agree with @Aatch that such alternatives could be written atop the intrinisic as specified). |
However, having said that, I think the RFC is actually perfectly fine as written, and if presented the choice between merging it as-is versus not merging it at all, I would choose the former. So I think we should merge it. A motivated individual can always submit a PR documenting the alternatives, if that is of interest. |
Thanks! Given how long it's taken to move on this RFC, I don't think we should ask the author to do further revisions of that kind before we merge. |
Sorry, I'm late to the party. I am basically in favor of this RFC, but I do want to note that the intrinsic as defined introduces some forwards-compatibility hazards. In particular, reordering variants can potentially break code even for enums that don't implement That said, I feel good about this fn since it is going to be unsafe and in the documentation we can spell out valid and invalid use cases -- basically, using this on enums that you don't control is a Bad Idea and may introduce hazards if the maintainers of those enums make changes. (On a related note, it is not clear to me whether one should consider Regarding staging, I think we should follow the policy we've been mostly doing for all RFCs. That is, land the new feature unstable, and make an express decision by core team to move it to stable. I of course agree with @sfackler that it'd be nice to make this available to deriving sooner rather than later though. |
This seems like a useful intrinsic but I don't find the motivating example in the RFC to be very convincing - replacing the obvious safe code with an unsafe intrinsic. That is such a simple piece of code surely there must be a way to write it safely that is performant. |
How is the intrinsic unsafe (other than the fact that all intrinsics are tagged as unsafe)? All it does is return a u64. In any case, "surely there must be a better way" is not a very convincing counter-example, IMO. Comparing disciminants seems to me at least like the obvious way to make this kind of function performant. |
I'm in favor of this RFC, and I'm in favor of updating
You mean like using an intrinsic? The intrinsic seems to actually be completely safe to call on all inputs, it just requires The only other approach I can think of is teaching Rust to try to optimize match statements such that it can detect when a bunch of match arms are equivalent to simply comparing the discriminant, but that seems like a lot of work that's only necessary because we aren't allowing ourselves to look at the discriminant, and it's work that would negatively impact compilation time for probably no benefit at all on code outside of As for |
If we have it return a |
Another option would be to have the i64 case shift the discriminant up by 2^63 which would preserve ordering but not the raw value. EDIT: same problem for all signed discriminant types, really. |
Why not just use (sign: i8, abs: u64)? |
Oh, yeah, that'd be a decent way to deal with it. |
Not quite. If sign is negative, then abs has to be u64::MAX - abs to preserve the ordering. |
Given that this RFC includes In the meantime, how about |
On the "alternative ideas" front, if we're ok with having the compiler define traits for us, we could have the #[lang="enumtrait"]
pub trait Enum {
type Repr;
fn discriminant(&self) -> Self::Repr;
} This way we don't have to worry about signed vs unsigned, and code can compare discriminant values correctly. We could also then consider doing things like making this #[lang="enumtrait"]
pub trait Enum {
type Repr;
unsafe fn unsafe_discriminant(&self) -> Self::Repr;
}
#[lang="clikeenumtrait"]
pub trait CLikeEnum: Enum {
fn discriminant(&self) -> Self::Repr;
fn from_discriminant(value: Self::Repr) -> Option<Self>;
} That way All that said, I think having an intrinsic is a good first step. And if we're planning on writing a safe wrapper around it, then it doesn't really matter very much what the actual return type is. |
I'd like to have at least either the |
@glaebhoerl a good point, for some reason it took me a bit to recognize what you were saying, but it seems obvious now. To spell it out: fn foo<T>(x: &T, y: &T) { ... } is able to do a kind of "partial equality" comparison on |
@nikomatsakis Yes, that might be a good fit. :) |
@nikomatsakis at this point I would like to see us move forward on this in some fashion, if only via a feature-gated intrinsic, because that would be very helpful in resolving rust-lang/rust#15523 |
reassigning to @nikomatsakis so that he remembers to update its state (or merge it outright). |
This RFC is provisionally accepted. I've included some unresolved concerns at the end of the RFC. These can be worked out over the course of implementation. Thanks all. |
Implement an intrinsic for extracting the discriminant from a value.
Rendered