Add `unnecessary_debug_formatting` lint #13893

smoelius · 2024-12-29T01:26:39Z

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

Nits are welcome.

changelog: Add unnecessary_debug_formatting lint

rustbot · 2024-12-29T01:26:45Z

rustbot has assigned @blyxyas.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

smoelius · 2024-12-29T01:28:56Z

cc: @kornelski

smoelius · 2024-12-29T11:56:05Z

@blyxyas Sorry for the CI failures and the force push. But I think the lint should be good now.

blyxyas

Great first patch!

blyxyas · 2025-01-03T20:01:47Z

clippy_lints/src/format_args.rs

+    #[clippy::version = "1.85.0"]
+    pub UNNECESSARY_DEBUG_FORMATTING,
+    restriction,
+    "`Debug` formatting applied to an `OsStr` or `Path`"


Suggested change

"`Debug` formatting applied to an `OsStr` or `Path`"

"`Debug` formatting applied to an `OsStr` or `Path` when `.display()` is available."

blyxyas · 2025-01-14T19:08:20Z

clippy_lints/src/format_args.rs

+    /// ```
+    #[clippy::version = "1.85.0"]
+    pub UNNECESSARY_DEBUG_FORMATTING,
+    restriction,


I think that this would fit better into pedantic.

I considered this. But I was concerned there could be legitimate reasons to print a path with debug formatting, and that having to allow them could get annoying.

I'll change it if you feel strongly.

I think it'd be better in pedantic.

This isn't a subjective restriction, but future-proofing for a feature that can disable Debug entirely, and a warning that Rust never promised any stable syntax of the Debug format. I've seen a couple of libraries assume the Debug format is a correct serialization of paths into Rust string syntax, which it is not, and it breaks on non-UTF-8.

So it is pedantically warning about edge cases that may affect all programs.

OK. I'll change it.

blyxyas · 2025-01-14T19:50:04Z

clippy_lints/src/format_args.rs

@@ -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()


Should this also check for procedural macros? Usually new lints want to make sure we aren't linting in those cases.

Frankly, I'm not sure, though I don't think it would hurt.

You mention "new lints" but are there larger guidelines for when is_from_proc_macro should be used?

In general, everywhere. There are not many lints that we want to lint in procedural macros. Proc macros are outside of the user's control and could produce any arbitrary output based on any arbitrary input.

For example, a while back we had an issue that a clap procedural macro produced let _ = _, Clippy warned about that and the user was confused because there was nothing they could do apart from reporting it to either Clap or Clippy. I'm not sure if there's a case that we want to explicitly lint in procedural macros.

blyxyas · 2025-01-14T20:35:39Z

clippy_lints/src/format_args.rs

+        // 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")


We need to check that ty implements Deref before calling this function (this would save us a lot of work, as there are not a lot of types that implement Deref after a .peel_refs().)

blyxyas · 2025-01-14T21:32:21Z

tests/ui/unnecessary_path_debug_formatting.rs

+    println!("{:?}", path);
+    println!("{:?}", path_buf);
+
+    let _: String = format!("{:?}", path);
+    let _: String = format!("{:?}", path_buf);
+
+    let deref_path = DerefPath { path };
+    println!("{:?}", &*deref_path);


Suggested change

println!("{:?}", path);

println!("{:?}", path_buf);

let _: String = format!("{:?}", path);

let _: String = format!("{:?}", path_buf);

let deref_path = DerefPath { path };

println!("{:?}", &*deref_path);

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

blyxyas · 2025-01-14T21:49:59Z

tests/ui/unnecessary_os_str_debug_formatting.rs

+    println!("{:?}", os_str);
+    println!("{:?}", os_string);
+
+    let _: String = format!("{:?}", os_str);
+    let _: String = format!("{:?}", os_string);


Suggested change

println!("{:?}", os_str);

println!("{:?}", os_string);

let _: String = format!("{:?}", os_str);

let _: String = format!("{:?}", os_string);

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

Required now that the lint is pedantic

smoelius · 2025-01-16T10:23:36Z

@blyxyas I think I have addressed all of your comments thus far.

samueltardieu · 2025-01-16T17:41:49Z

tests/ui/unnecessary_path_debug_formatting.rs

+
+    // positive tests
+    println!("{:?}", path); //~ unnecessary_debug_formatting
+    println!("{:?}", path_buf); //~ unnecessary_debug_formatting


Could you add an inline format test as well?

println!("{path_buf:?}"); //~ unnecessary_debug_formatting

(ditto for OsStr)

rustbot assigned blyxyas Dec 29, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 29, 2024

Add unnecessary_debug_formatting lint

4e1cad1

smoelius force-pushed the unnecessary-debug-formatting branch from 8a7ccf4 to 4e1cad1 Compare December 29, 2024 11:44

kornelski approved these changes Dec 30, 2024

View reviewed changes

blyxyas requested changes Jan 14, 2025

View reviewed changes

smoelius added 2 commits January 16, 2025 05:10

Address review comments

86ed6a7

Fix adjacent code

7f61ad5

Required now that the lint is pedantic

samueltardieu reviewed Jan 16, 2025

View reviewed changes

Add inline formatting tests

1ea17d2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `unnecessary_debug_formatting` lint #13893

Add `unnecessary_debug_formatting` lint #13893

smoelius commented Dec 29, 2024

rustbot commented Dec 29, 2024

smoelius commented Dec 29, 2024

smoelius commented Dec 29, 2024

blyxyas left a comment

blyxyas Jan 3, 2025

blyxyas Jan 14, 2025

smoelius Jan 15, 2025

kornelski Jan 15, 2025

smoelius Jan 15, 2025

blyxyas Jan 14, 2025

smoelius Jan 15, 2025

blyxyas Jan 15, 2025

blyxyas Jan 14, 2025

blyxyas Jan 14, 2025

blyxyas Jan 14, 2025

smoelius commented Jan 16, 2025

samueltardieu Jan 16, 2025

	"`Debug` formatting applied to an `OsStr` or `Path`"
	"`Debug` formatting applied to an `OsStr` or `Path` when `.display()` is available."

Add unnecessary_debug_formatting lint #13893

Are you sure you want to change the base?

Add unnecessary_debug_formatting lint #13893

Conversation

smoelius commented Dec 29, 2024

rustbot commented Dec 29, 2024

smoelius commented Dec 29, 2024

smoelius commented Dec 29, 2024

blyxyas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smoelius commented Jan 16, 2025

Choose a reason for hiding this comment

Add `unnecessary_debug_formatting` lint #13893

Add `unnecessary_debug_formatting` lint #13893