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

fix(derive)!: Compile-error on nested subcommands #2586

Merged
merged 2 commits into from
Jul 16, 2021
Merged

Conversation

epage
Copy link
Member

@epage epage commented Jul 14, 2021

Before, partial command lines would panic at runtime. Now it'll be a
compile error

For example:

#[derive(Clap)]
pub enum Opt {
  Daemon(DaemonCommand),
}

#[derive(Clap)]
pub enum DaemonCommand {
  Start,
  Stop,
}

Gives:

error[E0277]: the trait bound `DaemonCommand: clap::Args` is not satisfied
   --> clap_derive/tests/subcommands.rs:297:16
    |
297 |         Daemon(DaemonCommand),
    |                ^^^^^^^^^^^^^ the trait `clap::Args` is not implemented for `DaemonCommand`
    |
    = note: required by `augment_args`

To nest this, you currently need enum -> struct -> enum. A later
change will make it so you can use the subcommand attribute within
enums to cover this case.

This is done by splitting out an Args trait from IntoApp, like Subcommand previously did in #1681.

As a side benefit of this work, a user can now derive(Args) for a struct while before they could derive(IntoApp) but must manually implement FromArgMatches since no derive existed for it.

Unlike with #1681, I did not move from/update logic into the trait but moved all of that logic into the FromArgMatches trait and it a supertrait for Args and Subcommand, deriving them all together. For the top-level, we need FromArgMatches, like IntoApp but unlike IntoApp, we don't need divergent APIs. A FromArgMatches impl is also coupled to the augment impl.

To support FromArgMatches being used in all cases, I had to switch its return type from Self to Option<Self> for when you have #[clap(subcommand)] cmd: Option<SubCmd>. The caller has control over whether it is required or not.

So parsing call graph looks like:

  • Clap::parse
    • IntoApp::into_app
      • Either Subcommand::augment_subcommands or Args:augment_args
    • FromArgMatches::from_arg_matches

BREAKING CHANGES:

  • You can no longer nest enums-in-enums to implicitly get a sub-sub-command
  • The derived traits went through a lot of changes

Depends on #2595
This is a part of #2005

epage added a commit to epage/clap that referenced this pull request Jul 14, 2021
When debugging clap-rs#2586, I noticed we were developing match cases for
variant names that were flattened.  At minimum, this is dead code and at
worst this could cause the wrong behavior if a user does an update with
one of those names.

Depends on clap-rs#2587

Fixes clap-rs#2588
@pksunkara
Copy link
Member

Want to let you know that it will take me a bit of time to review this.

@epage
Copy link
Member Author

epage commented Jul 14, 2021

Want to let you know that it will take me a bit of time to review this.

Understandable. If we're good enough with the direction and just wanting to check details, I can take time to create an intermediate PR that moves functions to where they'll end up without any functionality changes, so the functionality changes stand out more.

@pksunkara
Copy link
Member

I would appreciate that.

epage added a commit to epage/clap that referenced this pull request Jul 14, 2021
When debugging clap-rs#2586, I noticed we were developing match cases for
variant names that were flattened.  At minimum, this is dead code and at
worst this could cause the wrong behavior if a user does an update with
one of those names.

Depends on clap-rs#2587

Fixes clap-rs#2588
epage added a commit to epage/clap that referenced this pull request Jul 14, 2021
When debugging clap-rs#2586, I noticed we were developing match cases for
variant names that were flattened.  At minimum, this is dead code and at
worst this could cause the wrong behavior if a user does an update with
one of those names.

Depends on clap-rs#2587

Fixes clap-rs#2588
@pksunkara
Copy link
Member

Waiting for rebase. Please only rebase this PR, no need to touch the other PRs until this is merged, that way we don't consume too much of the github CI and end up wasting time waiting for the CI queue.

@epage
Copy link
Member Author

epage commented Jul 14, 2021

Rebased

Copy link
Member

@pksunkara pksunkara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add ui tests for the all the new errors we are sending?

src/derive.rs Outdated
///
/// Implementing this trait let's a parent container delegate subcommand behavior to `Self`.
/// with:
/// - `#[clap(subcommand)] field: SubCmd`: Can be used with either struct fields or enum variants.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be used with either struct fields or enum variants.

What do you mean?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing to

/// - `#[clap(subcommand)] field: SubCmd`: Attribute can be used with either struct fields or enum
///   variants that impl `Subcommand`.
/// - `#[clap(flatten)] Variant(SubCmd)`: Attribute can only be used with enum variants that impl
///   `Subcommand`.

Does that help?

src/derive.rs Outdated Show resolved Hide resolved
src/derive.rs Outdated
/// Implementing this trait let's a parent container delegate argument parsing behavior to `Self`.
/// with:
/// - `#[clap(flatten)] args: ChildArgs`: Can only be used with struct fields.
/// - Implicitly: Can only be used with enum variants.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// - Implicitly: Can only be used with enum variants.
/// - `Variant(ChildArgs)`: Can only be used with enum variants.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The descriptions here aren't clear enough.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing to

/// - `#[clap(flatten)] args: ChildArgs`: Attribute can only be used with struct fields that impl
///   `Args`.
/// - `Variant(ChildArgs)`: No attribute is used with enum variants that impl `Args`.

src/derive.rs Show resolved Hide resolved
clap_derive/src/lib.rs Outdated Show resolved Hide resolved
clap_derive/src/derives/args.rs Show resolved Hide resolved
src/derive.rs Outdated
/// Implementing this trait let's a parent container delegate subcommand behavior to `Self`.
/// with:
/// - `#[clap(subcommand)] field: SubCmd`: Can be used with either struct fields or enum variants.
/// - `#[clap(flatten)] Variant(SubCmd)`: Can only be used with enum variants.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The descriptions here aren't clear enough.

clap_derive/src/derives/args.rs Show resolved Hide resolved
clap_derive/src/derives/subcommand.rs Outdated Show resolved Hide resolved
Before, partial command lines would panic at runtime.  Now it'll be a
compile error

For example:
```
pub enum Opt {
  Daemon(DaemonCommand),
}

pub enum DaemonCommand {
  Start,
  Stop,
}
```

Gives:
```
error[E0277]: the trait bound `DaemonCommand: clap::Args` is not satisfied
   --> clap_derive/tests/subcommands.rs:297:16
    |
297 |         Daemon(DaemonCommand),
    |                ^^^^^^^^^^^^^ the trait `clap::Args` is not implemented for `DaemonCommand`
    |
    = note: required by `augment_args`
```

To nest this, you currently need `enum -> struct -> enum`.  A later
change will make it so you can use the `subcommand` attribute within
enums to cover this case.

This is a part of clap-rs#2005
epage added a commit to epage/clap that referenced this pull request Jul 15, 2021
When debugging clap-rs#2586, I noticed we were developing match cases for
variant names that were flattened.  At minimum, this is dead code and at
worst this could cause the wrong behavior if a user does an update with
one of those names.

Depends on clap-rs#2587

Fixes clap-rs#2588
@epage
Copy link
Member Author

epage commented Jul 16, 2021

I've updated everything, including adding ui tests.

@pksunkara pksunkara merged commit 62588bd into clap-rs:master Jul 16, 2021
@epage epage deleted the args branch July 16, 2021 19:57
epage added a commit to epage/clap that referenced this pull request Jul 16, 2021
When debugging clap-rs#2586, I noticed we were developing match cases for
variant names that were flattened.  At minimum, this is dead code and at
worst this could cause the wrong behavior if a user does an update with
one of those names.

Depends on clap-rs#2587

Fixes clap-rs#2588
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants