diff --git a/cargo-dylint/Cargo.toml b/cargo-dylint/Cargo.toml index 3823e5f83..5c14f8546 100644 --- a/cargo-dylint/Cargo.toml +++ b/cargo-dylint/Cargo.toml @@ -42,3 +42,4 @@ dylint_internal = { version = "=2.6.1", path = "../internal", features = [ default = ["cargo-lib"] cargo-lib = ["dylint/__cargo_lib"] cargo-cli = ["dylint/__cargo_cli"] +__clap_headings = [] diff --git a/cargo-dylint/src/main.rs b/cargo-dylint/src/main.rs index 055bef5aa..8170833d6 100644 --- a/cargo-dylint/src/main.rs +++ b/cargo-dylint/src/main.rs @@ -41,36 +41,15 @@ METADATA EXAMPLE: ] "#, )] -// smoelius: Please keep the field `name_opts` first, and the fields `subcmd`, `names`, and `args` -// last. Please keep all other fields sorted. +// smoelius: Please keep the last four fields `args`, `operation`, `lib_sel`, and `output`, in that +// order. Please keep all other fields sorted. struct Dylint { - #[clap(flatten)] - name_opts: NameOpts, - - #[clap(long, hide = true)] - allow_downgrade: bool, - - #[clap(long, hide = true)] - bisect: bool, - #[clap(long, help = "Automatically apply lint suggestions")] fix: bool, - #[clap(long, hide = true)] - force: bool, - - #[clap(long, hide = true)] - isolate: bool, - #[clap(long, help = "Continue if `cargo check` fails")] keep_going: bool, - #[clap(long, hide = true)] - list: bool, - - #[clap(long = "new", hide = true)] - new_path: Option, - #[clap(long, help = "Do not check other packages within the workspace")] no_deps: bool, @@ -84,42 +63,24 @@ struct Dylint { )] packages: Vec, - #[clap(long, value_name = "path", help = "Path to pipe stderr to")] - pipe_stderr: Option, - - #[clap(long, value_name = "path", help = "Path to pipe stdout to")] - pipe_stdout: Option, - - #[clap( - global = true, - short, - long, - help = "Do not show warnings or progress running commands besides `cargo check` and \ - `cargo fix`" - )] - quiet: bool, - - #[clap(long, hide = true)] - rust_version: Option, - - #[clap(long = "upgrade", hide = true)] - upgrade_path: Option, - #[clap(long, help = "Check all packages in the workspace")] workspace: bool, + #[clap(last = true, help = "Arguments for `cargo check`")] + args: Vec, + #[clap(subcommand)] - subcmd: Option, + operation: Option, - #[clap(hide = true)] - names: Vec, + #[clap(flatten)] + lib_sel: LibrarySelection, - #[clap(last = true, help = "Arguments for `cargo check`")] - args: Vec, + #[clap(flatten)] + output: OutputOptions, } #[derive(Debug, Parser)] -enum DylintSubcommand { +enum Operation { #[clap( about = "List libraries or lints", long_about = "If no libraries are named, list the name, toolchain, and location of all \ @@ -132,7 +93,7 @@ Combine with `--all` to list all lints in all discovered libraries." )] List { #[clap(flatten)] - name_opts: NameOpts, + lib_sel: LibrarySelection, }, #[clap( @@ -156,13 +117,6 @@ Combine with `--all` to list all lints in all discovered libraries." #[clap(long, hide = true)] allow_downgrade: bool, - #[clap( - long, - help = "Unix only/experimental: Update dependencies and search for the most recent \ - applicable toolchain" - )] - bisect: bool, - #[clap( long, value_name = "version", @@ -176,7 +130,8 @@ Combine with `--all` to list all lints in all discovered libraries." } #[derive(Debug, Parser)] -struct NameOpts { +#[cfg_attr(feature = "__clap_headings", clap(next_help_heading = Some("Library Selection")))] +struct LibrarySelection { #[clap(long, help = "Load all discovered libraries")] all: bool, @@ -261,130 +216,81 @@ struct NameOpts { tag: Option, } -#[allow(deprecated)] +#[derive(Debug, Parser)] +#[cfg_attr(feature = "__clap_headings", clap(next_help_heading = Some("Output Options")))] +struct OutputOptions { + #[clap(long, value_name = "path", help = "Path to pipe stderr to")] + pipe_stderr: Option, + + #[clap(long, value_name = "path", help = "Path to pipe stdout to")] + pipe_stdout: Option, + + #[clap( + global = true, + short, + long, + help = "Do not show warnings or progress running commands besides `cargo check` and \ + `cargo fix`" + )] + quiet: bool, +} + impl From for dylint::opts::Dylint { fn from(opts: Dylint) -> Self { - let opts = process_deprecated_options(opts); let Dylint { - name_opts: - NameOpts { - all, - branch, - git, - lib_paths, - libs, - manifest_path, - no_build, - no_metadata, - paths, - pattern, - rev, - tag, - }, - allow_downgrade, - bisect, fix, - force, - isolate, keep_going, - list, - new_path, no_deps, packages, - pipe_stderr, - pipe_stdout, - quiet, - rust_version, - upgrade_path, workspace, - subcmd: _, - names, args, + operation, + mut lib_sel, + output: + OutputOptions { + pipe_stderr, + pipe_stdout, + quiet, + }, } = opts; - Self { - all, - allow_downgrade, - bisect, - branch, - fix, - force, - git, - isolate, - keep_going, - lib_paths, - libs, - list, - manifest_path, - new_path, - no_build, - no_deps, - no_metadata, - packages, - paths, - pattern, - pipe_stderr, - pipe_stdout, - quiet, - rev, - rust_version, - tag, - upgrade_path, - workspace, - names, - args, - } - } -} - -fn process_deprecated_options(mut opts: Dylint) -> Dylint { - if opts.list { - dylint::__warn( - &dylint::opts::Dylint::default(), - "`--list` is deprecated. Use subcommand `list`.", - ); - } - if opts.new_path.is_some() { - dylint::__warn( - &dylint::opts::Dylint::default(), - "`--new` is deprecated. Use subcommand `new`.", - ); - } - if opts.upgrade_path.is_some() { - dylint::__warn( - &dylint::opts::Dylint::default(), - "`--upgrade` is deprecated. Use subcommand `upgrade`.", - ); - } - if !opts.names.is_empty() { - dylint::__warn( - &dylint::opts::Dylint::default(), - "Referring to libraries by bare name is deprecated. Use `--lib` or `--lib-path`.", - ); - } - if let Some(subcmd) = opts.subcmd.take() { - match subcmd { - DylintSubcommand::List { name_opts } => { - opts.name_opts.absorb(name_opts); - opts.list = true; + let operation = match operation { + None => dylint::opts::Operation::Check({ + dylint::opts::Check { + lib_sel: lib_sel.into(), + fix, + keep_going, + no_deps, + packages, + workspace, + args, + } + }), + Some(Operation::List { lib_sel: other }) => { + lib_sel.absorb(other); + dylint::opts::Operation::List(dylint::opts::List { + lib_sel: lib_sel.into(), + }) } - DylintSubcommand::New { isolate, path } => { - opts.isolate |= isolate; - opts.new_path = Some(path); + Some(Operation::New { isolate, path }) => { + dylint::opts::Operation::New(dylint::opts::New { isolate, path }) } - DylintSubcommand::Upgrade { + Some(Operation::Upgrade { allow_downgrade, - bisect, rust_version, path, - } => { - opts.allow_downgrade |= allow_downgrade; - opts.bisect |= bisect; - opts.rust_version = rust_version; - opts.upgrade_path = Some(path); - } + }) => dylint::opts::Operation::Upgrade(dylint::opts::Upgrade { + allow_downgrade, + rust_version, + path, + }), + }; + Self { + pipe_stderr, + pipe_stdout, + quiet, + operation, } } - opts } macro_rules! option_absorb { @@ -400,7 +306,7 @@ macro_rules! option_absorb { }; } -impl NameOpts { +impl LibrarySelection { pub fn absorb(&mut self, other: Self) { let Self { all, @@ -431,6 +337,39 @@ impl NameOpts { } } +impl From for dylint::opts::LibrarySelection { + fn from(lib_sel: LibrarySelection) -> Self { + let LibrarySelection { + all, + branch, + git, + lib_paths, + libs, + manifest_path, + no_build, + no_metadata, + paths, + pattern, + rev, + tag, + } = lib_sel; + Self { + all, + branch, + git, + lib_paths, + libs, + manifest_path, + no_build, + no_metadata, + paths, + pattern, + rev, + tag, + } + } +} + fn main() -> dylint::ColorizedResult<()> { env_logger::try_init().unwrap_or_else(|error| { dylint::__warn( diff --git a/cargo-dylint/tests/fix.rs b/cargo-dylint/tests/fix.rs index e93fa906c..60e630306 100644 --- a/cargo-dylint/tests/fix.rs +++ b/cargo-dylint/tests/fix.rs @@ -62,7 +62,7 @@ fn fix() { std::process::Command::cargo_bin("cargo-dylint") .unwrap() .current_dir(&tempdir) - .args(["dylint", "--fix", LIB_NAME, "--", "--allow-dirty"]) + .args(["dylint", "--lib", LIB_NAME, "--fix", "--", "--allow-dirty"]) .assert() .success(); diff --git a/cargo-dylint/tests/package_options.rs b/cargo-dylint/tests/package_options.rs index e5eab484d..cc12fb615 100644 --- a/cargo-dylint/tests/package_options.rs +++ b/cargo-dylint/tests/package_options.rs @@ -105,38 +105,25 @@ fn downgrade_upgrade_package() { .success() .unwrap(); - if cfg!(not(unix)) { - std::process::Command::cargo_bin("cargo-dylint") - .unwrap() - .args(["dylint", "upgrade", &tempdir.path().to_string_lossy()]) - .assert() - .success(); - } else { - std::process::Command::cargo_bin("cargo-dylint") - .unwrap() - .args([ - "dylint", - "upgrade", - &tempdir.path().to_string_lossy(), - "--bisect", - ]) - .assert() - .success(); - - dylint_internal::cargo::build("upgraded dylint-template") - .build() - .sanitize_environment() - .current_dir(&tempdir) - .success() - .unwrap(); - - dylint_internal::cargo::test("upgraded dylint-template") - .build() - .sanitize_environment() - .current_dir(&tempdir) - .success() - .unwrap(); - } + std::process::Command::cargo_bin("cargo-dylint") + .unwrap() + .args(["dylint", "upgrade", &tempdir.path().to_string_lossy()]) + .assert() + .success(); + + dylint_internal::cargo::build("upgraded dylint-template") + .build() + .sanitize_environment() + .current_dir(&tempdir) + .success() + .unwrap(); + + dylint_internal::cargo::test("upgraded dylint-template") + .build() + .sanitize_environment() + .current_dir(&tempdir) + .success() + .unwrap(); } #[allow(dead_code)] diff --git a/dylint/src/lib.rs b/dylint/src/lib.rs index fcfd7737a..842fa8603 100644 --- a/dylint/src/lib.rs +++ b/dylint/src/lib.rs @@ -1,4 +1,3 @@ -#![allow(deprecated)] #![cfg_attr(dylint_lib = "general", allow(crate_wide_allow))] #![deny(clippy::expect_used)] #![deny(clippy::unwrap_used)] @@ -52,64 +51,46 @@ static REQUIRED_FORM: Lazy = Lazy::new(|| { ) }); +#[cfg_attr(dylint_lib = "general", allow(non_local_effect_before_error_return))] +#[cfg_attr(dylint_lib = "overscoped_allow", allow(overscoped_allow))] pub fn run(opts: &opts::Dylint) -> Result<()> { let opts = { + let opts_orig = opts; + let mut opts = opts.clone(); - if opts.force { - warn( - &opts, - "`--force` is deprecated and its meaning may change in the future. Use \ - `--allow-downgrade`.", - ); - opts.allow_downgrade = true; - opts.force = false; - } + if matches!( + opts.operation, + opts::Operation::Check(_) | opts::Operation::List(_) + ) { + let lib_sel = opts.library_selection_mut(); - let path_refers_to_libraries = - opts.paths - .iter() - .try_fold(false, |is_file, path| -> Result<_> { - let metadata = - metadata(path).with_context(|| "Could not read file metadata")?; - Ok(is_file || metadata.is_file()) - })?; - - if path_refers_to_libraries { - warn( - &opts, - "Referring to libraries with `--path` is deprecated. Use `--lib-path`.", - ); - opts.lib_paths.extend(opts.paths.split_off(0)); - }; + let path_refers_to_libraries = + lib_sel + .paths + .iter() + .try_fold(false, |is_file, path| -> Result<_> { + let metadata = + metadata(path).with_context(|| "Could not read file metadata")?; + Ok(is_file || metadata.is_file()) + })?; + + if path_refers_to_libraries { + warn( + opts_orig, + "Referring to libraries with `--path` is deprecated. Use `--lib-path`.", + ); + lib_sel.lib_paths.extend(lib_sel.paths.split_off(0)); + }; - // smoelius: Use of `--git` or `--path` implies `--all`. - opts.all |= opts.git_or_path(); + // smoelius: Use of `--git` or `--path` implies `--all`. + lib_sel.all |= lib_sel.git_or_path(); + } opts }; - if opts.allow_downgrade && opts.upgrade_path.is_none() { - bail!("`--allow-downgrade` can be used only with `--upgrade`"); - } - - if opts.bisect { - #[cfg(not(unix))] - bail!("`--bisect` is supported only on Unix platforms"); - - #[cfg(unix)] - warn(&opts, "`--bisect` is experimental"); - } - - if opts.bisect && opts.upgrade_path.is_none() { - bail!("`--bisect` can be used only with `--upgrade`"); - } - - if opts.isolate && opts.new_path.is_none() { - bail!("`--isolate` can be used only with `--new`"); - } - - if opts.pattern.is_some() && !opts.git_or_path() { + if opts.library_selection().pattern.is_some() && !opts.git_or_path() { bail!("`--pattern` can be used only with `--git` or `--path`"); } @@ -121,31 +102,28 @@ pub fn run(opts: &opts::Dylint) -> Result<()> { warn(&opts, "`--pipe-stdout` is experimental"); } - if opts.rust_version.is_some() && opts.upgrade_path.is_none() { - bail!("`--rust-version` can be used only with `--upgrade`"); - } - - #[cfg(feature = "package_options")] - if let Some(path) = &opts.new_path { - return package_options::new_package(&opts, Path::new(path)); - } - - #[cfg(feature = "package_options")] - if let Some(path) = &opts.upgrade_path { - return package_options::upgrade_package(&opts, Path::new(path)); + match &opts.operation { + opts::Operation::Check(_) | opts::Operation::List(_) => { + let name_toolchain_map = NameToolchainMap::new(&opts); + run_with_name_toolchain_map(&opts, &name_toolchain_map) + } + #[cfg(feature = "package_options")] + opts::Operation::New(new_opts) => package_options::new_package(&opts, new_opts), + #[cfg(feature = "package_options")] + opts::Operation::Upgrade(upgrade_opts) => { + package_options::upgrade_package(&opts, upgrade_opts) + } } - - let name_toolchain_map = NameToolchainMap::new(&opts); - - run_with_name_toolchain_map(&opts, &name_toolchain_map) } fn run_with_name_toolchain_map( opts: &opts::Dylint, name_toolchain_map: &NameToolchainMap, ) -> Result<()> { - if opts.libs.is_empty() && opts.lib_paths.is_empty() && opts.names.is_empty() && !opts.all { - if opts.list { + let lib_sel = opts.library_selection(); + + if lib_sel.libs.is_empty() && lib_sel.lib_paths.is_empty() && !lib_sel.all { + if matches!(opts.operation, opts::Operation::List(_)) { warn_if_empty(opts, name_toolchain_map)?; return list_libs(name_toolchain_map); } @@ -157,21 +135,21 @@ fn run_with_name_toolchain_map( let resolved = resolve(opts, name_toolchain_map)?; if resolved.is_empty() { - assert!(opts.libs.is_empty()); - assert!(opts.lib_paths.is_empty()); - assert!(opts.names.is_empty()); + assert!(lib_sel.libs.is_empty()); + assert!(lib_sel.lib_paths.is_empty()); let name_toolchain_map_is_empty = warn_if_empty(opts, name_toolchain_map)?; // smoelius: If `name_toolchain_map` is NOT empty, then it had better be the case that // `--all` was not passed. - assert!(name_toolchain_map_is_empty || !opts.all); + assert!(name_toolchain_map_is_empty || !lib_sel.all); } - if opts.list { - list_lints(opts, &resolved) - } else { - check_or_fix(opts, &resolved) + match &opts.operation { + opts::Operation::Check(check_opts) => check_or_fix(opts, check_opts, &resolved), + opts::Operation::List(_) => list_lints(opts, &resolved), + #[allow(unreachable_patterns)] + _ => unreachable!(), } } @@ -219,9 +197,11 @@ fn list_libs(name_toolchain_map: &NameToolchainMap) -> Result<()> { allow(question_mark_in_expression) )] fn resolve(opts: &opts::Dylint, name_toolchain_map: &NameToolchainMap) -> Result { + let lib_sel = opts.library_selection(); + let mut toolchain_map = ToolchainMap::new(); - if opts.all { + if lib_sel.all { let name_toolchain_map = name_toolchain_map.get_or_try_init()?; for other in name_toolchain_map.values() { @@ -238,51 +218,19 @@ fn resolve(opts: &opts::Dylint, name_toolchain_map: &NameToolchainMap) -> Result } } - for name in &opts.libs { - ensure!(!opts.all, "`--lib` cannot be used with `--all`"); + for name in &lib_sel.libs { + ensure!(!lib_sel.all, "`--lib` cannot be used with `--all`"); let (toolchain, maybe_library) = name_as_lib(name_toolchain_map, name, true)?.unwrap_or_else(|| unreachable!()); let path = maybe_library.build(opts)?; toolchain_map.entry(toolchain).or_default().insert(path); } - for name in &opts.lib_paths { + for name in &lib_sel.lib_paths { let (toolchain, path) = name_as_path(name, true)?.unwrap_or_else(|| unreachable!()); toolchain_map.entry(toolchain).or_default().insert(path); } - let mut not_found = Vec::new(); - - for name in &opts.names { - if let Some((toolchain, maybe_library)) = name_as_lib(name_toolchain_map, name, false)? { - ensure!( - !opts.all, - "`{}` is a library name and cannot be used with `--all`; if a path was meant, use \ - `--lib-path {}`", - name, - name - ); - let path = maybe_library.build(opts)?; - toolchain_map.entry(toolchain).or_default().insert(path); - } else if let Some((toolchain, path)) = name_as_path(name, false)? { - toolchain_map.entry(toolchain).or_default().insert(path); - } else { - not_found.push(name); - } - } - - #[allow(clippy::format_collect)] - if !not_found.is_empty() { - not_found.sort_unstable(); - bail!( - "Could not find the following libraries:{}", - not_found - .iter() - .map(|name| format!("\n {name}")) - .collect::() - ); - } - Ok(toolchain_map) } @@ -425,7 +373,11 @@ fn display_location(path: &Path) -> Result { .to_string()) } -fn check_or_fix(opts: &opts::Dylint, resolved: &ToolchainMap) -> Result<()> { +fn check_or_fix( + opts: &opts::Dylint, + check_opts: &opts::Check, + resolved: &ToolchainMap, +) -> Result<()> { let clippy_disable_docs_links = clippy_disable_docs_links()?; let mut failures = Vec::new(); @@ -444,23 +396,23 @@ fn check_or_fix(opts: &opts::Dylint, resolved: &ToolchainMap) -> Result<()> { .unwrap_or_default() .to_string(); let description = format!("with toolchain `{toolchain}`"); - let mut command = if opts.fix { + let mut command = if check_opts.fix { dylint_internal::cargo::fix(&description) } else { dylint_internal::cargo::check(&description) } .build(); let mut args = vec!["--target-dir", &target_dir_str]; - if let Some(path) = &opts.manifest_path { + if let Some(path) = &check_opts.lib_sel.manifest_path { args.extend(["--manifest-path", path]); } - for spec in &opts.packages { + for spec in &check_opts.packages { args.extend(["-p", spec]); } - if opts.workspace { + if check_opts.workspace { args.extend(["--workspace"]); } - args.extend(opts.args.iter().map(String::as_str)); + args.extend(check_opts.args.iter().map(String::as_str)); // smoelius: Set CLIPPY_DISABLE_DOCS_LINKS to prevent lints from accidentally linking to the // Clippy repository. But set it to the JSON-encoded original value so that the Clippy @@ -479,7 +431,10 @@ fn check_or_fix(opts: &opts::Dylint, resolved: &ToolchainMap) -> Result<()> { ), (env::DYLINT_LIBS, &dylint_libs), (env::DYLINT_METADATA, &dylint_metadata_str), - (env::DYLINT_NO_DEPS, if opts.no_deps { "1" } else { "0" }), + ( + env::DYLINT_NO_DEPS, + if check_opts.no_deps { "1" } else { "0" }, + ), (env::RUSTC_WORKSPACE_WRAPPER, &*driver.to_string_lossy()), (env::RUSTUP_TOOLCHAIN, toolchain), ]) @@ -505,7 +460,7 @@ fn check_or_fix(opts: &opts::Dylint, resolved: &ToolchainMap) -> Result<()> { let result = command.success(); if result.is_err() { - if !opts.keep_going { + if !check_opts.keep_going { return result .with_context(|| format!("Compilation failed with toolchain `{toolchain}`")); }; @@ -525,7 +480,7 @@ fn check_or_fix(opts: &opts::Dylint, resolved: &ToolchainMap) -> Result<()> { fn target_dir(opts: &opts::Dylint, toolchain: &str) -> Result { let mut command = MetadataCommand::new(); - if let Some(path) = &opts.manifest_path { + if let Some(path) = &opts.library_selection().manifest_path { command.manifest_path(path); } let metadata = command.no_deps().exec()?; @@ -559,8 +514,14 @@ mod test { static MUTEX: Mutex<()> = Mutex::new(()); static OPTS: Lazy = Lazy::new(|| opts::Dylint { - no_metadata: true, - ..opts::Dylint::default() + operation: opts::Operation::Check(opts::Check { + lib_sel: opts::LibrarySelection { + no_metadata: true, + ..Default::default() + }, + ..Default::default() + }), + ..Default::default() }); fn name_toolchain_map() -> NameToolchainMap<'static> { @@ -606,11 +567,17 @@ mod test { ); let opts = opts::Dylint { - libs: vec![ - "question_mark_in_expression".to_owned(), - "straggler".to_owned(), - ], - ..opts::Dylint::default() + operation: opts::Operation::Check(opts::Check { + lib_sel: opts::LibrarySelection { + libs: vec![ + "question_mark_in_expression".to_owned(), + "straggler".to_owned(), + ], + ..Default::default() + }, + ..Default::default() + }), + ..Default::default() }; run_with_name_toolchain_map(&opts, &name_toolchain_map).unwrap(); @@ -652,11 +619,17 @@ mod test { ); let opts = opts::Dylint { - libs: vec![ - "clippy".to_owned(), - "question_mark_in_expression".to_owned(), - ], - ..opts::Dylint::default() + operation: opts::Operation::Check(opts::Check { + lib_sel: opts::LibrarySelection { + libs: vec![ + "clippy".to_owned(), + "question_mark_in_expression".to_owned(), + ], + ..Default::default() + }, + ..Default::default() + }), + ..Default::default() }; run_with_name_toolchain_map(&opts, &name_toolchain_map).unwrap(); diff --git a/dylint/src/library_packages/cargo_cli/util/hex.rs b/dylint/src/library_packages/cargo_cli/util/hex.rs index 7911a89e2..912f573e7 100644 --- a/dylint/src/library_packages/cargo_cli/util/hex.rs +++ b/dylint/src/library_packages/cargo_cli/util/hex.rs @@ -1,6 +1,7 @@ // smoelius: This file is a slight modification of: // https://github.com/rust-lang/cargo/blob/aab416f6e68d555e8c9a0f02098a24946e0725fb/src/cargo/util/hex.rs +#![allow(deprecated)] #![allow(clippy::module_name_repetitions)] #![cfg_attr(dylint_lib = "overscoped_allow", allow(overscoped_allow))] #![cfg_attr(dylint_lib = "supplementary", allow(unnamed_constant))] diff --git a/dylint/src/library_packages/mod.rs b/dylint/src/library_packages/mod.rs index b91c0e9f3..7109788dc 100644 --- a/dylint/src/library_packages/mod.rs +++ b/dylint/src/library_packages/mod.rs @@ -84,16 +84,18 @@ struct Library { } pub fn from_opts(opts: &opts::Dylint) -> Result> { + let lib_sel = opts.library_selection(); + let maybe_metadata = cargo_metadata(opts)?; let metadata = maybe_metadata.ok_or_else(|| anyhow!("Could not read cargo metadata"))?; ensure!( - opts.paths.len() <= 1, + lib_sel.paths.len() <= 1, "At most one library package can be named with `--path`" ); - let path = if let Some(path) = opts.paths.first() { + let path = if let Some(path) = lib_sel.paths.first() { let canonical_path = canonicalize(path).with_context(|| format!("Could not canonicalize {path:?}"))?; Some(canonical_path.to_string_lossy().to_string()) @@ -103,10 +105,10 @@ pub fn from_opts(opts: &opts::Dylint) -> Result> { let toml: toml::map::Map<_, _> = vec![ to_map_entry("path", path.as_ref()), - to_map_entry("git", opts.git.as_ref()), - to_map_entry("branch", opts.branch.as_ref()), - to_map_entry("tag", opts.tag.as_ref()), - to_map_entry("rev", opts.rev.as_ref()), + to_map_entry("git", lib_sel.git.as_ref()), + to_map_entry("branch", lib_sel.branch.as_ref()), + to_map_entry("tag", lib_sel.tag.as_ref()), + to_map_entry("rev", lib_sel.rev.as_ref()), ] .into_iter() .flatten() @@ -116,7 +118,7 @@ pub fn from_opts(opts: &opts::Dylint) -> Result> { let library = Library { details, - pattern: opts.pattern.clone(), + pattern: lib_sel.pattern.clone(), }; library_packages(opts, metadata, &[library]) @@ -163,20 +165,22 @@ static CARGO_METADATA: OnceCell> = OnceCell::new(); fn cargo_metadata(opts: &opts::Dylint) -> Result> { CARGO_METADATA .get_or_try_init(|| { - if opts.no_metadata { + let lib_sel = opts.library_selection(); + + if lib_sel.no_metadata { return Ok(None); } let mut command = MetadataCommand::new(); - if let Some(path) = &opts.manifest_path { + if let Some(path) = &lib_sel.manifest_path { command.manifest_path(path); } match command.exec() { Ok(metadata) => Ok(Some(metadata)), Err(err) => { - if opts.manifest_path.is_none() { + if lib_sel.manifest_path.is_none() { if_chain! { if let Error::CargoMetadata { stderr } = err; if let Some(line) = stderr.lines().next(); @@ -380,7 +384,7 @@ pub fn build_library(opts: &opts::Dylint, package: &Package) -> Result let path = package.path(); - if !opts.no_build { + if !opts.library_selection().no_build { // smoelius: Clear `RUSTFLAGS` so that changes to it do not cause workspace metadata entries // to be rebuilt. dylint_internal::cargo::build(&format!("workspace metadata entry `{}`", package.id.name())) diff --git a/dylint/src/opts.rs b/dylint/src/opts.rs index 4c80968d6..9afd90b9f 100644 --- a/dylint/src/opts.rs +++ b/dylint/src/opts.rs @@ -1,79 +1,178 @@ +//! Warning: Addition of public fields to the structs in this module is considered a non-breaking +//! change. +//! +//! For this reason, we recommend using [struct update syntax] when initializing instances of the +//! structs in this module. +//! +//! Example: +//! +//! ``` +//! # use crate::dylint::opts::Dylint; +//! let opts = Dylint { +//! quiet: true, +//! ..Default::default() +//! }; +//! ``` +//! +//! [struct update syntax]: https://doc.rust-lang.org/book/ch05-01-defining-structs.html#creating-instances-from-other-instances-with-struct-update-syntax + +#[cfg(feature = "package_options")] +use once_cell::sync::Lazy; + #[allow(clippy::struct_excessive_bools)] #[derive(Clone, Debug, Default)] -// smoelius: Please keep the fields `names` and `args` last. Please keep all other fields sorted. pub struct Dylint { - pub all: bool, + pub pipe_stderr: Option, - #[deprecated] - pub allow_downgrade: bool, + pub pipe_stdout: Option, - #[deprecated] - pub bisect: bool, + pub quiet: bool, - pub branch: Option, + pub operation: Operation, +} - pub fix: bool, +#[derive(Clone, Debug, Default)] +pub struct LibrarySelection { + pub all: bool, - #[deprecated] - pub force: bool, + pub branch: Option, pub git: Option, - #[deprecated] - pub isolate: bool, - - pub keep_going: bool, - pub lib_paths: Vec, pub libs: Vec, - #[deprecated] - pub list: bool, - pub manifest_path: Option, - #[deprecated] - pub new_path: Option, - pub no_build: bool, - pub no_deps: bool, - pub no_metadata: bool, - pub packages: Vec, - pub paths: Vec, pub pattern: Option, - pub pipe_stderr: Option, + pub rev: Option, - pub pipe_stdout: Option, + pub tag: Option, +} - pub quiet: bool, +#[derive(Clone, Debug)] +#[non_exhaustive] +pub enum Operation { + Check(Check), + List(List), + #[cfg(feature = "package_options")] + New(New), + #[cfg(feature = "package_options")] + Upgrade(Upgrade), +} - pub rev: Option, +#[allow(clippy::struct_excessive_bools)] +#[derive(Clone, Debug, Default)] +pub struct Check { + pub lib_sel: LibrarySelection, - #[deprecated] - pub rust_version: Option, + pub fix: bool, - pub tag: Option, + pub keep_going: bool, - #[deprecated] - pub upgrade_path: Option, + pub no_deps: bool, - pub workspace: bool, + pub packages: Vec, - #[deprecated] - pub names: Vec, + pub workspace: bool, pub args: Vec, } +#[derive(Clone, Debug, Default)] +pub struct List { + pub lib_sel: LibrarySelection, +} + +#[cfg(feature = "package_options")] +#[derive(Clone, Debug, Default)] +pub struct New { + pub isolate: bool, + + pub path: String, +} + +#[cfg(feature = "package_options")] +#[derive(Clone, Debug, Default)] +pub struct Upgrade { + pub allow_downgrade: bool, + + pub rust_version: Option, + + pub path: String, +} + impl Dylint { + #[must_use] + pub fn library_selection(&self) -> &LibrarySelection { + self.operation.library_selection() + } + + pub fn library_selection_mut(&mut self) -> &mut LibrarySelection { + self.operation.library_selection_mut() + } + + pub(crate) fn git_or_path(&self) -> bool { + self.library_selection().git_or_path() + } +} + +impl LibrarySelection { pub(crate) fn git_or_path(&self) -> bool { self.git.is_some() || !self.paths.is_empty() } } + +#[cfg(feature = "package_options")] +static LIBRARY_SELECTION: Lazy = Lazy::new(LibrarySelection::default); + +impl Operation { + fn library_selection(&self) -> &LibrarySelection { + match self { + Self::Check(check) => &check.lib_sel, + Self::List(list) => &list.lib_sel, + #[cfg(feature = "package_options")] + Self::New(_) | Self::Upgrade(_) => { + if cfg!(debug_assertions) { + eprintln!( + "[{}:{}] {}", + file!(), + line!(), + "`library_selection` called on an `Operation` with no `LibrarySelection` \ + field" + ); + } + &LIBRARY_SELECTION + } + } + } + + #[allow(clippy::panic)] + fn library_selection_mut(&mut self) -> &mut LibrarySelection { + match self { + Self::Check(check) => &mut check.lib_sel, + Self::List(list) => &mut list.lib_sel, + #[cfg(feature = "package_options")] + Self::New(_) | Self::Upgrade(_) => { + panic!( + "`library_selection_mut` called on an `Operation` with no `LibrarySelection` \ + field" + ) + } + } + } +} + +impl Default for Operation { + fn default() -> Self { + Self::Check(Check::default()) + } +} diff --git a/dylint/src/package_options/bisect.rs b/dylint/src/package_options/bisect.rs deleted file mode 100644 index bc8e09783..000000000 --- a/dylint/src/package_options/bisect.rs +++ /dev/null @@ -1,156 +0,0 @@ -use crate::opts::Dylint; -use anyhow::{anyhow, Context, Result}; -use dylint_internal::{rustup::SanitizeEnvironment, CommandExt}; -use is_terminal::IsTerminal; -use std::os::unix::fs::PermissionsExt; -use std::{ - fs::{remove_file, rename}, - io::Write, - path::Path, - process::{Command, Stdio}, -}; -use tempfile::NamedTempFile; - -const SCRIPT: &str = r#"#! /bin/bash - -set -euo pipefail - -CHANNEL="$(echo "$RUSTUP_TOOLCHAIN" | sed -n 's/^bisector-\(nightly-[0-9]\{4\}-[0-9]\{2\}-[0-9]\{2\}\)-.*$/\1/;T;p')" - -unset RUSTUP_TOOLCHAIN - -if [[ -z "$CHANNEL" ]]; then - if [[ ! -f first_commit_seen ]]; then - touch first_commit_seen - exit 1 - else - exit 0 - fi -fi - -# smoelius: The rust-toolchain file should reflect the most recent successful build. So if the -# current build fails, restore the rust-toolchain file's previous contents. -TMP="$(mktemp -p . -t rust-toolchain.XXXXXXXXXX)" -cp rust-toolchain "$TMP" -trap ' - STATUS="$?" - if [[ "$STATUS" -eq 0 ]]; then - rm "$TMP" - else - mv "$TMP" rust-toolchain - fi - exit "$STATUS" -' EXIT - -sed -i "s/^channel = \"[^\"]*\"$/channel = \"$CHANNEL\"/" rust-toolchain - -if [[ ! -f first_channel_seen ]]; then - cargo build --all-targets || (touch first_channel_seen && false) -elif [[ ! -f successful_build_seen ]]; then - # smoelius: Pretend the build succeeds (even if it doesn't) and proceed backwards until we find - # one that actually succeeds. - (cargo build --all-targets && touch successful_build_seen) || true -else - cargo build --all-targets -fi -"#; - -const TEMPORARY_FILES: &[&str] = &[ - "first_channel_seen", - "first_commit_seen", - "successful_build_seen", -]; - -pub fn bisect(opts: &Dylint, path: &Path, start: &str) -> Result<()> { - Command::new("cargo") - .args(["bisect-rustc", "-V"]) - .success() - .with_context(|| "Could not find `cargo-bisect-rustc`. Is it installed?")?; - - let script = script()?; - - let test_dir = path - .canonicalize() - .with_context(|| format!("Could not canonicalize {path:?}"))?; - - remove_temporary_files(path); - - let mut command = Command::new("cargo"); - command.args([ - "bisect-rustc", - "--start", - start, - "--preserve", - "--regress=success", - "--script", - &*script.path().to_string_lossy(), - "--test-dir", - &*test_dir.to_string_lossy(), - ]); - if opts.quiet { - command.stderr(Stdio::null()); - } - // smoelius: `cargo-bisect-rustc`'s progress bars are displayed on `stdout`. - if opts.quiet || !std::io::stdout().is_terminal() { - command.stdout(Stdio::null()); - } - let result = command.success(); - - remove_temporary_files(path); - - result?; - - // smoelius: The build might not actually have succeeded. (See the note above about proceeding - // backwards.) So verify that the build succeeded before returning `Ok(())`. - let file_name = path - .file_name() - .ok_or_else(|| anyhow!("Could not get file name"))?; - let description = format!("`{}`", file_name.to_string_lossy()); - dylint_internal::cargo::build(&description) - .quiet(opts.quiet) - .build() - .sanitize_environment() - .current_dir(path) - .args(["--all-targets"]) - .success() -} - -fn script() -> Result { - let tempfile_unopened = - NamedTempFile::new_in(".").with_context(|| "Could not create named temp file")?; - { - let mut tempfile_opened = - NamedTempFile::new_in(".").with_context(|| "Could not create named temp file")?; - tempfile_opened - .write_all(SCRIPT.as_bytes()) - .with_context(|| format!("Could not write to {:?}", tempfile_opened.path()))?; - - let metadata = tempfile_opened - .as_file() - .metadata() - .with_context(|| format!("Could not get metadata of {:?}", tempfile_opened.path()))?; - let mut permissions = metadata.permissions(); - permissions.set_mode(permissions.mode() | 0o111); - tempfile_opened - .as_file() - .set_permissions(permissions) - .with_context(|| { - format!("Could not set permissions of {:?}", tempfile_opened.path()) - })?; - - rename(&tempfile_opened, &tempfile_unopened).with_context(|| { - format!( - "Could not rename {:?} to {:?}", - tempfile_opened.path(), - tempfile_unopened.path() - ) - })?; - } - Ok(tempfile_unopened) -} - -fn remove_temporary_files(path: &Path) { - for temporary_file in TEMPORARY_FILES { - remove_file(path.join(temporary_file)).unwrap_or_default(); - } -} diff --git a/dylint/src/package_options/mod.rs b/dylint/src/package_options/mod.rs index 804798af4..b9db8b903 100644 --- a/dylint/src/package_options/mod.rs +++ b/dylint/src/package_options/mod.rs @@ -1,4 +1,4 @@ -use crate::opts::Dylint; +use crate::opts; use anyhow::{anyhow, bail, Context, Result}; use dylint_internal::{ clippy_utils::{ @@ -18,19 +18,15 @@ use std::{ use tempfile::tempdir; use walkdir::WalkDir; -#[cfg(unix)] -use dylint_internal::{rustup::SanitizeEnvironment, CommandExt}; - -#[cfg(unix)] -mod bisect; - mod backup; use backup::Backup; mod revs; use revs::Revs; -pub fn new_package(opts: &Dylint, path: &Path) -> Result<()> { +pub fn new_package(_opts: &opts::Dylint, new_opts: &opts::New) -> Result<()> { + let path = Path::new(&new_opts.path); + let name = path .file_name() .map(|s| s.to_string_lossy().to_string()) @@ -41,7 +37,7 @@ pub fn new_package(opts: &Dylint, path: &Path) -> Result<()> { new_template(tempdir.path())?; // smoelius: Isolation is now the default. - if !opts.isolate { + if !new_opts.isolate { find_and_replace( &tempdir.path().join("Cargo.toml"), &[r"s/\r?\n\[workspace\]\r?\n//"], @@ -111,11 +107,13 @@ fn fill_in(name: &str, from: &Path, to: &Path) -> Result<()> { Ok(()) } -pub fn upgrade_package(opts: &Dylint, path: &Path) -> Result<()> { +pub fn upgrade_package(opts: &opts::Dylint, upgrade_opts: &opts::Upgrade) -> Result<()> { + let path = Path::new(&upgrade_opts.path); + let rev = { let revs = Revs::new(opts.quiet)?; let mut iter = revs.iter()?; - match &opts.rust_version { + match &upgrade_opts.rust_version { Some(rust_version) => { let clippy_utils_version = clippy_utils_version_from_rust_version(rust_version)?; iter.find(|result| { @@ -138,23 +136,18 @@ pub fn upgrade_package(opts: &Dylint, path: &Path) -> Result<()> { let old_channel = toolchain_channel(path)?; - let should_find_and_replace = if_chain! { - if !opts.allow_downgrade; + if_chain! { + if !upgrade_opts.allow_downgrade; if let Some(new_nightly) = parse_as_nightly(&rev.channel); if let Some(old_nightly) = parse_as_nightly(&old_channel); if new_nightly < old_nightly; then { - if !opts.bisect { - bail!( - "Refusing to downgrade toolchain from `{}` to `{}`. \ - Use `--allow-downgrade` to override.", - old_channel, - rev.channel - ); - } - false - } else { - true + bail!( + "Refusing to downgrade toolchain from `{}` to `{}`. \ + Use `--allow-downgrade` to override.", + old_channel, + rev.channel + ); } }; @@ -166,46 +159,8 @@ pub fn upgrade_package(opts: &Dylint, path: &Path) -> Result<()> { let mut cargo_toml_backup = Backup::new(cargo_toml_path).with_context(|| "Could not backup `Cargo.toml`")?; - if should_find_and_replace { - set_toolchain_channel(path, &rev.channel)?; - set_clippy_utils_dependency_revision(path, &rev.rev)?; - } - - #[cfg(unix)] - if opts.bisect { - let file_name = path - .file_name() - .ok_or_else(|| anyhow!("Could not get file name"))?; - let description = format!("`{}`", file_name.to_string_lossy()); - - dylint_internal::cargo::update(&description) - .quiet(opts.quiet) - .build() - .sanitize_environment() - .current_dir(path) - .success()?; - - if dylint_internal::cargo::build(&description) - .quiet(opts.quiet) - .build() - .sanitize_environment() - .current_dir(path) - .args(["--all-targets"]) - .success() - .is_err() - { - let new_nightly = parse_as_nightly(&rev.channel).ok_or_else(|| { - anyhow!("Could not not parse channel `{}` as nightly", rev.channel) - })?; - - let start = format!( - "{:04}-{:02}-{:02}", - new_nightly[0], new_nightly[1], new_nightly[2] - ); - - bisect::bisect(opts, path, &start)?; - } - } + set_toolchain_channel(path, &rev.channel)?; + set_clippy_utils_dependency_revision(path, &rev.rev)?; cargo_toml_backup .disable() diff --git a/examples/testing/clippy/tests/ui.rs b/examples/testing/clippy/tests/ui.rs index 20f14aa98..3eeb74b30 100644 --- a/examples/testing/clippy/tests/ui.rs +++ b/examples/testing/clippy/tests/ui.rs @@ -36,7 +36,8 @@ fn ui() { let dylint_libs = dylint_testing::dylint_libs("clippy").unwrap(); let driver = - dylint::driver_builder::get(&dylint::Dylint::default(), env!("RUSTUP_TOOLCHAIN")).unwrap(); + dylint::driver_builder::get(&dylint::opts::Dylint::default(), env!("RUSTUP_TOOLCHAIN")) + .unwrap(); // smoelius: Clippy's `compile-test` panics if multiple rlibs exist for certain crates (see // `third_party_crates` in