Include spanless diagnostics as file annotations in new compiler path#4517
Include spanless diagnostics as file annotations in new compiler path#4517stefanvanburen wants to merge 2 commits intomainfrom
Conversation
Matches the old compiler behavior where errors without source positions were still surfaced as FileAnnotations (with nil fileInfo), rather than being dropped or returned as raw errors. Updates TestWorkspaceDuplicateFailSpecificModule to expect exit code 100 (FileAnnotationSet) and the file-prefixed error format instead of the previous exit code 1 with a "Failure: " prefix. Supersedes the approach in #4499.
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
| t, | ||
| nil, | ||
| 1, | ||
| 100, |
There was a problem hiding this comment.
Urgh, sorry for being annoying about this, and this gives me a little bit of extra insight on how the compiler diagnostics have always been... so unfortunately, this would be considered a breaking change (exit code + stdout/stdwarn behaviour)...
So I think the approach we need to take here is:
- We continue to suppress the diagnostics with no primary span
- And then have
DuplicateProtoPathErrorcontinue to be surfaced through the module build
We should document this in the code path where we do this pretty carefully, since this can be confusing to the reader of the code, but also, not introducing breaking behaviours is the priority.
Ideally, we would do an audit of the diagnostic surface area now that we have the new compiler in, but that may not be feasible right now -- in the short term, we need to prioritise retaining consistency in the high-level behaviour...
When a proto file is contained in multiple modules (DuplicateProtoPathError), the new experimental compiler was silently swallowing the typed error: FDS did not check the fatal error from its Link sub-query, producing an empty FileDescriptorSet instead of propagating the error upward. This meant callers of compileImage could not inspect it via errors.As, and the downstream "nil FileDescriptor" panic was unhelpful. The fix is in protocompile (bufbuild/protocompile#722): FDS.Execute now checks linkResult[0].Fatal before iterating over IR files, propagating the Link fatal as its own. With this, results[0].Fatal in compileImage is DuplicateProtoPathError, which is returned directly so callers retain its type information and the existing "Failure: " / exit code 1 behavior is preserved.
|
@doriable I'll update the PR description if you agree, but PTAL at 0808f44, which pulls in bufbuild/protocompile#722 instead and handles the fatal errors first, which I think should be what we want to do... |
3 participants
Matches the old compiler behavior where errors without source positions were still surfaced as FileAnnotations (with nil fileInfo), rather than being dropped or returned as raw errors. Updates TestWorkspaceDuplicateFailSpecificModule to expect exit code 100 (FileAnnotationSet) and the file-prefixed error format instead of the previous exit code 1 with a "Failure: " prefix.
Supersedes the approach in #4499.