-
Notifications
You must be signed in to change notification settings - Fork 933
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.
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
Consolidate diagnostic presentation logic into library error types #4494
Comments
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.
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 😄 |
@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:
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:
…and created a new issue here: #4429 |
Some progress on this front: #6436 makes it so that the |
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:
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:log::error!(…)
output ofsrc/valid/{compose,expression}.rs
, to actually learn about the types that aren't matching.WithSpan::spans
, accessed inemit_annotated_error
.stderr
output ofnaga-cli
emitted byprint_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):
…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
andwgpu-core
transform specific errors intocodespan_reporting::Diagnostic
s that are then rendered as separate terminal output.naga
-the-library should own the conversion of errors it returns intoDiagnostic
s, and offer consumers likewgpu-core
andnaga-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 inResult::Err
s returned from its method and function calls.The (non-exhaustive) list of known cases of this include:
Footnotes
For reference, resolving this is scoped to [wgsl-in] Implement automatic type conversions (abstract types) #4400. ↩
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. ↩The text was updated successfully, but these errors were encountered: