diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e31e8f0d981..0b9ac6039071 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4461,6 +4461,7 @@ Released 2018-09-13 [`partialeq_ne_impl`]: https://rust-lang.github.io/rust-clippy/master/index.html#partialeq_ne_impl [`partialeq_to_none`]: https://rust-lang.github.io/rust-clippy/master/index.html#partialeq_to_none [`path_buf_push_overwrite`]: https://rust-lang.github.io/rust-clippy/master/index.html#path_buf_push_overwrite +[`paths_from_format`]: https://rust-lang.github.io/rust-clippy/master/index.html#paths_from_format [`pattern_type_mismatch`]: https://rust-lang.github.io/rust-clippy/master/index.html#pattern_type_mismatch [`permissions_set_readonly_false`]: https://rust-lang.github.io/rust-clippy/master/index.html#permissions_set_readonly_false [`positional_named_format_parameters`]: https://rust-lang.github.io/rust-clippy/master/index.html#positional_named_format_parameters diff --git a/clippy_dev/src/update_lints.rs b/clippy_dev/src/update_lints.rs index 837618c9294b..693d7bf19033 100644 --- a/clippy_dev/src/update_lints.rs +++ b/clippy_dev/src/update_lints.rs @@ -397,7 +397,7 @@ pub fn deprecate(name: &str, reason: Option<&String>) { let Some(lint) = lints.iter().find(|l| l.name == name_lower) else { eprintln!("error: failed to find lint `{name}`"); return; }; let mod_path = { - let mut mod_path = PathBuf::from(format!("clippy_lints/src/{}", lint.module)); + let mut mod_path = Path::new("clippy_lints").join("src").join(&lint.module); if mod_path.is_dir() { mod_path = mod_path.join("mod"); } diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index d8e2ae02c5a6..be4403353463 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -234,6 +234,7 @@ mod partial_pub_fields; mod partialeq_ne_impl; mod partialeq_to_none; mod pass_by_ref_or_value; +mod paths_from_format; mod pattern_type_mismatch; mod permissions_set_readonly_false; mod precedence; @@ -908,6 +909,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|_| Box::new(fn_null_check::FnNullCheck)); store.register_late_pass(|_| Box::new(permissions_set_readonly_false::PermissionsSetReadonlyFalse)); store.register_late_pass(|_| Box::new(size_of_ref::SizeOfRef)); + store.register_late_pass(|_| Box::new(paths_from_format::PathsFromFormat)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/paths_from_format.rs b/clippy_lints/src/paths_from_format.rs new file mode 100644 index 000000000000..c33680f96c61 --- /dev/null +++ b/clippy_lints/src/paths_from_format.rs @@ -0,0 +1,124 @@ +use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::macros::{root_macro_call, FormatArgsExpn}; +use clippy_utils::sugg::Sugg; +use clippy_utils::ty::is_type_diagnostic_item; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::sym; +use std::fmt::Write as _; +use std::path::Path; + +declare_clippy_lint! { + /// ### What it does + /// Checks for `PathBuf::from(format!(..))` calls. + /// + /// ### Why is this bad? + /// It is not OS-agnostic, and can be harder to read. + /// + /// ### Known Problems + /// `.join()` introduces additional allocations that are not present when `PathBuf::push` is + /// used instead. + /// + /// ### Example + /// ```rust + /// use std::path::PathBuf; + /// let base_path = "/base"; + /// PathBuf::from(format!("{}/foo/bar", base_path)); + /// ``` + /// Use instead: + /// ```rust + /// use std::path::Path; + /// let base_path = "/base"; + /// Path::new(&base_path).join("foo").join("bar"); + /// ``` + #[clippy::version = "1.62.0"] + pub PATHS_FROM_FORMAT, + pedantic, + "builds a `PathBuf` from a format macro" +} + +declare_lint_pass!(PathsFromFormat => [PATHS_FROM_FORMAT]); + +impl<'tcx> LateLintPass<'tcx> for PathsFromFormat { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { + if_chain! { + if let ExprKind::Call(_, args) = expr.kind; + if let ty = cx.typeck_results().expr_ty(expr); + if is_type_diagnostic_item(cx, ty, sym::PathBuf); + if !args.is_empty(); + if let Some(macro_call) = root_macro_call(args[0].span); + if cx.tcx.item_name(macro_call.def_id) == sym::format; + if let Some(format_args) = FormatArgsExpn::find_nested(cx, &args[0], macro_call.expn); + then { + let format_string_parts = format_args.format_string.parts; + let format_value_args = format_args.args; + let string_parts: Vec<&str> = format_string_parts.iter().map(rustc_span::Symbol::as_str).collect(); + let mut applicability = Applicability::MachineApplicable; + let real_vars: Vec> = format_value_args.iter().map(|x| Sugg::hir_with_applicability(cx, x.param.value, "..", &mut applicability)).collect(); + let mut paths_zip = string_parts.iter().take(real_vars.len()).zip(real_vars.clone()); + let mut sugg = String::new(); + if let Some((part, arg)) = paths_zip.next() { + if is_valid_use_case(string_parts.first().unwrap_or(&""), string_parts.get(1).unwrap_or(&"")) { + return; + } + if part.is_empty() { + sugg = format!("Path::new(&{arg})"); + } + else { + push_comps(&mut sugg, part); + let _ = write!(sugg, ".join(&{arg})"); + } + } + for n in 1..real_vars.len() { + if let Some((part, arg)) = paths_zip.next() { + if is_valid_use_case(string_parts.get(n).unwrap_or(&""), string_parts.get(n+1).unwrap_or(&"")) { + return; + } + else if n < real_vars.len() { + push_comps(&mut sugg, part); + let _ = write!(sugg, ".join(&{arg})"); + } + else { + sugg = format!("{sugg}.join(&{arg})"); + } + } + } + if real_vars.len() < string_parts.len() { + push_comps(&mut sugg, string_parts[real_vars.len()]); + } + span_lint_and_sugg( + cx, + PATHS_FROM_FORMAT, + expr.span, + "`format!(..)` used to form `PathBuf`", + "consider using `Path::new()` and `.join()` to make it OS-agnostic and improve code readability", + sugg, + Applicability::MaybeIncorrect, + ); + } + } + } +} + +fn push_comps(string: &mut String, path: &str) { + let mut path = path.to_string(); + if !string.is_empty() { + path = path.trim_start_matches(|c| c == '\\' || c == '/').to_string(); + } + for n in Path::new(&path).components() { + let mut x = n.as_os_str().to_string_lossy().to_string(); + if string.is_empty() { + let _ = write!(string, "Path::new(\"{x}\")"); + } else { + x = x.trim_end_matches(|c| c == '/' || c == '\\').to_string(); + let _ = write!(string, ".join(\"{x}\")"); + } + } +} + +fn is_valid_use_case(string: &str, string2: &str) -> bool { + !(string.is_empty() || string.ends_with('/') || string.ends_with('\\')) + || !(string2.is_empty() || string2.starts_with('/') || string2.starts_with('\\')) +} diff --git a/tests/ui/paths_from_format.rs b/tests/ui/paths_from_format.rs new file mode 100644 index 000000000000..718d63c64c08 --- /dev/null +++ b/tests/ui/paths_from_format.rs @@ -0,0 +1,19 @@ +#![warn(clippy::paths_from_format)] + +use std::path::PathBuf; + +fn main() { + let mut base_path1 = ""; + let mut base_path2 = ""; + PathBuf::from(format!("{base_path1}/foo/bar")); + PathBuf::from(format!("/foo/bar/{base_path1}")); + PathBuf::from(format!("/foo/{base_path1}/bar")); + PathBuf::from(format!("foo/{base_path1}/bar")); + PathBuf::from(format!("foo/foooo/{base_path1}/bar/barrr")); + PathBuf::from(format!("foo/foooo/{base_path1}/bar/barrr/{base_path2}")); + PathBuf::from(format!("{base_path2}/foo/{base_path1}/bar")); + PathBuf::from(format!("foo/{base_path1}a/bar")); + PathBuf::from(format!("foo/a{base_path1}/bar")); + PathBuf::from(format!(r"C:\{base_path2}\foo\{base_path1}\bar")); + PathBuf::from(format!("C:\\{base_path2}\\foo\\{base_path1}\\bar")); +} diff --git a/tests/ui/paths_from_format.stderr b/tests/ui/paths_from_format.stderr new file mode 100644 index 000000000000..2754d87b435b --- /dev/null +++ b/tests/ui/paths_from_format.stderr @@ -0,0 +1,102 @@ +error: `format!(..)` used to form `PathBuf` + --> $DIR/paths_from_format.rs:8:5 + | +LL | PathBuf::from(format!("{base_path1}/foo/bar")); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::paths-from-format` implied by `-D warnings` +help: consider using `Path::new()` and `.join()` to make it OS-agnostic and improve code readability + | +LL | Path::new(&base_path1).join("foo").join("bar"); + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: `format!(..)` used to form `PathBuf` + --> $DIR/paths_from_format.rs:9:5 + | +LL | PathBuf::from(format!("/foo/bar/{base_path1}")); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider using `Path::new()` and `.join()` to make it OS-agnostic and improve code readability + | +LL | Path::new("/").join("foo").join("bar").join(&base_path1); + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: `format!(..)` used to form `PathBuf` + --> $DIR/paths_from_format.rs:10:5 + | +LL | PathBuf::from(format!("/foo/{base_path1}/bar")); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider using `Path::new()` and `.join()` to make it OS-agnostic and improve code readability + | +LL | Path::new("/").join("foo").join(&base_path1).join("bar"); + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: `format!(..)` used to form `PathBuf` + --> $DIR/paths_from_format.rs:11:5 + | +LL | PathBuf::from(format!("foo/{base_path1}/bar")); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider using `Path::new()` and `.join()` to make it OS-agnostic and improve code readability + | +LL | Path::new("foo").join(&base_path1).join("bar"); + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: `format!(..)` used to form `PathBuf` + --> $DIR/paths_from_format.rs:12:5 + | +LL | PathBuf::from(format!("foo/foooo/{base_path1}/bar/barrr")); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider using `Path::new()` and `.join()` to make it OS-agnostic and improve code readability + | +LL | Path::new("foo").join("foooo").join(&base_path1).join("bar").join("barrr"); + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: `format!(..)` used to form `PathBuf` + --> $DIR/paths_from_format.rs:13:5 + | +LL | PathBuf::from(format!("foo/foooo/{base_path1}/bar/barrr/{base_path2}")); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider using `Path::new()` and `.join()` to make it OS-agnostic and improve code readability + | +LL | Path::new("foo").join("foooo").join(&base_path1).join("bar").join("barrr").join(&base_path2); + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: `format!(..)` used to form `PathBuf` + --> $DIR/paths_from_format.rs:14:5 + | +LL | PathBuf::from(format!("{base_path2}/foo/{base_path1}/bar")); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider using `Path::new()` and `.join()` to make it OS-agnostic and improve code readability + | +LL | Path::new(&base_path2).join("foo").join(&base_path1).join("bar"); + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: `format!(..)` used to form `PathBuf` + --> $DIR/paths_from_format.rs:17:5 + | +LL | PathBuf::from(format!(r"C:/{base_path2}/foo/{base_path1}/bar")); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider using `Path::new()` and `.join()` to make it OS-agnostic and improve code readability + | +LL | Path::new("C:/").join(&base_path2).join("foo").join(&base_path1).join("bar"); + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: `format!(..)` used to form `PathBuf` + --> $DIR/paths_from_format.rs:18:5 + | +LL | PathBuf::from(format!("C:/{base_path2}/foo/{base_path1}/bar")); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider using `Path::new()` and `.join()` to make it OS-agnostic and improve code readability + | +LL | Path::new("C:/").join(&base_path2).join("foo").join(&base_path1).join("bar"); + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: aborting due to 9 previous errors +