gfx-rs / wgpu

A cross-platform, safe, pure-Rust graphics API.
https://wgpu.rs
Apache License 2.0
12.45k stars 913 forks source link

Consolidate diagnostic presentation logic into library error types #4494

Open ErichDonGubler opened 1 year ago

ErichDonGubler commented 1 year ago

Problem statement

Diagnostic information for type mismatch errors (and, from what I've seen, most validation errors) is fragmented to the point that it undermines all but immediate consumers of Naga CLI. Some quick examples:

image

image

Ignore that we're rejecting perfectly fine code[^1] for a second. Note how there are three places we had to consult naga-cli's output to consume all information provided[^2] for this message:

  1. The log::error!(…) output of src/valid/{compose,expression}.rs, to actually learn about the types that aren't matching.
  2. A nicely formatted set of spans provided by WithSpan::spans, accessed in emit_annotated_error.
  3. Finally, the stderr output of naga-cli emitted by print_err, which actually tells us the kind of expression that failed.

Because of this, we are not creating a uniformly good experience between users of Naga CLI and users of Naga as a library. Evidence of this can already be found in current Nightly builds of Firefox where only (2) is actually presented in the JS console (😢 related bug):

image

…where Firefox is missing critical information from (1) and (3) entirely. 😬❗

Solution statement

A great example of how the above can all presented to a user can be found in the front page of the miette project:

My proposed solution is to expose an API to combine all error information into one or more codespan_reporting::Diagnostic (with spans, if available), which downstream can work with as they determine fit. Concretely, the next short-term steps I see are:

[^1]: For reference, resolving this is scoped to gfx-rs/wgpu#4400. [^2]: There is still something missing: we don't get any span help showing us which type corresponds to what locations in source, still. 😮‍💨 The miette example I provide later on should make this clear.

Wumpf commented 1 year ago

I'm all for removing as much as possible unwarranted logging for compilation error. The only kind of error I'd expect on log are error concerning Naga failing to do something. Failing compilation is an expected outcome of a compilation, thus not an error to be logged.

Move away from transparently exposing error details in naga

Less happy about that. That makes it much harder for tools to reason about errors. Ideally IDEs/language servers could use Naga's output to suggest fixes, jump to code lines etc.. The more structured information is available the better.

With that in mind I think a layered system would be optimal: Naga should give out (fairly complex) error types that are then convertible to human readable errors as an independent step

EDIT: I'm maybe reading "transparently" wrong here in the issue description. If that's what you meant nevermind, I'm all for it then 😄

ErichDonGubler commented 1 year ago

@Wumpf: I definitely don't want to block use cases for code assists and the like! What I meant by "transparent" and "opaque" here mostly refers to the representation/layout of data structures. I'm of two minds WRT how tooling on top of Naga should access relevant information:

  1. Being transparent by default is not necessarily a requirement for implementing my proposal. Also, we shouldn't have breaking changes without a suitable replacement, if users already depend on the concrete error types we return. I can de-scope that from this proposal. On the other hand…
  2. It might be easier for maintainers of Naga to not expose the exhaustive set of internal error variants as part of its public interface. Right now, every struct field and enum variant in Naga's error types is a public API decision. Changing this, however, requires designing another public API, and transitioning to it. I don't see a clear-cut win in that direction, either, though. 🤔 CC @teoxoy, @jimblandy.

I think the best resolution here is to discuss the above further in a separate issue. I've edited the OP that you quoted to:

Provide a "blessed" way to present errors returned by naga:

…and created a new issue here: gfx-rs/wgpu#4429

ErichDonGubler commented 3 days ago

Some progress on this front: #6436 makes it so that the Display implementation of ShaderError<WithSpan<ValidationError>>> is the same as Naga's CLI output, and includes the error chain output originally complained about in the OP. This satisfies the first checkbox in the OP.