Add unnecessary_debug_formatting lint

smoelius · smoelius · commit 4e1cad138e8e · 2024-12-29T06:42:34.000-05:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6110,6 +6110,7 @@ Released 2018-09-13
 [`unnecessary_box_returns`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_box_returns
 [`unnecessary_cast`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
 [`unnecessary_clippy_cfg`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_clippy_cfg
+[`unnecessary_debug_formatting`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_debug_formatting
 [`unnecessary_fallible_conversions`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_fallible_conversions
 [`unnecessary_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_filter_map
 [`unnecessary_find_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_find_map
diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs
@@ -191,6 +191,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
     crate::format_args::FORMAT_IN_FORMAT_ARGS_INFO,
     crate::format_args::TO_STRING_IN_FORMAT_ARGS_INFO,
     crate::format_args::UNINLINED_FORMAT_ARGS_INFO,
+    crate::format_args::UNNECESSARY_DEBUG_FORMATTING_INFO,
     crate::format_args::UNUSED_FORMAT_SPECS_INFO,
     crate::format_impl::PRINT_IN_FORMAT_IMPL_INFO,
     crate::format_impl::RECURSIVE_FORMAT_IMPL_INFO,
diff --git a/clippy_lints/src/format_args.rs b/clippy_lints/src/format_args.rs
@@ -1,26 +1,27 @@
 use arrayvec::ArrayVec;
 use clippy_config::Conf;
-use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
+use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg, span_lint_and_then};
 use clippy_utils::is_diag_trait_item;
 use clippy_utils::macros::{
     FormatArgsStorage, FormatParamUsage, MacroCall, find_format_arg_expr, format_arg_removal_span,
     format_placeholder_format_span, is_assert_macro, is_format_macro, is_panic, matching_root_macro_call,
     root_macro_call_first_node,
 };
 use clippy_utils::msrvs::{self, Msrv};
-use clippy_utils::source::SpanRangeExt;
+use clippy_utils::source::{SpanRangeExt, snippet};
 use clippy_utils::ty::{implements_trait, is_type_lang_item};
 use itertools::Itertools;
 use rustc_ast::{
     FormatArgPosition, FormatArgPositionKind, FormatArgsPiece, FormatArgumentKind, FormatCount, FormatOptions,
     FormatPlaceholder, FormatTrait,
 };
+use rustc_data_structures::fx::FxHashMap;
 use rustc_errors::Applicability;
 use rustc_errors::SuggestionStyle::{CompletelyHidden, ShowCode};
 use rustc_hir::{Expr, ExprKind, LangItem};
 use rustc_lint::{LateContext, LateLintPass, LintContext};
-use rustc_middle::ty::Ty;
 use rustc_middle::ty::adjustment::{Adjust, Adjustment};
+use rustc_middle::ty::{List, Ty, TyCtxt};
 use rustc_session::impl_lint_pass;
 use rustc_span::edition::Edition::Edition2021;
 use rustc_span::{Span, Symbol, sym};
@@ -50,6 +51,33 @@ declare_clippy_lint! {
     "`format!` used in a macro that does formatting"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for `Debug` formatting (`{:?}`) applied to an `OsStr` or `Path`.
+    ///
+    /// ### Why is this bad?
+    /// Rust doesn't guarantee what `Debug` formatting looks like, and it could
+    /// change in the future. `OsStr`s and `Path`s can be `Display` formatted
+    /// using their `display` methods.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// # use std::path::Path;
+    /// let path = Path::new("...");
+    /// println!("The path is {:?}", path);
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// # use std::path::Path;
+    /// let path = Path::new("…");
+    /// println!("The path is {}", path.display());
+    /// ```
+    #[clippy::version = "1.85.0"]
+    pub UNNECESSARY_DEBUG_FORMATTING,
+    restriction,
+    "`Debug` formatting applied to an `OsStr` or `Path`"
+}
+
 declare_clippy_lint! {
     /// ### What it does
     /// Checks for [`ToString::to_string`](https://doc.rust-lang.org/std/string/trait.ToString.html#tymethod.to_string)
@@ -162,31 +190,35 @@ declare_clippy_lint! {
     "use of a format specifier that has no effect"
 }
 
-impl_lint_pass!(FormatArgs => [
+impl_lint_pass!(FormatArgs<'_> => [
     FORMAT_IN_FORMAT_ARGS,
     TO_STRING_IN_FORMAT_ARGS,
     UNINLINED_FORMAT_ARGS,
+    UNNECESSARY_DEBUG_FORMATTING,
     UNUSED_FORMAT_SPECS,
 ]);
 
 #[allow(clippy::struct_field_names)]
-pub struct FormatArgs {
+pub struct FormatArgs<'tcx> {
     format_args: FormatArgsStorage,
     msrv: Msrv,
     ignore_mixed: bool,
+    ty_feature_map: FxHashMap<Ty<'tcx>, Option<Symbol>>,
 }
 
-impl FormatArgs {
-    pub fn new(conf: &'static Conf, format_args: FormatArgsStorage) -> Self {
+impl<'tcx> FormatArgs<'tcx> {
+    pub fn new(tcx: TyCtxt<'tcx>, conf: &'static Conf, format_args: FormatArgsStorage) -> Self {
+        let ty_feature_map = make_ty_feature_map(tcx);
         Self {
             format_args,
             msrv: conf.msrv.clone(),
             ignore_mixed: conf.allow_mixed_uninlined_format_args,
+            ty_feature_map,
         }
     }
 }
 
-impl<'tcx> LateLintPass<'tcx> for FormatArgs {
+impl<'tcx> LateLintPass<'tcx> for FormatArgs<'tcx> {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
         if let Some(macro_call) = root_macro_call_first_node(cx, expr)
             && is_format_macro(cx, macro_call.def_id)
@@ -198,6 +230,7 @@ impl<'tcx> LateLintPass<'tcx> for FormatArgs {
                 macro_call: &macro_call,
                 format_args,
                 ignore_mixed: self.ignore_mixed,
+                ty_feature_map: &self.ty_feature_map,
             };
 
             linter.check_templates();
@@ -217,9 +250,10 @@ struct FormatArgsExpr<'a, 'tcx> {
     macro_call: &'a MacroCall,
     format_args: &'a rustc_ast::FormatArgs,
     ignore_mixed: bool,
+    ty_feature_map: &'a FxHashMap<Ty<'tcx>, Option<Symbol>>,
 }
 
-impl FormatArgsExpr<'_, '_> {
+impl<'tcx> FormatArgsExpr<'_, 'tcx> {
     fn check_templates(&self) {
         for piece in &self.format_args.template {
             if let FormatArgsPiece::Placeholder(placeholder) = piece
@@ -237,6 +271,11 @@ impl FormatArgsExpr<'_, '_> {
                     self.check_format_in_format_args(name, arg_expr);
                     self.check_to_string_in_format_args(name, arg_expr);
                 }
+
+                if placeholder.format_trait == FormatTrait::Debug {
+                    let name = self.cx.tcx.item_name(self.macro_call.def_id);
+                    self.check_unnecessary_debug_formatting(name, arg_expr);
+                }
             }
         }
     }
@@ -439,6 +478,24 @@ impl FormatArgsExpr<'_, '_> {
         }
     }
 
+    fn check_unnecessary_debug_formatting(&self, name: Symbol, value: &Expr<'_>) {
+        let cx = self.cx;
+        if !value.span.from_expansion()
+            && let ty = cx.typeck_results().expr_ty(value)
+            && self.can_display_format(ty)
+        {
+            let snippet = snippet(cx.sess(), value.span, "..");
+            span_lint_and_help(
+                cx,
+                UNNECESSARY_DEBUG_FORMATTING,
+                value.span,
+                format!("unnecessary `Debug` formatting in `{name}!` args"),
+                None,
+                format!("use `Display` formatting and change this to `{snippet}.display()`"),
+            );
+        }
+    }
+
     fn format_arg_positions(&self) -> impl Iterator<Item = (&FormatArgPosition, FormatParamUsage)> {
         self.format_args.template.iter().flat_map(|piece| match piece {
             FormatArgsPiece::Placeholder(placeholder) => {
@@ -465,6 +522,40 @@ impl FormatArgsExpr<'_, '_> {
             .at_most_one()
             .is_err()
     }
+
+    fn can_display_format(&self, ty: Ty<'tcx>) -> bool {
+        let ty = ty.peel_refs();
+
+        if let Some(feature) = self.ty_feature_map.get(&ty)
+            && feature.map_or(true, |feature| self.cx.tcx.features().enabled(feature))
+        {
+            return true;
+        }
+
+        // Even if `ty` is not in `self.ty_feature_map`, check whether `ty` implements `Deref` with with a
+        // `Target` that is in `self.ty_feature_map`.
+        if let Some(deref_trait_id) = self.cx.tcx.lang_items().deref_trait()
+            && let Some(target_ty) = self.cx.get_associated_type(ty, deref_trait_id, "Target")
+            && let Some(feature) = self.ty_feature_map.get(&target_ty)
+            && feature.map_or(true, |feature| self.cx.tcx.features().enabled(feature))
+        {
+            return true;
+        }
+
+        false
+    }
+}
+
+fn make_ty_feature_map(tcx: TyCtxt<'_>) -> FxHashMap<Ty<'_>, Option<Symbol>> {
+    [(sym::OsStr, Some(Symbol::intern("os_str_display"))), (sym::Path, None)]
+        .into_iter()
+        .filter_map(|(name, feature)| {
+            tcx.get_diagnostic_item(name).map(|def_id| {
+                let ty = Ty::new_adt(tcx, tcx.adt_def(def_id), List::empty());
+                (ty, feature)
+            })
+        })
+        .collect()
 }
 
 fn count_needed_derefs<'tcx, I>(mut ty: Ty<'tcx>, mut iter: I) -> (usize, Ty<'tcx>)
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -835,7 +835,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     store.register_late_pass(move |_| Box::new(non_send_fields_in_send_ty::NonSendFieldInSendTy::new(conf)));
     store.register_late_pass(move |_| Box::new(undocumented_unsafe_blocks::UndocumentedUnsafeBlocks::new(conf)));
     let format_args = format_args_storage.clone();
-    store.register_late_pass(move |_| Box::new(format_args::FormatArgs::new(conf, format_args.clone())));
+    store.register_late_pass(move |tcx| Box::new(format_args::FormatArgs::new(tcx, conf, format_args.clone())));
     store.register_late_pass(|_| Box::new(trailing_empty_array::TrailingEmptyArray));
     store.register_early_pass(|| Box::new(octal_escapes::OctalEscapes));
     store.register_late_pass(|_| Box::new(needless_late_init::NeedlessLateInit));
diff --git a/tests/ui/unnecessary_os_str_debug_formatting.rs b/tests/ui/unnecessary_os_str_debug_formatting.rs
@@ -0,0 +1,20 @@
+#![feature(os_str_display)]
+#![warn(clippy::unnecessary_debug_formatting)]
+
+use std::ffi::{OsStr, OsString};
+
+fn main() {
+    let os_str = OsStr::new("abc");
+    let os_string = os_str.to_os_string();
+
+    // negative tests
+    println!("{}", os_str.display());
+    println!("{}", os_string.display());
+
+    // positive tests
+    println!("{:?}", os_str);
+    println!("{:?}", os_string);
+
+    let _: String = format!("{:?}", os_str);
+    let _: String = format!("{:?}", os_string);
+}
diff --git a/tests/ui/unnecessary_os_str_debug_formatting.stderr b/tests/ui/unnecessary_os_str_debug_formatting.stderr
@@ -0,0 +1,36 @@
+error: unnecessary `Debug` formatting in `println!` args
+  --> tests/ui/unnecessary_os_str_debug_formatting.rs:15:22
+   |
+LL |     println!("{:?}", os_str);
+   |                      ^^^^^^
+   |
+   = help: use `Display` formatting and change this to `os_str.display()`
+   = note: `-D clippy::unnecessary-debug-formatting` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::unnecessary_debug_formatting)]`
+
+error: unnecessary `Debug` formatting in `println!` args
+  --> tests/ui/unnecessary_os_str_debug_formatting.rs:16:22
+   |
+LL |     println!("{:?}", os_string);
+   |                      ^^^^^^^^^
+   |
+   = help: use `Display` formatting and change this to `os_string.display()`
+
+error: unnecessary `Debug` formatting in `format!` args
+  --> tests/ui/unnecessary_os_str_debug_formatting.rs:18:37
+   |
+LL |     let _: String = format!("{:?}", os_str);
+   |                                     ^^^^^^
+   |
+   = help: use `Display` formatting and change this to `os_str.display()`
+
+error: unnecessary `Debug` formatting in `format!` args
+  --> tests/ui/unnecessary_os_str_debug_formatting.rs:19:37
+   |
+LL |     let _: String = format!("{:?}", os_string);
+   |                                     ^^^^^^^^^
+   |
+   = help: use `Display` formatting and change this to `os_string.display()`
+
+error: aborting due to 4 previous errors
+
diff --git a/tests/ui/unnecessary_path_debug_formatting.rs b/tests/ui/unnecessary_path_debug_formatting.rs
@@ -0,0 +1,41 @@
+#![warn(clippy::unnecessary_debug_formatting)]
+
+use std::ffi::{OsStr, OsString};
+use std::ops::Deref;
+use std::path::{Path, PathBuf};
+
+struct DerefPath<'a> {
+    path: &'a Path,
+}
+
+impl Deref for DerefPath<'_> {
+    type Target = Path;
+    fn deref(&self) -> &Self::Target {
+        self.path
+    }
+}
+
+fn main() {
+    let path = Path::new("/a/b/c");
+    let path_buf = path.to_path_buf();
+    let os_str = OsStr::new("abc");
+    let os_string = os_str.to_os_string();
+
+    // negative tests
+    println!("{}", path.display());
+    println!("{}", path_buf.display());
+
+    // should not fire because feature `os_str_display` is not enabled
+    println!("{:?}", os_str);
+    println!("{:?}", os_string);
+
+    // positive tests
+    println!("{:?}", path);
+    println!("{:?}", path_buf);
+
+    let _: String = format!("{:?}", path);
+    let _: String = format!("{:?}", path_buf);
+
+    let deref_path = DerefPath { path };
+    println!("{:?}", &*deref_path);
+}
diff --git a/tests/ui/unnecessary_path_debug_formatting.stderr b/tests/ui/unnecessary_path_debug_formatting.stderr
@@ -0,0 +1,44 @@
+error: unnecessary `Debug` formatting in `println!` args
+  --> tests/ui/unnecessary_path_debug_formatting.rs:33:22
+   |
+LL |     println!("{:?}", path);
+   |                      ^^^^
+   |
+   = help: use `Display` formatting and change this to `path.display()`
+   = note: `-D clippy::unnecessary-debug-formatting` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::unnecessary_debug_formatting)]`
+
+error: unnecessary `Debug` formatting in `println!` args
+  --> tests/ui/unnecessary_path_debug_formatting.rs:34:22
+   |
+LL |     println!("{:?}", path_buf);
+   |                      ^^^^^^^^
+   |
+   = help: use `Display` formatting and change this to `path_buf.display()`
+
+error: unnecessary `Debug` formatting in `format!` args
+  --> tests/ui/unnecessary_path_debug_formatting.rs:36:37
+   |
+LL |     let _: String = format!("{:?}", path);
+   |                                     ^^^^
+   |
+   = help: use `Display` formatting and change this to `path.display()`
+
+error: unnecessary `Debug` formatting in `format!` args
+  --> tests/ui/unnecessary_path_debug_formatting.rs:37:37
+   |
+LL |     let _: String = format!("{:?}", path_buf);
+   |                                     ^^^^^^^^
+   |
+   = help: use `Display` formatting and change this to `path_buf.display()`
+
+error: unnecessary `Debug` formatting in `println!` args
+  --> tests/ui/unnecessary_path_debug_formatting.rs:40:22
+   |
+LL |     println!("{:?}", &*deref_path);
+   |                      ^^^^^^^^^^^^
+   |
+   = help: use `Display` formatting and change this to `&*deref_path.display()`
+
+error: aborting due to 5 previous errors
+
