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

Closure-consuming helper functions for Debug{Struct,List,Set,Tuple} #288

Closed
jmillikin opened this issue Oct 28, 2023 · 6 comments
Closed
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@jmillikin
Copy link

Proposal

Problem statement

Implementing fmt::Debug for FFI types can be difficult because their layouts often don't match Rust conventions, for example by using a bitset for boolean properties or a (tag, union) pair instead of an enumeration.

Currently the formatting of these values can require creating auxiliary wrapper types that exist just to impl Debug for one particular field, which is verbose and unergonomic.

Motivating examples or use cases

Example one: a bitset representing a set of boolean options. The bits should be formatted with names, and unknown bits should be formatted as numbers.

#[derive(Eq, PartialEq)]
struct FancyBitset(u32);

impl FancyBitset {
    const A: FancyBitset = FancyBitset(1 << 0);
    const B: FancyBitset = FancyBitset(1 << 1);
    const C: FancyBitset = FancyBitset(1 << 2);
    const D: FancyBitset = FancyBitset(1 << 3);
    const E: FancyBitset = FancyBitset(1 << 4);
}

impl fmt::Debug for FancyBitset {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        f.write_str("FancyBitset ")?;
        let mut dbg = f.debug_set();
        for bit in 0..32 {
            let mask: u32 = 1 << bit;
            if self.0 & mask == 0 {
                continue;
            }
            dbg.entry(&DebugFancyBit(mask));
        }
        dbg.finish()
    }
}

// This wrapper type shouldn't be necessary
struct DebugFancyBit(u32);
impl fmt::Debug for DebugFancyBit {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        match FancyBitset(self.0) {
            FancyBitset::A => f.write_str("A"),
            FancyBitset::B => f.write_str("B"),
            FancyBitset::C => f.write_str("C"),
            FancyBitset::D => f.write_str("D"),
            FancyBitset::E => f.write_str("E"),
            _ => write!(f, "{:#010X}", self.0),
        }
    }
}

Example two: a union field conceptually only has one of its sub-fields populated at any given time, but figuring out which sub-field is valid requires inspecting the tag.

struct StructWithUnion {
    tag: u32, // 0 = u32, 1 = i32, 2 = f32
    union_value: TheUnion
}

union TheUnion {
    as_u32: u32,
    as_i32: i32,
    as_f32: f32,
}

impl fmt::Debug for StructWithUnion {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        f.debug_struct("StructWithUnion")
            .field("tag", &self.tag)
            .field("value", &DebugTheUnion(self.tag, &self.union_value))
            .finish()
    }
}

// This wrapper type shouldn't be necessary
struct DebugTheUnion<'a>(u32, &'a TheUnion);
impl fmt::Debug for DebugTheUnion<'_> {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        let mut dbg = f.debug_struct("TheUnion");
        match self.0 {
            0 => dbg.field("as_u32", unsafe { &self.1.as_u32 }),
            1 => dbg.field("as_i32", unsafe { &self.1.as_i32 }),
            2 => dbg.field("as_f32", unsafe { &self.1.as_f32 }),
            _ => dbg.field("", &"<unknown tag>"),
        };
        dbg.finish()
    }
}

Solution sketch

Add methods to DebugStruct, DebugList, DebugSet, and DebugTuple that make it easy to embed a formatting closure function directly into the main impl Debug of a type.

I'm ignoring DebugMap, but if the libs team thinks it should have similar functionality then i suggest key_with() and value_with() rather than trying to have a double-closure entry_with().

Also, I'm not married to the names, so you'd prefer field_fmt() or whatever then that's fine.

impl DebugStruct<'_, '_> {
    fn field_with<F>(&mut self, name: &str, fmt_fn: F) -> &mut Self
    where
        F: FnOnce(&mut Formatter) -> Result;
}

impl DebugList<'_, '_> {
    fn entry_with<F>(&mut self, fmt_fn: F) -> &mut Self
    where
        F: FnOnce(&mut fmt::Formatter) -> fmt::Result;
}

impl DebugSet<'_, '_> {
    fn entry_with<F>(&mut self, fmt_fn: F) -> &mut Self
    where
        F: FnOnce(&mut fmt::Formatter) -> fmt::Result;
}

impl DebugTuple<'_, '_> {  // also DebugSet
    fn field_with<F>(&mut self, fmt_fn: F) -> &mut Self
    where
        F: FnOnce(&mut fmt::Formatter) -> fmt::Result;
}

They would be used like this:

impl fmt::Debug for FancyBitset {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        f.write_str("FancyBitset ")?;
        let mut dbg = f.debug_set();
        for bit in 0..32 {
            let mask: u32 = 1 << bit;
            if self.0 & mask == 0 {
                continue;
            }
            dbg.entry_with(|f| match FancyBitset(mask) {
                FancyBitset::A => f.write_str("A"),
                FancyBitset::B => f.write_str("B"),
                FancyBitset::C => f.write_str("C"),
                FancyBitset::D => f.write_str("D"),
                FancyBitset::E => f.write_str("E"),
                _ => write!(f, "{:#010X}", mask),
            });
        }
        dbg.finish()
    }
}

impl fmt::Debug for StructWithUnion {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        f.debug_struct("StructWithUnion")
            .field("tag", &self.tag)
            .field_with("value", |f| {
                let mut u = f.debug_struct("TheUnion");
                match self.tag {
                    0 => u.field("as_u32", unsafe { &self.union_value.as_u32 }),
                    1 => u.field("as_i32", unsafe { &self.union_value.as_i32 }),
                    2 => u.field("as_f32", unsafe { &self.union_value.as_f32 }),
                    _ => u.field("", &"<unknown tag>"),
                };
                u.finish()
            })
            .finish()
    }
}

