Add new derive: AsVariant #415
Conversation
Similar to TryUnwrap but generates functions returning an Option instead of a Result.
There was a problem hiding this comment.
This is a feature that I want to see. It looks similar to the unwrap and try_unwrap derives and seems like it implements the principle of least surprise.
In terms of releasing this, would it be possible to release this on a 1.x version instead of waiting for a 2.0 release (if that's going to be delayed)?
I noticed a few minor things that could be improved.
| @@ -53,6 +53,7 @@ default = ["std"] | |||
| add = ["derive_more-impl/add"] | |||
| add_assign = ["derive_more-impl/add_assign"] | |||
| as_ref = ["derive_more-impl/as_ref"] | |||
| as_variant = ["derive_more-impl/as_variant"] | |||
There was a problem hiding this comment.
This is missing from the "full" feature flag below.
| let fn_name = format_ident!( | ||
| "as_{}", | ||
| variant.ident.to_string().to_case(Case::Snake), | ||
| span = variant.ident.span(), | ||
| ); | ||
| let ref_fn_name = format_ident!( | ||
| "as_{}_ref", | ||
| variant.ident.to_string().to_case(Case::Snake), | ||
| span = variant.ident.span(), | ||
| ); | ||
| let mut_fn_name = format_ident!( | ||
| "as_{}_mut", | ||
| variant.ident.to_string().to_case(Case::Snake), | ||
| span = variant.ident.span(), | ||
| ); | ||
| let variant_ident = &variant.ident; |
There was a problem hiding this comment.
| let fn_name = format_ident!( | |
| "as_{}", | |
| variant.ident.to_string().to_case(Case::Snake), | |
| span = variant.ident.span(), | |
| ); | |
| let ref_fn_name = format_ident!( | |
| "as_{}_ref", | |
| variant.ident.to_string().to_case(Case::Snake), | |
| span = variant.ident.span(), | |
| ); | |
| let mut_fn_name = format_ident!( | |
| "as_{}_mut", | |
| variant.ident.to_string().to_case(Case::Snake), | |
| span = variant.ident.span(), | |
| ); | |
| let variant_ident = &variant.ident; | |
| let snake = variant.ident.to_string().to_case(Case::Snake); | |
| let span = variant.ident.span(); | |
| let fn_name = format_ident!("as_{snake}", span = span); | |
| let ref_fn_name = format_ident!("as_{snake}_ref", span = span); | |
| let mut_fn_name = format_ident!("as_{snake}_mut", span = span); | |
| let variant_ident = &variant.ident; |
| assert_eq!(Maybe::<()>::Nothing.as_nothing(), Some(())); | ||
| assert_eq!(Maybe::Just(1).as_just_ref(), Some(&1)); | ||
| assert_eq!(Maybe::Just(42).as_just_mut(), Some(&mut 42)); | ||
|
|
||
| assert_eq!(Maybe::<()>::Nothing.as_just(), None); | ||
| assert_eq!(Maybe::Just(1).as_nothing_ref(), None); | ||
| assert_eq!(Maybe::Just(42).as_nothing_mut(), None); |
There was a problem hiding this comment.
Misses a few cases (and rearranged a little):
| assert_eq!(Maybe::<()>::Nothing.as_nothing(), Some(())); | |
| assert_eq!(Maybe::Just(1).as_just_ref(), Some(&1)); | |
| assert_eq!(Maybe::Just(42).as_just_mut(), Some(&mut 42)); | |
| assert_eq!(Maybe::<()>::Nothing.as_just(), None); | |
| assert_eq!(Maybe::Just(1).as_nothing_ref(), None); | |
| assert_eq!(Maybe::Just(42).as_nothing_mut(), None); | |
| assert_eq!(Maybe::<()>::Nothing.as_nothing(), Some(())); | |
| assert_eq!(Maybe::<()>::Nothing.as_nothing_ref(), Some(())); | |
| assert_eq!(Maybe::<()>::Nothing.as_nothing_mut(), Some(())); | |
| assert_eq!(Maybe::<()>::Nothing.as_just(), None); | |
| assert_eq!(Maybe::<()>::Nothing.as_just_ref(), None); | |
| assert_eq!(Maybe::<()>::Nothing.as_just_mut(), None); | |
| assert_eq!(Maybe::Just(1).as_nothing(), None); | |
| assert_eq!(Maybe::Just(1).as_nothing_ref(), None); | |
| assert_eq!(Maybe::Just(1).as_nothing_mut(), None); | |
| assert_eq!(Maybe::Just(42).as_just(), Some(42)); | |
| assert_eq!(Maybe::Just(42).as_just_ref(), Some(&42)); | |
| assert_eq!(Maybe::Just(42).as_just_mut(), Some(&mut 42)); |
There was a problem hiding this comment.
I feel quite uncomfortable about this feature, because of the following reasons:
-
#[derive(AsVariant)]doesn't add any new functionality to the library. It just fully mirrors the#[derive(TryUnwrap)]with the only exception of usingOptioninstead ofResultin method signatures, which is insignificant, since we always can just add.ok()if we don't want aResult. What is the purpose of it, then? A whole new macro just for another naming? Is this worth it? -
Regarding the naming itself, there are semantic problems. Frankly,
#[derive(TryUnwrap)]doesn't have them. However, in Rust, by best practices and some common ecosystem conventions,as_*,into_*andto_*method prefixes are expected to point to the concrete method signatures:// `ref` to `ref` conversion fn as_something(&self) -> PossibleContainer<&Something>; // `mut ref` to `mut ref` conversion fn as_mut_something(&mut self) -> PossibleContainer<&mut Something>; // `ref` to value conversion fn to_something(&self) -> PossibleContainer<Something>; // infallible value to value conversion fn into_something(&self) -> Something; // fallible value to value conversion fn try_into_something(&self) -> Container<Something>;
And so, seeing
fn as_something(self) -> Option<Something>method just doesn't feel right for me.
I would intuitive expect the#[derive(AsVariant)]macro to only generate the following methods:fn as_something(&self) -> Option<&Something>; fn as_mut_something(&mut self) -> Option<&mut Something>;
Just because
As/as_is expected to be about reference-to-reference semantics only.
Given the above, I'm quite unsure in which direction to go:
- We may reduce the
#[derive(AsVariant)]macro scope to reference-to-reference semantics only, but then, again, it's covered by#[try_unwrap(ref, ref_mut)]already. Should we duplicate the feature, or remove the#[try_unwrap(ref, ref_mut)]from the#[derive(TryUnwrap)]? - We may split
#[derive(TryUnwrap)]into#[derive(AsVariant)]+#[derive(IntoVariant)]which are same but returnOption. Is it worth it? - We may not accept
#[derive(AsVariant)]at all, but folks raise concerns that#[derive(TryUnwrap)]is unintuitive.
@JelteF I would appreciate any thoughts on this from you.
This is a feature that I want to see. It looks similar to the unwrap and try_unwrap derives and seems like it implements the principle of least surprise.
@joshka could you elaborate more on this? Why TryUnwrap doesn't sastisfy your needs?
There are several examples of returning Option from an as_method in the std lib. Click a few types and look for as_ methods. https://doc.rust-lang.org/std/primitive.char.html#method.as_ascii I do however think you might be right though despite that example. https://rust-lang.github.io/api-guidelines/naming.html#c-conv suggests as_ is for Borrowed -> Borrowed. The various cell types have @joshka could you elaborate more on this? Why TryUnwrap doesn't sastisfy your needs? The specific place that I want these is in the events enums in the crossterm events module (link above). Conversion from an enum which isn't the anticipated type has no value to me having any extra information that the Error adds. Option conveys the information succinctly and will better represent the reality of what is happening than an error. The error fundamentally is that this is not the thing you asked for. There's nothing more to add ever. Put another way, there's a very real semantic difference between enums where you're expecting a certain variant at some time (try_unwrap) vs one where you are taking some action only if it is some variant (as_variant). In the places where I want to use this, the latter is closer to what I want the library to convey and not the former.
I don't think it's worth removing TryUnwrap, but it's good to give people reasonable Options (see what I did there ;)). I'm not wedded to a particular as_ naming for that however. If you're curious, perhaps take a look at the various places the existing derives are used https://github.com/search?q=derive_more+tryunwrap&type=code perhaps there's some additional insight in how each of those cases end up calling the derived methods? |
|
I'm fine with this feature being added. I agree with @joshka that there are definitely situations where it's not an error when you cannot cast an enum variant to a specific member. I personally probably wouldn't derive TryUnwrap and AsVariant for the same struct, as indeed you can use one to achieve the other fairly easily. But if a user of derive_more only cares about getting an Option everywhere in their codebase for a specific enum, it's a bit silly that we would only allow them to generate a function that returns a Result, requiring them to add The amount of additional code is also fairly limited. Only the naming really remains then. I think |
The search link above confirms that this is a thing that happens: E.g. (these are many lines from the same file:) assert_that!(record.is_data(), eq(true));
let data = record.try_unwrap_data().unwrap();
let original: String = data.decode().unwrap();
assert_that!(record.is_filtered_gap(), eq(true));
let gap = record.try_unwrap_filtered_gap().unwrap();
assert_that!(gap.to, eq(LogletOffset::new(5)));
let data = record.try_unwrap_data().unwrap();
let gap = record.try_unwrap_filtered_gap().unwrap();
let data = record.try_unwrap_data().unwrap();
let gap = record.try_unwrap_trim_gap().unwrap();
let data = record.try_unwrap_data().unwrap();
let gap = record.try_unwrap_trim_gap().unwrap();This sort of pattern of Or https://github.com/search?q=repo%3Atangramdotdev%2Ftangram%20try_unwrap&type=code };
let object = if let Some(subpath) = &referent.subpath {
let directory = object
.try_unwrap_directory()
.ok()
.ok_or_else(|| tg::error!("expected a directory"))?;
directory.get(&handle, subpath).await?.into()I agree with @tyranron that #[derive(Into)]
enum Foo {
A,
B(Bar),
C(u16, u16),
}
// generates
fn into_a() -> Option<()> {}
fn into_b() -> Option<Bar> {}
fn into_c() -> Option<(u16, u16)> {} |
|
Rust 1.83 that came out today has a couple of stabilized functions |
@JelteF fine, then. If we're okay with naming and the design, let's polish and merge it.
@joshka @JelteF given the above, I would like to propose the following design, which seems the least confusing to me: #[derive(AsVariant)] // `ref` semantics by default (the most common case)
enum Maybe<T> {
Nothing, // units are ignored by default, since has no sense
Just(T),
Pair { a: u8, b: T },
}
// Expands to:
fn as_just(&self) -> Option<&T> {}
fn as_pair(&self) -> Option<(&u8, &T)> {}
// We can tune each variant separately:
#[derive(AsVariant)]
enum Maybe<T> {
#[as_variant(owned, ref_mut)]
Nothing,
#[as_variant(owned, ref)]
Just(T),
#[as_variant(ignore)]
Pair { a: u8, b: T },
}
// Expands to:
fn to_nothing(self) -> Option<()> {}
fn as_mut_nothing(&mut self) -> Option<&mut ()> {}
fn to_just(self) -> Option<T> {}
fn as_just(&self) -> Option<&T> {}
// Or all of them together:
#[derive(AsVariant)]
#[as_variant(owned, ref, ref_mut)]
enum Maybe<T> {
Nothing, // still ignored by default
Just(T),
#[as_variant(owned)]
Pair { a: u8, b: T }, // variant-level stuff takes precedence
}
// Expands to:
fn to_just(self) -> Option<T> {}
fn as_just(&self) -> Option<&T> {}
fn as_mut_just(&mut self) -> Option<&mut T> {}
fn to_pair(self) -> Option<(u8, T)> {}This way, in my opinion, we have reasonable defaults in semantics (without generating likely unnecessary code) and conventional naming by default. While still have the ability to opt-in other behavior if it's required. Regarding the naming, we may additionally allow renaming methods (in a separate PR, though): #[derive(AsVariant)]
#[as_variant(owned = "into_{name}", ref, ref_mut = "as_ref_mut_{name}")]
enum Maybe<T> {
Nothing, // still ignored by default
Just(T),
#[as_variant(owned, ref_mut = "as_{name}_mut")]
Pair { a: u8, b: T }, // variant-level stuff takes precedence
}
// Expands to:
fn into_just(self) -> Option<T> {}
fn as_just(&self) -> Option<&T> {}
fn as_ref_mut_just(&mut self) -> Option<&mut T> {}
fn into_pair(self) -> Option<(u8, T)> {}
fn as_pair_mut(&mut self) -> Option<(&mut u8, &mut T)> {}And, another thought just slipped into my mind: we only need an additional knob to cover #[derive(AsVariant)]
#[as_variant(owned = "into_{name}", ref, ref_mut = "as_ref_mut_{name}")]
#[as_variant(result)] // default is `option`
enum Maybe<T> {
Nothing, // still ignored by default
#[as_variant(option)]
Just(T), // variant-level stuff takes precedence
#[as_variant(owned, ref_mut = "as_{name}_mut")]
Pair { a: u8, b: T }, // variant-level stuff takes precedence
}
// Expands to:
fn into_just(self) -> Option<T> {}
fn as_just(&self) -> Option<&T> {}
fn as_ref_mut_just(&mut self) -> Option<&mut T> {}
fn into_pair(self) -> Result<(u8, T), Error> {}
fn as_pair_mut(&mut self) -> Result<(&mut u8, &mut T), Error> {}Assuming that users would like to have |
|
@tyranron thanks for the detailed example. That's super helpful to get a sense of how this works out. I did some more research, and I agree that in the ecosystem What the default ownership for the derive should be I'm not sure. But given how all the interested parties so far seem to want the owned behaviour, I'd say that that is a good default. If we do that though, and we go with the names I propose above, then maybe we shouldn't call the derive |
|
@JelteF so just to see the whole thing, I'll describe your alterations to the proposed design. Please, correct me if anything is wrong: #[derive(Variant)] // `owned` semantics by default (the most common case)
enum Maybe<T> {
Nothing, // units are ignored by default, since has no sense
Just(T),
Pair { a: u8, b: T },
}
// Expands to:
fn just(self) -> Option<T> {}
fn pair(self) -> Option<(u8, T)> {}
// We can tune each variant separately:
#[derive(Variant)]
enum Maybe<T> {
#[variant(owned, ref_mut)]
Nothing,
#[variant(owned, ref)]
Just(T),
#[variant(ignore)]
Pair { a: u8, b: T },
}
// Expands to:
fn nothing(self) -> Option<()> {}
fn as_mut_nothing(&mut self) -> Option<&mut ()> {}
fn just(self) -> Option<T> {}
fn as_just(&self) -> Option<&T> {}
// Or all of them together:
#[derive(Variant)]
#[variant(owned, ref, ref_mut)]
enum Maybe<T> {
Nothing, // still ignored by default
Just(T),
#[variant(owned)]
Pair { a: u8, b: T }, // variant-level stuff takes precedence
}
// Expands to:
fn just(self) -> Option<T> {}
fn as_just(&self) -> Option<&T> {}
fn as_mut_just(&mut self) -> Option<&mut T> {}
fn pair(self) -> Option<(u8, T)> {}
// With custom renaming:
#[derive(Variant)]
#[variant(owned = "into_{name}", ref, ref_mut = "as_ref_mut_{name}")]
enum Maybe<T> {
Nothing, // still ignored by default
Just(T),
#[variant(owned, ref_mut = "as_{name}_mut")]
Pair { a: u8, b: T }, // variant-level stuff takes precedence
}
// Expands to:
fn into_just(self) -> Option<T> {}
fn as_just(&self) -> Option<&T> {}
fn as_ref_mut_just(&mut self) -> Option<&mut T> {}
fn into_pair(self) -> Option<(u8, T)> {}
fn as_pair_mut(&mut self) -> Option<(&mut u8, &mut T)> {}
// With returning `Result`:
#[derive(Variant)]
#[variant(result)] // default is `option`
enum Maybe<T> {
Nothing, // still ignored by default
#[variant(option)]
Just(T), // variant-level stuff takes precedence
#[variant(owned, ref_mut)]
Pair { a: u8, b: T },
}
// Expands to:
fn just(self) -> Option<T> {}
fn pair(self) -> Result<(u8, T), Error> {}
fn as_mut_pair(&mut self) -> Result<(&mut u8, &mut T), Error> {}And my thoughts about it:
As @joshka pointed out above, just the snake_case version of the variant name is quite often used for variant constructors (e.g.
I agree that's a very often used feature, however, I'm unsure that
Very seductive, however, I do think that people will intuitively look for |
|
TL;DR: suffixes / prefixes should be additional configuration, not there by default. A good way to frame why this is needed is that it implements getters for an enum. Because the enum can be valid as any of the variants, the getters must return QQ: are there existing derive_more attributes where the prefix / suffix are configurable already? The ordering of suffixes in the comment above is generally a bit wrong. Based on idiomatic ordering,
I think not prefixing by default might make more sense than doing this to avoid constructors. It fits better with https://rust-lang.github.io/api-guidelines/naming.html#c-getter
I don't have a good gauge of which would be most common, but rather than looking at it from that perspective, I look at the above and see a lot of complexity in the rules about naming. A much simpler alternative which seems intuitive to me is to make the default // #[variant(owned)
fn just(self) -> Option<T>
fn pair(self) -> Option<(u8, T)>
// #[variant(ref)
fn just_ref(&self) -> Option<&T>
fn pair_ref(&self) -> Option<&(u8, T)>
// #[variant(mut)
fn just_mut(&mut self) -> Option<&mut T>
fn pair_mut(&mut self) -> Option<&mut (u8, T)>This seems much more predictable and intuitve, with their still being the option to enhance this with prefixes if needed, while the opposite of removing the prefixes seems like a chore for when you there is no conflict with constructors.
TryUnwrap still has its place, and removing it would mean that the owned/ref/mut config gets twice as large in the variant case. I think I'd still call this |
Resolves #358
Synopsis
A new derive for generating
as_foo,as_barmethods for an enum with fieldsfooandbar.Solution
Similar to TryUnwrap but generates functions returning an Option instead of a Result.
Checklist