add lint paths_from_format

merelymyself · merelymyself · commit fe92ccb4b8d7 · 2022-10-03T19:49:29.000+08:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4133,6 +4133,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
 [`positional_named_format_parameters`]: https://rust-lang.github.io/rust-clippy/master/index.html#positional_named_format_parameters
 [`possible_missing_comma`]: https://rust-lang.github.io/rust-clippy/master/index.html#possible_missing_comma
diff --git a/clippy_dev/src/update_lints.rs b/clippy_dev/src/update_lints.rs
@@ -419,7 +419,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.register_lints.rs b/clippy_lints/src/lib.register_lints.rs
@@ -479,6 +479,7 @@ store.register_lints(&[
     partialeq_to_none::PARTIALEQ_TO_NONE,
     pass_by_ref_or_value::LARGE_TYPES_PASSED_BY_VALUE,
     pass_by_ref_or_value::TRIVIALLY_COPY_PASS_BY_REF,
+    paths_from_format::PATHS_FROM_FORMAT,
     pattern_type_mismatch::PATTERN_TYPE_MISMATCH,
     precedence::PRECEDENCE,
     ptr::CMP_NULL,
diff --git a/clippy_lints/src/lib.register_pedantic.rs b/clippy_lints/src/lib.register_pedantic.rs
@@ -82,6 +82,7 @@ store.register_group(true, "clippy::pedantic", Some("clippy_pedantic"), vec![
     LintId::of(operators::VERBOSE_BIT_MASK),
     LintId::of(pass_by_ref_or_value::LARGE_TYPES_PASSED_BY_VALUE),
     LintId::of(pass_by_ref_or_value::TRIVIALLY_COPY_PASS_BY_REF),
+    LintId::of(paths_from_format::PATHS_FROM_FORMAT),
     LintId::of(ranges::RANGE_MINUS_ONE),
     LintId::of(ranges::RANGE_PLUS_ONE),
     LintId::of(redundant_else::REDUNDANT_ELSE),
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -327,6 +327,7 @@ mod panic_unimplemented;
 mod partialeq_ne_impl;
 mod partialeq_to_none;
 mod pass_by_ref_or_value;
+mod paths_from_format;
 mod pattern_type_mismatch;
 mod precedence;
 mod ptr;
@@ -906,6 +907,9 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|_| Box::new(bool_to_int_with_if::BoolToIntWithIf));
     store.register_late_pass(|_| Box::new(box_default::BoxDefault));
     store.register_late_pass(|_| Box::new(implicit_saturating_add::ImplicitSaturatingAdd));
+    store.register_late_pass(|| Box::new(bool_to_int_with_if::BoolToIntWithIf));
+    store.register_late_pass(|| Box::new(box_default::BoxDefault));
+    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
@@ -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<Sugg<'_>> = 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/src/docs.rs b/src/docs.rs
@@ -394,6 +394,7 @@ docs! {
     "partialeq_ne_impl",
     "partialeq_to_none",
     "path_buf_push_overwrite",
+    "paths_from_format",
     "pattern_type_mismatch",
     "possible_missing_comma",
     "precedence",
diff --git a/src/docs/paths_from_format.txt b/src/docs/paths_from_format.txt
@@ -0,0 +1,22 @@
+### 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
+```
+use std::path::PathBuf;
+let base_path = "/base";
+PathBuf::from(format!("{}/foo/bar", base_path));
+```
+Use instead:
+```
+use std::path::Path;
+let base_path = "/base";
+Path::new(base_path).join("foo").join("bar");
+```
diff --git a/tests/ui/paths_from_format.rs 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!("{}/foo/bar", base_path1));
+    PathBuf::from(format!("/foo/bar/{}", base_path1));
+    PathBuf::from(format!("/foo/{}/bar", base_path1));
+    PathBuf::from(format!("foo/{}/bar", base_path1));
+    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
@@ -0,0 +1,102 @@
+error: `format!(..)` used to form `PathBuf`
+  --> $DIR/paths_from_format.rs:8:5
+   |
+LL |     PathBuf::from(format!("{}/foo/bar", base_path1));
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = 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/{}/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(&base_path1).join("bar");
+   |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+error: `format!(..)` used to form `PathBuf`
+  --> $DIR/paths_from_format.rs:11: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("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
+

Original file line number	Diff line number	Diff line change
`@@ -419,7 +419,7 @@ pub fn deprecate(name: &str, reason: Option<&String>) {`
`419`	`419`	let Some(lint) = lints.iter().find(\|l\| l.name == name_lower) else { eprintln!("error: failed to find lint `{name}`"); return; };
`420`	`420`
`421`	`421`	`let mod_path = {`
`422`		`- let mut mod_path = PathBuf::from(format!("clippy_lints/src/{}", lint.module));`
	`422`	`+ let mut mod_path = Path::new("clippy_lints").join("src").join(&lint.module);`
`423`	`423`	`if mod_path.is_dir() {`
`424`	`424`	`mod_path = mod_path.join("mod");`
`425`	`425`	`}`