cpp_demangle icon indicating copy to clipboard operation
cpp_demangle copied to clipboard

Don’t forward demangling errors in Display impl

Open surma opened this issue 1 year ago • 3 comments

In the Display implementation of Symbol the potential error after demangling is turned into a std::fmt::Error.

I’d like to propose to not do that, as this will lead to panics in many cases (even just a simple format!("{my_symbol}"). This PR changes the Display::fmt implementation to output "<Demangling failed>" in case where the demanging failed, avoiding the panic scenario.

surma avatar Sep 28 '24 11:09 surma

Can you provide a testcase that shows the issue you're talking about? I don't think I understand the problem.

khuey avatar Sep 29 '24 16:09 khuey

Yeah, I should have really provided that. Apologies.

I dug through the WebAssembly file that triggered the problem for me, and the problematic function name in this case was "RD4":

pub fn main() {
    let func_name = "RD4";
    let sym = cpp_demangle::Symbol::new(func_name).expect("new");
    // sym.demangle(&Default::default()) // <- would return an error
    println!("{}", sym);
}

The program above will panic with the following message:

❯ cargo run --example testcase
   Compiling cpp_demangle v0.4.4 (/Users/surma/src/github.com/gimli-rs/cpp_demangle)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.54s
     Running `target/debug/examples/testcase`
thread 'main' panicked at library/std/src/io/mod.rs:1836:21:
a formatting trait implementation returned an error when the underlying stream did not
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

As stated in the docs of Display::fmt:

This function should return Err if, and only if, the provided Formatter returns Err. String formatting is considered an infallible operation;

So even if demanging fails, Display::fmt should not return an Error. That’s what I am trying to fix in this PR.

surma avatar Sep 29 '24 21:09 surma

Ok, thanks. Looks like this assertion was added in rust-lang/rust#125012

khuey avatar Sep 30 '24 14:09 khuey

We decided to get rid of the Display impl entirely so this PR has been obsoleted.

khuey avatar Sep 24 '25 02:09 khuey