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: loosen lifetime constraint on mut_subcommand #3909

Merged
merged 1 commit into from Jul 12, 2022

Conversation

emersonford
Copy link
Contributor

@emersonford emersonford commented Jul 12, 2022

follow-up to PR #3882, as discussed in that PR's comments mut_subcommand does not resolve aliases and, if an alias name is passed for subcmd_id, would trigger invalid behavior as per #3888.

thus to avoid this, one might resolve the alias name to the actual name before calling mut_subcommand, but this is currently almost impossible to do due to mut_subcommand signature's with the 'help lifetime. specifically, looking at the test code included in this PR and without the lifetime changes, we get the following error:

error[E0597]: `true_name` does not live long enough
   --> tests/builder/tests.rs:459:32
    |
459 |     cmd = cmd.mut_subcommand(&*true_name, |subcmd| subcmd.about("modified about"));
    |                                ^^^^^^^^^ borrowed value does not live long enough
460 |     assert_eq!(cmd.find_subcommand("baz").unwrap().get_about().unwrap(), "modified about");
461 | }
    | -
    | |
    | `true_name` dropped here while still borrowed
    | borrow might be used here, when `cmd` is dropped and runs the destructor for type `App<'_>`
    |
    = note: values in a scope are dropped in the opposite order they are defined

For more information about this error, try `rustc --explain E0597`.
error: could not compile `clap` due to previous error

but loosening the lifetime constraint from 'help to 'a resolves this.

note, directly using cmd.find_subcommand("baz").unwrap().get_name() is invalid because of:

error[E0597]: `cmd` does not live long enough
   --> tests/builder/tests.rs:457:21
    |
457 |     let true_name = cmd.find_subcommand("baz").unwrap().get_name();
    |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^ borrowed value does not live long enough
...
465 | }
    | -
    | |
    | `cmd` dropped here while still borrowed
    | borrow might be used here, when `cmd` is dropped and runs the destructor for type `App<'_>`

error[E0506]: cannot assign to `cmd` because it is borrowed
   --> tests/builder/tests.rs:460:5
    |
457 |     let true_name = cmd.find_subcommand("baz").unwrap().get_name();
    |                     -------------------------- borrow of `cmd` occurs here
...
460 |     cmd = cmd.mut_subcommand(true_name, |subcmd| subcmd.about("modified about"));
    |     ^^^
    |     |
    |     assignment to borrowed `cmd` occurs here
    |     borrow later used here

error[E0505]: cannot move out of `cmd` because it is borrowed
   --> tests/builder/tests.rs:460:11
    |
457 |     let true_name = cmd.find_subcommand("baz").unwrap().get_name();
    |                     -------------------------- borrow of `cmd` occurs here
...
460 |     cmd = cmd.mut_subcommand(true_name, |subcmd| subcmd.about("modified about"));
    |           ^^^                --------- borrow later used here
    |           |
    |           move out of `cmd` occurs here

Some errors have detailed explanations: E0505, E0506, E0597.
For more information about an error, try `rustc --explain E0505`.
error: could not compile `clap` due to 3 previous errors

I'd also like to do this for mut_arg, but that's a more involved change so will put that in a separate PR. :)

@epage
Copy link
Member

epage commented Jul 12, 2022

I'd also like to do this for mut_arg, but that's a more involved change so will put that in a separate PR. :)

We'll also be removing all of the lifetimes in 4.0, so the problem will be minimized with time.

@epage epage merged commit 3802a35 into clap-rs:master Jul 12, 2022
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