ArgValue derive #2762

ModProg · 2021-09-09T12:52:42Z

Support ArgValue in ArgEnum derive macro.

merge after: #2758

fully closes: #2756
fully closes: #2731

src/derive.rs

epage · 2021-09-16T16:09:11Z

Bleh, the pain of sharing code between all macros. alias is a method for Args but is purely magic for ArgEnum. Its ugly but we could add a Attrs::arg_enum_methods function that filters out alias.

ModProg · 2021-09-18T12:21:41Z

@epage Or add a parameter filter_neg:[&str] to methods.
I implemented this now using a new method: field_methods_filtered.

I would like some feedback on changes required for a merge. As IMO all features are implemented now.

epage · 2021-09-19T00:21:19Z

Honestly, I've only minimally been looking at the derive, hoping for the builder to get in soon, so that the diff is easier to browse. If this PR is limited to only the last commit (unsure if it is), that would make it easy.

I wish Github did a better job of handling stacked PRs.

pksunkara

I don't see test cases in derive for hidden and name and about. Let's rebase this PR once the other is merged

ModProg · 2021-09-19T12:05:33Z

This is rebased now, I'll try to look into adding tests later

epage

There are several open questions on this PR, including one that re-evaluates an already existing API. At least for me, I can see us resoling some of these in this PR but splitting some others out into clap3 Issues so the remaining design work does not block getting this in, allowing you to use it, and allowing us to start collecting feedback.

I'm also good with owning that remaining design work if you are limited on time. I don't want these details to stall out what will be a great feature!

clap_derive/src/attrs.rs

clap_derive/src/derives/arg_enum.rs

epage · 2021-09-20T20:32:36Z

clap_derive/src/derives/arg_enum.rs

