Tracking PR Custom Derive (#146)

[kbknapp]

This is the tracking PR for Custom Derive discussed in #146

This initial PR should cover ArgMatches->MyArgs conversion and potentially MyArgs->App conversion. If one is holding back the other, let's move it to another PR and merge.

---

Currently Outstanding Discussion Points / Questions

• Should TryFromArgMatches be implied by FromArgMatches?

Todo from previous discussion points

• Rename DefineX to X in traits (DefineApp->App)
• Move code to public code_gen module (stomp->code_gen)
• Have #[derive(FromArgMatches)] actually generate From<ArgMatches>

Misc.

• Add other get_matches variants (get_matches_from, etc.)
• Ensure feature parity with all App and Arg methods
• Apply the unstable clap feature (will be used until Rust 1.17 ships stable, to maintain Stable-2 compatibility)
• Rebase commits to clog conventions (can be done last)

---

To Do

Derive FromArgMatches

#[derive(FromArgMatches)]
struct MyApp {
    verb: bool
}

fn main() {
    let m: MyApp = App::new("test")
        .arg_from_usage("-v, --verbose 'turns on verbose printing'")
        .get_matches()
        .into();

    println!("Verbose on: {:?}", m.verb);
}

• #[derive(FromArgMatches)]
  • Docs
  • Tests
  • Ensure requirements can't be violated (i.e. use Default where possible and document such)

Derive App

/// Does stuff
#[derive(App)]
struct MyApp {
    /// Turns on verbose printing
    #[clap(short = 'v', long = "verbose")]
    verb: bool
}

• #[derive(App)]
  • Docs
  • Tests

Derive Subcommands

#[derive(SubCommands)]
pub enum Commands {
    Test(Test),
}

#[derive(FromArgMatches)]
struct Test {
    config: String
}

• #[derive(SubCommands)]
  • Docs
  • Tests

---

This change is 

[Nemo157]

• Propose changing DefineX to X in traits (DefineApp->App)
• Propose moving code to public code_gen module (stomp->code_gen)
• Propose renaming FromArgMatches::from to FromArgMatches::from_matches to avoid conflicts with From (assuming we can't just use std::convert::From which would be most optimal)

Those all sound good to me. I just tried #[proc_macro_derive(From<ArgMatches>)] and #[proc_macro_derive("From<ArgMatches>")] in case one worked, but unsurprisingly not. I was hoping we could have something like impl<T> Into<T> for ArgMatches where T: FromArgMatches, but it doesn't look like there's any way to do that without coherence issues 🙁

Actually, I guess #[derive(FromArgMatches)] could just generate an implementation of From<ArgMatches>, there's no reason it needs to generate the trait it says it does...

Rebase commits to clog conventions

Yeah, I was thinking of creating some nice, pretty, fake history for this 😆 there's definitely some internal stuff that needs to be designed properly before doing that (looking at it again I really dislike the code parsing the attributes, there must be a cleaner way to do it).

[kbknapp]

Actually, I guess #[derive(FromArgMatches)] could just generate an implementation of From, there's no reason it needs to generate the trait it says it does...

I like this a lot because it allows using the standard From and Into traits.

Yeah, I was thinking of creating some nice, pretty, fake history for this 😆

No worries! This can be done last. In all honesty, so long as the relevant commits get picked up by clog in order to add the changelog I'm good 😉

[Nemo157]

Made those changes and also

SubCommands imply SubCommandFromArgMatches

I'm tempted to actually just merge those two traits back together.

I was having lifetime issues with the constraints on the ParseApp trait so killed it until there's a better design for it, something that follows the different get_matches* functions and works with TryFrom<&ArgMatches> as well.

[kbknapp]

I've put in a PR with some very minor changes. I'm just starting to dig into the code, so I expect we'll both be making changes for a little bit and additions/deletions, so I view the code as living for the time being 😄

Once we start to knock off some of these todos and we can clean up the commits and ensure everyhing is properly exported/visible. I also don't expect doc comments until we're settled on the implementation so don't worry about those yet unless something gets finalized.

[Nemo157]

I've been thinking about the attributes parsing a bit, it should be possible to create a macro based crate (probably macros 1.1 for extra metaness) to actually parse them, which will make using the values much nicer. It will probably also make updating them as the design changes a lot easier.

I threw together a macro_rules prototype, but extending it to support all the literal types would require a massive amount of duplication. You can see the simplification it would allow in Arg::from(syn::Field) though.

I think I could probably get a working derive crate for attributes up tomorrow and switch clap-macros over to it.

[Nemo157]

Took a little longer than I thought, but I've got a near production ready attribute parser up (https://github.com/Nemo157/prom-attire-rs) and opened a PR for switching clap-macros over to using it so you can take a look before I merge it Nemo157#3

[TeXitoi]

Hi, StructOpt author here.

First, I must say that there is high quality code and features in this PR. In fact, I feel quite depressed to see that my idea and implementation is present here with much more features and better error handling.

After reading all the related issues and lightly read this PR, I just have a few remarks:

• Looking at the 61 stars of structopt in less that 24h, this feature is highly wanted. Thus I think that's important to publish this PR soon. If, that's not mergeable now, providing this in a temporary experimental crate (structopt, stomp or something else) will be very interesting for user experience and to get feedback without the need of something highly polished.
• There is some divergences with the clap interface. I think to the automatic Arg::long implementation when clap doesn't do such thing and the doc thing that generate some App::help. I think that's important to have an easy translation from attributes to builder pattern.
• Most remarks say that the simple API is a killer feature. So, for the developper experience, I think that having something similar to MyApp::from_args() is very important. We can imagine a #[derive(Clap)] that implement App, FromArgMatches and an hypothetic FromArgs.
• This code abuse implementing the From trait with possible panic. But the official rust doc explicitly say that "this trait must not fail". Thus, I think that's better not implementing this trait if failure is possible.
• The TODO list at the top of this PR is quite confusing. I think that expressing them in the form of a simple test will help to understand the goal.
• Some cosmetic stylish remarks.

If you have any remarks or thing I can do to help, just talk! ;-)

[kbknapp]

@TeXitoi Thanks for the comments! I agree this is a much wanted feature and something we need to look at merging soon. I'm good with merging something early under the unstable cargo feature as well, so it doesn't need to be perfect. I do however want to have most of the ergonomics and user-experience pieces to be worked out first though.

My only concern with having something like stomp or StructOpt around too long, is if their design goes against anything clap provides it presents a fracture in the ecosystem and makes it harder for people to adopt/change.

To be clear, I'm more concerned with deriving the ArgMatches->MyArgs conversion. The MyArgs->App can come later. Having said that, the MyArgs->App piece may be easier to implement in which case, if ti's done it's done, we can merge that part. I don't have hard requirement to finish one before the other. I'm only saying that I think people want the ArgMatches->MyArgs the most.

Here's what I mean by the two and how I imagine them working. Literally, this is my only requirements and once a PR reaches this state I'll be happy with a merge.

ArgMatches->MyArgs

User code:

enum VerbLevel {
    Off,
    Some,
    All
}

#[derive(FromArgMatches)]
struct MyArgs {
    verb_lvl: VerbLevel,
    files: Vec<PathBuf>,
    debug: bool
}

fn main() {
    let args: MyArgs = App::new("myprog")
        // ..snip
        .get_matches()
        .into();

    println!("Debug turned on?: {:?}", args.debug);
}

Subcommands can complicate this. But here's how Imagine those working:

enum VerbLevel {
    Off,
    Some,
    All
}

#[derive(FromArgMatches)]
struct MyArgs {
    verb_lvl: VerbLevel,
    files: Vec<PathBuf>,
    debug: bool,
    cmd: MySubCommands
}

#[derive(SubCommands)]
enum MySubCommands {
    Test(Test),
    Stuff(Stuff)
}

#[derive(FromArgMatches)]
struct Test {
    val: String
}

#[derive(FromArgMatches)]
struct Stuff {
    config: Option<String>
}

fn main() {
    let args: MyArgs = App::new("myprog")
        // ..snip
        .get_matches()
        .into();

    println!("Debug turned on?: {:?}", args.debug);
    match args.cmd {
        Test(t) => println!("val: {}", t.val),
        Stuff(s) => println("Config: {:?}", s.config),
    }
}

MyArgs->App

/// Some text that goes into App::about
#[derive(App)]
struct MyArgs {
    /// Some text that goes into Arg::help
    #[clap(long = "verb", short = "v", takes_value=true, default_value="Off")]
    verb_lvl: VerbLevel,
    /// Some text describing --files <path>...
    #[clap(long="files", takes_value=true, required=true, multiple=true)]
    files: Vec<PathBuf>,
}

fn main() {
    let args: App = MyArgs::into()
        .get_matches();
}

Again, this may be more simple than ArgMatches->MyArgs and I'm fine if this merges independently from it.

---

Hopefully that clears some things up.

---

This code abuse implementing the From trait with possible panic.

Yes, but it shouldn't panic. Meaning, if it panics, there was a developer error and the program can't run anyways. In which case I think it should panic. I can't imagine a case where failing to parse should be allowed, or is desired. IMO it's perfectly acceptable, and even within the Rust guidelines, to panic due to developer error or when the program shouldn't be allowed to run further, which I view as this case.

I think that's important to have an easy translation from attributes to builder pattern.

I 100% agree! 👍 And I'm OK with /// some text being used Arg::help or App::about, but also 100% want to support #[clap(help = "some text")] too. In clap there are already multiple ways to do things, and I'm OK supporting multiple ways so long as they make sense. For now though, /// text is really the only "special" case I'm OK supporting. I can't think of any others that make sense right now.

To be clear, if we start by simply supporting #[clap(help = "text")] and add /// text later. I'm perfectly good with that.

The automatic long handling, I'm less inclined to support. I don't want to do something magically unless the user requested it. This could create issues when the user doesn't want a long.

Most remarks say that the simple API is a killer feature.

Again, I 100% agree with your comments 😄

The TODO list at the top of this PR is quite confusing. I think that expressing them in the form of a simple test will help to understand the goal.

I'll rework the TODO 😉

[Nemo157]

@TeXitoi Welcome 😄 I had a brief look at StructOpt when it was posted to users and I was very impressed at how much it supported with such a small codebase.

the automatic Arg::long implementation when clap doesn't do such thing

The automatic long handling, I'm less inclined to support. I don't want to do something magically unless the user requested it. This could create issues when the user doesn't want a long.

I agree, that was just an artifact of my philosophy on CLI design. It should be disabled and instead there could be the shortcut attribute #[clap(long)] that just uses the field name to determine the long option name.

the doc thing that generate some App::help.

And I'm OK with /// some text being used Arg::help or App::about, but also 100% want to support #[clap(help = "some text")] too.

Should be pretty simple, if there is a help or about attribute specified disable the doc handling and just use the values specified.

something similar to MyApp::from_args() is very important.

I had something like that as an extension trait originally, but when switching to implementing From<&ArgMatches> instead of a custom FromArgMatches I had difficulty getting the lifetimes to line up. I guess having it as an inherent impl on the type might avoid the lifetime issues.

I can't imagine a case where failing to parse should be allowed, or is desired.

I can't either, but I could definitely see it happening in the future. One way to future-proof it would be to implement TryFrom as well and have From::from be TryFrom::try_from(...).unwrap() (or something similar with nicer error messages). That way we would be forced to add proper errors for issues now, but most users can avoid doing any real error handling and treat any failures here as bugs in their application.

---

I also have some thoughts on the overall crate/code structure after setting up tests on prom-attire. I'll try and post something about them later today.

[kbknapp]

One way to future-proof it would be to implement TryFrom as well and have From::from be TryFrom::try_from(...).unwrap() (or something similar with nicer error messages).

I absolutely agree. But this is also something that can be added post initial merge too, so long as we think about it during the design.

[Nemo157]

The main change to the crate structure I think is useful is to divide clap-macros into two crates, the user-facing crate clap-macros (or maybe clap-derive) just defines the proc_macro and does minimal work. That then calls into an "internal" crate clap-codegen (or something similar) that has a function like pub fn derive(input: &str) -> Result<String, Vec<Error>> that does the actual derive.

This division makes it possible to test error cases, you can write tests in clap-codegen that pass in invalid strings and check the errors that are returned make sense. Passing back multiple errors lets you report on as many issues as possible each time the user attempts compilation.

Positive test cases can be put into the clap-macros test folder, they can just use #[derive(FromArgMatches)] like normal code would and check that the derived implementation behaves correctly.

clap-codegen should avoid panic! as much as possible, instead returning errors, clap-macros should pretty-print the errors returned then panic! to tell rustc that the derive invocation failed.

[kbknapp]

In addition, I may want to consider moving all the user facing macros to clap-macros.

Edit: in otherwords, I like the clap-macros over clap-derive because it includes the already existing macros. Whereas the clap-codegen can do the heavy lifting.

[Nemo157]

I don't think a proc-macro crate can export macro_rules macros. But it could have non-derive proc-macros once they're stabilised.

[kbknapp]

Ah ok, no worries.