Add unnecessary_debug_formatting lint (#13893)

Fixes #12674, i.e., adds a lint to flag `Path`s printed with `{:?}`.

Nits are welcome.

changelog: Add `unnecessary_debug_formatting` lint

CHANGELOG.md

@@ -6210,6 +6210,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

clippy_lints/src/declared_lints.rs

@@ -193,6 +193,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,

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::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 clippy_utils::{is_diag_trait_item, is_from_proc_macro};
 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,36 @@ 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.
+    ///
+    /// Furthermore, with `Debug` formatting, certain characters are escaped.
+    /// Thus, a `Debug` formatted `Path` is less likely to be clickable.
+    ///
+    /// ### 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.87.0"]
+    pub UNNECESSARY_DEBUG_FORMATTING,
+    pedantic,
+    "`Debug` formatting applied to an `OsStr` or `Path` when `.display()` is available"
+}
+
 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 +193,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 +233,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 +253,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 +274,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 +481,33 @@ impl FormatArgsExpr<'_, '_> {
         }
     }
 
+    fn check_unnecessary_debug_formatting(&self, name: Symbol, value: &Expr<'tcx>) {
+        let cx = self.cx;
+        if !value.span.from_expansion()
+            && !is_from_proc_macro(cx, value)
+            && let ty = cx.typeck_results().expr_ty(value)
+            && self.can_display_format(ty)
+        {
+            let snippet = snippet(cx.sess(), value.span, "..");
+            span_lint_and_then(
+                cx,
+                UNNECESSARY_DEBUG_FORMATTING,
+                value.span,
+                format!("unnecessary `Debug` formatting in `{name}!` args"),
+                |diag| {
+                    diag.help(format!(
+                        "use `Display` formatting and change this to `{snippet}.display()`"
+                    ));
+                    diag.note(
+                        "switching to `Display` formatting will change how the value is shown; \
+                         escaped characters will no longer be escaped and surrounding quotes will \
+                         be removed",
+                    );
+                },
+            );
+        }
+    }
+
     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 +534,41 @@ 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.is_none_or(|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
+        // a `Target` that is in `self.ty_feature_map`.
+        if let Some(deref_trait_id) = self.cx.tcx.lang_items().deref_trait()
+            && implements_trait(self.cx, ty, deref_trait_id, &[])
+            && 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.is_none_or(|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>)

clippy_lints/src/lib.rs

@@ -841,7 +841,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));

lintcheck/src/input.rs

@@ -288,11 +288,11 @@ impl CrateWithSource {
                 // as a result of this filter.
                 let dest_crate_root = PathBuf::from(LINTCHECK_SOURCES).join(name);
                 if dest_crate_root.exists() {
-                    println!("Deleting existing directory at {dest_crate_root:?}");
+                    println!("Deleting existing directory at `{}`", dest_crate_root.display());
                     fs::remove_dir_all(&dest_crate_root).unwrap();
                 }
 
-                println!("Copying {path:?} to {dest_crate_root:?}");
+                println!("Copying `{}` to `{}`", path.display(), dest_crate_root.display());
 
                 for entry in WalkDir::new(path).into_iter().filter_entry(|e| !is_cache_dir(e)) {
                     let entry = entry.unwrap();

tests/compile-test.rs

@@ -383,13 +383,15 @@ fn ui_cargo_toml_metadata() {
                 .map(|component| component.as_os_str().to_string_lossy().replace('-', "_"))
                 .any(|s| *s == name)
                 || path.starts_with(&cargo_common_metadata_path),
-            "{path:?} has incorrect package name"
+            "`{}` has incorrect package name",
+            path.display(),
         );
 
         let publish = package.get("publish").and_then(toml::Value::as_bool).unwrap_or(true);
         assert!(
             !publish || publish_exceptions.contains(&path.parent().unwrap().to_path_buf()),
-            "{path:?} lacks `publish = false`"
+            "`{}` lacks `publish = false`",
+            path.display(),
         );
     }
 }

tests/ui/unnecessary_os_str_debug_formatting.rs

@@ -0,0 +1,23 @@
+#![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); //~ unnecessary_debug_formatting
+    println!("{:?}", os_string); //~ unnecessary_debug_formatting
+
+    println!("{os_str:?}"); //~ unnecessary_debug_formatting
+    println!("{os_string:?}"); //~ unnecessary_debug_formatting
+
+    let _: String = format!("{:?}", os_str); //~ unnecessary_debug_formatting
+    let _: String = format!("{:?}", os_string); //~ unnecessary_debug_formatting
+}

tests/ui/unnecessary_os_str_debug_formatting.stderr

@@ -0,0 +1,58 @@
+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: switching to `Display` formatting will change how the value is shown; escaped characters will no longer be escaped and surrounding quotes will be removed
+   = 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()`
+   = note: switching to `Display` formatting will change how the value is shown; escaped characters will no longer be escaped and surrounding quotes will be removed
+
+error: unnecessary `Debug` formatting in `println!` args
+  --> tests/ui/unnecessary_os_str_debug_formatting.rs:18:16
+   |
+LL |     println!("{os_str:?}");
+   |                ^^^^^^
+   |
+   = help: use `Display` formatting and change this to `os_str.display()`
+   = note: switching to `Display` formatting will change how the value is shown; escaped characters will no longer be escaped and surrounding quotes will be removed
+
+error: unnecessary `Debug` formatting in `println!` args
+  --> tests/ui/unnecessary_os_str_debug_formatting.rs:19:16
+   |
+LL |     println!("{os_string:?}");
+   |                ^^^^^^^^^
+   |
+   = help: use `Display` formatting and change this to `os_string.display()`
+   = note: switching to `Display` formatting will change how the value is shown; escaped characters will no longer be escaped and surrounding quotes will be removed
+
+error: unnecessary `Debug` formatting in `format!` args
+  --> tests/ui/unnecessary_os_str_debug_formatting.rs:21:37
+   |
+LL |     let _: String = format!("{:?}", os_str);
+   |                                     ^^^^^^
+   |
+   = help: use `Display` formatting and change this to `os_str.display()`
+   = note: switching to `Display` formatting will change how the value is shown; escaped characters will no longer be escaped and surrounding quotes will be removed
+
+error: unnecessary `Debug` formatting in `format!` args
+  --> tests/ui/unnecessary_os_str_debug_formatting.rs:22:37
+   |
+LL |     let _: String = format!("{:?}", os_string);
+   |                                     ^^^^^^^^^
+   |
+   = help: use `Display` formatting and change this to `os_string.display()`
+   = note: switching to `Display` formatting will change how the value is shown; escaped characters will no longer be escaped and surrounding quotes will be removed
+
+error: aborting due to 6 previous errors
+