Skip to content

Commit b583568

Browse files
blyxyasblyxyas
authored
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
2 parents b821f97 + 6af901c commit b583568

10 files changed

+314
-14
lines changed

‎CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -6210,6 +6210,7 @@ Released 2018-09-13
62106210
[`unnecessary_box_returns`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_box_returns
62116211
[`unnecessary_cast`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
62126212
[`unnecessary_clippy_cfg`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_clippy_cfg
6213+
[`unnecessary_debug_formatting`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_debug_formatting
62136214
[`unnecessary_fallible_conversions`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_fallible_conversions
62146215
[`unnecessary_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_filter_map
62156216
[`unnecessary_find_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_find_map

‎clippy_lints/src/declared_lints.rs

+1
Original file line numberDiff line numberDiff line change
@@ -193,6 +193,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
193193
crate::format_args::FORMAT_IN_FORMAT_ARGS_INFO,
194194
crate::format_args::TO_STRING_IN_FORMAT_ARGS_INFO,
195195
crate::format_args::UNINLINED_FORMAT_ARGS_INFO,
196+
crate::format_args::UNNECESSARY_DEBUG_FORMATTING_INFO,
196197
crate::format_args::UNUSED_FORMAT_SPECS_INFO,
197198
crate::format_impl::PRINT_IN_FORMAT_IMPL_INFO,
198199
crate::format_impl::RECURSIVE_FORMAT_IMPL_INFO,

‎clippy_lints/src/format_args.rs

+113-9
Original file line numberDiff line numberDiff line change
@@ -1,26 +1,27 @@
11
use arrayvec::ArrayVec;
22
use clippy_config::Conf;
33
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
4-
use clippy_utils::is_diag_trait_item;
54
use clippy_utils::macros::{
65
FormatArgsStorage, FormatParamUsage, MacroCall, find_format_arg_expr, format_arg_removal_span,
76
format_placeholder_format_span, is_assert_macro, is_format_macro, is_panic, matching_root_macro_call,
87
root_macro_call_first_node,
98
};
109
use clippy_utils::msrvs::{self, Msrv};
11-
use clippy_utils::source::SpanRangeExt;
10+
use clippy_utils::source::{SpanRangeExt, snippet};
1211
use clippy_utils::ty::{implements_trait, is_type_lang_item};
12+
use clippy_utils::{is_diag_trait_item, is_from_proc_macro};
1313
use itertools::Itertools;
1414
use rustc_ast::{
1515
FormatArgPosition, FormatArgPositionKind, FormatArgsPiece, FormatArgumentKind, FormatCount, FormatOptions,
1616
FormatPlaceholder, FormatTrait,
1717
};
18+
use rustc_data_structures::fx::FxHashMap;
1819
use rustc_errors::Applicability;
1920
use rustc_errors::SuggestionStyle::{CompletelyHidden, ShowCode};
2021
use rustc_hir::{Expr, ExprKind, LangItem};
2122
use rustc_lint::{LateContext, LateLintPass, LintContext};
22-
use rustc_middle::ty::Ty;
2323
use rustc_middle::ty::adjustment::{Adjust, Adjustment};
24+
use rustc_middle::ty::{List, Ty, TyCtxt};
2425
use rustc_session::impl_lint_pass;
2526
use rustc_span::edition::Edition::Edition2021;
2627
use rustc_span::{Span, Symbol, sym};
@@ -50,6 +51,36 @@ declare_clippy_lint! {
5051
"`format!` used in a macro that does formatting"
5152
}
5253

54+
declare_clippy_lint! {
55+
/// ### What it does
56+
/// Checks for `Debug` formatting (`{:?}`) applied to an `OsStr` or `Path`.
57+
///
58+
/// ### Why is this bad?
59+
/// Rust doesn't guarantee what `Debug` formatting looks like, and it could
60+
/// change in the future. `OsStr`s and `Path`s can be `Display` formatted
61+
/// using their `display` methods.
62+
///
63+
/// Furthermore, with `Debug` formatting, certain characters are escaped.
64+
/// Thus, a `Debug` formatted `Path` is less likely to be clickable.
65+
///
66+
/// ### Example
67+
/// ```no_run
68+
/// # use std::path::Path;
69+
/// let path = Path::new("...");
70+
/// println!("The path is {:?}", path);
71+
/// ```
72+
/// Use instead:
73+
/// ```no_run
74+
/// # use std::path::Path;
75+
/// let path = Path::new("…");
76+
/// println!("The path is {}", path.display());
77+
/// ```
78+
#[clippy::version = "1.87.0"]
79+
pub UNNECESSARY_DEBUG_FORMATTING,
80+
pedantic,
81+
"`Debug` formatting applied to an `OsStr` or `Path` when `.display()` is available"
82+
}
83+
5384
declare_clippy_lint! {
5485
/// ### What it does
5586
/// 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! {
162193
"use of a format specifier that has no effect"
163194
}
164195

165-
impl_lint_pass!(FormatArgs => [
196+
impl_lint_pass!(FormatArgs<'_> => [
166197
FORMAT_IN_FORMAT_ARGS,
167198
TO_STRING_IN_FORMAT_ARGS,
168199
UNINLINED_FORMAT_ARGS,
200+
UNNECESSARY_DEBUG_FORMATTING,
169201
UNUSED_FORMAT_SPECS,
170202
]);
171203

172204
#[allow(clippy::struct_field_names)]
173-
pub struct FormatArgs {
205+
pub struct FormatArgs<'tcx> {
174206
format_args: FormatArgsStorage,
175207
msrv: Msrv,
176208
ignore_mixed: bool,
209+
ty_feature_map: FxHashMap<Ty<'tcx>, Option<Symbol>>,
177210
}
178211

179-
impl FormatArgs {
180-
pub fn new(conf: &'static Conf, format_args: FormatArgsStorage) -> Self {
212+
impl<'tcx> FormatArgs<'tcx> {
213+
pub fn new(tcx: TyCtxt<'tcx>, conf: &'static Conf, format_args: FormatArgsStorage) -> Self {
214+
let ty_feature_map = make_ty_feature_map(tcx);
181215
Self {
182216
format_args,
183217
msrv: conf.msrv.clone(),
184218
ignore_mixed: conf.allow_mixed_uninlined_format_args,
219+
ty_feature_map,
185220
}
186221
}
187222
}
188223

189-
impl<'tcx> LateLintPass<'tcx> for FormatArgs {
224+
impl<'tcx> LateLintPass<'tcx> for FormatArgs<'tcx> {
190225
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
191226
if let Some(macro_call) = root_macro_call_first_node(cx, expr)
192227
&& is_format_macro(cx, macro_call.def_id)
@@ -198,6 +233,7 @@ impl<'tcx> LateLintPass<'tcx> for FormatArgs {
198233
macro_call: &macro_call,
199234
format_args,
200235
ignore_mixed: self.ignore_mixed,
236+
ty_feature_map: &self.ty_feature_map,
201237
};
202238

203239
linter.check_templates();
@@ -217,9 +253,10 @@ struct FormatArgsExpr<'a, 'tcx> {
217253
macro_call: &'a MacroCall,
218254
format_args: &'a rustc_ast::FormatArgs,
219255
ignore_mixed: bool,
256+
ty_feature_map: &'a FxHashMap<Ty<'tcx>, Option<Symbol>>,
220257
}
221258

222-
impl FormatArgsExpr<'_, '_> {
259+
impl<'tcx> FormatArgsExpr<'_, 'tcx> {
223260
fn check_templates(&self) {
224261
for piece in &self.format_args.template {
225262
if let FormatArgsPiece::Placeholder(placeholder) = piece
@@ -237,6 +274,11 @@ impl FormatArgsExpr<'_, '_> {
237274
self.check_format_in_format_args(name, arg_expr);
238275
self.check_to_string_in_format_args(name, arg_expr);
239276
}
277+
278+
if placeholder.format_trait == FormatTrait::Debug {
279+
let name = self.cx.tcx.item_name(self.macro_call.def_id);
280+
self.check_unnecessary_debug_formatting(name, arg_expr);
281+
}
240282
}
241283
}
242284
}
@@ -439,6 +481,33 @@ impl FormatArgsExpr<'_, '_> {
439481
}
440482
}
441483

484+
fn check_unnecessary_debug_formatting(&self, name: Symbol, value: &Expr<'tcx>) {
485+
let cx = self.cx;
486+
if !value.span.from_expansion()
487+
&& !is_from_proc_macro(cx, value)
488+
&& let ty = cx.typeck_results().expr_ty(value)
489+
&& self.can_display_format(ty)
490+
{
491+
let snippet = snippet(cx.sess(), value.span, "..");
492+
span_lint_and_then(
493+
cx,
494+
UNNECESSARY_DEBUG_FORMATTING,
495+
value.span,
496+
format!("unnecessary `Debug` formatting in `{name}!` args"),
497+
|diag| {
498+
diag.help(format!(
499+
"use `Display` formatting and change this to `{snippet}.display()`"
500+
));
501+
diag.note(
502+
"switching to `Display` formatting will change how the value is shown; \
503+
escaped characters will no longer be escaped and surrounding quotes will \
504+
be removed",
505+
);
506+
},
507+
);
508+
}
509+
}
510+
442511
fn format_arg_positions(&self) -> impl Iterator<Item = (&FormatArgPosition, FormatParamUsage)> {
443512
self.format_args.template.iter().flat_map(|piece| match piece {
444513
FormatArgsPiece::Placeholder(placeholder) => {
@@ -465,6 +534,41 @@ impl FormatArgsExpr<'_, '_> {
465534
.at_most_one()
466535
.is_err()
467536
}
537+
538+
fn can_display_format(&self, ty: Ty<'tcx>) -> bool {
539+
let ty = ty.peel_refs();
540+
541+
if let Some(feature) = self.ty_feature_map.get(&ty)
542+
&& feature.is_none_or(|feature| self.cx.tcx.features().enabled(feature))
543+
{
544+
return true;
545+
}
546+
547+
// Even if `ty` is not in `self.ty_feature_map`, check whether `ty` implements `Deref` with
548+
// a `Target` that is in `self.ty_feature_map`.
549+
if let Some(deref_trait_id) = self.cx.tcx.lang_items().deref_trait()
550+
&& implements_trait(self.cx, ty, deref_trait_id, &[])
551+
&& let Some(target_ty) = self.cx.get_associated_type(ty, deref_trait_id, "Target")
552+
&& let Some(feature) = self.ty_feature_map.get(&target_ty)
553+
&& feature.is_none_or(|feature| self.cx.tcx.features().enabled(feature))
554+
{
555+
return true;
556+
}
557+
558+
false
559+
}
560+
}
561+
562+
fn make_ty_feature_map(tcx: TyCtxt<'_>) -> FxHashMap<Ty<'_>, Option<Symbol>> {
563+
[(sym::OsStr, Some(Symbol::intern("os_str_display"))), (sym::Path, None)]
564+
.into_iter()
565+
.filter_map(|(name, feature)| {
566+
tcx.get_diagnostic_item(name).map(|def_id| {
567+
let ty = Ty::new_adt(tcx, tcx.adt_def(def_id), List::empty());
568+
(ty, feature)
569+
})
570+
})
571+
.collect()
468572
}
469573

470574
fn count_needed_derefs<'tcx, I>(mut ty: Ty<'tcx>, mut iter: I) -> (usize, Ty<'tcx>)

‎clippy_lints/src/lib.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -841,7 +841,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
841841
store.register_late_pass(move |_| Box::new(non_send_fields_in_send_ty::NonSendFieldInSendTy::new(conf)));
842842
store.register_late_pass(move |_| Box::new(undocumented_unsafe_blocks::UndocumentedUnsafeBlocks::new(conf)));
843843
let format_args = format_args_storage.clone();
844-
store.register_late_pass(move |_| Box::new(format_args::FormatArgs::new(conf, format_args.clone())));
844+
store.register_late_pass(move |tcx| Box::new(format_args::FormatArgs::new(tcx, conf, format_args.clone())));
845845
store.register_late_pass(|_| Box::new(trailing_empty_array::TrailingEmptyArray));
846846
store.register_early_pass(|| Box::new(octal_escapes::OctalEscapes));
847847
store.register_late_pass(|_| Box::new(needless_late_init::NeedlessLateInit));

‎lintcheck/src/input.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -288,11 +288,11 @@ impl CrateWithSource {
288288
// as a result of this filter.
289289
let dest_crate_root = PathBuf::from(LINTCHECK_SOURCES).join(name);
290290
if dest_crate_root.exists() {
291-
println!("Deleting existing directory at {dest_crate_root:?}");
291+
println!("Deleting existing directory at `{}`", dest_crate_root.display());
292292
fs::remove_dir_all(&dest_crate_root).unwrap();
293293
}
294294

295-
println!("Copying {path:?} to {dest_crate_root:?}");
295+
println!("Copying `{}` to `{}`", path.display(), dest_crate_root.display());
296296

297297
for entry in WalkDir::new(path).into_iter().filter_entry(|e| !is_cache_dir(e)) {
298298
let entry = entry.unwrap();

‎tests/compile-test.rs

+4-2
Original file line numberDiff line numberDiff line change
@@ -383,13 +383,15 @@ fn ui_cargo_toml_metadata() {
383383
.map(|component| component.as_os_str().to_string_lossy().replace('-', "_"))
384384
.any(|s| *s == name)
385385
|| path.starts_with(&cargo_common_metadata_path),
386-
"{path:?} has incorrect package name"
386+
"`{}` has incorrect package name",
387+
path.display(),
387388
);
388389

389390
let publish = package.get("publish").and_then(toml::Value::as_bool).unwrap_or(true);
390391
assert!(
391392
!publish || publish_exceptions.contains(&path.parent().unwrap().to_path_buf()),
392-
"{path:?} lacks `publish = false`"
393+
"`{}` lacks `publish = false`",
394+
path.display(),
393395
);
394396
}
395397
}

‎tests/ui/unnecessary_os_str_debug_formatting.rs

+23
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
#![feature(os_str_display)]
2+
#![warn(clippy::unnecessary_debug_formatting)]
3+
4+
use std::ffi::{OsStr, OsString};
5+
6+
fn main() {
7+
let os_str = OsStr::new("abc");
8+
let os_string = os_str.to_os_string();
9+
10+
// negative tests
11+
println!("{}", os_str.display());
12+
println!("{}", os_string.display());
13+
14+
// positive tests
15+
println!("{:?}", os_str); //~ unnecessary_debug_formatting
16+
println!("{:?}", os_string); //~ unnecessary_debug_formatting
17+
18+
println!("{os_str:?}"); //~ unnecessary_debug_formatting
19+
println!("{os_string:?}"); //~ unnecessary_debug_formatting
20+
21+
let _: String = format!("{:?}", os_str); //~ unnecessary_debug_formatting
22+
let _: String = format!("{:?}", os_string); //~ unnecessary_debug_formatting
23+
}

‎tests/ui/unnecessary_os_str_debug_formatting.stderr

+58
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
error: unnecessary `Debug` formatting in `println!` args
2+
--> tests/ui/unnecessary_os_str_debug_formatting.rs:15:22
3+
|
4+
LL | println!("{:?}", os_str);
5+
| ^^^^^^
6+
|
7+
= help: use `Display` formatting and change this to `os_str.display()`
8+
= 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
9+
= note: `-D clippy::unnecessary-debug-formatting` implied by `-D warnings`
10+
= help: to override `-D warnings` add `#[allow(clippy::unnecessary_debug_formatting)]`
11+
12+
error: unnecessary `Debug` formatting in `println!` args
13+
--> tests/ui/unnecessary_os_str_debug_formatting.rs:16:22
14+
|
15+
LL | println!("{:?}", os_string);
16+
| ^^^^^^^^^
17+
|
18+
= help: use `Display` formatting and change this to `os_string.display()`
19+
= 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
20+
21+
error: unnecessary `Debug` formatting in `println!` args
22+
--> tests/ui/unnecessary_os_str_debug_formatting.rs:18:16
23+
|
24+
LL | println!("{os_str:?}");
25+
| ^^^^^^
26+
|
27+
= help: use `Display` formatting and change this to `os_str.display()`
28+
= 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
29+
30+
error: unnecessary `Debug` formatting in `println!` args
31+
--> tests/ui/unnecessary_os_str_debug_formatting.rs:19:16
32+
|
33+
LL | println!("{os_string:?}");
34+
| ^^^^^^^^^
35+
|
36+
= help: use `Display` formatting and change this to `os_string.display()`
37+
= 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
38+
39+
error: unnecessary `Debug` formatting in `format!` args
40+
--> tests/ui/unnecessary_os_str_debug_formatting.rs:21:37
41+
|
42+
LL | let _: String = format!("{:?}", os_str);
43+
| ^^^^^^
44+
|
45+
= help: use `Display` formatting and change this to `os_str.display()`
46+
= 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
47+
48+
error: unnecessary `Debug` formatting in `format!` args
49+
--> tests/ui/unnecessary_os_str_debug_formatting.rs:22:37
50+
|
51+
LL | let _: String = format!("{:?}", os_string);
52+
| ^^^^^^^^^
53+
|
54+
= help: use `Display` formatting and change this to `os_string.display()`
55+
= 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
56+
57+
error: aborting due to 6 previous errors
58+

0 commit comments

Comments
 (0)