From b5b0d15f0c4a42d31fe8e04966717bfb892fa14b Mon Sep 17 00:00:00 2001 From: GnomedDev Date: Wed, 23 Oct 2024 23:15:00 +0100 Subject: [PATCH] Implement a lint for replacable Command::new --- CHANGELOG.md | 2 + README.md | 2 +- book/src/README.md | 2 +- book/src/lint_configuration.md | 10 ++ clippy_config/src/conf.rs | 11 ++ clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 2 + clippy_lints/src/unnecessary_shell_command.rs | 107 ++++++++++++++++++ clippy_utils/src/ty.rs | 25 ++-- .../toml_unknown_key/conf_unknown_key.stderr | 3 + tests/ui/unnecessary_shell_command.rs | 18 +++ tests/ui/unnecessary_shell_command.stderr | 44 +++++++ 12 files changed, 216 insertions(+), 11 deletions(-) create mode 100644 clippy_lints/src/unnecessary_shell_command.rs create mode 100644 tests/ui/unnecessary_shell_command.rs create mode 100644 tests/ui/unnecessary_shell_command.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 4bdbc91db939..681a02649385 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6075,6 +6075,7 @@ Released 2018-09-13 [`unnecessary_safety_comment`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_safety_comment [`unnecessary_safety_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_safety_doc [`unnecessary_self_imports`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_self_imports +[`unnecessary_shell_command`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_shell_command [`unnecessary_sort_by`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_sort_by [`unnecessary_struct_initialization`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_struct_initialization [`unnecessary_to_owned`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_to_owned @@ -6221,6 +6222,7 @@ Released 2018-09-13 [`trivial-copy-size-limit`]: https://doc.rust-lang.org/clippy/lint_configuration.html#trivial-copy-size-limit [`type-complexity-threshold`]: https://doc.rust-lang.org/clippy/lint_configuration.html#type-complexity-threshold [`unnecessary-box-size`]: https://doc.rust-lang.org/clippy/lint_configuration.html#unnecessary-box-size +[`unnecessary-commands`]: https://doc.rust-lang.org/clippy/lint_configuration.html#unnecessary-commands [`unreadable-literal-lint-fractions`]: https://doc.rust-lang.org/clippy/lint_configuration.html#unreadable-literal-lint-fractions [`upper-case-acronyms-aggressive`]: https://doc.rust-lang.org/clippy/lint_configuration.html#upper-case-acronyms-aggressive [`vec-box-size-threshold`]: https://doc.rust-lang.org/clippy/lint_configuration.html#vec-box-size-threshold diff --git a/README.md b/README.md index ec76a6dfb08e..1690e2beb16f 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are over 700 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are over 750 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) Lints are divided into categories, each with a default [lint level](https://doc.rust-lang.org/rustc/lints/levels.html). You can choose how much Clippy is supposed to ~~annoy~~ help you by changing the lint level by category. diff --git a/book/src/README.md b/book/src/README.md index 7bdfb97c3acf..23527ba896af 100644 --- a/book/src/README.md +++ b/book/src/README.md @@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are over 700 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are over 750 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) Lints are divided into categories, each with a default [lint level](https://doc.rust-lang.org/rustc/lints/levels.html). You can choose how diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md index 43b551ae2161..a4ed9bc0d737 100644 --- a/book/src/lint_configuration.md +++ b/book/src/lint_configuration.md @@ -893,6 +893,16 @@ The byte size a `T` in `Box` can have, below which it triggers the `clippy::u * [`unnecessary_box_returns`](https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_box_returns) +## `unnecessary-commands` +The list of shell commands to recommend replacing. + +**Default Value:** `{ curl = "use `std::net`, `ureq`, or `reqwest` instead", jq = "use `serde_json`, or another native json parser instead", ls = "use `std::fs::read_dir` instead", sed = "use `regex`, or the methods on `str` instead", wget = "use `std::net`, `ureq`, or `reqwest` instead" }` + +--- +**Affected lints:** +* [`unnecessary_shell_commands`](https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_shell_commands) + + ## `unreadable-literal-lint-fractions` Should the fraction of a decimal be linted to include separators. diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs index 4757c0b1339a..94e9007d9c32 100644 --- a/clippy_config/src/conf.rs +++ b/clippy_config/src/conf.rs @@ -7,6 +7,8 @@ use rustc_span::edit_distance::edit_distance; use rustc_span::{BytePos, Pos, SourceFile, Span, SyntaxContext}; use serde::de::{IgnoredAny, IntoDeserializer, MapAccess, Visitor}; use serde::{Deserialize, Deserializer, Serialize}; +use std::borrow::Cow; +use std::collections::BTreeMap; use std::fmt::{Debug, Display, Formatter}; use std::ops::Range; use std::path::PathBuf; @@ -648,6 +650,15 @@ define_Conf! { /// The byte size a `T` in `Box` can have, below which it triggers the `clippy::unnecessary_box` lint #[lints(unnecessary_box_returns)] unnecessary_box_size: u64 = 128, + /// The list of shell commands to recommend replacing. + #[lints(unnecessary_shell_commands)] + unnecessary_commands: BTreeMap, Option>> = [ + ["ls", "use `std::fs::read_dir` instead"], + ["curl", "use `std::net`, `ureq`, or `reqwest` instead"], + ["wget", "use `std::net`, `ureq`, or `reqwest` instead"], + ["sed", "use `regex`, or the methods on `str` instead"], + ["jq", "use `serde_json`, or another native json parser instead"], + ].into_iter().map(|[k, v]| (Box::::from(k), Some(Cow::Borrowed(v)))).collect(), /// Should the fraction of a decimal be linted to include separators. #[lints(unreadable_literal)] unreadable_literal_lint_fractions: bool = true, diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index d8918d37afa9..7b852f5c6c85 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -744,6 +744,7 @@ pub static LINTS: &[&crate::LintInfo] = &[ crate::unnecessary_map_on_constructor::UNNECESSARY_MAP_ON_CONSTRUCTOR_INFO, crate::unnecessary_owned_empty_strings::UNNECESSARY_OWNED_EMPTY_STRINGS_INFO, crate::unnecessary_self_imports::UNNECESSARY_SELF_IMPORTS_INFO, + crate::unnecessary_shell_command::UNNECESSARY_SHELL_COMMAND_INFO, crate::unnecessary_struct_initialization::UNNECESSARY_STRUCT_INITIALIZATION_INFO, crate::unnecessary_wraps::UNNECESSARY_WRAPS_INFO, crate::unnested_or_patterns::UNNESTED_OR_PATTERNS_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index a0ff8316d5cd..22033dc146f6 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -367,6 +367,7 @@ mod unnecessary_literal_bound; mod unnecessary_map_on_constructor; mod unnecessary_owned_empty_strings; mod unnecessary_self_imports; +mod unnecessary_shell_command; mod unnecessary_struct_initialization; mod unnecessary_wraps; mod unnested_or_patterns; @@ -949,5 +950,6 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { store.register_late_pass(move |_| Box::new(unused_trait_names::UnusedTraitNames::new(conf))); store.register_late_pass(|_| Box::new(manual_ignore_case_cmp::ManualIgnoreCaseCmp)); store.register_late_pass(|_| Box::new(unnecessary_literal_bound::UnnecessaryLiteralBound)); + store.register_late_pass(move |_| Box::new(unnecessary_shell_command::UnnecessaryShellCommand::new(conf))); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/unnecessary_shell_command.rs b/clippy_lints/src/unnecessary_shell_command.rs new file mode 100644 index 000000000000..52859736d829 --- /dev/null +++ b/clippy_lints/src/unnecessary_shell_command.rs @@ -0,0 +1,107 @@ +use std::borrow::Cow; +use std::collections::BTreeMap; + +use clippy_config::Conf; +use clippy_utils::consts::{ConstEvalCtxt, Constant}; +use clippy_utils::diagnostics::{span_lint, span_lint_and_help}; +use clippy_utils::path_def_id; +use clippy_utils::ty::get_inherent_method; +use rustc_hir::def_id::DefId; +use rustc_hir::{Expr, ExprKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::impl_lint_pass; +use rustc_span::sym; + +declare_clippy_lint! { + /// ### What it does + /// + /// Detects calls to `std::process::Command::new` that can easily be replaced with Rust code. + /// + /// ### Why is this bad? + /// + /// "Shelling out" is slow, non-portable, and generally unnecessary (especially to these programs). + /// + /// ### Example + /// ```no_run + /// use std::io; + /// use std::process::Command; + /// + /// fn list_files() -> io::Result> { + /// let output = Command::new("ls").output()?; + /// if !output.status.success() { + /// return Err(io::Error::new( + /// io::ErrorKind::Other, + /// String::from_utf8_lossy(&output.stderr) + /// )); + /// } + /// + /// let stdout = std::str::from_utf8(&output.stdout).expect("should be UTF-8 output"); + /// Ok(stdout.split_whitespace().map(String::from).collect()) + /// } + /// // example code where clippy issues a warning + /// ``` + /// Use instead: + /// ```no_run + /// fn list_files() -> std::io::Result> { + /// let mut buf = Vec::new(); + /// for entry_res in std::fs::read_dir(".")? { + /// let path = entry_res?.path(); + /// let os_name = path.into_os_string(); + /// buf.push(os_name.into_string().expect("should be UTF-8 paths")) + /// } + /// + /// Ok(buf) + /// } + /// ``` + #[clippy::version = "1.84.0"] + pub UNNECESSARY_SHELL_COMMAND, + pedantic, + "using the simple shell utilities instead of Rust code" +} + +pub struct UnnecessaryShellCommand { + std_process_command_new: Option, + unnecessary_commands: &'static BTreeMap, Option>>, +} + +impl UnnecessaryShellCommand { + pub fn new(conf: &'static Conf) -> Self { + Self { + std_process_command_new: None, + unnecessary_commands: &conf.unnecessary_commands, + } + } +} + +impl_lint_pass!(UnnecessaryShellCommand => [UNNECESSARY_SHELL_COMMAND]); + +impl LateLintPass<'_> for UnnecessaryShellCommand { + fn check_crate(&mut self, cx: &LateContext<'_>) { + if let Some(command_did) = cx.tcx.get_diagnostic_item(sym::Command) + && let Some(fn_item) = get_inherent_method(cx, command_did, sym::new) + { + self.std_process_command_new = Some(fn_item.def_id); + } + } + + fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) { + let Some(std_process_command_new) = self.std_process_command_new else { + return; + }; + + if let ExprKind::Call(func, [command]) = expr.kind + && path_def_id(cx, func) == Some(std_process_command_new) + && let Some(Constant::Str(command_lit)) = ConstEvalCtxt::new(cx).eval(command) + && let command_lit = command_lit.strip_suffix(".exe").unwrap_or(&command_lit) + && let Some(help) = self.unnecessary_commands.get(command_lit) + { + let lint = UNNECESSARY_SHELL_COMMAND; + let msg = "unnecessarily shelling out for trivial operation"; + if let Some(help) = help.as_deref() { + span_lint_and_help(cx, lint, command.span, msg, None, help); + } else { + span_lint(cx, lint, command.span, msg); + } + } + } +} diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs index b7a3569ccf0f..99bdccc8b677 100644 --- a/clippy_utils/src/ty.rs +++ b/clippy_utils/src/ty.rs @@ -1323,22 +1323,29 @@ pub fn deref_chain<'cx, 'tcx>(cx: &'cx LateContext<'tcx>, ty: Ty<'tcx>) -> impl /// Checks if a Ty<'_> has some inherent method Symbol. /// -/// This does not look for impls in the type's `Deref::Target` type. -/// If you need this, you should wrap this call in `clippy_utils::ty::deref_chain().any(...)`. +/// This is a helper for [`get_inherent_method`]. pub fn get_adt_inherent_method<'a>(cx: &'a LateContext<'_>, ty: Ty<'_>, method_name: Symbol) -> Option<&'a AssocItem> { if let Some(ty_did) = ty.ty_adt_def().map(AdtDef::did) { - cx.tcx.inherent_impls(ty_did).iter().find_map(|&did| { - cx.tcx - .associated_items(did) - .filter_by_name_unhygienic(method_name) - .next() - .filter(|item| item.kind == AssocKind::Fn) - }) + get_inherent_method(cx, ty_did, method_name) } else { None } } +/// Checks if the [`DefId`] of a Ty has some inherent method Symbol. +/// +/// This does not look for impls in the type's `Deref::Target` type. +/// If you need this, you should wrap this call in `clippy_utils::ty::deref_chain().any(...)`. +pub fn get_inherent_method<'a>(cx: &'a LateContext<'_>, ty_did: DefId, method_name: Symbol) -> Option<&'a AssocItem> { + cx.tcx.inherent_impls(ty_did).iter().find_map(|&did| { + cx.tcx + .associated_items(did) + .filter_by_name_unhygienic(method_name) + .next() + .filter(|item| item.kind == AssocKind::Fn) + }) +} + /// Get's the type of a field by name. pub fn get_field_by_name<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>, name: Symbol) -> Option> { match *ty.kind() { diff --git a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr index 5cf9c0fb2710..3313188c104a 100644 --- a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr +++ b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr @@ -71,6 +71,7 @@ error: error reading Clippy's configuration file: unknown field `foobar`, expect trivial-copy-size-limit type-complexity-threshold unnecessary-box-size + unnecessary-commands unreadable-literal-lint-fractions upper-case-acronyms-aggressive vec-box-size-threshold @@ -155,6 +156,7 @@ error: error reading Clippy's configuration file: unknown field `barfoo`, expect trivial-copy-size-limit type-complexity-threshold unnecessary-box-size + unnecessary-commands unreadable-literal-lint-fractions upper-case-acronyms-aggressive vec-box-size-threshold @@ -239,6 +241,7 @@ error: error reading Clippy's configuration file: unknown field `allow_mixed_uni trivial-copy-size-limit type-complexity-threshold unnecessary-box-size + unnecessary-commands unreadable-literal-lint-fractions upper-case-acronyms-aggressive vec-box-size-threshold diff --git a/tests/ui/unnecessary_shell_command.rs b/tests/ui/unnecessary_shell_command.rs new file mode 100644 index 000000000000..6c72e3f2d538 --- /dev/null +++ b/tests/ui/unnecessary_shell_command.rs @@ -0,0 +1,18 @@ +#![warn(clippy::unnecessary_shell_command)] + +use std::process::Command; + +fn main() { + let _ = Command::new("ls"); + //~^ error: unnecessarily shelling out for trivial operation + let _ = Command::new("curl"); + //~^ error: unnecessarily shelling out for trivial operation + let _ = Command::new("wget"); + //~^ error: unnecessarily shelling out for trivial operation + let _ = Command::new("sed"); + //~^ error: unnecessarily shelling out for trivial operation + let _ = Command::new("jq"); + //~^ error: unnecessarily shelling out for trivial operation + + let _ = Command::new("ffmpeg"); +} diff --git a/tests/ui/unnecessary_shell_command.stderr b/tests/ui/unnecessary_shell_command.stderr new file mode 100644 index 000000000000..26ff6aff5155 --- /dev/null +++ b/tests/ui/unnecessary_shell_command.stderr @@ -0,0 +1,44 @@ +error: unnecessarily shelling out for trivial operation + --> tests/ui/unnecessary_shell_command.rs:6:26 + | +LL | let _ = Command::new("ls"); + | ^^^^ + | + = help: use `std::fs::read_dir` instead + = note: `-D clippy::unnecessary-shell-command` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::unnecessary_shell_command)]` + +error: unnecessarily shelling out for trivial operation + --> tests/ui/unnecessary_shell_command.rs:8:26 + | +LL | let _ = Command::new("curl"); + | ^^^^^^ + | + = help: use `std::net`, `ureq`, or `reqwest` instead + +error: unnecessarily shelling out for trivial operation + --> tests/ui/unnecessary_shell_command.rs:10:26 + | +LL | let _ = Command::new("wget"); + | ^^^^^^ + | + = help: use `std::net`, `ureq`, or `reqwest` instead + +error: unnecessarily shelling out for trivial operation + --> tests/ui/unnecessary_shell_command.rs:12:26 + | +LL | let _ = Command::new("sed"); + | ^^^^^ + | + = help: use `regex`, or the methods on `str` instead + +error: unnecessarily shelling out for trivial operation + --> tests/ui/unnecessary_shell_command.rs:14:26 + | +LL | let _ = Command::new("jq"); + | ^^^^ + | + = help: use `serde_json`, or another native json parser instead + +error: aborting due to 5 previous errors +