Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Derive: specifying invalid struct field type fails with Epic error message #4994

Closed
2 tasks done
LunarLambda opened this issue Jul 5, 2023 · 6 comments
Closed
2 tasks done
Labels
C-bug Category: Updating dependencies

Comments

@LunarLambda
Copy link

Please complete the following tasks

Rust Version

1.70

Clap Version

4.3.10

Minimal reproducible code

use clap::Parser;

#[derive(Parser)]
struct Cli {
    #[arg(short = 'C')]
    foo: Option<Box<std::path::Path>>
}

fn main() {
    let cli = Cli::parse();
}

Steps to reproduce the bug with the above code

cargo check

Actual Behaviour

The user is presented with a truly threatening error message:

error[E0599]: the method `value_parser` exists for reference `&&&&&&_AutoValueParser<Box<Path>>`, but its trait bounds were not satisfied
    --> src/main.rs:5:5
     |
5    |       #[arg(short = 'C')]
     |       ^ method cannot be called on `&&&&&&_AutoValueParser<Box<Path>>` due to unsatisfied trait bounds
     |
    ::: /home/luna/.cargo/registry/src/index.crates.io-6f17d22bba15001f/clap_builder-4.3.10/src/builder/value_parser.rs:2221:1
     |
2221 |   pub struct _AutoValueParser<T>(std::marker::PhantomData<T>);
     |   ------------------------------ doesn't satisfy `_: _ValueParserViaParse`
     |
    ::: /home/luna/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/boxed.rs:195:1
     |
195  | / pub struct Box<
196  | |     T: ?Sized,
197  | |     #[unstable(feature = "allocator_api", issue = "32838")] A: Allocator = Global,
198  | | >(Unique<T>, A);
     | | -
     | | |
     | | doesn't satisfy `Box<Path>: From<&'s std::ffi::OsStr>`
     | | doesn't satisfy `Box<Path>: From<&'s str>`
     | | doesn't satisfy `Box<Path>: From<OsString>`
     | | doesn't satisfy `Box<Path>: From<std::string::String>`
     | | doesn't satisfy `Box<Path>: FromStr`
     | |_doesn't satisfy `Box<Path>: ValueEnum`
     |   doesn't satisfy `Box<Path>: ValueParserFactory`
     |
     = note: the following trait bounds were not satisfied:
             `Box<Path>: ValueEnum`
             which is required by `&&&&&_AutoValueParser<Box<Path>>: clap::builder::via_prelude::_ValueParserViaValueEnum`
             `Box<Path>: ValueParserFactory`
             which is required by `&&&&&&_AutoValueParser<Box<Path>>: clap::builder::via_prelude::_ValueParserViaFactory`
             `Box<Path>: From<OsString>`
             which is required by `&&&&_AutoValueParser<Box<Path>>: clap::builder::via_prelude::_ValueParserViaFromOsString`
             `Box<Path>: From<&'s std::ffi::OsStr>`
             which is required by `&&&_AutoValueParser<Box<Path>>: clap::builder::via_prelude::_ValueParserViaFromOsStr`
             `Box<Path>: From<std::string::String>`
             which is required by `&&_AutoValueParser<Box<Path>>: clap::builder::via_prelude::_ValueParserViaFromString`
             `Box<Path>: From<&'s str>`
             which is required by `&_AutoValueParser<Box<Path>>: clap::builder::via_prelude::_ValueParserViaFromStr`
             `Box<Path>: FromStr`
             which is required by `_AutoValueParser<Box<Path>>: clap::builder::via_prelude::_ValueParserViaParse`
     = note: this error originates in the macro `clap::value_parser` (in Nightly builds, run with -Z macro-backtrace for more info)

Expected Behaviour

The derive macro should produce a human-readable error when it detects an invalid type being used, rather than produce a horrific type error after expansion,

Additionally, it would be nice if parsing into smart pointers was supported. Using a PathBuf would be wasteful in my case, as I don't intend on modifying the path, and would like to encode this directly into my type. Using Rc or Arc would also allow cheaply cloning the struct to pass it to other code. Serde allows this afaik.

Additional Context

No response

Debug Output

No response

@LunarLambda LunarLambda added the C-bug Category: Updating dependencies label Jul 5, 2023
@LunarLambda
Copy link
Author

LunarLambda commented Jul 5, 2023

Worked around it like so (which is a little ugly, but it works)

use clap::builder::{PathBufValueParser, TypedValueParser};

fn box_path() -> impl TypedValueParser<Value = Box<Path>> {
    PathBufValueParser::new().map(|p| p.into())
}

#[derive(Parser)] 
struct Cli {
    #[arg(short = 'C', value_parser = box_path())]
    foo: Option<Box<Path>>
}

@epage
Copy link
Member

epage commented Jul 5, 2023

The derive macro should produce a human-readable error when it detects an invalid type being used, rather than produce a horrific type error after expansion,

The derive macro can only see the textual name for a type (ie PathBuf and std::path::PathBuf are different). We are delegating to rust code to map types to value parsers so we don't have to worry about how a type is named and to workaround the lack of specialization in the language (having a single trait with a blanket impl plus some hand impls)

A subset of that error actually provides some really good hints

     | | doesn't satisfy `Box<Path>: From<&'s std::ffi::OsStr>`
     | | doesn't satisfy `Box<Path>: From<&'s str>`
     | | doesn't satisfy `Box<Path>: From<OsString>`
     | | doesn't satisfy `Box<Path>: From<std::string::String>`
     | | doesn't satisfy `Box<Path>: FromStr`
     | |_doesn't satisfy `Box<Path>: ValueEnum`
     |   doesn't satisfy `Box<Path>: ValueParserFactory`

though it can't also suggest your workaround.

@epage
Copy link
Member

epage commented Jul 5, 2023

#4995 adds support for some serde types

@epage
Copy link
Member

epage commented Jul 7, 2023

As part of the message is intentional and our options are limited for resolving this and with us adding support for more types, I'm not seeing what more can be done for this and am closing this.

If this needs further discussion, feel free to say something!

@epage epage closed this as not planned Won't fix, can't repro, duplicate, stale Jul 7, 2023
@Kobzol
Copy link

Kobzol commented Jul 31, 2023

I also hit this error message when implementing FromStr with an Err type of () (https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=99878a7c31bf79048c92a77c06096969). It was quite confusing to me, until I realized that the problem goes away when Err gets set to String.

@SteveLauC
Copy link

Hi @Kobzol, thanks for that comment (and workaround), I filed an issue for it: #5360

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Updating dependencies
Projects
None yet
Development

No branches or pull requests

4 participants