Print VerifierErrors in Display for CodegenResult

[jyn514]

• This has been discussed in issue #..., or if not, please tell us why
  here.
  This fixes a regression in Cranelift.
• A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.
  These error messages seem to have gotten lost in the switch to thiseerror
  (d23030f#diff-55f6fffe972a2ad7efb7edd56cb29afcR15)

That change made it very hard to debug bad generated code.
This change prints the message again.

Before: Verifier errors
After:

Verifier errors:
- inst16: Branch must have an encoding

• This PR contains test cases, if meaningful.
  N/A
• A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so and/or ping
  bnjbvr. The list of suggested reviewers on the right can help you.
  I am not sure who should review this @bnjbvr.

[bjorn3]

The Debug impl already shows the origin error. Wasmtime did the exact opposite of this PR a few days ago: bytecodealliance/wasmtime#532. Also doing this may cause a warning in the future: dtolnay/thiserror#38.

[jyn514]

I don't understand. Alex says

it's already rendered in the final error via #[from]

but that is just not the case, at least on my machine.

[bjorn3]

For Display it doesnt, but for Debug (used by .unwrap()) it does.

[bjorn3]

Also anyhow::Error shows it nice: see second bullet point at https://docs.rs/anyhow/1.0.20/anyhow/.

[jyn514]

Ah so maybe this is a bug in thiserror, it should show the root cause in both

[jyn514]

Because as it stands the display output is essentially useless

[bjorn3]

Even the docs of std::error::Error::source do it this way: The Display impl only displays the error itself, and not the lower-level source of the error.

// [...]
impl fmt::Display for SuperError {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        write!(f, "SuperError is here!") // <----- self.side not displayed here
    }
}

impl Error for SuperError {
    fn description(&self) -> &str {
        "I'm the superhero of errors"
    }

    fn source(&self) -> Option<&(dyn Error + 'static)> {
        Some(&self.side)
    }
}
// [...]

