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.

Sign up for GitHub

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

Jump to bottom

requires is bypassed when using conflicts_with a certain way #4520

Open
2 tasks done
pierrechevalier83 opened this issue Nov 29, 2022 · 2 comments · May be fixed by #4523
Open
2 tasks done

requires is bypassed when using conflicts_with a certain way #4520

pierrechevalier83 opened this issue Nov 29, 2022 · 2 comments · May be fixed by #4523
Labels
A-validators Area: ArgMatches validation logi C-bug Category: Updating dependencies M-breaking-change Meta: Implementing or merging this will introduce a breaking change. S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing

Comments

@pierrechevalier83
Copy link

Please complete the following tasks

Rust Version

1.65.0

Clap Version

4.0.26

Minimal reproducible code

use clap::Parser;

#[derive(Parser, Debug)]
struct Cli {
    #[clap(long)]
    destroy: bool,
    #[clap(long, conflicts_with = "destroy" )]
    enhance: bool,
    /// Let's assume, `--dry_run` is only implemented for the `--enhance`
    /// functionality for some reason
    /// Attempting to use `--dry-run --destroy` should fail and inform the user
    /// that these arguments conflict.
    /// The last thing we would want would be to go ahead and destroy their
    /// system when they're thinking only a dry run is going to happen.
    /// This fiction is inspired by real events :p
    #[clap(long, requires = "enhance" )]
    dry_run: bool,
}

fn main() {
    let res = Cli::try_parse_from(vec!["cli", "--destroy", "--dry-run"]);
    // *Unexpectedly*, this assertion fails.
    // One would think that because `--dry-run` requires `--enhance`, the result
    // would be an error complaining that this combination cannot be used
    // together.
    // In actuality, clap will happily parse this.
    assert!(res.is_err());
}

Steps to reproduce the bug with the above code

See the surprising code in the rust playground.

Actual Behaviour

The last assertion fails: clap is happy to let the user pass the --dry-run flag with the --destroy flag and without the --enhance flag, bypassing the fact that --dry-run should require --enhance.

Expected Behaviour

The last assertion should pass: clap should report to the user that --dry-run can only be used in combination with the --enhance flag.

Additional Context

This issue can be worked around by adding an explicit conflicts_with configuration like so:

    #[clap(long, requires = "enhance", conflicts_with = "destroy")]
    dry_run: bool,

but I find it surprising that this is needed to obtain the expected behaviour.

Debug Output

No response
@pierrechevalier83 pierrechevalier83 added the C-bug Category: Updating dependencies label Nov 29, 2022
@epage epage added A-validators Area: ArgMatches validation logi S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing M-breaking-change Meta: Implementing or merging this will introduce a breaking change. labels Nov 29, 2022
@epage
Copy link
Member

epage commented Nov 29, 2022

Generally, conflicts override required arguments. However, I do agree that we should evaluate these rules to make sure correctly apply for required relationships like this.

facebook-github-bot pushed a commit to facebook/sapling that referenced this issue Nov 29, 2022
@facebook-github-bot
Fix --dry-run flag
85cf1d8 
Summary:
Currently, `mononoke_newadmin` can be used with a command like
```
monoke_newadmin -- --prod hg-sync --repo-id 2103 last-processed --dry-run  --set 42
```

It won't dry run, but it will actually set the last processed ID in prod.

That is scary.

It is due to a surprising behaviour of clap that I reported upstream here: clap-rs/clap#4520.

Work around what is likely an upstream bug by adding an explicit `conflicts_with` configuration.

Reviewed By: YousefSalama, yancouto

Differential Revision: D41555183

fbshipit-source-id: 482046ade24b60961c3f3c19c309695ba4116b40
pierrechevalier83 added a commit to pierrechevalier83/clap-rs that referenced this issue Nov 29, 2022
@pierrechevalier83
test: Add failing test for issue clap-rs#4520
2c51d8f 
This test is semantically equivalent to the situation described in that
issue's description:
a first option conflicts with a second option.
a third option requires the second option.
Clap lets us provide the first and the third options only, which
shouldn't be allowed.

The test fails here, but is fixed by the next commit in this PR.
pierrechevalier83 added a commit to pierrechevalier83/clap-rs that referenced this issue Nov 29, 2022
@pierrechevalier83
fix: don't bypass conflicts when using requires
26a8c18 
The intent for `is_missing_required_ok` is to allow a situation where
two args are required, but they're part of the same group, or one
overrides the other, which is equivalent, so having either of them is
good enough.

The way it was implemented, by reusing `gather_conflicts` introduced a
loophole where `required` could be bypassed when it shouldn't.

Fixes clap-rs#4520
@pierrechevalier83 pierrechevalier83 linked a pull request Nov 29, 2022 that will close this issue
Open
pierrechevalier83 added a commit to pierrechevalier83/clap-rs that referenced this issue Nov 29, 2022
@pierrechevalier83
fix: Don't bypass conflicts when using requires
56ed5e1 
The intent for `is_missing_required_ok` is to allow a situation where
two args are required, but they're part of the same group, or one
overrides the other, which is equivalent, so having either of them is
good enough.

The way it was implemented, by reusing `gather_conflicts` introduced a
loophole where `required` could be bypassed when it shouldn't.

Fixes clap-rs#4520
pierrechevalier83 added a commit to pierrechevalier83/clap-rs that referenced this issue Nov 29, 2022
@pierrechevalier83
fix: Don't bypass conflicts when using requires
de00ad7 
The intent for `is_missing_required_ok` is to allow a situation where
two args are required, but they're part of the same group, or one
overrides the other, which is equivalent, so having either of them is
good enough.

The way it was implemented, by reusing `gather_conflicts` introduced a
loophole where `required` could be bypassed when it shouldn't.

Fixes clap-rs#4520
@pierrechevalier83
Copy link
Author

Generally, conflicts override required arguments. However, I do agree that we should evaluate these rules to make sure correctly apply for required relationships like this.

I've had a look at the code, and I think I understand why things are done how they are and how to keep the desired properties while fixing this particular bug. See the PR #5423 which introduces and fixes a new unit test, but doesn't break existing unit tests that rely on groups overriding required relationships.

@epage epage mentioned this issue Feb 13, 2023
Open
2 tasks
@rgiot rgiot mentioned this issue Jul 22, 2023
Open
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-validators Area: ArgMatches validation logi C-bug Category: Updating dependencies M-breaking-change Meta: Implementing or merging this will introduce a breaking change. S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: Don't bypass conflicts when using requires pierrechevalier83/clap-rs
2 participants
@epage @pierrechevalier83