Use clap::ArgEnum to auto-generate possible cli arguments from Rust enums #10369

petr-tik · 2022-02-07T21:03:55Z

What does this PR try to resolve?

I was familiarising myself with the codebase after the upgrade to clap-3.0 PR #10265 and
spotted a couple of low-hanging refactors. Vectors of strings were duplicated as cli representations
of enums. Those strings can be exhaustively auto-generated from the enum variants.

Tangentially related to #6104, since this automates the need to manually keep the
vectors of static strings up to date with the possible values of ConfigFormat and tree::Prefix.

How should we test and review this PR?

The PR is split into 2 commits - one to set up the whole cargo workspace with
clap + derive feature and refactor ConfigFormat. The second follows up with a
refactor to use clap derive ArgEnum for tree::Prefix.
Feel free to suggest tests, if you feel this is too untested as is.

Additional information

Confirm that the derive feature of clap is acceptable as a dependency addition

rust-highfive · 2022-02-07T21:03:58Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

Please see the contribution instructions for more information.

Eh2406 · 2022-02-07T22:00:21Z

cc @epage

epage · 2022-02-07T22:22:09Z

src/bin/cargo/commands/config.rs

@@ -34,7 +34,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
        Some(("get", args)) => {
            let opts = cargo_config::GetOptions {
                key: args.value_of("key"),
-                format: args.value_of("format").unwrap().parse()?,
+                format: args.value_of_t_or_exit("format"),


We shouldn't be short-circuiting cargo's exiting. We'll change expected exit codes, if nothing else.

Thanks for the swift review. can I use value_of_t("format")? instead?

ta! will change to that

epage · 2022-02-07T22:22:38Z

src/bin/cargo/commands/tree.rs

    } else {
-        args.value_of("prefix").unwrap()
+        args.value_of_t_or_exit("prefix")


epage · 2022-02-07T22:24:15Z

src/cargo/ops/cargo_config.rs

+impl AsRef<str> for ConfigFormat {
+    fn as_ref(&self) -> &str {
+        match self {
+            ConfigFormat::Toml => "toml",
+            ConfigFormat::Json => "json",
+            ConfigFormat::JsonValue => "json-value",
+        }
+    }
 }


I think you can use PossibleValue to help with this, like we do Display in clap_complete::Shell.

impl Display for Shell { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { self.to_possible_value() .expect("no values are skipped") .get_name() .fmt(f) } }

for my education, what's the difference between using a PossibleValue vs creating a match arm that returns static strings? Are both of them exhaustive checks?

This single-sources the values so you don't have to worry about typos.

To confirm, should I replace the derived clap::ArgEnum implementation with my own impl that has to_possible_value() and value_variants() implementations?

error[E0119]: conflicting implementations of trait `clap::ArgEnum` for type `ops::cargo_config::ConfigFormat` --> src/cargo/ops/cargo_config.rs:13:10 | 13 | #[derive(clap::ArgEnum, Clone)] | ^^^^^^^^^^^^^ conflicting implementation for `ops::cargo_config::ConfigFormat` ... 28 | impl ArgEnum for ConfigFormat { | ----------------------------- first implementation here

No, you can keep deriving, just use the functions from the trait to get the generated values. Shell doesn't derive to avoid foisting the derive feature on people not using it.

thanks for the tip and sorry if I am being daft here.

self.to_possible_value() .expect("no values are skipped") .get_name()

this looks like a runtime check that might throw at runtime, while the current implementation is an exhaustive compile-time check. Should I still use the to_possible_value().expect("Should be fine") and if so, what's the safest way to do that?

to_possible_value only "fails" if you have a #[clap(skip)] field. None of them do which is why an expect is ok here to act as an assert (the expect is documenting the invariant that it is relying on).

This is better because the other approach would have required tests for each field to make sure there were no typos. People adding new fields would need to remember to update those tests.

epage · 2022-02-07T22:24:31Z

src/cargo/ops/tree/mod.rs

+impl AsRef<str> for Prefix {
+    fn as_ref(&self) -> &str {
+        match self {
+            Prefix::None => "none",
+            Prefix::Indent => "indent",
+            Prefix::Depth => "depth",
+        }
+    }
+}
+


src/cargo/ops/cargo_config.rs

epage

Looks good!

bors · 2022-02-17T16:32:29Z

☔ The latest upstream changes (presumably #10396) made this pull request unmergeable. Please resolve the merge conflicts.

Use the derive feature of clap to stop relying on hardcoded vector of strings and programmatically derive config values from the enum type

Stop returning static &str and use the clap-required possible_values trait method

early bailing and messing up return code

delegate &str creation to the to_possible_value() method provided by clap::ArgEnum. Since both arguments are non-skipped, we can safely use expect()

petr-tik · 2022-02-17T22:47:57Z

@epage @ehuss Rebased and dealt with the merge conflict reported by bors.

Can you please review if you get the chance.

epage

Thanks for doing this!

ehuss · 2022-02-20T16:01:49Z

This adds several new dependencies, and adds some more boilerplate code. Is it possible to do this without the derive macros? Are the AsRef impls necessary? Is there some way this can be trimmed down a little?

petr-tik · 2022-02-20T16:31:32Z

Thanks for reviewing!

Is there some way this can be trimmed down a little?

Would be happy to, what specifically do you want me to trim down?

Is it possible to do this without the derive macros?

From the documentation for ArgEnum, using the trait requires the derive feature
https://docs.rs/clap/latest/clap/trait.ArgEnum.html

The AsRef implementations are needed for the value_variants trait method.

I appreciate this might increase compile-times. It brings the benefits of automatically maintaining and generating lists of possible ConfigFormats and Prefix types by delegating to clap-based derives instead of keeping those updated manually.

epage · 2022-02-21T14:05:57Z

Is it possible to do this without the derive macros?

Since cargo is using the builder API, the main value ArgEnum is providing is automatically generating the to/from string conversions. If cargo were using the derive API, then we wouldn't need the AsRef, Display, and FromStr boiler plate. We'd just have

/// Display format
#[long, arg_enum, default_value_t = cargo_config::ConfigFormat::Toml]
format: cargo_config::ConfigFormat

Are the AsRef impls necessary? Is there some way this can be trimmed down a little?

Clap accepts &str and stores them, so the easiest strings to work with are &'static str (we plan to fix this in clap 4), so we can't use the existing Display which is why we have the AsRef for using cargo_config::ConfigFormat to set the default value rather than hand-writing the default as a string.

epage · 2022-02-21T14:08:30Z

src/cargo/ops/cargo_config.rs

+        ConfigFormat::value_variants()
+            .iter()
+            .filter_map(ArgEnum::to_possible_value)
+    }
 }

 impl FromStr for ConfigFormat {


If we move forward with this, we should switch the FromStrs to be implemented in terms of ArgEnum as well

For example, clap_completes Shell:

impl FromStr for Shell { type Err = String; fn from_str(s: &str) -> Result<Self, Self::Err> { for variant in Self::value_variants() { if variant.to_possible_value().expect("No value is skipped").matches(s, false) { return Ok(*variant); } } Err(format!("Invalid variant: {}", s)) } }

ehuss · 2022-03-23T01:07:10Z

Closing as I would prefer to avoid pulling in several new dependencies for a relatively minor improvement (and it also seems to add quite a bit more lines of code and subjectively seems more complex). Thank you for the PR, though!

rust-highfive assigned ehuss Feb 7, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 7, 2022

petr-tik force-pushed the arguments_values_from_enum branch from 83ce9af to 87e28eb Compare February 7, 2022 21:28

epage reviewed Feb 7, 2022

View reviewed changes

src/bin/cargo/commands/tree.rs Outdated

} else {

args.value_of("prefix").unwrap()

args.value_of_t_or_exit("prefix")

Copy link

Contributor

epage Feb 7, 2022

Choose a reason for hiding this comment

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

ditto

epage reviewed Feb 7, 2022

View reviewed changes

epage reviewed Feb 8, 2022

View reviewed changes

src/cargo/ops/cargo_config.rs Outdated Show resolved Hide resolved

epage approved these changes Feb 8, 2022

View reviewed changes

petr-tik added 5 commits February 17, 2022 21:54

Use clap-generated possible values in ConfigFormat

30bd2d8

Use the derive feature of clap to stop relying on hardcoded vector of strings and programmatically derive config values from the enum type

Replace static list of strings with clap::ArgEnum

ec80cda

Stop returning static &str and use the clap-required possible_values trait method

Use a Result-returning clap method to prevent

672dcde

early bailing and messing up return code

Prevent typos and the need for tests

5b1a975

delegate &str creation to the to_possible_value() method provided by clap::ArgEnum. Since both arguments are non-skipped, we can safely use expect()

Derive the same traits in the same order types

8879d98

petr-tik force-pushed the arguments_values_from_enum branch from 6822cdd to 8879d98 Compare February 17, 2022 21:57

epage approved these changes Feb 17, 2022

View reviewed changes

epage reviewed Feb 21, 2022

View reviewed changes

ehuss closed this Mar 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use clap::ArgEnum to auto-generate possible cli arguments from Rust enums #10369

Use clap::ArgEnum to auto-generate possible cli arguments from Rust enums #10369

petr-tik commented Feb 7, 2022

rust-highfive commented Feb 7, 2022

Eh2406 commented Feb 7, 2022

epage Feb 7, 2022

petr-tik Feb 7, 2022

epage Feb 7, 2022

petr-tik Feb 8, 2022

epage Feb 7, 2022

epage Feb 7, 2022

petr-tik Feb 7, 2022

epage Feb 7, 2022

petr-tik Feb 8, 2022

epage Feb 8, 2022

petr-tik Feb 8, 2022

epage Feb 8, 2022 •

edited

epage Feb 7, 2022

epage left a comment

bors commented Feb 17, 2022

petr-tik commented Feb 17, 2022

epage left a comment

ehuss commented Feb 20, 2022

petr-tik commented Feb 20, 2022

epage commented Feb 21, 2022

epage Feb 21, 2022

ehuss commented Mar 23, 2022

Use clap::ArgEnum to auto-generate possible cli arguments from Rust enums #10369

Use clap::ArgEnum to auto-generate possible cli arguments from Rust enums #10369

Conversation

petr-tik commented Feb 7, 2022

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rust-highfive commented Feb 7, 2022

Eh2406 commented Feb 7, 2022

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

Choose a reason for hiding this comment

epage Feb 8, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epage left a comment

Choose a reason for hiding this comment

bors commented Feb 17, 2022

petr-tik commented Feb 17, 2022

epage left a comment

Choose a reason for hiding this comment

ehuss commented Feb 20, 2022

petr-tik commented Feb 20, 2022

epage commented Feb 21, 2022

Choose a reason for hiding this comment

ehuss commented Mar 23, 2022

epage Feb 8, 2022 •

edited