anyhow::Error does support writing the lower-level errors by using the {:#} or {:?} formatter.

[dtolnay]

The Display representation is not supposed to also print causes. That would break the ability for an application to render a cause chain in the way that it wants.

See this example from the standard library:

use std::ffi::CString;
use std::error::Error;

fn main() {
    let cstring = CString::new(vec![255]).unwrap();
    let error = cstring.into_string().unwrap_err();
    println!("DISPLAY: {}", error);
    println!("CAUSE: {}", error.source().unwrap());
}

When you print an error, you can print causes along with it by looping:

eprintln!("Error: {}", error);
if error.source().is_some() {
    eprintln!("\nCaused by:");
}
let mut error = &error as &dyn Error;
while let Some(cause) = error.source() {
    eprintln!("    - {}", cause);
    error = cause;
}

Or use one of the builtin representations from anyhow::Error which can do that loop.

[bnjbvr]

Interesting. From the last comments I imagine that this PR isn't needed, and instead users would need to use the debug / pretty-print display mode instead. If this is in the context of clif-util, then we should change it, otherwise, this is out of our control. Does it make sense?

[abrown]

Makes sense to me; thanks @bjorn3 and @dtolnay for explaining how these errors should be used. Now I'm wondering if I made this same mistake over in wasmtime: https://github.com/bytecodealliance/wasmtime/pull/583/files#diff-d6f540e93d7b2f9e1ec39b5e6d0a3c7aR160. Could you take a look?

@jyn514, should we close this?

[jyn514]

@jyn514, should we close this?

I haven't gotten a chance to test the built-in representation that @dtolnay mentioned yet. Once I make sure that has reasonably similar output to what it used to be I'm fine with closing this.

One thing I don't think has been mentioned is that this is a very different API from how it used to be, and that it's not consistent with the rest of the errors in the codebase (see linked commit at the top). It would be nice to have documentation for how it's supposed to be used.

[jyn514]

this is a very different API from how it used to be, and that it's not consistent with the rest of the errors in the codebase

In other words, even though you've convinced me it's not a bug, it still looks like a bug.

[jyn514]

I'm still not quite happy with this ... format!("{:#}", err); gives me Compilation error: Verifier errors still, I think because the outer error isn't an instance of anyhow::Error, it's a ModuleError instead. Is there a way to pass the formatting specifier down to the inner error?

This also only prints 'Verifier errors':

if let ModuleError::Compilation(inner) = err {
    utils::fatal(format!("{:#}", inner), 4);
}

[jyn514]

It seems to me the issue is that anyerror is used inconsistently. If ModuleError were an instance of anyhow::Error, "{:#}" would work fine, but because it's not, I don't have a way to print the cause for the error at all without using Debug, which I don't want.

[bjorn3]

The Error trait has a cause function.

[dtolnay]

To print the error and all its causes you would need to loop over them:

use std::error::Error as StdError;

let mut error = &error as &dyn StdError;
eprint!("{}", error);
while let Some(next) = error.source() {
    eprint!(": {}", next);
    error = next;
}
eprintln!();

[jyn514]

That results in duplicate errors.

Compilation error: Verifier errors: Verifier errors: - inst14: Branch must have an encoding

code:

if let Err(err) = self.module.define_function(func_id, &mut ctx) {
            let mut error = &err as &dyn std::error::Error;
            eprint!("{}", error);
            while let Some(next) = error.source() {
                eprint!(": {}", next);
                error = next;
            }
}

[dtolnay]

That would mean that one of the errors has a bad Display impl or Error impl. The causes aren't supposed to duplicate each others messages.

[jyn514]

Right, this is exactly my point, the error display is inconsistent between ModuleError and VerifierError. The error handling could be changed the other way, so that all errors require you to loop over cause(), but that would require changing a lot more code. All this PR is doing is making the Display for VerifierError be consistent with the rest of the errors in Cranelift.

[jyn514]

See e.g. d23030f#diff-db3d631459309d14f5195f1646e48e09L127 for an example of how most of the other errors look.

[dtolnay]

ModuleError looks fine to me because none of those errors declare a cause. Duplication happens only when an error's Display impl prints its cause and also exposes its cause in its Error impl to print itself again. That would be a bug that needs to be fixed.

[jyn514]

ModuleError looks fine to me because none of those errors declare a cause.

I'm not sure how you concluded this. When I call cause on a ModuleError with an inner CodegenError, it gives me the inner error. You can see this above: Compilation error: Verifier errors is printed by ModuleError, then Verifier errors is printed by CodegenError, then - inst14: Branch must have an encoding is printed by VerifierError

That would be a bug

If this is a bug, so is almost every other Display impl in the whole codebase.

[dtolnay]

Based on the first commit you linked (d23030f#diff-db3d631459309d14f5195f1646e48e09R143-R144):

    #[error("Compilation error: {0}")]
    Compilation(CodegenError),

There would need to be a #[source] or #[from] attribute for the field to be recognized as a cause. Maybe it was added later?

The two correct ways to write that variant would be either exactly like that, or else:

    #[error("Compilation error")]
    Compilation(#[source] CodegenError), // or #[from]

[jyn514]

You're quite right, it was added later: 8a486a4. Thanks for catching that.

[bnjbvr]

@jyn514 my understanding from a quick glance at previous comments is that this PR needs to be reworked a bit. Is this correct? (No pressure of course, I'm just trying to understand what's the status here.)

[jyn514]

That is @bjorn3 and @dtolnay 's opinion but not mine. This PR just makes the CodegenResult impl consistent with all other impls in the Cranelift codebase. Whether those other impls are wrong is IMO a separate issue.

[alexcrichton]

Thanks for the PR again, and as a procedural note the Cranelift repository has now merged into the wasmtime repository.

PRs are no longer landing in this repository, and unfortunately there's no "one button" solution to move a PR to the wasmtime repository. A script has been prepared, however, to assist you in transferring this PR to the wasmtime repo. Feel free to reach out on Zulip with any questions!

[jyn514]

I don't think this PR is welcome, I'd rather just close it.