From 9d87b6333a17abc9ddbbdc1ef5d998e26c1b9b4e Mon Sep 17 00:00:00 2001 From: Stefan VanBuren Date: Tue, 28 Apr 2026 16:07:45 -0400 Subject: [PATCH 1/2] Include spanless diagnostics as file annotations in new compiler path 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. --- cmd/buf/workspace_test.go | 8 +++---- private/bufpkg/bufimage/build_image.go | 32 ++++++++++++++++++-------- 2 files changed, 27 insertions(+), 13 deletions(-) diff --git a/cmd/buf/workspace_test.go b/cmd/buf/workspace_test.go index 6398555883..00af209ff3 100644 --- a/cmd/buf/workspace_test.go +++ b/cmd/buf/workspace_test.go @@ -1269,9 +1269,9 @@ func TestWorkspaceDuplicateFailSpecificModule(t *testing.T) { testRunStdoutStderrNoWarn( t, nil, - 1, + 100, ``, - `Failure: foo.proto is contained in multiple modules: + `foo.proto:1:1:foo.proto is contained in multiple modules: path: "other/proto" path: "proto"`, "build", @@ -1280,9 +1280,9 @@ func TestWorkspaceDuplicateFailSpecificModule(t *testing.T) { testRunStdoutStderrNoWarn( t, nil, - 1, + 100, ``, - `Failure: v1/foo.proto is contained in multiple modules: + `v1/foo.proto:1:1:v1/foo.proto is contained in multiple modules: path: "other/proto", includes: "other/proto/v1", excludes: "other/proto/v1/inner" path: "proto", includes: ["proto/v1", "proto/v2"], excludes: ["proto/v1/inner", "proto/v2/inner"]`, diff --git a/private/bufpkg/bufimage/build_image.go b/private/bufpkg/bufimage/build_image.go index f8b97dcd26..ff63cc1de3 100644 --- a/private/bufpkg/bufimage/build_image.go +++ b/private/bufpkg/bufimage/build_image.go @@ -129,7 +129,6 @@ func compileImage( } var fileAnnotations []bufanalysis.FileAnnotation - var rawErrors []error for _, diagnostic := range diagnostics.Diagnostics { if diagnostic.Level() > report.Error { // We only surface [report.Error] level or more severe diagnostics as build errors. @@ -138,12 +137,30 @@ func compileImage( } primary := diagnostic.Primary() if primary.IsZero() { - // Diagnostics without a source span (e.g. file-open errors such as - // DuplicateProtoPathError) cannot be represented as FileAnnotations. - // Surface them as raw errors so callers see the full message. - if diagnostic.File() != "" { - rawErrors = append(rawErrors, errors.New(diagnostic.Message())) + // Diagnostics without a source span are still included as FileAnnotations + // with no position information, matching the behavior of the old compiler. + var fi bufanalysis.FileInfo + if f := diagnostic.File(); f != "" { + path := moduleFileResolver.PathForLocalPath(f) + if path == "" { + path = f + } + fi = &fileInfo{ + path: path, + externalPath: moduleFileResolver.ExternalPath(path), + } } + fileAnnotations = append(fileAnnotations, bufanalysis.NewFileAnnotation( + fi, + 0, + 0, + 0, + 0, + "COMPILE", + diagnostic.Message(), + "", // pluginName + "", // policyName + )) continue } start := primary.Location(primary.Start, length.Bytes) @@ -174,9 +191,6 @@ func compileImage( ), ) } - if len(rawErrors) > 0 { - return nil, errors.Join(rawErrors...) - } if len(fileAnnotations) > 0 { return nil, bufanalysis.NewFileAnnotationSet(fileAnnotations...) } From 0808f448f4c57a52611356ad506f587c5b6597e0 Mon Sep 17 00:00:00 2001 From: Stefan VanBuren Date: Tue, 28 Apr 2026 19:28:53 -0400 Subject: [PATCH 2/2] Propagate DuplicateProtoPathError as typed error from new compiler path 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. --- cmd/buf/workspace_test.go | 8 ++--- go.mod | 2 +- go.sum | 4 +-- private/bufpkg/bufimage/build_image.go | 48 ++++++++------------------ 4 files changed, 22 insertions(+), 40 deletions(-) diff --git a/cmd/buf/workspace_test.go b/cmd/buf/workspace_test.go index 00af209ff3..6398555883 100644 --- a/cmd/buf/workspace_test.go +++ b/cmd/buf/workspace_test.go @@ -1269,9 +1269,9 @@ func TestWorkspaceDuplicateFailSpecificModule(t *testing.T) { testRunStdoutStderrNoWarn( t, nil, - 100, + 1, ``, - `foo.proto:1:1:foo.proto is contained in multiple modules: + `Failure: foo.proto is contained in multiple modules: path: "other/proto" path: "proto"`, "build", @@ -1280,9 +1280,9 @@ func TestWorkspaceDuplicateFailSpecificModule(t *testing.T) { testRunStdoutStderrNoWarn( t, nil, - 100, + 1, ``, - `v1/foo.proto:1:1:v1/foo.proto is contained in multiple modules: + `Failure: v1/foo.proto is contained in multiple modules: path: "other/proto", includes: "other/proto/v1", excludes: "other/proto/v1/inner" path: "proto", includes: ["proto/v1", "proto/v2"], excludes: ["proto/v1/inner", "proto/v2/inner"]`, diff --git a/go.mod b/go.mod index cf4ef6de4e..66b8d03a52 100644 --- a/go.mod +++ b/go.mod @@ -18,7 +18,7 @@ require ( connectrpc.com/connect v1.19.2 connectrpc.com/grpcreflect v1.3.0 connectrpc.com/otelconnect v0.9.0 - github.com/bufbuild/protocompile v0.14.2-0.20260427235036-6b040a6b602b + github.com/bufbuild/protocompile v0.14.2-0.20260428232503-17b06932e73e github.com/bufbuild/protoplugin v0.0.0-20260414125817-25d1d281b46b github.com/cli/browser v1.3.0 github.com/gofrs/flock v0.13.0 diff --git a/go.sum b/go.sum index 5bcf293ff3..abe10817f3 100644 --- a/go.sum +++ b/go.sum @@ -42,8 +42,8 @@ github.com/bmatcuk/doublestar/v4 v4.10.0 h1:zU9WiOla1YA122oLM6i4EXvGW62DvKZVxIe6 github.com/bmatcuk/doublestar/v4 v4.10.0/go.mod h1:xBQ8jztBU6kakFMg+8WGxn0c6z1fTSPVIjEY1Wr7jzc= github.com/brianvoe/gofakeit/v6 v6.28.0 h1:Xib46XXuQfmlLS2EXRuJpqcw8St6qSZz75OUo0tgAW4= github.com/brianvoe/gofakeit/v6 v6.28.0/go.mod h1:Xj58BMSnFqcn/fAQeSK+/PLtC5kSb7FJIq4JyGa8vEs= -github.com/bufbuild/protocompile v0.14.2-0.20260427235036-6b040a6b602b h1:cPYlWl0BN1bITspyWdVW5B1Z3e7WYWFeJIiIpOi3O6g= -github.com/bufbuild/protocompile v0.14.2-0.20260427235036-6b040a6b602b/go.mod h1:DhgqsRznX/F0sGkUYtTQJRP+q8xMReQRQ3qr+n1opWU= +github.com/bufbuild/protocompile v0.14.2-0.20260428232503-17b06932e73e h1:wwAUu3QGBzfHahrk6uEymIqvs1XwG55GtHjc/ooOTYM= +github.com/bufbuild/protocompile v0.14.2-0.20260428232503-17b06932e73e/go.mod h1:DhgqsRznX/F0sGkUYtTQJRP+q8xMReQRQ3qr+n1opWU= github.com/bufbuild/protoplugin v0.0.0-20260414125817-25d1d281b46b h1:b7wvo9ZhjLzCp7tGbOUMvgtYTnd33zGSAmMxcdxMnhQ= github.com/bufbuild/protoplugin v0.0.0-20260414125817-25d1d281b46b/go.mod h1:c5D8gWRIZ2HLWO3gXYTtUfw/hbJyD8xikv2ooPxnklQ= github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs= diff --git a/private/bufpkg/bufimage/build_image.go b/private/bufpkg/bufimage/build_image.go index ff63cc1de3..a6e7bb9ece 100644 --- a/private/bufpkg/bufimage/build_image.go +++ b/private/bufpkg/bufimage/build_image.go @@ -128,6 +128,18 @@ func compileImage( return nil, err } + // Validate that there is a single result for all files. + if len(results) != 1 { + return nil, fmt.Errorf("expected a single result from query, instead got: %d", len(results)) + } + + // Check for fatal errors before processing diagnostics so that callers can + // inspect typed errors (e.g. DuplicateProtoPathError) via errors.As. Spanless + // diagnostics that accompany these fatal errors are suppressed in the loop below. + if results[0].Fatal != nil { + return nil, results[0].Fatal + } + var fileAnnotations []bufanalysis.FileAnnotation for _, diagnostic := range diagnostics.Diagnostics { if diagnostic.Level() > report.Error { @@ -137,30 +149,9 @@ func compileImage( } primary := diagnostic.Primary() if primary.IsZero() { - // Diagnostics without a source span are still included as FileAnnotations - // with no position information, matching the behavior of the old compiler. - var fi bufanalysis.FileInfo - if f := diagnostic.File(); f != "" { - path := moduleFileResolver.PathForLocalPath(f) - if path == "" { - path = f - } - fi = &fileInfo{ - path: path, - externalPath: moduleFileResolver.ExternalPath(path), - } - } - fileAnnotations = append(fileAnnotations, bufanalysis.NewFileAnnotation( - fi, - 0, - 0, - 0, - 0, - "COMPILE", - diagnostic.Message(), - "", // pluginName - "", // policyName - )) + // Spanless diagnostics (e.g. from file-open errors like DuplicateProtoPathError) + // are companion messages to fatal errors already handled above. Suppress them here + // so their typed errors are preserved for callers rather than being lost as strings. continue } start := primary.Location(primary.Start, length.Bytes) @@ -194,15 +185,6 @@ func compileImage( if len(fileAnnotations) > 0 { return nil, bufanalysis.NewFileAnnotationSet(fileAnnotations...) } - - // Validate that there is a single result for all files - if len(results) != 1 { - return nil, fmt.Errorf("expected a single result from query, instead got: %d", len(results)) - } - - if results[0].Fatal != nil { - return nil, results[0].Fatal - } fds, resolver, err := resolverForFDS(results[0].Value) if err != nil { return nil, err