Improve WASM layer error handling #149
Assignees
Labels
enhancement
New feature or request
Comments
Closed
|
Not sure if was meant to be covered in this issue, but another concern is that errors contain This is also the root cause of #750 , I think |
github-merge-queue bot
pushed a commit
that referenced
this issue
Oct 17, 2023
Fixes #750 . Addresses some of #149 . Runtime error squiggles in the playground were off by one. The root cause was that we actually use `SourceMap` offsets, not `Source` offsets, to report the error. This sometimes used to "just work" since we always have a single file in the compilation. However, this really broke down when #614 inserted an additional EOF character between sources... and then when #622 moved the entry expression (`""` in this case) to the _beginning_ of the source map. Moreover, when runtime error spans came from stdlib, the offsets were just wildly off (since the spans were actually coming from stdlib). This probably wasn't noticeable since the UI "covered up" the problem by mapping the squiggles to the very end of the document. So, the bulk of this PR is properly resolving `SourceMap` offsets to `Source` offsets in the wasm layer. When the span doesn't belong in the current document, we just show a squiggle at the beginning of the document. But we also take advantage of the "related information" feature of Monaco to still include the relevant stdlib labels in the hover text (see screenshots below). This brings spans to the realm of "technically correct", but I'm still not happy with the experience in the playground UI for runtime errors. For a better experienece, we should consider: - Either, grabbing spans from the call stack and squiggle the statements in the stack. - Or, not bothering with runtime error squiggles at all.... However, this change was already getting too big, and the callstack changes would have been pretty complicated. I decided to just go with this as an incremental improvement, and debate the rest later. ### Playground runtime error changes **Case 1: The runtime error has a label that belongs in the current file** This had the off-by-one error, now fixed. **Before**  **After**  **Case 2: The runtime error has a label that belongs in stdlib** This used to map to the end of the file (just by luck), now it maps to the beginning of the file and shows the label from the original location in the hover text. **Before**  **After**  **Case 3: The runtime error has a label that belongs in stdlib, _and_ the label doesn't contain a message** This is the worst experience - we could show the stdlib location in the hover, but there's no message in the label so it would be entirely pointless to display. So we omit it. The location of the squiggle is now technically correct but just as unhelpful. **Before**  **After**  ### Other goodness: - In the playground, we clear the last runtime squiggle when the user starts typing (or click on a different sample). - Properly reporting compiler errors with multiple spans (e.g. [`Ambiguous`](https://github.com/microsoft/qsharp/blob/c75940913518763000a3b18b053ef683cc0e3ad7/compiler/qsc_frontend/src/resolve.rs#L57)) using VSCode/Monaco's "related information" feature. These related spans show up in the error hover.  - `qsc::compile::compile()` now just returns a `WithSource<Error>` instead of relying on the caller to use the `SourceMap` to resolve error spans to source files. Every caller has to do this mapping at _some_ point to report errors, so it makes sense for the errors to just contain that information from the start. Using `WithSource` as the error type also mirrors the error types that `Interpreter` and `Incremental` already return. - Somehow `VSDiagnostic` was defined in two places. One with the name `IDiagnostic` and the other `VSDiagnostic`. They describe the same thing. Deleted one of the definitions.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The
qsc_wasmlayer does not fully handle the errors reported bycheck_internal, as some information gets lost in translation (secondary labels, help text, etc). In addition,run_internalAPI currently only supports returning a single error rather than a list, which might not work for errors from passes like extracting entry point. We need to update the error handling to support multiple, detailed errors, and work on the compilation/execution context caching semantics to better provide actionable errors to the user via WASM.Tasks
The text was updated successfully, but these errors were encountered: