With derive(ValueEnum), the rename_all attribute is silently ignored when the value is not a string literal

[jturner314-nrl]

Please complete the following tasks

• [X] I have searched the discussions
• [X] I have searched the open and rejected issues

Rust Version

rustc 1.62.0 (a8314ef7d 2022-06-27)

Clap Version

3.2.8

Minimal reproducible code

use clap::ValueEnum;

#[derive(Clone, Debug, ValueEnum)]
#[clap(rename_all = verbatim)]
pub enum Enum {
    Var1,
    Var2,
}

fn main() {
    assert_eq!(
        vec!["Var1", "Var2"],
        Enum::value_variants()
            .iter()
            .map(|var| var.to_possible_value().unwrap().get_name())
            .collect::<Vec<_>>()
    );
    println!("hi");
}

(Rust Playground)

Steps to reproduce the bug with the above code

cargo run

Actual Behaviour

The #[clap(rename_all = verbatim)] attribute is silently ignored, so clap uses the default kebab-case instead, and the program panics:

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `["Var1", "Var2"]`,
 right: `["var1", "var2"]`', src/main.rs:11:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Expected Behaviour

There should be a compilation error/warning to notify the user that clap doesn't understand the attribute and is ignoring it. In general, clap should warn the user about anything in #[clap(...)] which it doesn't understand.

Additional Context

The user can fix the issue in this particular example by replacing verbatim with "verbatim", but:

• The user may not even realize that the attribute is ignored until they try running the program and using the enum.
• It's not obvious what the problem is, since there's no warning/error. Additionally, the derive reference doesn't make it very obvious that quotes are needed, aside from saying that the right side of the = is an <expr>. (I'm about to create a PR to clarify the reference in this area.)

Debug Output

Adding the debug feature has no effect.

[epage]

So rename_all only accepts literals. Otherwise, we turn it into a method call. For ValueEnum, we apparently don't call the methods, causing this to fail silently.

Always a weird one on when to consider a bug a breaking change.

[anshulrgoyal]

@epage Adding something like this should work for compilation error for renam_all.

 match input.parse::<Expr>() {
                    Ok(expr) => match &*name_str {
                        "skip" => Ok(Skip(name, Some(expr))),
                        "default_value_t" => Ok(DefaultValueT(name, Some(expr))),
                        "default_values_t" => Ok(DefaultValuesT(name, expr)),
                        "default_value_os_t" => Ok(DefaultValueOsT(name, Some(expr))),
                        "default_values_os_t" => Ok(DefaultValuesOsT(name, expr)),
                        "next_display_order" => Ok(NextDisplayOrder(name, expr)),
                        "next_help_heading" => Ok(NextHelpHeading(name, expr)),
                        "help_heading" => Ok(HelpHeading(name, expr)),
                        "rename_all" => abort!(assign_token,"expected `string literal` for rename_all"),
                        _ => Ok(NameExpr(name, expr)),
                    },

                    Err(_) => abort! {
                        assign_token,
                        "expected `string literal` or `expression` after `=`"
                    },
                }

[epage]

Forgot about this issue; it was fixed along with some clap v4 changes

• https://github.com/clap-rs/clap/blob/master/clap_derive/src/item.rs#L790 to ensure we only accept string literals
• https://github.com/clap-rs/clap/blob/master/clap_derive/src/item.rs#L106 to ensure #[value(...)] attributes aren't accepted on the enum itself.

[anshulrgoyal]

Shouldn't we add a check for v3 since not everyone will update to v4 immediately.

[epage]

As we are on the cusp of the v4 release, v3 is going into maintenance mode. At the moment, the policy will be only trivial backports. This is not a trivial back port but would require a re-implementation, even if trivial. Besides reducing maintenance work, this also helps lower risk for clap 3 users. For example, I already brought up in this thread that I was unsure whether to consider this a breaking change because we'd be taking a working case to a failing case.