add lint paths_from_format

rust-lang · Jan 19, 2023 · b031924 · b031924
1 parent 89d443a
commit b031924
Show file tree

Hide file tree

Showing 6 changed files with 249 additions and 1 deletion.
diff --git 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
@@ -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
@@ -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
@@ -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/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!("{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
@@ -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
+