Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace all fmt.pad with debug_struct #84013

Merged
merged 3 commits into from Apr 22, 2021
Merged

Replace all fmt.pad with debug_struct #84013

merged 3 commits into from Apr 22, 2021

Conversation

CDirkx
Copy link
Contributor

@CDirkx CDirkx commented Apr 8, 2021

This replaces any occurrence of:

  • f.pad("X") with f.debug_struct("X").finish()
  • f.pad("X { .. }") with f.debug_struct("X").finish_non_exhaustive()

This is in line with existing formatting code such as

#[stable(feature = "mpsc_debug", since = "1.8.0")]
impl<T> fmt::Debug for Receiver<T> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.debug_struct("Receiver").finish_non_exhaustive()
}
}

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 8, 2021
@kennytm
Copy link
Member

kennytm commented Apr 9, 2021

i guess this is fine since Debug output is not guaranteed to be stable, but note that pad and debug_struct produce different output when padding is involved:

use std::fmt;

struct A;
struct B;

impl fmt::Debug for A {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        f.debug_struct("X").finish()
    }
}

impl fmt::Debug for B {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        f.pad("X")
    }
}

fn main() {
    assert_eq!(format!("<{:3?}>", A), "<X>");
    assert_eq!(format!("<{:3?}>", B), "<X  >");
}

(sending this to a real T-libs member) r? @m-ou-se

@rust-highfive rust-highfive assigned m-ou-se and unassigned kennytm Apr 9, 2021
@m-ou-se
Copy link
Member

m-ou-se commented Apr 21, 2021

Some of these should probably use finish_non_exhaustive instead of finish. E.g. UnsafeCell { .. } instead of UnsafeCell since it contains stuff. Same for Any I suppose.

(I replaced a bunch of those recently in #83558, but I missed these since they used .pad() instead of .debug_struct().)

@m-ou-se
Copy link
Member

m-ou-se commented Apr 21, 2021

i guess this is fine since Debug output is not guaranteed to be stable, but note that pad and debug_struct produce different output when padding is involved:

We somewhat regularly change or improve Debug output, so I'm not worried about stability here. Consistency with all other Debug implementations is useful. The other Debug implementations (including those from #[derive(Debug)]) forward the formatting options to each field individually, and don't apply it to the type names:

#[derive(Debug)]
struct A {
    x: i32,
    y: i32,
}

fn main() {
    println!("{:#010?}", A { x: 1, y: 2 });
}
A {
    x: 0000000001,
    y: 0000000002,
}

The only downside of the change in this PR is that in 'pretty' mode ({:#?}, as used by dbg!()), .finish_non_exhaustive puts the .. on a separate line:

A {
    ..
}

taking up a lot more space than A { .. } like before for these types. But that's something we can fix in the .finish_non_exhaustive() implementation itself: #84390

@CDirkx
Copy link
Contributor Author

CDirkx commented Apr 21, 2021

I agree with the use of finish_non_exhaustive for UnsafeCell and Any. I checked the others, but those two are the only ones with meaningful internal data that should be represented (so excluding PhantomData).

I also found two more cases (PoisonError and SendError) that were using "X { .. }".fmt(f).

@m-ou-se
Copy link
Member

m-ou-se commented Apr 21, 2021

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Apr 21, 2021

📌 Commit fdae757 has been approved by m-ou-se

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 21, 2021
@rust-log-analyzer

This comment has been minimized.

@m-ou-se
Copy link
Member

m-ou-se commented Apr 21, 2021

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 21, 2021
@CDirkx
Copy link
Contributor Author

CDirkx commented Apr 21, 2021

I fixed alloc::test::test_show for now, by changing assert_eq!(s, "Any") to assert_eq!(s, "Any { .. }"). It's still fragile to any future changes to the Any Debug implementations, but reworking the test could be done in a future PR.

@m-ou-se
Copy link
Member

m-ou-se commented Apr 21, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Apr 21, 2021

📌 Commit fccc75c has been approved by m-ou-se

@bors
Copy link
Contributor

bors commented Apr 21, 2021

🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 21, 2021
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Apr 21, 2021
Replace all `fmt.pad` with `debug_struct`

This replaces any occurrence of:
- `f.pad("X")` with `f.debug_struct("X").finish()`
- `f.pad("X { .. }")` with `f.debug_struct("X").finish_non_exhaustive()`

This is in line with existing formatting code such as
https://github.com/rust-lang/rust/blob/125505306744a0a5bb01d62337260a95d9ff8d57/library/std/src/sync/mpsc/mod.rs#L1470-L1475
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 22, 2021
Rollup of 12 pull requests

Successful merges:

 - rust-lang#84013 (Replace all `fmt.pad` with `debug_struct`)
 - rust-lang#84119 (Move `sys::vxworks` code to `sys::unix`)
 - rust-lang#84212 (Replace `Void` in `sys` with never type)
 - rust-lang#84251 (fix 'const-stable since' for NonZeroU*::new_unchecked)
 - rust-lang#84301 (Document that `index` and `index_mut` can panic)
 - rust-lang#84365 (Improve the docstrings of the `Lto` struct.)
 - rust-lang#84378 (Fix broken doc link)
 - rust-lang#84379 (Add GAT related tests)
 - rust-lang#84380 (Write Rustdoc titles like "x in crate::mod - Rust")
 - rust-lang#84390 (Format `Struct { .. }` on one line even with `{:#?}`.)
 - rust-lang#84393 (Support `x.py doc std --open`)
 - rust-lang#84406 (Remove `delete` alias from `mem::drop`.)

Failed merges:

 - rust-lang#84387 (Move `sys_common::poison` to `sync::poison`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a7a7737 into rust-lang:master Apr 22, 2021
@rustbot rustbot added this to the 1.53.0 milestone Apr 22, 2021
@CDirkx CDirkx deleted the fmt branch April 22, 2021 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants