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

initial working version of cargo fix --clippy #7069

Merged
merged 22 commits into from
Jul 19, 2019
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/bin/cargo/commands/clippy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
.into());
}

let wrapper = util::process("clippy-driver");
let wrapper = util::process(util::config::clippy_driver());
compile_opts.build_config.rustc_wrapper = Some(wrapper);

ops::compile(&ws, &compile_opts)?;
Expand Down
28 changes: 28 additions & 0 deletions src/bin/cargo/commands/fix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,15 @@ pub fn cli() -> App {
.long("allow-staged")
.help("Fix code even if the working directory has staged changes"),
)
.arg(
Arg::with_name("clippy")
.long("clippy")
.help("Get fix suggestions from clippy instead of rustc")
.hidden(true)
.multiple(true)
.min_values(0)
.number_of_values(1),
)
.after_help(
"\
This Cargo subcommand will automatically take rustc's suggestions from
Expand Down Expand Up @@ -125,6 +134,23 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
// code as we can.
let mut opts = args.compile_options(config, mode, Some(&ws))?;

let use_clippy = args.is_present("clippy");

let clippy_args = args
.value_of("clippy")
.map(|s| s.split(' ').map(|s| s.to_string()).collect())
.or_else(|| Some(vec![]))
.filter(|_| use_clippy);

// dbg!(&clippy_args, use_clippy);
yaahc marked this conversation as resolved.
Show resolved Hide resolved

if use_clippy && !config.cli_unstable().unstable_options {
return Err(failure::format_err!(
"`cargo fix --clippy` is unstable, pass `-Z unstable-options` to enable it"
)
.into());
}

if let CompileFilter::Default { .. } = opts.filter {
opts.filter = CompileFilter::Only {
all_targets: true,
Expand All @@ -135,6 +161,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
tests: FilterRule::All,
}
}

ops::fix(
&ws,
&mut ops::FixOptions {
Expand All @@ -146,6 +173,7 @@ pub fn exec(config: &mut Config, args: &ArgMatches<'_>) -> CliResult {
allow_no_vcs: args.is_present("allow-no-vcs"),
allow_staged: args.is_present("allow-staged"),
broken_code: args.is_present("broken-code"),
clippy_args,
},
)?;
Ok(())
Expand Down
5 changes: 4 additions & 1 deletion src/cargo/core/compiler/build_config.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use std::cell::RefCell;
use std::path::Path;
use std::path::{Path, PathBuf};

use serde::ser;

Expand All @@ -26,6 +26,8 @@ pub struct BuildConfig {
pub build_plan: bool,
/// An optional wrapper, if any, used to wrap rustc invocations
pub rustc_wrapper: Option<ProcessBuilder>,
/// An optional override of the rustc path for primary units only
pub primary_unit_rustc: Option<PathBuf>,
pub rustfix_diagnostic_server: RefCell<Option<RustfixDiagnosticServer>>,
/// Whether or not Cargo should cache compiler output on disk.
cache_messages: bool,
Expand Down Expand Up @@ -99,6 +101,7 @@ impl BuildConfig {
force_rebuild: false,
build_plan: false,
rustc_wrapper: None,
primary_unit_rustc: None,
rustfix_diagnostic_server: RefCell::new(None),
cache_messages: config.cli_unstable().cache_messages,
})
Expand Down
33 changes: 27 additions & 6 deletions src/cargo/core/compiler/compilation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ pub struct Compilation<'cfg> {

config: &'cfg Config,
rustc_process: ProcessBuilder,
primary_unit_rustc_process: Option<ProcessBuilder>,

target_runner: Option<(PathBuf, Vec<String>)>,
}
Expand All @@ -77,13 +78,19 @@ impl<'cfg> Compilation<'cfg> {
pub fn new<'a>(bcx: &BuildContext<'a, 'cfg>) -> CargoResult<Compilation<'cfg>> {
let mut rustc = bcx.rustc.process();

let mut primary_unit_rustc_process = bcx
.build_config
.primary_unit_rustc
.as_ref()
.map(|primary_unit_rustc| bcx.rustc.process_with(primary_unit_rustc));

if bcx.config.extra_verbose() {
rustc.display_env_vars();
primary_unit_rustc_process.as_mut().map(|rustc| {
rustc.display_env_vars();
});
}
let srv = bcx.build_config.rustfix_diagnostic_server.borrow();
if let Some(server) = &*srv {
server.configure(&mut rustc);
}

Ok(Compilation {
// TODO: deprecated; remove.
native_dirs: BTreeSet::new(),
Expand All @@ -100,15 +107,29 @@ impl<'cfg> Compilation<'cfg> {
rustdocflags: HashMap::new(),
config: bcx.config,
rustc_process: rustc,
primary_unit_rustc_process,
host: bcx.host_triple().to_string(),
target: bcx.target_triple().to_string(),
target_runner: target_runner(bcx)?,
})
}

/// See `process`.
pub fn rustc_process(&self, pkg: &Package, target: &Target) -> CargoResult<ProcessBuilder> {
let mut p = self.fill_env(self.rustc_process.clone(), pkg, true)?;
pub fn rustc_process(
&self,
pkg: &Package,
target: &Target,
is_primary: bool,
) -> CargoResult<ProcessBuilder> {
let rustc = if is_primary {
self.primary_unit_rustc_process
.clone()
.unwrap_or_else(|| self.rustc_process.clone())
} else {
self.rustc_process.clone()
};

let mut p = self.fill_env(rustc, pkg, true)?;
if target.edition() != Edition::Edition2015 {
p.arg(format!("--edition={}", target.edition()));
}
Expand Down
13 changes: 9 additions & 4 deletions src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,9 +180,6 @@ fn rustc<'a, 'cfg>(
exec: &Arc<dyn Executor>,
) -> CargoResult<Work> {
let mut rustc = prepare_rustc(cx, &unit.target.rustc_crate_types(), unit)?;
if cx.is_primary_package(unit) {
rustc.env("CARGO_PRIMARY_PACKAGE", "1");
}
let build_plan = cx.bcx.build_config.build_plan;

let name = unit.pkg.name().to_string();
Expand Down Expand Up @@ -593,7 +590,15 @@ fn prepare_rustc<'a, 'cfg>(
crate_types: &[&str],
unit: &Unit<'a>,
) -> CargoResult<ProcessBuilder> {
let mut base = cx.compilation.rustc_process(unit.pkg, unit.target)?;
let is_primary = cx.is_primary_package(unit);
let mut base = cx
.compilation
.rustc_process(unit.pkg, unit.target, is_primary)?;

if is_primary {
base.env("CARGO_PRIMARY_PACKAGE", "1");
}

base.inherit_jobserver(&cx.jobserver);
build_base_args(cx, &mut base, unit, crate_types)?;
build_deps_args(&mut base, cx, unit)?;
Expand Down
39 changes: 39 additions & 0 deletions src/cargo/ops/fix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ const BROKEN_CODE_ENV: &str = "__CARGO_FIX_BROKEN_CODE";
const PREPARE_FOR_ENV: &str = "__CARGO_FIX_PREPARE_FOR";
const EDITION_ENV: &str = "__CARGO_FIX_EDITION";
const IDIOMS_ENV: &str = "__CARGO_FIX_IDIOMS";
const CLIPPY_FIX_ENV: &str = "__CARGO_FIX_CLIPPY_PLZ";
const CLIPPY_FIX_ARGS: &str = "__CARGO_FIX_CLIPPY_ARGS";

pub struct FixOptions<'a> {
pub edition: bool,
Expand All @@ -73,6 +75,7 @@ pub struct FixOptions<'a> {
pub allow_no_vcs: bool,
pub allow_staged: bool,
pub broken_code: bool,
pub clippy_args: Option<Vec<String>>,
yaahc marked this conversation as resolved.
Show resolved Hide resolved
}

pub fn fix(ws: &Workspace<'_>, opts: &mut FixOptions<'_>) -> CargoResult<()> {
Expand All @@ -99,11 +102,35 @@ pub fn fix(ws: &Workspace<'_>, opts: &mut FixOptions<'_>) -> CargoResult<()> {
wrapper.env(IDIOMS_ENV, "1");
}

if opts.clippy_args.is_some() {
if let Err(e) = util::process("clippy-driver").arg("-V").exec_with_output() {
eprintln!("Warning: clippy-driver not found: {:?}", e);
}
wrapper.env(CLIPPY_FIX_ENV, "1");
opts.compile_opts.build_config.primary_unit_rustc = Some(util::config::clippy_driver());
}

if let Some(clippy_args) = &opts.clippy_args {
let clippy_args = serde_json::to_string(&clippy_args).unwrap();
wrapper.env(CLIPPY_FIX_ARGS, clippy_args);
}

*opts
.compile_opts
.build_config
.rustfix_diagnostic_server
.borrow_mut() = Some(RustfixDiagnosticServer::new()?);

if let Some(server) = opts
.compile_opts
.build_config
.rustfix_diagnostic_server
.borrow()
.as_ref()
{
server.configure(&mut wrapper);
}

opts.compile_opts.build_config.rustc_wrapper = Some(wrapper);

ops::compile(ws, &opts.compile_opts)?;
Expand Down Expand Up @@ -565,6 +592,7 @@ struct FixArgs {
other: Vec<OsString>,
primary_package: bool,
rustc: Option<PathBuf>,
clippy_args: Vec<String>,
}

enum PrepareFor {
Expand All @@ -583,6 +611,11 @@ impl FixArgs {
fn get() -> FixArgs {
let mut ret = FixArgs::default();
ret.rustc = env::args_os().nth(1).map(PathBuf::from);

if let Ok(clippy_args) = env::var(CLIPPY_FIX_ARGS) {
ret.clippy_args = serde_json::from_str(&clippy_args).unwrap();
}

for arg in env::args_os().skip(2) {
let path = PathBuf::from(arg);
if path.extension().and_then(|s| s.to_str()) == Some("rs") && path.exists() {
Expand All @@ -608,6 +641,7 @@ impl FixArgs {
} else if env::var(EDITION_ENV).is_ok() {
ret.prepare_for_edition = PrepareFor::Next;
}

ret.idioms = env::var(IDIOMS_ENV).is_ok();
ret.primary_package = env::var("CARGO_PRIMARY_PACKAGE").is_ok();
ret
Expand All @@ -617,6 +651,11 @@ impl FixArgs {
if let Some(path) = &self.file {
cmd.arg(path);
}

if !self.clippy_args.is_empty() {
cmd.args(&self.clippy_args);
}

cmd.args(&self.other).arg("--cap-lints=warn");
if let Some(edition) = &self.enabled_edition {
cmd.arg("--edition").arg(edition);
Expand Down
9 changes: 9 additions & 0 deletions src/cargo/util/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1784,3 +1784,12 @@ impl Drop for PackageCacheLock<'_> {
}
}
}

/// returns path to clippy-driver binary
///
/// Allows override of the path via `CARGO_CLIPPY_DRIVER` env variable
pub fn clippy_driver() -> PathBuf {
env::var("CARGO_CLIPPY_DRIVER")
.unwrap_or_else(|_| "clippy-driver".into())
.into()
}
11 changes: 8 additions & 3 deletions src/cargo/util/rustc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,17 +67,22 @@ impl Rustc {
}

/// Gets a process builder set up to use the found rustc version, with a wrapper if `Some`.
pub fn process(&self) -> ProcessBuilder {
pub fn process_with(&self, path: impl AsRef<Path>) -> ProcessBuilder {
match self.wrapper {
Some(ref wrapper) if !wrapper.get_program().is_empty() => {
let mut cmd = wrapper.clone();
cmd.arg(&self.path);
cmd.arg(path.as_ref());
cmd
}
_ => self.process_no_wrapper(),
_ => util::process(path.as_ref()),
}
}

/// Gets a process builder set up to use the found rustc version, with a wrapper if `Some`.
pub fn process(&self) -> ProcessBuilder {
self.process_with(&self.path)
}

pub fn process_no_wrapper(&self) -> ProcessBuilder {
util::process(&self.path)
}
Expand Down
38 changes: 0 additions & 38 deletions tests/testsuite/cache_messages.rs
Original file line number Diff line number Diff line change
Expand Up @@ -239,44 +239,6 @@ fn rustdoc() {
assert_eq!(as_str(&rustdoc_output.stderr), rustdoc_stderr);
}

#[cargo_test]
fn clippy() {
if !is_nightly() {
// --json-rendered is unstable
return;
}
if let Err(e) = process("clippy-driver").arg("-V").exec_with_output() {
eprintln!("clippy-driver not available, skipping clippy test");
eprintln!("{:?}", e);
return;
}

// Caching clippy output.
// This is just a random clippy lint (assertions_on_constants) that
// hopefully won't change much in the future.
let p = project()
.file("src/lib.rs", "pub fn f() { assert!(true); }")
.build();

p.cargo("clippy-preview -Zunstable-options -Zcache-messages")
.masquerade_as_nightly_cargo()
.with_stderr_contains("[..]assert!(true)[..]")
.run();

// Again, reading from the cache.
p.cargo("clippy-preview -Zunstable-options -Zcache-messages")
.masquerade_as_nightly_cargo()
.with_stderr_contains("[..]assert!(true)[..]")
.run();

// FIXME: Unfortunately clippy is sharing the same hash with check. This
// causes the cache to be reused when it shouldn't.
p.cargo("check -Zcache-messages")
.masquerade_as_nightly_cargo()
.with_stderr_contains("[..]assert!(true)[..]") // This should not be here.
.run();
}

#[cargo_test]
fn fix() {
if !is_nightly() {
Expand Down
39 changes: 39 additions & 0 deletions tests/testsuite/clippy.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
use crate::support::{clippy_is_available, is_nightly, project};

#[cargo_test]
fn clippy() {
yaahc marked this conversation as resolved.
Show resolved Hide resolved
yaahc marked this conversation as resolved.
Show resolved Hide resolved
if !is_nightly() {
// --json-rendered is unstable
eprintln!("skipping test: requires nightly");
return;
}

if !clippy_is_available() {
return;
}

// Caching clippy output.
// This is just a random clippy lint (assertions_on_constants) that
// hopefully won't change much in the future.
let p = project()
.file("src/lib.rs", "pub fn f() { assert!(true); }")
.build();

p.cargo("clippy-preview -Zunstable-options -Zcache-messages")
.masquerade_as_nightly_cargo()
.with_stderr_contains("[..]assert!(true)[..]")
.run();

// Again, reading from the cache.
p.cargo("clippy-preview -Zunstable-options -Zcache-messages")
.masquerade_as_nightly_cargo()
.with_stderr_contains("[..]assert!(true)[..]")
.run();

// FIXME: Unfortunately clippy is sharing the same hash with check. This
// causes the cache to be reused when it shouldn't.
yaahc marked this conversation as resolved.
Show resolved Hide resolved
p.cargo("check -Zcache-messages")
.masquerade_as_nightly_cargo()
.with_stderr_contains("[..]assert!(true)[..]") // This should not be here.
.run();
}
Loading