diff options
| -rw-r--r-- | CHANGELOG.md | 1 | ||||
| -rw-r--r-- | clippy_lints/src/declared_lints.rs | 1 | ||||
| -rw-r--r-- | clippy_lints/src/format_args.rs | 122 | ||||
| -rw-r--r-- | clippy_lints/src/lib.rs | 2 | ||||
| -rw-r--r-- | lintcheck/src/input.rs | 4 | ||||
| -rw-r--r-- | tests/compile-test.rs | 6 | ||||
| -rw-r--r-- | tests/ui/unnecessary_os_str_debug_formatting.rs | 23 | ||||
| -rw-r--r-- | tests/ui/unnecessary_os_str_debug_formatting.stderr | 58 | ||||
| -rw-r--r-- | tests/ui/unnecessary_path_debug_formatting.rs | 44 | ||||
| -rw-r--r-- | tests/ui/unnecessary_path_debug_formatting.stderr | 67 |
10 files changed, 314 insertions, 14 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 664a7e76630..f7263237129 100644 --- 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 index 7451fb909ef..985278e4780 100644 --- 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 index da5825b7ab2..ea617dd1bab 100644 --- 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::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}; @@ -52,6 +53,36 @@ declare_clippy_lint! { 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) /// applied to a type that implements [`Display`](https://doc.rust-lang.org/std/fmt/trait.Display.html) /// in a macro that does formatting. @@ -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: ¯o_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>) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index fad6f9d0880..ea7566a8e85 100644 --- 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/lintcheck/src/input.rs b/lintcheck/src/input.rs index 3383d50fa02..83eb0a577d6 100644 --- a/lintcheck/src/input.rs +++ b/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(); diff --git a/tests/compile-test.rs b/tests/compile-test.rs index e2e4d92df79..10a3e85ed1d 100644 --- a/tests/compile-test.rs +++ b/tests/compile-test.rs @@ -377,13 +377,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(), ); } } diff --git a/tests/ui/unnecessary_os_str_debug_formatting.rs b/tests/ui/unnecessary_os_str_debug_formatting.rs new file mode 100644 index 00000000000..3c264e5fc59 --- /dev/null +++ b/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 +} diff --git a/tests/ui/unnecessary_os_str_debug_formatting.stderr b/tests/ui/unnecessary_os_str_debug_formatting.stderr new file mode 100644 index 00000000000..382e59b0461 --- /dev/null +++ b/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 + diff --git a/tests/ui/unnecessary_path_debug_formatting.rs b/tests/ui/unnecessary_path_debug_formatting.rs new file mode 100644 index 00000000000..96ad2ee2bf2 --- /dev/null +++ b/tests/ui/unnecessary_path_debug_formatting.rs @@ -0,0 +1,44 @@ +#![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); //~ unnecessary_debug_formatting + println!("{:?}", path_buf); //~ unnecessary_debug_formatting + + println!("{path:?}"); //~ unnecessary_debug_formatting + println!("{path_buf:?}"); //~ unnecessary_debug_formatting + + let _: String = format!("{:?}", path); //~ unnecessary_debug_formatting + let _: String = format!("{:?}", path_buf); //~ unnecessary_debug_formatting + + let deref_path = DerefPath { path }; + println!("{:?}", &*deref_path); //~ unnecessary_debug_formatting +} diff --git a/tests/ui/unnecessary_path_debug_formatting.stderr b/tests/ui/unnecessary_path_debug_formatting.stderr new file mode 100644 index 00000000000..c9f649dbc0a --- /dev/null +++ b/tests/ui/unnecessary_path_debug_formatting.stderr @@ -0,0 +1,67 @@ +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: 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_path_debug_formatting.rs:34:22 + | +LL | println!("{:?}", path_buf); + | ^^^^^^^^ + | + = help: use `Display` formatting and change this to `path_buf.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_path_debug_formatting.rs:36:16 + | +LL | println!("{path:?}"); + | ^^^^ + | + = help: use `Display` formatting and change this to `path.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_path_debug_formatting.rs:37:16 + | +LL | println!("{path_buf:?}"); + | ^^^^^^^^ + | + = help: use `Display` formatting and change this to `path_buf.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_path_debug_formatting.rs:39:37 + | +LL | let _: String = format!("{:?}", path); + | ^^^^ + | + = help: use `Display` formatting and change this to `path.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_path_debug_formatting.rs:40:37 + | +LL | let _: String = format!("{:?}", path_buf); + | ^^^^^^^^ + | + = help: use `Display` formatting and change this to `path_buf.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_path_debug_formatting.rs:43:22 + | +LL | println!("{:?}", &*deref_path); + | ^^^^^^^^^^^^ + | + = help: use `Display` formatting and change this to `&*deref_path.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 7 previous errors + |