+    let lit = lits
+        .iter()
+        .map(|(name, _, attrs)| {
+            let fields = attrs.field_methods_filtered(&["alias"]);


There is an inconsistency between Arg and ArgValue that I would consider a bug. Arg::alias is hidden by default and requires Arg::visible_alias. Before, we only had visible alias for ArgValue but with this, we can now handle hidden aliases and so alias should be hidden. We can add support for visible_alias but it doesn't need to be in this PR.

makes sense, and then that would mean we probably should move it to ArgValue. (it would be possible without, but with the consistency point as well makes more sense to change it.)

I'll probably have a look into it in the next days. Thanks for all the help on these PRs.

Thanks for your patience and diligence in following through! Too many times we, as a community, lose valuable contributions because people don't have the time to polish

I'll implemnt only alias for now which will be hidden in completions etc.

src/build/arg/arg_value.rs

src/derive.rs

epage · 2021-09-27T13:47:52Z

clap_derive/src/derives/arg_enum.rs

        fn from_str(input: &str, case_insensitive: bool) -> ::std::result::Result<Self, String> {
-            let func = if case_insensitive {
-                ::std::ascii::AsciiExt::eq_ignore_ascii_case
-            } else {
-                str::eq
-            };
-
            match input {
-                #(val if func(val, #lit) => Ok(Self::#variant),)*
+                #(val if #lit.matches(val, case_insensitive) => Ok(Self::#variant),)*
                e => Err(format!("Invalid variant: {}", e)),
            }
        }


Can and should we make from_str an inherent method? It looks like we could loop over Self::variants() and I don't think it would make much of a difference, performance wise.

We could, but the ArgValue has no idea of the enum variant. Or what did you mean?
We could make variants return a Vec<(ArgValue, ArgEnum)>.

You're right. For this to work, we'd need an iterator over the variants that we could then call as_arg on.

On one hand, it adds a function to the trait that is only used for implementing the inherent method. On the other hand, it simplifies the trait for people manually implementing it (not common but possible, I have a PR doing just that)

Overall I can go either way

src/build/arg/arg_value.rs

ModProg · 2021-09-27T19:31:46Z

I will resolve all the documentation in this PR when the methods are stablelized

epage · 2021-09-27T20:16:15Z

src/derive.rs

-    /// All possible argument choices, in display order.
-    const VARIANTS: &'static [&'static str];
+    /// All possible argument values, in display order.
+    fn arg_values() -> Vec<ArgValue<'static>>;


Sorry, I missed this Vec on my last pass. This is requiring an allocation for possible values. It is only once and not much data is being copied, so its not the worst, but it'd still be nice to avoid.

Can't use [] because a size is needed.

I know &[] is annoying but it seems like it'd give best results and the annoying part is limited to one piece of code we generate.

This can only be done through a constant again, otherwise we are returning temporarry values.
But alias relies on a Vec which means the Iterator cannot be coppied to avoid implementing From<&ArgValue>.
And also not const:

error[E0658]: mutable references are not allowed in constant functions --> src/build/arg/arg_value.rs:149:9 | 149 | self.aliases.push(name); | ^^^^^^^^^^^^ | = note: see issue #57349 <https://github.com/rust-lang/rust/issues/57349> for more information error[E0015]: calls in constant functions are limited to constant functions, tuple structs and tuple variants --> src/build/arg/arg_value.rs:149:9 | 149 | self.aliases.push(name); | ^^^^^^^^^^^^^^^^^^^^^^^ error[E0493]: destructors cannot be evaluated at compile-time --> src/build/arg/arg_value.rs:148:24 | 148 | pub const fn alias(mut self, name: &'help str) -> Self { | ^^^^^^^^ constant functions cannot evaluate destructors ... 151 | } | - value is dropped here ```

maybe there is a way I am not seeing, I would also like to avoid Vec here.

Thanks for the reminder about alias.

The only workaround I can come up with is if we instead provide the value variants (function or constant). The user still pays for the cost for aliases, if they use them, but no other allocations would be made. This also opens us up to providing an inherent method for from_str.

What are your thoughts?

I'm not understanding what your proposing.
could you write down the signatures you are proposing?

This will have problems with & vs no &:

error[E0308]: mismatched types --> src/derive.rs:290:9 | 284 | / pub trait ArgEnum: Sized + 'static { 285 | | /// All possible argument values, in display order. 286 | | fn value_variants() -> &'static [Self]; 287 | | 288 | | /// Parse an argument into `Self`. 289 | | fn from_str(input: &str, case_insensitive: bool) -> Result<Self, String> { | | -------------------- expected `std::result::Result<Self, String>` because of return type 290 | /| Self::value_variants() 291 | || .into_iter() 292 | || .find(|v| v.arg_value().unwrap().matches(input, case_insensitive)) 293 | || .ok_or_else(|| format!("Invalid variant: {}", input)) | ||_________________________________________________________________^ expected type parameter `Self`, found `&Self` ... | 299 | | fn arg_value(&self) -> Option<ArgValue<'static>>; 300 | | } | |_- this type parameter | = note: expected enum `std::result::Result<Self, _>` found enum `std::result::Result<&Self, _>` For more information about this error, try `rustc --explain E0308`.

We either need to call cloned/copied or return Result<&Self>

If we do this, I lean towards Result<&Self>. Its ok for the proc macro to assume copy/clone and it is probably fine for the trait to make Copy or Clone a super trait, but I'm concerned with what people might put in skipped variants. We'd want to at most do Clone and not Copy so people can have something like a String in it. Returning &Self though gives us full flexibility at the cost of ergonomics.

So the question is ergonomics (Result<Self>) is worth the cost of flexibility (requiring Clone).

I feel like I'm running against a brick wall with this trait definition and lifetimes etc. maybe I'll come back on your ofter to speak on Discord

I'd recommend getting this into a working state, even if not ideal, and we merge it as-is. We can then play with it and meet up on platform-of-choice and discuss trade offs.

Alright I have pushed a solution using a slice but requiring Clone.

ModProg · 2021-09-29T16:43:22Z

If there are no changes required for the new ArgValue methods, I'd write documentation for them now.

Concerning the ArgEnum trait, I'm not sure if requiring Clone is the best solution, but returning a Reference is probably even more nuisance to the user as they can then only have structs containing references to ArgEnums.

The only solution here would be to go back to deriving from_str using, we can maybe use a lit here omitting hidden and about:

#(val if #lit.matches(val, case_insensitive) => Ok(Self::#variant),)*

epage · 2021-09-29T18:23:46Z

Looks great! Go ahead and fill out the docs.

epage

Thanks for putting in all of this effort to make this top notch!

ModProg · 2021-09-29T19:34:23Z

Thanks for putting in all of this effort to make this top notch!

Thanks, the only thing I'm not the biggest fan of is the Clone requirenment. I hope we can find something there to lift that.

pksunkara

Hmm.. I think we can fix it such that we don't need Clone. Unless I am missing something fundamental? Don't worry about doing that now though since you have had issues getting that to work. We can always do it later.

Just a few changes needed.

pksunkara · 2021-09-30T03:06:59Z

tests/possible_values.rs

+        .try_get_matches_from(vec!["pv", "--option", "123"]);
+
+    assert!(m.is_ok());
+    assert!(m.unwrap().value_of("option").unwrap().eq("123"));


Not sure why this and the below tests are needed. Is it for documenting that the aliases wouldn't change the given value?

pksunkara · 2021-09-30T03:46:01Z

clap_derive/src/dummies.rs

            fn from_str(_input: &str, _case_insensitive: bool) -> Result<Self, String> {
                unimplemented!()
            }
-            fn as_arg(&self) -> Option<&'static str> {
+            fn arg_value<'a>(&self) -> Option<clap::ArgValue<'a>>{


WDYT about naming this as_arg_value?

The call might allocate (for aliases) and so as_ wouldn't work, though to_ would. I can go either way.

I renamed it to to_arg_value now

pksunkara · 2021-09-30T03:47:16Z

src/derive.rs

+    fn from_str(input: &str, case_insensitive: bool) -> Result<Self, String> {
+        Self::value_variants()
+            .iter()
+            .find(|v| v.arg_value().unwrap().matches(input, case_insensitive))


Bad unwrap? Should be handled correctly

The unwrap is correct. value_variants should only return variants that have an associated ArgValue, so arg_value will always return Some(_) in this call.

The .unwrap() could be replaced by an except(). I can go either way; my general rule of thumb has been how close and obvious are the two sides of the contract. The less obvious, the more clear you need to be. I would have assumed this would have been obvious but I just got proven wrong.

pksunkara · 2021-09-30T03:48:15Z

clap_derive/src/derives/arg_enum.rs

-    let from_str = gen_from_str(&lits);
-    let as_arg = gen_as_arg(&lits);
+    let arg_values = gen_arg_values(&lits);
+    let arg_value = gen_arg_value(&lits);


Suggested change

let arg_value = gen_arg_value(&lits);

let as_arg_value = gen_as_arg_value(&lits);

Once the trait method is renamed

pksunkara · 2021-09-30T03:54:51Z

src/derive.rs

-    const VARIANTS: &'static [&'static str];
+pub trait ArgEnum: Sized + Clone {
+    /// All possible argument values, in display order.
+    fn value_variants<'a>() -> &'a [Self];


@epage With the recent const improvements, do you think we change the return to be [Self; T] where T is the number of variants?

I don't think so, const generics are very limited.

I gave it a try on the playground and I didn't see a way to make it work.
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=75afb67dc29136f6341e6ea4f044fea9

Could actually be possible, I tried that to replace the variants slice befor, the issue I had, was I didn't find all the places where I needed to specify the generic. But I can try again, maybe you get more out of the error messages then me.

This worked though. https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=8ccbfd753fb15dab675ba0491c7f48a3

Yes, but that severally limits how the trait can work because all calls to it have to be generic over the lenth.

I don't see many calls at all. Am I missing something?

this would not solve the Clone requirenment, well maybe in 2021 but not yet if I understand this correctly:

this method call resolves to `<&[T; N] as IntoIterator>::into_iter` (due to backwards compatibility), but will resolve to <[T; N] as IntoIterator>::into_iter in Rust 2021. `#[warn(array_into_iter)]` on by default this changes meaning in Rust 2021 for more information, see issue #66145 <https://github.com/rust-lang/rust/issues/66145> ```

@ModProg, depending on what the code looks like, you can force it to use the correct IntoIterator without the new edition

@pksunkara Yes, there aren't many calls but we have to weigh out the API design for which is less likely to break us. A Clone is not a heavy requirement to put on these types. A per implementation const generic severely limits how the trait can be used.

However, as I said before, I think resolving of this needs to be split out into its own issue and that we should merge this as-is. As we've seen through these discussions, there is a lot of nuance to the implementation for any proposed solution, so rather than continue having a lot of back-and-forth in these PRs, we should merge this as-is rather than block it on incremental improvements and allow someone to focus on both the API design and implementation to reduce back and forths on this.

ModProg · 2021-09-30T09:48:26Z

The clone requirement can be dropped if instead of using the slice of Self we generate the returned Self value inside the from_str method. So it is a new one from scratch.

pksunkara · 2021-10-01T13:57:38Z

Agree but #2762 (comment) and #2762 (comment) hasn't been addressed. @ModProg Can you please make a follow up PR for them?

epage · 2021-10-01T15:22:54Z

Agree but #2762 (comment) and #2762 (comment) hasn't been addressed. @ModProg Can you please make a follow up PR for them?

Sorry, I had assumed my responses resolved it as you were responding to one my my comments but not the others.

pksunkara · 2021-10-01T15:58:06Z

Your responses proposed another way of addressing them with to_ and expect which I agreed with and was expecting @ModProg to fix them, which is why I didn't reply in there.

epage · 2021-10-01T16:18:19Z

In each one, I said both the original and the idea I mentioned were ok.

ModProg mentioned this pull request Sep 9, 2021

ArgValue builder #2758

Merged

ModProg force-pushed the ArgValue-derive branch from 0ecd8fa to 25e35a3 Compare September 9, 2021 13:06

epage reviewed Sep 9, 2021

View reviewed changes

src/derive.rs Outdated Show resolved Hide resolved

ModProg force-pushed the ArgValue-derive branch 3 times, most recently from 0877e5a to 2d4abfa Compare September 12, 2021 23:50

This comment has been minimized.

Sign in to view

ModProg force-pushed the ArgValue-derive branch from 51c1823 to f2a0161 Compare September 18, 2021 12:14

ModProg force-pushed the ArgValue-derive branch from f2a0161 to 90f0c2d Compare September 18, 2021 12:45

pksunkara requested changes Sep 19, 2021

View reviewed changes

ModProg force-pushed the ArgValue-derive branch from 90f0c2d to e81845f Compare September 19, 2021 12:04

epage requested changes Sep 20, 2021

View reviewed changes

clap_derive/src/attrs.rs Outdated Show resolved Hide resolved

clap_derive/src/attrs.rs Outdated Show resolved Hide resolved

clap_derive/src/derives/arg_enum.rs Outdated Show resolved Hide resolved

epage reviewed Sep 20, 2021

View reviewed changes

ModProg added 2 commits September 26, 2021 16:32

feat(derive:arg_enum): use ArgValue in ArgEnum trait

b589a6c

Working Implementation

88d7d02

ModProg force-pushed the ArgValue-derive branch from e81845f to 88d7d02 Compare September 26, 2021 14:33

ModProg added 3 commits September 26, 2021 22:46

add hidden aliases to ArgValue

d3f0534

move alias implementation to ArgValue for derive

f002cdc

small changes

76f7211

epage requested changes Sep 27, 2021

View reviewed changes

address epage's remarks

ac1a9d6

epage reviewed Sep 27, 2021

View reviewed changes

ArgEnum: Slice instead of array, from_str in ArgEnum implemented

480035a

fix dummies

15fcf81

ModProg force-pushed the ArgValue-derive branch from 1229bb3 to 15fcf81 Compare September 29, 2021 17:05

fix ui

d53778e

documentation for ArgValue

47ff3ed

epage approved these changes Sep 29, 2021

View reviewed changes

epage requested a review from pksunkara September 29, 2021 19:21

pksunkara requested changes Sep 30, 2021

View reviewed changes

pksunkara reviewed Sep 30, 2021

View reviewed changes

epage merged commit 3b2e3ff into clap-rs:master Oct 1, 2021

epage mentioned this pull request Oct 1, 2021

Can ArgEnum be both more general and performant? #2799

Closed

epage mentioned this pull request Oct 1, 2021

Rename ArgEnum::arg_value to to_arg_value #2800

Merged

epage mentioned this pull request Dec 6, 2021

Hidden possible values #3061

Closed

2 tasks

	let arg_value = gen_arg_value(&lits);
	let as_arg_value = gen_as_arg_value(&lits);

ArgValue derive #2762

ArgValue derive #2762

Conversation

ModProg commented Sep 9, 2021 • edited Loading

This comment has been minimized.

epage commented Sep 16, 2021

This comment has been minimized.

ModProg commented Sep 18, 2021

epage commented Sep 19, 2021

pksunkara left a comment

Choose a reason for hiding this comment

ModProg commented Sep 19, 2021

epage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ModProg commented Sep 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ModProg commented Sep 29, 2021

epage commented Sep 29, 2021

epage left a comment

Choose a reason for hiding this comment

ModProg commented Sep 29, 2021

pksunkara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epage Sep 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epage Sep 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ModProg commented Sep 30, 2021

pksunkara commented Oct 1, 2021

epage commented Oct 1, 2021

pksunkara commented Oct 1, 2021 • edited Loading

epage commented Oct 1, 2021

ModProg commented Sep 9, 2021 •

edited

Loading

epage Sep 30, 2021 •

edited

Loading

epage Sep 30, 2021 •

edited

Loading

pksunkara commented Oct 1, 2021 •

edited

Loading