Stream: git-wasmtime

Topic: wasmtime / PR #5318 cranelift-isle: Rewrite error reporting

Wasmtime GitHub notifications bot (Nov 23 2022 at 00:30):

jameysharp opened PR #5318 from isle-errors to main:

There were several issues with ISLE's existing error reporting implementation.

• When using Miette for more readable error reports, it would panic if errors were reported from multiple files in the same run.
• Miette is pretty heavy-weight for what we're doing, with a lot of dependencies.
• The Error::Errors enum variant led to normalization steps in many places, to avoid using that variant to represent a single error.

This commit:

• replaces Miette with codespan-reporting
• gets rid of a bunch of cargo-vet exemptions
• replaces the Error::Errors variant with a new Errors type
• removes source info from Error variants so they're easy to construct
• adds source info only when formatting Errors
• formats Errors with a custom Debug impl
• shares common code between ISLE's callers, islec and cranelift-codegen
• includes a source snippet even with fancy-errors disabled

I tried to make this a series of smaller commits but I couldn't find any good split points; everything was too entangled with everything else.

I still need to test this with input that actually has errors in it, but I wanted to get early review at this point since it at least builds cleanly both with and without the fancy-errors feature.

Wasmtime GitHub notifications bot (Nov 23 2022 at 00:30):

jameysharp requested fitzgen for a review on PR #5318.

Wasmtime GitHub notifications bot (Nov 23 2022 at 00:45):

jameysharp updated PR #5318 from isle-errors to main.

Wasmtime GitHub notifications bot (Nov 23 2022 at 00:46):

jameysharp updated PR #5318 from isle-errors to main.

Wasmtime GitHub notifications bot (Nov 23 2022 at 00:49):

jameysharp updated PR #5318 from isle-errors to main.

Wasmtime GitHub notifications bot (Nov 23 2022 at 00:49):

jameysharp has marked PR #5318 as ready for review.

Wasmtime GitHub notifications bot (Nov 23 2022 at 22:09):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 23 2022 at 22:09):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 23 2022 at 22:09):

cfallin created PR review comment:

Likewise here, for deploy -> run.

Wasmtime GitHub notifications bot (Nov 23 2022 at 22:09):

cfallin created PR review comment:

Can you say more about this exemption update? As far as I understood it, we decided we wouldn't add any more exemptions past the initial set that we had when we adopted cargo-vet, so this might need a true audit rather than an expanded exemption. (But I don't claim to fully understand our policies and cargo-vet subtleties here, so others please feel free to chime in too...)

Wasmtime GitHub notifications bot (Nov 23 2022 at 22:12):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 23 2022 at 22:12):

cfallin created PR review comment:

Ah, actually, reading more, it seems that safe-to-run is a subset of safe-to-deploy (a weaker claim), so this is a downgrade and should be automatically fine; does that match your understanding? Was this change due to some indication that we didn't need the wider claim anymore?

Wasmtime GitHub notifications bot (Nov 23 2022 at 22:13):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Nov 23 2022 at 22:13):

jameysharp created PR review comment:

I didn't think about it too hard because this is something cargo vet did automatically, and I figured it knew what it was doing. But I believe this is weakening the exemption. If I understand correctly, safe-to-deploy implies safe-to-run, but not the other way around, so with this change we're no longer saying we trust these exempted crates as much as we did before.

Wasmtime GitHub notifications bot (Nov 23 2022 at 22:19):

jameysharp created PR review comment:

Jinx :laughing:

All the changes to config.toml are because Miette had a bunch of dependencies while codespan-reporting only depends on a couple of crates, which I guess we were already using elsewhere. termcolor is used by clap, cranelift-tools, and others, while unicode-width is used by clap, wast, and others.

Wasmtime GitHub notifications bot (Nov 23 2022 at 22:19):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Nov 23 2022 at 22:20):

jameysharp merged PR #5318.

Last updated: Nov 22 2024 at 17:03 UTC