Unexpected behaviour of default_value_t when From<&str> impl isn't the inverse of Display impl

[cyqsimon]

Please complete the following tasks

• I have searched the discussions
• I have searched the open and rejected issues

Rust Version

1.66.0

Clap Version

4.0.32

Minimal reproducible code

use clap::Parser;

#[derive(Clone, Debug, Parser)]
struct CliArgs {
    #[arg(long = "arg", default_value_t)]
    arg: Foo,
}

#[derive(Clone, Debug, Default)]
enum Foo {
    #[default]
    A,
    B,
    Custom(String),
}
impl std::fmt::Display for Foo {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        match self {
            Foo::A => write!(f, "Variant A"),
            Foo::B => write!(f, "Variant B"),
            Foo::Custom(s) => write!(f, "Custom: {s}"),
        }
    }
}
impl From<&str> for Foo {
    fn from(value: &str) -> Self {
        match value {
            "A" => Foo::A,
            "B" => Foo::B,
            v => Foo::Custom(v.into()),
        }
    }
}

fn main() {
    let CliArgs { arg } = CliArgs::parse();
    dbg!(arg);
}

Steps to reproduce the bug with the above code

cargo run

Actual Behaviour

arg is Foo::Custom("Variant A").

Expected Behaviour

arg is Foo::A.

---

I think the main issue here is a gap between user expectation and actual implementation. What I expected (and I think what most users would expect) is that when I use default_value_t, Foo::default is directly called to fetch the default value.

However in reality, it seems like clap is doing a "roundtrip" to string and back when default_value_t is used. The default argument goes from Foo to String via Display, then back to Foo via From<&str>.

The problem with this, is that it assumes Foo::from::<&str> is the inverse operation of Foo::to_string, when that's not necessarily the case. In which case strange behaviour like this suddenly pop up.

So in my amateur opinion, either:

• the implementation of default_value_t should be changed to use Foo::default directly (this is the ideal solution, but I don't know whether this will be difficult to implement),
• or the documentation needs to explicitly clarify this requirement that Foo::from::<&str> should be the inverse operation of Foo::to_string

[epage]

With the current architecture, tracking things to handle this isn't ideal. I've updated the documentation.