Could function pointers and closures implement Debug using std::any::type_name()?

language design
1

I was recently writing some code that looks something like this:

use std::fmt::Debug;
use std::borrow::Cow;

/// A stage of the build process.
trait Stage: Debug {
    fn run(&self);
}

/// A struct which will search through some source code, rewriting each 
/// attribute.
struct RewriteAttribute<F> {
    /// A function called on each attribute to update its text.
    process_attribute: F,
    description: &'static str,
}

impl<F> Stage for RewriteAttribute<F> 
  where F: Fn(&str) -> Cow<'_, str>
{
    fn run(&self) { unimplemented!() }
}

(there are legitimate reasons for wanting Stage: Debug, primarily for debugging and logging)

This code won't compile at the moment because F: Fn(&str) -> Cow<'_, str> doesn't implement Debug. I've worked around this by writing an explicit impl instead of using #[derive(Debug)].

impl<F> Debug for RewriteAttribute<F> {
    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
        f.debug_struct("RewriteAttribute")
            .field("process_attribute", &std::any::type_name::<F>())
            .field("description", &self.description)
            .finish()
    }
}

If you squint, you can see this is pretty much identical to what #[derive(Debug)] would expand to, except instead of passing &self.process_attribute to .field() I've used std::any::type_name::<F>().

Would it be reasonable to add an impl Debug block for functions and closures to core which just defers to core::any::type_name()? That way people can write #[derive(Debug)] and still implement Debug when their type contains a function or closure.

This should be a backwards-compatible change because functions and closures aren't currently Debug. I think the only problem would be that you can't be generic over arity, so we'd need to use a macro which implements Debug for all functions with 0-16 (or some other arbitrary number) arguments. Additionally, if Rust is able to be generic over arity in the future, the Debug impl could be updated in a backwards-compatible way because we're adding strictly more impls.

2

Actually, as all function types are generated by the compiler anyway, it'd not be unreasonable to implement Debug for them.

However, making impl Fn(...) -> _ imply impl Debug would be incorrect; what you'd want to write would be F: (Fn(_) -> _) + Debug (parentheses for clarity). Of course, the unnameable types of closures ([closure@souece.rs:LL:CC]) and function ZSTs (fn() {name}) don't implement Debug yet, so they wouldn't be usable with such a signature yet. ((Most) function pointers (fn(...) -> _) actually do implement Debug today.

I agree that for function ZSTs, type_name is probably the most correct Debug implementation they could have (especially now that we have type_name and the question around parametricity of type_name is dead :crab:.)

But closures are an interesting additional question: they have state! And it'd be nice to tell apart the state of closures when printing them via Debug. So maybe their "most correct" Debug implementation would be to call debug_struct with the type name, and provide the fields that it's captured? I'm not sure what's optimal here, and there definitely is still some room for discussion.

But yes, ultimately, all compiler generated function types should have a Debug implementation (though you still should have to ask for it on top of the Fn trait if you want to use it (which the derive would and currently does actually require)).

5 Likes
3

Actually, there is reason to avoid using type_name: the ability to strip/anonymize the code.

Currently, if you strip debug symbols (and maybe also set panic=abort), you should get a binary that has no more references to your filesystem and project layout anymore. Adding the generated type names of these function types would re-add the discoverable source information into the binary.

2 Likes
4

That's a good point, although it would only embed the name for types we've invoked std::any::type_name() on, wouldn't it? I feel like that should be okay, because if you're asking what a function's name is you do want to keep that information around after stripping.

Short of some complicated hacks trickery (e.g. dynamically inspecting the binary's debug info or playing around with linkers) there would be no way to make sure the string returned by type_name() is removed when you strip a binary, would there?

Wow, I didn't know function pointers implemented Debug, let alone the fact it was stabilised around 1.4.0!

The current implementation just prints the function's address though, which is a bit opaque and kinda useless for debugging...

#[stable(feature = "fnptr_impls", since = "1.4.0")]
impl<Ret, $($Arg),*> fmt::Debug for $FnTy {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        fmt::Pointer::fmt(&(*self as *const ()), f)
    }
}

I also notice it's for function pointers (i.e. fn(...) -> _) and not the function ZSTs. The impl for function pointers is more specific though, so you could still add a default impl for F: Fn(...) -> _ and use specialisation to make sure we don't break the previous behaviour for function pointers?

That would be quite cool. I hadn't thought of recursively invoking Debug on the captured fields, but it makes sense, a closure is just an unnameable struct after all.

Generating those impls would be a lot of work and require access to compiler internals though, it's not something you can do with a library change (like the function ZST case) or a simple macro.

5

Is that even true at all? The panic messages all contain the file paths and they are independent of the debug symbols, so you can't strip them.

1 Like
6

(That's why you get rid of unwinding with panic=abort, this removes panic messages in favor of smaller binaries)

edit: must be too early to think right now

7

That's not true. The panic messages are still there with panic=abort, it just doesn't unwind.

8

Well, it's an ideal that we're slowly trying to move towards, at least, and I'd rather avoid accidentally regressing progress towards it. (You could always set panic=unreachable_unchecked if you're insane!)

2 Likes
9

..is that for security reasons?

10

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.