Skip to content
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

Add new derive: AsVariant #415

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

crazymerlyn
Copy link

Resolves #358

Synopsis

A new derive for generating as_foo, as_bar methods for an enum with fields foo and bar.

Solution

Similar to TryUnwrap but generates functions returning an Option instead of a Result.

Checklist

  • Documentation is updated (if required)
  • Tests are added/updated (if required)
  • CHANGELOG entry is added (if required)

Similar to TryUnwrap but generates functions returning an Option instead
of a Result.
Copy link

@joshka joshka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"]
Copy link

@joshka joshka Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing from the "full" feature flag below.

Comment on lines +34 to +49
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;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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;

Comment on lines +76 to +82
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);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Misses a few cases (and rearranged a little):

Suggested change
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));

Copy link
Collaborator

@tyranron tyranron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel quite uncomfortable about this feature, because of the following reasons:

  1. #[derive(AsVariant)] doesn't add any new functionality to the library. It just fully mirrors the #[derive(TryUnwrap)] with the only exception of using Option instead of Result in method signatures, which is insignificant, since we always can just add .ok() if we don't want a Result. What is the purpose of it, then? A whole new macro just for another naming? Is this worth it?

  2. 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_* and to_* 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 return Option. 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?

@tyranron tyranron removed this from the 2.0.0 milestone Nov 27, 2024
@joshka
Copy link

joshka commented Nov 27, 2024

However, in Rust, by best practices and some common ecosystem conventions, as_, into_ and to_* method prefixes are expected to point to the concrete method signatures:

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

pub const fn as_ascii(&self) -> Option<AsciiChar>

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 into_inner methods that return Option<T>. I don't think I've see try_ methods that return Options too often though (I think that was implied by your last example, but I'm not 100% certain).

@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.

Given the above, I'm quite unsure in which direction to go:...

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?

@JelteF
Copy link
Owner

JelteF commented Nov 28, 2024

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 .ok() everywhere.

The amount of additional code is also fairly limited.

Only the naming really remains then. I think as_variant is an acceptable naming scheme. I agree it's not super obvious, but I also don't think it's terrible. And it's nice and short, and fits well with is_variant.

@joshka
Copy link

joshka commented Nov 28, 2024

it's a bit silly that we would only allow them to generate a function that returns a Result, requiring them to add .ok() everywhere.

The search link above confirms that this is a thing that happens:

E.g.
https://github.com/search?q=repo%3Arestatedev%2Frestate%20try_unwrap&type=code

(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 try_unwrap_*().unwrap() is found in a few of the results.

Or https://github.com/search?q=repo%3Atangramdotdev%2Ftangram%20try_unwrap&type=code
Which has many instances of the same pattern:

		};
			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 as_ is probably the wrong name here, but I would like a singular trait that does the conversion to Option<>. Something that could be worth considering as an alternate approach to this is to expand the Into derive so that it generates static methods for enums. That feels like it could be less surprising:
e.g.

#[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)> {}

@joshka
Copy link

joshka commented Nov 29, 2024

Rust 1.83 that came out today has a couple of stabilized functions ControlFlow::break_value() -> Option<B> and ControlFlow::continue_value() -> that are basically this. They're suffixed _value. Result has just ok(() and err(), which might also be instructive on naming. To me this suggests that perhaps entirely dropping the idea of a prefix altogether and instead allow the user to add a suffix that is meaningful to them would be the right choice. This avoids possible conflicts with methods which are used to create variants (e.g. std::io::Error::other(...) is a constructor, but would conflict without some prefix/suffix).

@tyranron
Copy link
Collaborator

tyranron commented Nov 29, 2024

Only the naming really remains then. I think as_variant is an acceptable naming scheme. I agree it's not super obvious, but I also don't think it's terrible. And it's nice and short, and fits well with is_variant.

@JelteF fine, then. If we're okay with naming and the design, let's polish and merge it.


To me this suggests that perhaps entirely dropping the idea of a prefix altogether and instead allow the user to add a suffix that is meaningful to them would be the right choice. This avoids possible conflicts with methods which are used to create variants (e.g. std::io::Error::other(...) is a constructor, but would conflict without some prefix/suffix).

@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 TryUnwrap here too.

#[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 Result much rarer than Option, we could deprecate TryUnwrap altogether, since AsVariant covers it with little additional effort.

@JelteF
Copy link
Owner

JelteF commented Nov 29, 2024

@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 as_variant generally seems to return an Option<&T>. I think for owned (Option<T>) I think the into_variant prefix does not seem super common though. It seems more common to simply use the snake_case version of the variant name, e.g. ok() and err(), but also left() and right().

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 AsVariant, but simply Variant.

@tyranron
Copy link
Collaborator

tyranron commented Nov 29, 2024

@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:

I think the into_variant prefix does not seem super common though. It seems more common to simply use the snake_case version of the variant name, e.g. ok() and err(), but also left() and right().

As @joshka pointed out above, just the snake_case version of the variant name is quite often used for variant constructors (e.g. fn just(value: impl Into<u8>) -> Maybe<u8>) and I would like to avoid such collisions by default. I agree that into_variant prefix is not super common, and because into naming quite collides with Into trait, I often expect there infallible conversion, which is not the case. That's why I proposed the default naming as to_variant for owned conversions.

But given how all the interested parties so far seem to want the owned behaviour, I'd say that that is a good default.

I agree that's a very often used feature, however, I'm unsure that owned case is more common than ref case. At least in my codebases, I see the tendency to have ref case more often.

If we do that though, and we go with the names I propose above, then maybe we shouldn't call the derive AsVariant, but simply Variant.

Very seductive, however, I do think that people will intuitively look for AsVariant (the created issue is a somewhat proof), and Variant may be not that obvious from its name for unfamiliar users.

@joshka
Copy link

joshka commented Nov 30, 2024

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 Option<T>.

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, _ref / _mut should generally be at the end not in the middle of the function name. Background:

As @joshka pointed out above, just the snake_case version of the variant name is quite often used for variant constructors (e.g. fn just(value: impl Into) -> Maybe) and I would like to avoid such collisions by default. I agree that into_variant prefix is not super common, and because into naming quite collides with Into trait, I often expect there infallible conversion, which is not the case. That's why I proposed the default naming as to_variant for owned conversions.

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 agree that's a very often used feature, however, I'm unsure that owned case is more common than ref case. At least in my codebases, I see the tendency to have ref case more often.

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 owned (same default as TryInto) be unprefixed, and add additional ref and mut options which affect the suffix giving:

// #[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.

Assuming that users would like to have Result much rarer than Option, we could deprecate TryUnwrap altogether, since AsVariant covers it with little additional effort.

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 AsVariant, ToVariant,GetVariant, or even Getters even though it's not producing as_ / to_ methods (I'd lean towards ToVariant or GetVariant). This naming seems to communicate better what the derive macro will do when casually reading code more-so than Variant. The methods produced really are just getters that avoid panics / errors. This is a much weaker preference than the ones above though (perhaps this is 60% sure rather than 90%?).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AsVariant for enum
4 participants