clap
clap copied to clipboard
With derive(ValueEnum), the rename_all attribute is silently ignored when the value is not a string literal
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");
}
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.
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.
@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 `=`"
},
}
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.
Shouldn't we add a check for v3 since not everyone will update to v4 immediately.
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.