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

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)

blyxyas · 2025-02-05T15:36:52Z

Here's the FCP: https://rust-lang.zulipchat.com/#narrow/channel/257328-clippy/topic/FCP.3A.20unnecessary_debug_formatting

Alexendoo says: It should definitely note that there is a difference in the output.

I agree, we should probably mention that there may be a difference between outputs. (Although in version 1.85 there is not)

Alexendoo · 2025-02-05T16:00:54Z

I agree, we should probably mention that there may be a difference between outputs. (Although in version 1.85 there is not)

I don't think anything changed in 1.85, Debug escapes various characters including ' which is not uncommon in paths, it also adds double quotes where .display() does not

While Debug's format is not stable it is documented as escaping the path in .display(), plus it's unlikely the wrapping "" would be removed

smoelius · 2025-02-05T17:54:07Z

I'm proposing to add this text to the description:

Note that switching from Debug formatting to Display formatting will change how the OsStr or Path is shown. Escaped characters will no longer be escaped, and enclosing quotes ("...") will be removed.

Does this sound okay?

Where would you add this, under "Why is this bad?"?

EDIT: Added.

Alexendoo · 2025-02-06T14:47:39Z

I was thinking to add a diagnostic note

For the Why is this bad? angle you could phrase it from the perspective of not being able copy/paste a path correctly or click it in an integrated terminal if the characters are escaped

smoelius · 2025-02-06T15:31:59Z

I was thinking to add a diagnostic note

For the Why is this bad? angle you could phrase it from the perspective of not being able copy/paste a path correctly or click it in an integrated terminal if the characters are escaped

I'll make both changes.

smoelius · 2025-02-07T10:39:40Z

@Alexendoo Please tell me if what I pushed is not what you had in mind.

blyxyas

Sorry for the delay, I was waiting for Alex's input on the change but taking a look at it I think it's very agreeable.
LGTM, thanks! ❤️ Could you change the version to reflect the new current version and squash all commits?

smoelius · 2025-02-26T01:19:44Z

Sorry for the delay, I was waiting for Alex's input on the change but taking a look at it I think it's very agreeable. LGTM, thanks! ❤️ Could you change the version to reflect the new current version and squash all commits?

No worries at all. Please let me know if there's a Clippy version I missed. And thank you for the review. 🙏

blyxyas · 2025-02-26T13:43:06Z

clippy_lints/src/format_args.rs

+    /// let path = Path::new("…");
+    /// println!("The path is {}", path.display());
+    /// ```
+    #[clippy::version = "1.86.0"]


We decide the rust version for new Clippy lints by viewing the output of rustc -Vv in master 👀
This should be 1.87.0 🐱

Oof. Sorry.

Alexendoo · 2025-02-26T13:53:06Z

Ah sorry, I thought I had replied to this thread

Yeah, LGTM

Address review comments Fix adjacent code Required now that the lint is pedantic Add inline formatting tests Add note re formatting changes Address `unnecessary_map_or` warnings Address additional review comments Typo Update Clippy version

blyxyas

LGTM, thanks! ❤️

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

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

samueltardieu reviewed Jan 16, 2025

View reviewed changes

blyxyas reviewed Feb 25, 2025

View reviewed changes

smoelius force-pushed the unnecessary-debug-formatting branch from d5bdce7 to d467273 Compare February 26, 2025 01:16

blyxyas reviewed Feb 26, 2025

View reviewed changes

Add unnecessary_debug_formatting lint

6af901c

Address review comments Fix adjacent code Required now that the lint is pedantic Add inline formatting tests Add note re formatting changes Address `unnecessary_map_or` warnings Address additional review comments Typo Update Clippy version

smoelius force-pushed the unnecessary-debug-formatting branch from d467273 to 6af901c Compare February 26, 2025 14:26

blyxyas approved these changes Feb 26, 2025

View reviewed changes

blyxyas added this pull request to the merge queue Feb 26, 2025

Merged via the queue into rust-lang:master with commit b583568 Feb 26, 2025
11 checks passed

smoelius deleted the unnecessary-debug-formatting branch February 26, 2025 18:16

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

blyxyas commented Feb 5, 2025 •

edited

Loading

Alexendoo commented Feb 5, 2025

smoelius commented Feb 5, 2025 •

edited

Loading

Alexendoo commented Feb 6, 2025

smoelius commented Feb 6, 2025

smoelius commented Feb 7, 2025

blyxyas left a comment •

edited

Loading

smoelius commented Feb 26, 2025

blyxyas Feb 26, 2025

smoelius Feb 26, 2025

Alexendoo commented Feb 26, 2025

blyxyas left a comment

	"`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

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

blyxyas commented Feb 5, 2025 • edited Loading

Alexendoo commented Feb 5, 2025

smoelius commented Feb 5, 2025 • edited Loading

Alexendoo commented Feb 6, 2025

smoelius commented Feb 6, 2025

smoelius commented Feb 7, 2025

blyxyas left a comment • edited Loading

Choose a reason for hiding this comment

smoelius commented Feb 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alexendoo commented Feb 26, 2025

blyxyas left a comment

Choose a reason for hiding this comment

Add `unnecessary_debug_formatting` lint #13893

Add `unnecessary_debug_formatting` lint #13893

blyxyas commented Feb 5, 2025 •

edited

Loading

smoelius commented Feb 5, 2025 •

edited

Loading

blyxyas left a comment •

edited

Loading