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.

Sign up for GitHub

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

Jump to bottom

Consolidate diagnostic presentation logic into library error types #4494

Open
1 of 4 tasks
ErichDonGubler opened this issue Apr 25, 2023 · 3 comments · May be fixed by gfx-rs/naga#2319
Open
1 of 4 tasks

Consolidate diagnostic presentation logic into library error types #4494

ErichDonGubler opened this issue Apr 25, 2023 · 3 comments · May be fixed by gfx-rs/naga#2319
Assignees
@ErichDonGubler
Labels
kind: diagnostics Error message should be better naga Shader Translator type: enhancement New feature or request

Comments

@ErichDonGubler
Copy link
Member

ErichDonGubler commented Apr 25, 2023 •
edited
Loading

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 code1 for a second. Note how there are three places we had to consult naga-cli's output to consume all information provided2 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:

  • Currently, both naga-cli and wgpu-core transform specific errors into codespan_reporting::Diagnostics that are then rendered as separate terminal output. naga-the-library should own the conversion of errors it returns into Diagnostics, and offer consumers like wgpu-core and naga-cli a chance to work with those instead.

  • Instead of using log::error!(…) and the like as a separate fragmentary channel of error context, naga's front-end and validation errors should carry all relevant information in Result::Errs returned from its method and function calls.

    The (non-exhaustive) list of known cases of this include:

    • Type mismatches
      • Binary operations

Footnotes

  1. For reference, resolving this is scoped to [wgsl-in] Implement automatic type conversions (abstract types) #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. ↩
@teoxoy teoxoy added type: enhancement New feature or request kind: diagnostics Error message should be better labels Apr 26, 2023
This was referenced Apr 26, 2023
Draft
Closed
@ErichDonGubler ErichDonGubler changed the title Diagnostics lack essential information Consolidate diagnostic presentation logic into libraryt error types Apr 27, 2023
@ErichDonGubler ErichDonGubler changed the title Consolidate diagnostic presentation logic into libraryt error types Consolidate diagnostic presentation logic into library error types Apr 27, 2023
@ErichDonGubler ErichDonGubler mentioned this issue May 2, 2023
Closed
@Wumpf
Copy link
Member

Wumpf commented May 2, 2023 •
edited
Loading

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
Copy link
Member Author

ErichDonGubler commented May 2, 2023 •
edited
Loading

@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: #4429

@ErichDonGubler ErichDonGubler mentioned this issue Oct 25, 2023
Open
@cwfitzgerald cwfitzgerald added the naga Shader Translator label Oct 25, 2023
@cwfitzgerald cwfitzgerald transferred this issue from gfx-rs/naga Oct 25, 2023
@teoxoy teoxoy mentioned this issue Nov 3, 2023
Closed
@ErichDonGubler ErichDonGubler self-assigned this Jan 22, 2024
@ErichDonGubler ErichDonGubler mentioned this issue Oct 22, 2024
Merged
7 tasks
@ErichDonGubler
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ErichDonGubler ErichDonGubler

Labels
kind: diagnostics Error message should be better naga Shader Translator type: enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fold Error source chain Display output into shader errors ErichDonGubler/naga
4 participants
@ErichDonGubler @Wumpf @cwfitzgerald @teoxoy