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

Make the impl Debug for Any (+ Send) (and Box forms) more useful #1389

Open
abonander opened this issue Dec 1, 2015 · 12 comments
Open

Make the impl Debug for Any (+ Send) (and Box forms) more useful #1389

abonander opened this issue Dec 1, 2015 · 12 comments
Labels
T-libs-api Relevant to the library API team, which will review and decide on the RFC.

Comments

@abonander
Copy link

Belated follow-up to the discussion on rust-lang/rust#27719

Currently, the Debug impl for Box<Any> is almost useless. It just prints "Box<Any>".

One of the main uses of Box<Any> in the stdlib is to carry the message of a panic!() invocation back to the parent, out of thread::spawn().join() or thread::catch_panic(). In this case, the Box<Any> could be one of three possibilities:

  • &'static str, if panic!() was called with a string literal and no formatting arguments.
  • String, if panic!() was called with formatting arguments.
  • Any other Send type which was passed to panic!() besides a string literal.

While the first case is technically a subset of the third, I count it as its own item because it's a common pattern when panic!() wants to give an error message but doesn't have any dynamic data to add to it.

Given the comparatively negligible overhead of downcasting, I think impl Debug for Box<Any> should test the first two cases before printing its uselessly generic message:

impl Debug for Box<Any> {
    fn fmt(&self, fmt: &mut Formatter) -> fmt::Result<()>, {
        if let Ok(str_slice) = self.downcast_ref::<&'static str>() {
            fmt.pad(str_slice);
        } else if let Ok(string) = self.downcast_ref::<String>() {
            fmt.pad(string);
        } else {
            fmt.pad("Box<Any>");
        } 
    }
}

Of course, this applies to impl Debug for Any (+ Send) as well.

While this is technically a breaking change since it modifies stable behavior, I doubt anyone was relying on the exact value printed by this impl, given its completely uninformative message.

In fact, this should massively reduce the occurrence of the uninformative Thread ### panicked at Box`` message in ICE reports and StackOverflow questions. It appears this specialization is already being done: https://github.com/rust-lang/rust/blob/master/src/libstd/panicking.rs#L33

@sfackler
Copy link
Member

sfackler commented Dec 1, 2015

I'm not sure if it's a good idea to do this directly in Box<Any>'s Debug impl, but having a stable wrapper or something like that would definitely be nice, particularly in a world with custom panic handlers.

@abonander
Copy link
Author

@sfackler Do you have an example of a downside to this?

@sfackler
Copy link
Member

sfackler commented Dec 2, 2015

It seems a bit weird to hardcode special support for exactly two types because they show up in one of Box<Any>'s use cases.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 2, 2015

is there a possibility to use impl specialization once we get it to add a method to Any that calls the actual types Debug impl if there is one, and prints Box<Any> otherwise?

@abonander
Copy link
Author

@oli-obk You can't concatenate arbitrary traits into an object, so you could never specialize for Any + Debug because the type checker won't allow it. Even if that was possible, it still doesn't fix the useless Debug impl after erasure, which is my primary concern.

@sfackler So I'm gathering that your opinion is of the "all or nothing" sort, and since the former isn't exactly feasible, then the only reasonable thing we should do is nothing. You'll have to forgive me if I don't find that quite satisfactory.

We could extend this to all invariant primitive types for little additional overhead (if it can be optimized down to a jump table) but that would be a lot of code bloat for arguably little benefit. I would say that &'static str is the most informative invariant primitive we have, and String is a natural extension of that as its owned counterpart.

If at some point we gain dynamic trait object downcasting either through some implementation detail of the type system or vtable hacking, we could remove the hardcoded solution and print all applicable types. But for now, I think something like the above is much better than a wrapper type or nothing at all.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 2, 2015

I meant to add a try_debug method to the Any trait and then override it for all T: Debug.

@seanmonstar
Copy link
Contributor

@cybergeek94 his suggestion was to use a wrapper to do this.

pub struct Panicked(Box<Any>);

impl Debug for Panicked {
    fn fmt(&self, f: &mut Formatter) -> Result {
        if let Ok(str_slice) = self.0.downcast_ref::<&'static str>() {
            fmt.pad(str_slice);
        } else if let Ok(string) = self.0.downcast_ref::<String>() {
            fmt.pad(string);
        } else {
            fmt.pad("Box<Any>");
        } 
    }
}

And then use the wrapper:

match thread::spawn(do_work) {
    Ok(..) => (),
    Err(any) => println!("do_work error: {:?}", Panicked(any))
}

@abonander
Copy link
Author

That would probably be acceptable if it implemented From<Box<Any>> so it worked with try!() but I'm still not seeing much compelling evidence for not having it in the impl Debug for Box<Any> to begin with.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 3, 2015

I actually meant whether the following would be possible with specialization

static BOX_ANY: &'static &'static str = &"Box<Any>";
pub trait Any: Reflect + 'static {
    fn get_type_id(&self) -> TypeId;
    fn as_debug(&self) -> &Debug {
        BOX_ANY
    }
}

impl<T: Debug> Any for T {
    fn as_debug(&self) -> &Debug {
        &self
    }
}

impl Debug for Any {
    fn fmt(&self, f: &mut Formatter) -> Result {
        self.as_debug().fmt(f)
    }
}

See the error in the Playground

@abonander
Copy link
Author

I do want everyone to note that this exact thing is already being done in the panicking module, to print the panic message to stderr (I mentioned this in an edit of the original post but edits can be easy to miss): https://github.com/rust-lang/rust/blob/master/src/libstd/panicking.rs#L33

It would be nice to expose this conveniently to the user somehow, either by runtime specialization in the Debug impl or something like Any::as_str(&self) -> Option<&str> which @SimonSapin proposed in the original discussion linked in the OP. I wouldn't mind having both, if possible.

@nrc nrc added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Aug 19, 2016
@mzabaluev
Copy link
Contributor

I support the special case of downcasting to String even if it is implemented with hacks under the hood. Provision of dynamic type checking and debug formatting hooks from libcore up to the allocating runtime's owned form of the standard string can be considered nearly as worthwhile as the fill-in hook for panicking.

Meanwhile, the problem can be solved in an ad-hoc way by extension trait impls on &Any, std::thread::Result, and the like.

@mzabaluev
Copy link
Contributor

I have filed an RFC PR proposing a solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

No branches or pull requests

6 participants