Alternatives

Closure-wrapper function

This is more generic because it lets a closure be used in any place a &dyn Debug is expected, but it's slightly less flexible (F: Fn rather than F: FnOnce) and the call site is more verbose due to required type annotations.

struct DebugFn<F>(F);

impl<F> Debug for DebugFn<F>
where
    F: Fn(&mut Formatter) -> Result,
{
    fn fmt(&self, f: &mut Formatter) -> Result {
        (self.0)(f)
    }
}

impl fmt::Debug for StructWithUnion {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        f.debug_struct("SomeStruct")
            .field("value", &fmt::DebugFn(|f: &mut fmt::Formatter| match self.tag {
                // ...
            }))
            .finish()
    }
}

Extension trait

Mostly I just wanted to see if it was possible. It is, but it's horrible. trait Debug quite reasonably uses &self receivers, so the only way to get FnOnce is to play games with a Cell<Option<F>>

trait DebugStructWith {
    fn field_with<F>(&mut self, name: &str, fmt_fn: F) -> &mut Self
    where
        F: FnOnce(&mut fmt::Formatter) -> fmt::Result;
}

impl DebugStructWith for fmt::DebugStruct<'_, '_> {
    fn field_with<F>(&mut self, name: &str, fmt_fn: F) -> &mut Self
    where
        F: FnOnce(&mut fmt::Formatter) -> fmt::Result,
    {
        struct DebugFn<F>(Cell<Option<F>>);

        impl<F> fmt::Debug for DebugFn<F>
            where F: FnOnce(&mut fmt::Formatter) -> fmt::Result,
        {
            fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
                let fmt_fn = self.0.replace(None).unwrap();
                fmt_fn(f)
            }
        }
    
        self.field(name, &DebugFn(Cell::new(Some(fmt_fn))))
    }
}

Links and related work

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

  • We think this problem seems worth solving, and the standard library might be the right place to solve it.
  • We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

  • We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
  • We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.
@jmillikin jmillikin added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Oct 28, 2023
@pitaj
Copy link

pitaj commented Oct 28, 2023

Seems like really all you need is a way to write into DebugStruct::field etc directly. I believe the common solution to this is wrapping it in format_args!:

impl fmt::Debug for FancyBitset {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        f.write_str("FancyBitset ")?;
        let mut dbg = f.debug_set();
        for bit in 0..32 {
            let mask: u32 = 1 << bit;
            if self.0 & mask == 0 {
                continue;
            }
            match FancyBitset(mask) {
                FancyBitset::A => dbg.entry(format_args!("A")),
                FancyBitset::B => dbg.entry(format_args!("B")),
                FancyBitset::C => dbg.entry(format_args!("C")),
                FancyBitset::D => dbg.entry(format_args!("D")),
                FancyBitset::E => dbg.entry(format_args!("E")),
                _ => dbg.entry(format_args!("{:#010X}", mask)),
            };
        }
        dbg.finish()
    }
}

@jmillikin
Copy link
Author

I recognize the temptation to code-golf the simplified example code snippets, but it's probably better to think of them as gesturing toward a general shape of problem rather than trying to optimize them as-is.

@pitaj
Copy link

pitaj commented Oct 28, 2023

I was just pointing out an alternative, not optimizing.

@m-ou-se
Copy link
Member

m-ou-se commented Nov 8, 2023

We briefly discussed this in the libs API meeting yesterday, and are on board with adding this as an unstable feature.

Feel free to open a tracking issue and open a PR to rust-lang/rust to add it as unstable.

Please add open questions for the method names (_with, _fmt, ..?) and for what to with DebugMap::entry.

Thank you!

@m-ou-se m-ou-se closed this as completed Nov 8, 2023
@m-ou-se
Copy link
Member

m-ou-se commented Nov 8, 2023

I should also note that we also think your first alternative, the closure wrapping type, might be good to have in the standard library (in addition to the new methods on Debug{Struct, etc.}), although we think it should both implement Display and Debug (with identical impls, just like fmt::Arguments), and therefore have a different name.

I vaguely remember there was already another ACP for that, but I can't find it right now.

@m-ou-se m-ou-se added the ACP-accepted API Change Proposal is accepted (seconded with no objections) label Nov 8, 2023
@jmillikin
Copy link
Author

Thanks! Tracking issue is rust-lang/rust#117729 and implementation PR (with your suggested changes) is rust-lang/rust#117730.

I should also note that we also think your first alternative, the closure wrapping type, might be good to have in the standard library (in addition to the new methods on Debug{Struct, etc.}), although we think it should both implement Display and Debug (with identical impls, just like fmt::Arguments), and therefore have a different name.

I vaguely remember there was already another ACP for that, but I can't find it right now.

Sounds good to me. I've included this in the implementation PR as fmt::FormatterFn.

bors added a commit to rust-lang-ci/rust that referenced this issue Nov 10, 2023
…viper

Closure-consuming helper functions for `fmt::Debug` helpers

ACP: rust-lang/libs-team#288

Tracking issue: rust-lang#117729
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Nov 10, 2023
…cuviper

Closure-consuming helper functions for `fmt::Debug` helpers

ACP: rust-lang/libs-team#288

Tracking issue: rust-lang#117729
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 10, 2023
Rollup merge of rust-lang#117730 - jmillikin:fmt-debug-helper-fns, r=cuviper

Closure-consuming helper functions for `fmt::Debug` helpers

ACP: rust-lang/libs-team#288

Tracking issue: rust-lang#117729
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

3 participants