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

feat: make arg passing for fix match other subcmds #7232

Closed
wants to merge 2 commits into from

Conversation

yaahc
Copy link
Member

@yaahc yaahc commented Aug 10, 2019

Prior to this change, cargo fix had no defacto method for passing
additional arguments down to rustc to use when generating warnings to be
fixed. We did implement a way to do so for clippy, but the hack was
inconsistent with the way all other sub commands get args for rustc,
where they take all args after the -- and pass them down.

This change removes the clippy args method as implemented earlier and
adds a new primary_unit_args passing mechanism which will work for
cargo fix and cargo fix --clippy.

Prior to this change, `cargo fix` had no defacto method for passing
additional arguments down to rustc to use when generating warnings to be
fixed. We did implement a way to do so for clippy, but the hack was
inconsistent with the way all other sub commands get args for rustc,
where they take all args after the -- and pass them down.

This change removes the clippy args method as implemented earlier and
adds a new primary_unit_args passing mechanism which will work for
`cargo fix` and `cargo fix --clippy`.
@rust-highfive
Copy link

r? @nrc

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 10, 2019
@@ -132,6 +128,7 @@ pub fn fix(ws: &Workspace<'_>, opts: &mut FixOptions<'_>) -> CargoResult<()> {

let rustc = opts.compile_opts.config.load_global_rustc(Some(ws))?;
wrapper.arg(&rustc.path);
wrapper.args(&opts.primary_unit_args);
Copy link
Member Author

Choose a reason for hiding this comment

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

pass via wrapper's argument list. FixArgs::get() already grabs all but a few special args and forwards them to the subprocess. This change just exposes that feature to the end user.

@yaahc
Copy link
Member Author

yaahc commented Aug 10, 2019

r? @ehuss

@rust-highfive rust-highfive assigned ehuss and unassigned nrc Aug 10, 2019
@@ -586,7 +583,6 @@ struct FixArgs {
enabled_edition: Option<String>,
other: Vec<OsString>,
Copy link
Member Author

Choose a reason for hiding this comment

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

The args are now being pushed into this list.

@ehuss
Copy link
Contributor

ehuss commented Aug 20, 2019

I don't think we want to allow passing arbitrary args to rustc in the fix command. The cargo rustc command is intentionally a low-level command, and its ability to take arguments is intentionally unique.

@yaahc
Copy link
Member Author

yaahc commented Aug 20, 2019

Fwiw I didn't base it on cargo rustc, I based it on clippy. Both cargo clippy and cargo clippy-preview currently support passing args to clippy-driver after --. I'm mostly just concerned with keeping the usage consistent.

@yaahc yaahc closed this Aug 28, 2019
@yaahc yaahc deleted the argconsistency branch August 28, 2019 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants