diff --git a/cmd/gencopy/gencopy.go b/cmd/gencopy/gencopy.go index 7513237b3..4d8b9c550 100644 --- a/cmd/gencopy/gencopy.go +++ b/cmd/gencopy/gencopy.go @@ -11,6 +11,7 @@ import ( "os" "path/filepath" "strconv" + "strings" "github.com/google/go-cmp/cmp" ) @@ -105,7 +106,7 @@ func readFileAsString(filename string) (string, error) { return string(bytes), nil } -func check(_ *Config, pkg *PackageConfig, pairs []*SrcDst) error { +func check(cfg *Config, pkg *PackageConfig, pairs []*SrcDst) error { for _, pair := range pairs { expected, err := readFileAsString(pair.Src) if err != nil { @@ -117,7 +118,10 @@ func check(_ *Config, pkg *PackageConfig, pairs []*SrcDst) error { } if diff := cmp.Diff(expected, actual); diff != "" { - return fmt.Errorf("gencopy mismatch %q vs. %q (-want +got):\n%s", pair.Src, pair.Dst, diff) + return fmt.Errorf( + "gencopy mismatch %q vs. %q (-want +got):\n%s\n%s", + pair.Src, pair.Dst, diff, regenerateProTip(pkg.TargetLabel, cfg.UpdateTargetLabelName), + ) } } @@ -129,6 +133,22 @@ func check(_ *Config, pkg *PackageConfig, pairs []*SrcDst) error { return nil } +// regenerateProTip returns a friendly hint pointing the developer at the +// `.update` target that regenerates the checked-in copies. targetLabel is the +// proto_compile rule's label (e.g. "//proto:foo_proto_compile" or +// "@@repo//proto:foo_proto_compile"); updateName is the .update target's +// rule name (e.g. "foo_proto_compile.update"). +func regenerateProTip(targetLabel, updateName string) string { + if updateName == "" { + return "" + } + updateLabel := updateName + if idx := strings.LastIndex(targetLabel, ":"); idx >= 0 { + updateLabel = targetLabel[:idx+1] + updateName + } + return fmt.Sprintf("\nProTip: to regenerate, run:\n bazel run %s\n", updateLabel) +} + func update(cfg *Config, pkg *PackageConfig, pairs []*SrcDst) error { for _, pair := range pairs { pair.Dst += cfg.Extension diff --git a/cmd/gencopy/gencopy_test.go b/cmd/gencopy/gencopy_test.go index 19b2656c5..ed4e0527e 100644 --- a/cmd/gencopy/gencopy_test.go +++ b/cmd/gencopy/gencopy_test.go @@ -144,6 +144,42 @@ func TestRunPkg(t *testing.T) { } } +func TestRegenerateProTip(t *testing.T) { + for name, tc := range map[string]struct { + targetLabel string + updateName string + want string + }{ + "empty update name": { + targetLabel: "//proto:foo_proto_compile", + updateName: "", + want: "", + }, + "local target": { + targetLabel: "//proto:foo_proto_compile", + updateName: "foo_proto_compile.update", + want: "\nProTip: to regenerate, run:\n bazel run //proto:foo_proto_compile.update\n", + }, + "external repo target": { + targetLabel: "@@some_repo//pkg:foo_proto_compile", + updateName: "foo_proto_compile.update", + want: "\nProTip: to regenerate, run:\n bazel run @@some_repo//pkg:foo_proto_compile.update\n", + }, + "target with no colon": { + targetLabel: "raw_label_no_colon", + updateName: "foo_proto_compile.update", + want: "\nProTip: to regenerate, run:\n bazel run foo_proto_compile.update\n", + }, + } { + t.Run(name, func(t *testing.T) { + got := regenerateProTip(tc.targetLabel, tc.updateName) + if got != tc.want { + t.Errorf("regenerateProTip: got %q, want %q", got, tc.want) + } + }) + } +} + // listFiles - convenience debugging function to log the files under a given dir func listFiles(t *testing.T, dir string) error { return filepath.Walk(dir, func(path string, info os.FileInfo, err error) error { diff --git a/example/thing/BUILD.bazel b/example/thing/BUILD.bazel index 89324c20f..f39ef8a9e 100644 --- a/example/thing/BUILD.bazel +++ b/example/thing/BUILD.bazel @@ -11,7 +11,7 @@ proto_library( name = "thing_proto", srcs = ["thing.proto"], visibility = ["//visibility:public"], - deps = ["@protobufapis//google/protobuf:timestamp_proto"], + deps = ["@com_google_protobuf//:timestamp_proto"], ) proto_cc_library( diff --git a/pkg/language/protobuf/BUILD.bazel b/pkg/language/protobuf/BUILD.bazel index 5a3090c98..64f3a078d 100644 --- a/pkg/language/protobuf/BUILD.bazel +++ b/pkg/language/protobuf/BUILD.bazel @@ -28,6 +28,7 @@ go_library( go_test( name = "protobuf_test", srcs = [ + "config_filemode_test.go", "generate_test.go", "override_test.go", ], @@ -37,6 +38,7 @@ go_test( "@bazel_gazelle//config", "@bazel_gazelle//label", "@bazel_gazelle//language", + "@bazel_gazelle//language/proto", "@bazel_gazelle//resolve", "@bazel_gazelle//rule", "@bazel_gazelle//testtools", diff --git a/pkg/language/protobuf/config.go b/pkg/language/protobuf/config.go index dd13aad37..18693a8b7 100644 --- a/pkg/language/protobuf/config.go +++ b/pkg/language/protobuf/config.go @@ -117,10 +117,18 @@ func (pl *protobufLang) Configure(c *config.Config, rel string, f *rule.File) { // getOrCreatePackageConfig either inserts a new config into the map under the // language name or replaces it with a clone. +// +// When cloning from the parent directory's config we must overwrite the +// embedded *config.Config pointer to the current directory's config. Without +// this, lookups that walk `cfg.Config.Exts[...]` (e.g. +// `IsProtoFileMode` reading the standard proto language's per-directory mode) +// resolve against the parent's `Exts` and miss directives like +// `# gazelle:proto file` that were applied to the child only. func (pl *protobufLang) getOrCreatePackageConfig(config *config.Config) *protoc.PackageConfig { var cfg *protoc.PackageConfig if existingExt, ok := config.Exts[pl.name]; ok { cfg = existingExt.(*protoc.PackageConfig).Clone() + cfg.Config = config } else { cfg = protoc.NewPackageConfig(config) } diff --git a/pkg/language/protobuf/config_filemode_test.go b/pkg/language/protobuf/config_filemode_test.go new file mode 100644 index 000000000..cc7751c94 --- /dev/null +++ b/pkg/language/protobuf/config_filemode_test.go @@ -0,0 +1,37 @@ +package protobuf + +import ( + "testing" + + "github.com/bazelbuild/bazel-gazelle/config" + gproto "github.com/bazelbuild/bazel-gazelle/language/proto" +) + +// Regression: getOrCreatePackageConfig must point the cloned PackageConfig's +// embedded *config.Config at the CURRENT directory's config, not the parent's. +// Otherwise IsProtoFileMode reads the parent's proto-language config and +// misses a child directory that opted into `# gazelle:proto file`. +func TestGetOrCreatePackageConfig_RebindsConfig(t *testing.T) { + pl := &protobufLang{name: "protobuf"} + + // Parent: default mode. + parent := &config.Config{Exts: map[string]interface{}{}} + parent.Exts["proto"] = &gproto.ProtoConfig{Mode: gproto.DefaultMode} + pl.getOrCreatePackageConfig(parent) + + // Child: clone via gazelle's Config.Clone, then proto-lang flips mode to FileMode + // for this dir only. + child := parent.Clone() + childProto := &gproto.ProtoConfig{Mode: gproto.FileMode} + child.Exts["proto"] = childProto + + cfg := pl.getOrCreatePackageConfig(child) + if cfg.Config != child { + t.Fatalf("cloned PackageConfig.Config not rebound to child: got %p, want %p", cfg.Config, child) + } + + // IsProtoFileMode-equivalent check. + if gproto.GetProtoConfig(cfg.Config).Mode != gproto.FileMode { + t.Errorf("expected FileMode via cfg.Config, got %v", gproto.GetProtoConfig(cfg.Config).Mode) + } +} diff --git a/pkg/plugin/neoeinstein/prost/BUILD.bazel b/pkg/plugin/neoeinstein/prost/BUILD.bazel index 5ece9faad..262803d6d 100644 --- a/pkg/plugin/neoeinstein/prost/BUILD.bazel +++ b/pkg/plugin/neoeinstein/prost/BUILD.bazel @@ -12,6 +12,7 @@ go_library( "//pkg/protoc", "@bazel_gazelle//label", "@bazel_gazelle//rule", + "@com_github_emicklei_proto//:proto", ], ) @@ -19,10 +20,13 @@ go_test( name = "prost_test", srcs = [ "extern_paths_test.go", + "needs_cwkt_test.go", + "nested_type_path_test.go", "protoc-gen-prost_test.go", + "register_nested_test.go", ], + embed = [":prost"], deps = [ - ":prost", "//pkg/plugintest", "//pkg/protoc", "@bazel_gazelle//label", diff --git a/pkg/plugin/neoeinstein/prost/extern_paths.go b/pkg/plugin/neoeinstein/prost/extern_paths.go index d014575e0..c5a2d2869 100644 --- a/pkg/plugin/neoeinstein/prost/extern_paths.go +++ b/pkg/plugin/neoeinstein/prost/extern_paths.go @@ -12,6 +12,7 @@ import ( "github.com/stackb/rules_proto/v4/pkg/protoc" ) + const ( // TransitiveExternPathsKey caches the dependency-only extern_path option // strings on the library rule's private attrs. @@ -107,29 +108,54 @@ func ResolveExternPathOptionsForReferences(cfg *protoc.PluginConfiguration, r *r // proto files and builds an extern_path option string for each dependency // package. Self-extern overrides are NOT included — see // ResolveExternPathOptionsForReferences for the variant that adds them. +// +// Per-file routing is gated on the **upstream** package's mode (i.e. whether +// it has PerFileTypeProvideKind entries registered). Consumer mode is +// irrelevant: per-package consumers of per-file upstreams also need the +// per-type extern_paths because the upstream's facade label no longer +// exists. func ResolveTransitiveExternPaths(r *rule.Rule, from label.Label) []string { - lib := r.PrivateAttr(protoc.ProtoLibraryKey) - if lib == nil { + libraries := mergedLibraries(r) + if len(libraries) == 0 { return nil } - library := lib.(protoc.ProtoLibrary) - libRule := library.Rule() - - if cached, ok := libRule.PrivateAttr(TransitiveExternPathsKey).([]string); ok { + // Cache off the first library's underlying rule — merge can occur but the + // post-merge PrivateAttrs travel with the first library's rule for back- + // compat with the existing cache placement. + cacheRule := libraries[0].Rule() + if cached, ok := cacheRule.PrivateAttr(TransitiveExternPathsKey).([]string); ok { return cached } resolver := protoc.GlobalResolver() ownFiles := make(map[string]bool) - for _, src := range library.Srcs() { - ownFiles[path.Join(from.Pkg, src)] = true + for _, library := range libraries { + for _, src := range library.Srcs() { + ownFiles[path.Join(from.Pkg, src)] = true + } } + // Also treat any proto file whose registered prost_extern crate matches + // one of our own as own. This is a belt-and-suspenders guard for cases + // where ownFiles can't identify a merged-in file by path alone (the + // caller's `from.Pkg` is the rule's bazel package, but a merged proto_- + // library may sit in a different bazel package). + ownCrates := ownCrateNames(libraries, resolver, from) + // Per-file mode: sibling .proto files within the same proto package + // resolve to *different* per-file crates, so the ownCrates check above + // doesn't catch them. Tracking our own proto packages lets us suppress + // the per-package extern_path emission for siblings — they're routed + // per-type by perFileSiblingTypeExternPaths below. Without this, a + // `.=::` entry would (via prost's longest- + // prefix matching) hijack every type reference in our own crate. + ownPackages := ownProtoPackages(r, from) seen := make(map[string]bool) stack := list.New() - for _, src := range library.Srcs() { - stack.PushBack(path.Join(from.Pkg, src)) + for _, library := range libraries { + for _, src := range library.Srcs() { + stack.PushBack(path.Join(from.Pkg, src)) + } } externPathsByPackage := make(map[string]string) @@ -154,11 +180,6 @@ func ResolveTransitiveExternPaths(r *rule.Rule, from label.Label) []string { continue } - // Skip well-known types — prost ships these built-in. - if strings.HasPrefix(protofile, "google/protobuf/") { - continue - } - results := resolver.Resolve("proto", "prost_extern", protofile) if len(results) == 0 { continue @@ -170,25 +191,327 @@ func ResolveTransitiveExternPaths(r *rule.Rule, from label.Label) []string { if protoPackage == "" { continue } + // Skip files whose crate is one of ours — they're part of the + // merged library set, just couldn't be matched by file path. + if ownCrates[crateName] { + continue + } + // Skip same-proto-package siblings in per-file mode. Per-type + // extern_paths handle them downstream; a per-package entry here + // would hijack our own crate's references via prost's longest- + // prefix matching. + if ownPackages[protoPackage] { + continue + } if _, exists := externPathsByPackage[protoPackage]; exists { continue } - // extern_path=.{proto_package}=::{crate_name}::{rust_module_path} - rustModulePath := strings.ReplaceAll(protoPackage, ".", "::") - externPathsByPackage[protoPackage] = "extern_path=." + protoPackage + "=::" + crateName + "::" + rustModulePath + // Suppress the per-package facade extern_path whenever the + // upstream package is in per-file mode (has any + // PerFileTypeProvideKind entries). With the facade gone, that + // label no longer exists; references route through per-type + // extern_paths emitted by perFileCrossPackageTypeExternPaths + // below instead. This applies whether the consumer itself is + // per-file or per-package — a per-package consumer of a per- + // file upstream still needs per-type routing because there's + // no facade to depend on. + if hasPerFileTypeEntries(resolver, protoPackage) { + continue + } + + // extern_path=.{proto_package}=::{crate_name} + // The crate exposes all generated types at its root (see the + // proto_rust_library Starlark macro), so no nested module path is + // appended after the crate name. + externPathsByPackage[protoPackage] = "extern_path=." + protoPackage + "=::" + crateName } result := make([]string, 0, len(externPathsByPackage)) for _, ep := range externPathsByPackage { result = append(result, ep) } + + // Per-type extern_path entries for sibling per-file crates in the same + // proto package — without these, prost emits a relative path for + // cross-file same-package references that won't resolve once each + // file lives in its own crate. The siblings' types were registered + // against PerFileTypeProvideKind keyed by proto package; we look them + // up by our own packages and skip entries whose crate is one of ours. + result = append(result, perFileSiblingTypeExternPaths(resolver, ownPackages, ownCrates)...) + + // Per-type entries for cross-package per-file upstreams. Emitted + // unconditionally — `perFileCrossPackageTypeExternPaths` walks the + // transitive `seen` set and filters per-upstream-mode internally. + result = append(result, perFileCrossPackageTypeExternPaths(resolver, ownPackages, seen)...) + sort.Strings(result) - libRule.SetPrivateAttr(TransitiveExternPathsKey, result) + cacheRule.SetPrivateAttr(TransitiveExternPathsKey, result) return result } +// hasPerFileTypeEntries reports whether the resolver has any +// PerFileTypeProvideKind entry for the given proto package. Used as the +// per-file-mode detector for upstream packages. +func hasPerFileTypeEntries(resolver protoc.ImportResolver, protoPackage string) bool { + return len(resolver.Resolve("proto", PerFileTypeProvideKind, protoPackage)) > 0 +} + +// perFileCrossPackageTypeExternPaths emits per-type extern_path entries for +// every cross-package per-file upstream that appears in the transitive +// import graph walked above (the `seen` set). For each upstream proto file +// we look up its package via prost_extern, then enumerate the package's +// PerFileTypeProvideKind entries to emit one extern_path per declared type. +// +// Same-package types are not emitted here (they're handled by +// perFileSiblingTypeExternPaths). +func perFileCrossPackageTypeExternPaths( + resolver protoc.ImportResolver, + ownPackages map[string]bool, + seen map[string]bool, +) []string { + out := make([]string, 0) + emitted := make(map[string]bool) + emittedPackages := make(map[string]bool) + + for protofile := range seen { + externs := resolver.Resolve("proto", "prost_extern", protofile) + if len(externs) == 0 { + continue + } + protoPackage := externs[0].Label.Pkg + if protoPackage == "" || ownPackages[protoPackage] || emittedPackages[protoPackage] { + continue + } + entries := resolver.Resolve("proto", PerFileTypeProvideKind, protoPackage) + if len(entries) == 0 { + continue + } + emittedPackages[protoPackage] = true + for _, ent := range entries { + typeName := ent.Label.Pkg + crateName := ent.Label.Name + if typeName == "" || crateName == "" { + continue + } + // `typeName` is a dotted proto type path (e.g. `Outer.Inner`) + // for nested types. Convert through protoTypePathToRustPath so + // the rust_path becomes `outer::Inner`, matching prost-build's + // nested-module emission. The previous direct toUpperCamel + // call left the dot literal in the rust_path, producing the + // syntactically invalid `::::Outer.Inner`. + entry := "extern_path=." + protoPackage + "." + typeName + "=::" + crateName + "::" + protoTypePathToRustPath(typeName) + if emitted[entry] { + continue + } + emitted[entry] = true + out = append(out, entry) + } + } + return out +} + +// toUpperCamel converts a proto type name to the Rust UpperCamel form prost +// emits. Matches prost-build's `to_upper_camel` (which delegates to heck's +// `ToUpperCamelCase`): consecutive runs of uppercase letters are treated as +// a single word, with a new word starting when an uppercase letter is +// followed by a lowercase letter (e.g. `PIKInfo` → `PikInfo`, `URLLoader` → +// `UrlLoader`). +// +// The transform also splits on `_` and `-` and capitalizes each resulting +// word's first character. +func toUpperCamel(name string) string { + runes := []rune(name) + if len(runes) == 0 { + return name + } + var words []string + start := 0 + for i := 0; i < len(runes); i++ { + r := runes[i] + if r == '_' || r == '-' { + if start < i { + words = append(words, string(runes[start:i])) + } + start = i + 1 + continue + } + if i == 0 { + continue + } + prev := runes[i-1] + // lower → upper boundary: e.g. `MyType` splits at `T`. + if !unicodeUpper(prev) && unicodeUpper(r) { + words = append(words, string(runes[start:i])) + start = i + continue + } + // Acronym tail: prev=upper, curr=upper, next=lower splits before curr. + // e.g. `PIKInfo` at i=3 ('I'): prev='K'(upper), curr='I'(upper), + // next='n'(lower) → boundary before 'I' so the word is `PIK`. + if unicodeUpper(prev) && unicodeUpper(r) && i+1 < len(runes) && unicodeLower(runes[i+1]) { + words = append(words, string(runes[start:i])) + start = i + } + } + if start < len(runes) { + words = append(words, string(runes[start:])) + } + + var b strings.Builder + for _, w := range words { + if w == "" { + continue + } + wr := []rune(w) + b.WriteRune(unicodeToUpper(wr[0])) + for _, r := range wr[1:] { + b.WriteRune(unicodeToLower(r)) + } + } + return b.String() +} + +func unicodeUpper(r rune) bool { return r >= 'A' && r <= 'Z' } +func unicodeLower(r rune) bool { return r >= 'a' && r <= 'z' } +func unicodeToUpper(r rune) rune { return rune(strings.ToUpper(string(r))[0]) } +func unicodeToLower(r rune) rune { return rune(strings.ToLower(string(r))[0]) } + +// protoTypePathToRustPath converts a dotted proto type path like +// `Outer.Inner.Leaf` to the matching Rust path `outer::inner::Leaf` — +// every segment except the last becomes a snake-cased module name +// (matching prost-build's nested-message module layout), and the last +// segment becomes the upper-camel-cased struct/enum name. A single- +// segment input like `MyMessage` is returned as `MyMessage`. +func protoTypePathToRustPath(typePath string) string { + parts := strings.Split(typePath, ".") + if len(parts) == 1 { + return toUpperCamel(parts[0]) + } + var b strings.Builder + for i, p := range parts { + if i > 0 { + b.WriteString("::") + } + if i == len(parts)-1 { + b.WriteString(toUpperCamel(p)) + } else { + // Module name: snake_case of the upper-camel form, matching + // prost-build's `push_mod(&to_snake(...))` convention for + // nested messages. + b.WriteString(toSnakeFromCamel(p)) + } + } + return b.String() +} + +// toSnakeFromCamel converts an UpperCamel/lowerCamel/UNDERSCORE_CASE/`PIKInfo` +// identifier to snake_case. Mirrors heck's `ToSnakeCase` behavior used by +// prost-build for nested-message module names. +func toSnakeFromCamel(s string) string { + // Reuse our word-boundary detection (it's the same as toUpperCamel's), + // then lowercase each word and join with `_`. + runes := []rune(s) + if len(runes) == 0 { + return s + } + var words []string + start := 0 + for i := 0; i < len(runes); i++ { + r := runes[i] + if r == '_' || r == '-' { + if start < i { + words = append(words, string(runes[start:i])) + } + start = i + 1 + continue + } + if i == 0 { + continue + } + prev := runes[i-1] + if !unicodeUpper(prev) && unicodeUpper(r) { + words = append(words, string(runes[start:i])) + start = i + continue + } + if unicodeUpper(prev) && unicodeUpper(r) && i+1 < len(runes) && unicodeLower(runes[i+1]) { + words = append(words, string(runes[start:i])) + start = i + } + } + if start < len(runes) { + words = append(words, string(runes[start:])) + } + + var b strings.Builder + for i, w := range words { + if w == "" { + continue + } + if i > 0 { + b.WriteRune('_') + } + for _, r := range w { + b.WriteRune(unicodeToLower(r)) + } + } + return b.String() +} + +// perFileSiblingTypeExternPaths emits extern_path entries that route same- +// package cross-file type references through the correct sibling per-file +// crate. Returns nil if no per-file type entries are registered (the +// common case — only per-file-mode libraries register them). +// +// `ownPackages` is the set of proto packages we contribute (derived via +// `ownProtoPackages` from the resolver, not from File.Package() — the +// parsed proto-package field isn't reliably populated outside the real +// gazelle Generate pass, e.g. in unit tests). +func perFileSiblingTypeExternPaths( + resolver protoc.ImportResolver, + ownPackages map[string]bool, + ownCrates map[string]bool, +) []string { + var out []string + seen := make(map[string]bool) + for pkg := range ownPackages { + for _, ent := range resolver.Resolve("proto", PerFileTypeProvideKind, pkg) { + typeName := ent.Label.Pkg + crateName := ent.Label.Name + if typeName == "" || crateName == "" { + continue + } + // Skip types declared in our own crate(s) — prost emits them + // locally; an extern_path here would tell prost to skip + // generating the definition entirely. + if ownCrates[crateName] { + continue + } + // Per-type extern_path keys are EXACT-matched by prost-build, which + // returns the rust_path verbatim — without appending the trailing + // type segment. Spelling the type name into the rust_path so the + // generated code lands on `::::` rather than + // `::` (the latter would refer to the crate itself, not + // the type, and fails compilation as E0425/E0433). + // + // Nested types arrive as dotted paths (e.g. `Outer.Inner`). Each + // non-final segment becomes a snake-cased Rust module + // (`outer::Inner`), matching prost-build's nested-message layout. + // The leaf segment is upper-camel-cased — proto names like + // `PIKInfo` become `PikInfo` in generated Rust. + entry := "extern_path=." + pkg + "." + typeName + "=::" + crateName + "::" + protoTypePathToRustPath(typeName) + if seen[entry] { + continue + } + seen[entry] = true + out = append(out, entry) + } + } + return out +} + // mergeExternPathOptions strips any pre-existing extern_path= entries from // cfg.Options and returns the remainder concatenated with the supplied // extern_path strings. @@ -205,31 +528,68 @@ func mergeExternPathOptions(cfg *protoc.PluginConfiguration, externPaths []strin // ownProtoPackages returns the set of proto packages the library itself // contributes, computed from prost_extern resolver entries for each own -// proto file. Cached on the library rule. +// proto file across all merged proto_libraries. Cached on the rule. func ownProtoPackages(r *rule.Rule, from label.Label) map[string]bool { - lib := r.PrivateAttr(protoc.ProtoLibraryKey) - if lib == nil { + libraries := mergedLibraries(r) + if len(libraries) == 0 { return nil } - library := lib.(protoc.ProtoLibrary) - libRule := library.Rule() - - if cached, ok := libRule.PrivateAttr(OwnProtoPackagesKey).(map[string]bool); ok { + cacheRule := libraries[0].Rule() + if cached, ok := cacheRule.PrivateAttr(OwnProtoPackagesKey).(map[string]bool); ok { return cached } resolver := protoc.GlobalResolver() out := make(map[string]bool) - for _, src := range library.Srcs() { - ownFile := path.Join(from.Pkg, src) - for _, ext := range resolver.Resolve("proto", "prost_extern", ownFile) { - if ext.Label.Pkg != "" { - out[ext.Label.Pkg] = true + for _, library := range libraries { + for _, src := range library.Srcs() { + ownFile := path.Join(from.Pkg, src) + for _, ext := range resolver.Resolve("proto", "prost_extern", ownFile) { + if ext.Label.Pkg != "" { + out[ext.Label.Pkg] = true + } } } } - libRule.SetPrivateAttr(OwnProtoPackagesKey, out) + cacheRule.SetPrivateAttr(OwnProtoPackagesKey, out) + return out +} + +// mergedLibraries returns the full set of proto_libraries backing a proto_- +// compile / proto_compiled_sources rule. Prefers MergedProtoLibrariesKey +// (set by proto_compile.Rule for every merge — see protoc.MergedProtoLibrariesKey) +// and falls back to wrapping the single ProtoLibraryKey for callers that +// haven't migrated (e.g. proto_rust_library, hand-constructed test rules). +// Returns nil when the rule carries neither. +func mergedLibraries(r *rule.Rule) []protoc.ProtoLibrary { + if libs, ok := r.PrivateAttr(protoc.MergedProtoLibrariesKey).([]protoc.ProtoLibrary); ok && len(libs) > 0 { + return libs + } + if lib, ok := r.PrivateAttr(protoc.ProtoLibraryKey).(protoc.ProtoLibrary); ok && lib != nil { + return []protoc.ProtoLibrary{lib} + } + return nil +} + +// ownCrateNames returns the set of rust crate names registered (via +// prost_extern) for files belonging to any of the library's merged proto_- +// libraries. Used to recognise own-merged files in the dep walk even when +// their on-disk path doesn't share a prefix with from.Pkg (e.g. a +// proto_compiled_sources at //a:foo that merges in //b:bar_proto would +// otherwise see b/bar.proto as external). +func ownCrateNames(libraries []protoc.ProtoLibrary, resolver protoc.ImportResolver, from label.Label) map[string]bool { + out := make(map[string]bool) + for _, library := range libraries { + for _, src := range library.Srcs() { + ownFile := path.Join(from.Pkg, src) + for _, ext := range resolver.Resolve("proto", "prost_extern", ownFile) { + if ext.Label.Name != "" { + out[ext.Label.Name] = true + } + } + } + } return out } @@ -247,8 +607,9 @@ func selfExternPathsForOverride(ownPackages map[string]bool, parents []string) [ if !hasParentInImports(ownPkg, parentPkgs) { continue } - rustModulePath := strings.ReplaceAll(ownPkg, ".", "::") - out = append(out, "extern_path=."+ownPkg+"=crate::"+rustModulePath) + // All own types live at the crate root (flat convention), so the + // self-extern override maps the proto sub-package to bare `crate`. + out = append(out, "extern_path=."+ownPkg+"=crate") } return out } diff --git a/pkg/plugin/neoeinstein/prost/extern_paths_test.go b/pkg/plugin/neoeinstein/prost/extern_paths_test.go index fad87ee0a..4894e47c7 100644 --- a/pkg/plugin/neoeinstein/prost/extern_paths_test.go +++ b/pkg/plugin/neoeinstein/prost/extern_paths_test.go @@ -56,8 +56,8 @@ func TestResolveTransitiveExternPaths(t *testing.T) { sort.Strings(got) want := []string{ - "extern_path=.extern.dep_a=::depA_rs::extern::dep_a", - "extern_path=.extern.dep_b=::depB_rs::extern::dep_b", + "extern_path=.extern.dep_a=::depA_rs", + "extern_path=.extern.dep_b=::depB_rs", } if !reflect.DeepEqual(got, want) { t.Errorf("ResolveTransitiveExternPaths:\n got: %v\nwant: %v", got, want) @@ -113,7 +113,7 @@ func TestResolveTransitiveExternPaths_SubpackageOfImport(t *testing.T) { got := prost.ResolveTransitiveExternPaths(r, from) want := []string{ - "extern_path=.subpkg.parent=::parent_rs::subpkg::parent", + "extern_path=.subpkg.parent=::parent_rs", } if !reflect.DeepEqual(got, want) { t.Errorf("ResolveTransitiveExternPaths:\n got: %v\nwant: %v", got, want) @@ -146,8 +146,8 @@ func TestResolveExternPathOptionsForReferences_SubpackageOfImport(t *testing.T) cfg := &protoc.PluginConfiguration{Options: nil} got := prost.ResolveExternPathOptionsForReferences(cfg, r, from) want := []string{ - "extern_path=.refs.parent.child=crate::refs::parent::child", - "extern_path=.refs.parent=::parent_rs::refs::parent", + "extern_path=.refs.parent.child=crate", + "extern_path=.refs.parent=::parent_rs", } sort.Strings(want) sort.Strings(got) @@ -182,7 +182,7 @@ func TestResolveTransitiveExternPaths_SiblingNotFiltered(t *testing.T) { from := label.New("", "sibling/a/y", "y_proto") got := prost.ResolveTransitiveExternPaths(r, from) - want := []string{"extern_path=.sibling.a.x=::x_rs::sibling::a::x"} + want := []string{"extern_path=.sibling.a.x=::x_rs"} if !reflect.DeepEqual(got, want) { t.Errorf("ResolveTransitiveExternPaths:\n got: %v\nwant: %v", got, want) } @@ -197,13 +197,13 @@ func TestResolveExternPathOptions_FiltersExisting(t *testing.T) { cfg := &protoc.PluginConfiguration{ Options: []string{ "compile_well_known_types=true", - "extern_path=.stale.pkg=::stale_rs::stale::pkg", + "extern_path=.stale.pkg=::stale_rs", }, } got := prost.ResolveExternPathOptions(cfg, r, from) for _, opt := range got { - if opt == "extern_path=.stale.pkg=::stale_rs::stale::pkg" { + if opt == "extern_path=.stale.pkg=::stale_rs" { t.Errorf("stale extern_path option was not filtered: %v", got) } } @@ -213,3 +213,362 @@ func TestResolveExternPathOptions_FiltersExisting(t *testing.T) { t.Errorf("ResolveExternPathOptions:\n got: %v\nwant: %v", got, want) } } + +// TestResolveTransitiveExternPaths_GoogleProtobufNotSkipped guards against the +// historical hard-coded skip of google/protobuf/* in the dep walk. When a +// downstream repo registers a rust target for google.protobuf (via +// proto_language rust enable true on that package), the extern_path entry +// must flow through so consumers reference ::google_protobuf instead of the +// prost-build default ::prost_types — which carries no serde impls. +func TestResolveTransitiveExternPaths_GoogleProtobufNotSkipped(t *testing.T) { + resolver := protoc.GlobalResolver() + + resolver.Provide("proto", "prost_extern", + "google/protobuf/duration.proto", + label.New("", "google.protobuf", "google_protobuf")) + resolver.Provide("proto", "prost_extern", + "wkttest/consumer/c.proto", + label.New("", "wkttest.consumer", "wkttest_consumer")) + + resolver.Provide("proto", "depends", + "wkttest/consumer/c.proto", + label.New("", "google/protobuf", "duration.proto")) + + r := makeLibraryRule("c_proto", "wkttest/consumer", []string{"c.proto"}) + from := label.New("", "wkttest/consumer", "c_proto") + + got := prost.ResolveTransitiveExternPaths(r, from) + want := []string{"extern_path=.google.protobuf=::google_protobuf"} + if !reflect.DeepEqual(got, want) { + t.Errorf("ResolveTransitiveExternPaths:\n got: %v\nwant: %v", got, want) + } +} + +// TestResolveProstOptions_AddsCompileWellKnownTypes_OwnPackage verifies that +// when the library compiles google.protobuf protos itself, the prost plugin +// prepends compile_well_known_types=true so prost-build emits well-known +// type definitions locally (default behaviour is to skip them, which leaves +// the matching prost-serde impls referencing undefined structs). +func TestResolveProstOptions_AddsCompileWellKnownTypes_OwnPackage(t *testing.T) { + resolver := protoc.GlobalResolver() + + resolver.Provide("proto", "prost_extern", + "wktown/google/protobuf/any.proto", + label.New("", "google.protobuf", "google_protobuf")) + + r := makeLibraryRule("google_protobuf_proto", "wktown/google/protobuf", []string{"any.proto"}) + from := label.New("", "wktown/google/protobuf", "google_protobuf_proto") + + plugin := &prost.ProtocGenProstPlugin{} + got := plugin.ResolvePluginOptions(&protoc.PluginConfiguration{}, r, from) + + if len(got) == 0 || got[0] != "compile_well_known_types=true" { + t.Errorf("expected compile_well_known_types=true at head of options for own-google.protobuf library, got: %v", got) + } +} + +// TestResolveProstOptions_AddsCompileWellKnownTypes_ExternPath verifies that +// when a library *consumes* google.protobuf via a foreign crate, the prost +// plugin emits compile_well_known_types=true alongside the extern_path. +// Without the flag, prost-build registers its default +// .google.protobuf=::prost_types extern path and ExternPaths::insert errors +// out with "duplicate extern Protobuf path: .google.protobuf". +func TestResolveProstOptions_AddsCompileWellKnownTypes_ExternPath(t *testing.T) { + resolver := protoc.GlobalResolver() + + resolver.Provide("proto", "prost_extern", + "wktdep/google/protobuf/duration.proto", + label.New("", "google.protobuf", "google_protobuf")) + resolver.Provide("proto", "prost_extern", + "wktdep/consumer/c.proto", + label.New("", "wktdep.consumer", "wktdep_consumer")) + resolver.Provide("proto", "depends", + "wktdep/consumer/c.proto", + label.New("", "wktdep/google/protobuf", "duration.proto")) + + r := makeLibraryRule("c_proto", "wktdep/consumer", []string{"c.proto"}) + from := label.New("", "wktdep/consumer", "c_proto") + + plugin := &prost.ProtocGenProstPlugin{} + got := plugin.ResolvePluginOptions(&protoc.PluginConfiguration{}, r, from) + + wantHead := "compile_well_known_types=true" + wantExtern := "extern_path=.google.protobuf=::google_protobuf" + if len(got) == 0 || got[0] != wantHead { + t.Errorf("expected %q as first option, got: %v", wantHead, got) + } + found := false + for _, opt := range got { + if opt == wantExtern { + found = true + break + } + } + if !found { + t.Errorf("expected %q in options, got: %v", wantExtern, got) + } +} + +// TestResolveProstOptions_NoCompileWellKnownTypes_WhenIrrelevant verifies +// that the flag is NOT emitted when the library neither compiles nor +// consumes google.protobuf. Guards against accidental over-emission. +func TestResolveProstOptions_NoCompileWellKnownTypes_WhenIrrelevant(t *testing.T) { + resolver := protoc.GlobalResolver() + + resolver.Provide("proto", "prost_extern", + "wktneg/leaf/l.proto", + label.New("", "wktneg.leaf", "wktneg_leaf")) + + r := makeLibraryRule("l_proto", "wktneg/leaf", []string{"l.proto"}) + from := label.New("", "wktneg/leaf", "l_proto") + + plugin := &prost.ProtocGenProstPlugin{} + got := plugin.ResolvePluginOptions(&protoc.PluginConfiguration{}, r, from) + + for _, opt := range got { + if opt == "compile_well_known_types=true" { + t.Errorf("did not expect compile_well_known_types=true for irrelevant library, got: %v", got) + } + } +} + +// TestResolveTransitiveExternPaths_PerFileSiblingTypes covers the headline +// per-file behaviour: when our per-file proto_library imports another file +// in the *same* proto package (a sibling per-file crate), ResolveTransitive- +// ExternPaths emits per-type extern_path entries routing each sibling type +// through its own per-file crate, AND suppresses the per-package extern_path +// that would otherwise hijack our own crate's references via prost's +// longest-prefix matching. +func TestResolveTransitiveExternPaths_PerFileSiblingTypes(t *testing.T) { + resolver := protoc.GlobalResolver() + + // Two per-file crates in the same proto package. The per-file crate + // naming convention is `__` — see protoc-gen-prost's + // registerExternPaths(). + const pkg = "perfiletest.pkg" + const ourCrate = "perfiletest_pkg__order" + const siblingCrate = "perfiletest_pkg__trade" + + resolver.Provide("proto", "prost_extern", + "perfiletest/pkg/order.proto", + label.New("", pkg, ourCrate)) + resolver.Provide("proto", "prost_extern", + "perfiletest/pkg/trade.proto", + label.New("", pkg, siblingCrate)) + + // Per-type registry, keyed by proto package: each entry is + // (typeName, crateName). + resolver.Provide("proto", prost.PerFileTypeProvideKind, pkg, + label.New("", "Order", ourCrate)) + resolver.Provide("proto", prost.PerFileTypeProvideKind, pkg, + label.New("", "Trade", siblingCrate)) + + // order.proto imports trade.proto. + resolver.Provide("proto", "depends", + "perfiletest/pkg/order.proto", + label.New("", "perfiletest/pkg", "trade.proto")) + + r := makeLibraryRule("order_proto", "perfiletest/pkg", []string{"order.proto"}) + from := label.New("", "perfiletest/pkg", "order_proto") + + got := prost.ResolveTransitiveExternPaths(r, from) + sort.Strings(got) + + want := []string{ + // Per-type entry for the sibling's type — routes references in our + // crate to the sibling's per-file crate. The trailing `::Trade` + // is needed so prost lands on the type (not the crate itself) when + // substituting references. + "extern_path=.perfiletest.pkg.Trade=::perfiletest_pkg__trade::Trade", + } + if !reflect.DeepEqual(got, want) { + t.Errorf("ResolveTransitiveExternPaths:\n got: %v\nwant: %v", got, want) + } + + // Must NOT contain a per-package extern_path for our own package — + // that would hijack every type reference in our crate. + for _, opt := range got { + if opt == "extern_path=.perfiletest.pkg=::perfiletest_pkg__trade" { + t.Errorf("per-package extern_path for own proto package leaked: %v", got) + } + } + + // Must NOT contain a per-type entry for our own type (would prevent + // prost from generating its definition locally). + for _, opt := range got { + if opt == "extern_path=.perfiletest.pkg.Order=::perfiletest_pkg__order" { + t.Errorf("per-type extern_path for own type leaked: %v", got) + } + } +} + +// TestResolveTransitiveExternPaths_PerFileCrossPackageStillEmitsPerPackage +// ensures the per-file machinery is additive: cross-*package* references +// still get a per-package extern_path (because the dep isn't in our proto +// package), even when we're a per-file crate ourselves. +func TestResolveTransitiveExternPaths_PerFileCrossPackageStillEmitsPerPackage(t *testing.T) { + resolver := protoc.GlobalResolver() + + // Our per-file crate. + resolver.Provide("proto", "prost_extern", + "perfilecross/own/o.proto", + label.New("", "perfilecross.own", "perfilecross_own__o")) + + // A dep in a *different* proto package — its crate is the conventional + // per-package (façade) name. + resolver.Provide("proto", "prost_extern", + "perfilecross/dep/d.proto", + label.New("", "perfilecross.dep", "perfilecross_dep")) + + // o.proto imports d.proto (cross-package). + resolver.Provide("proto", "depends", + "perfilecross/own/o.proto", + label.New("", "perfilecross/dep", "d.proto")) + + r := makeLibraryRule("o_proto", "perfilecross/own", []string{"o.proto"}) + from := label.New("", "perfilecross/own", "o_proto") + + got := prost.ResolveTransitiveExternPaths(r, from) + want := []string{"extern_path=.perfilecross.dep=::perfilecross_dep"} + if !reflect.DeepEqual(got, want) { + t.Errorf("ResolveTransitiveExternPaths:\n got: %v\nwant: %v", got, want) + } +} + +// TestResolveTransitiveExternPaths_PerFileNoSiblingsRegistered verifies that +// when no PerFileTypeProvideKind entries are registered for our proto +// packages (i.e. the per-file plugin path wasn't taken — e.g. package-mode +// surroundings), ResolveTransitiveExternPaths emits zero per-type entries. +// Guards against regression for the default per-package convention. +func TestResolveTransitiveExternPaths_PerFileNoSiblingsRegistered(t *testing.T) { + resolver := protoc.GlobalResolver() + + resolver.Provide("proto", "prost_extern", + "perfilenone/own/own.proto", + label.New("", "perfilenone.own", "perfilenone_own")) + + r := makeLibraryRule("own_proto", "perfilenone/own", []string{"own.proto"}) + from := label.New("", "perfilenone/own", "own_proto") + + got := prost.ResolveTransitiveExternPaths(r, from) + if len(got) != 0 { + t.Errorf("expected no extern paths, got %v", got) + } +} + +// TestResolveTransitiveExternPaths_PerFileMultiFileOwn covers the merged +// per-file case: our gazelle rule actually owns *multiple* per-file +// libraries (each in its own proto_library) merged into a single +// proto_rust_library via MergedProtoLibrariesKey. Per-type entries for +// every type any of our own files defines must be skipped — only sibling +// types from siblings that aren't part of our merge set should emerge. +func TestResolveTransitiveExternPaths_PerFileMultiFileOwn(t *testing.T) { + resolver := protoc.GlobalResolver() + + const pkg = "perfilemulti.pkg" + const aCrate = "perfilemulti_pkg__a" + const bCrate = "perfilemulti_pkg__b" + const cCrate = "perfilemulti_pkg__c" + + // Three sibling per-file crates in the same proto package — we own + // `a` and `b`, `c` is an external sibling we'd reference via its own + // crate. (Contrived, but it covers the multi-own / single-sibling + // composition.) + resolver.Provide("proto", "prost_extern", + "perfilemulti/pkg/a.proto", label.New("", pkg, aCrate)) + resolver.Provide("proto", "prost_extern", + "perfilemulti/pkg/b.proto", label.New("", pkg, bCrate)) + resolver.Provide("proto", "prost_extern", + "perfilemulti/pkg/c.proto", label.New("", pkg, cCrate)) + + resolver.Provide("proto", prost.PerFileTypeProvideKind, pkg, + label.New("", "A", aCrate)) + resolver.Provide("proto", prost.PerFileTypeProvideKind, pkg, + label.New("", "B", bCrate)) + resolver.Provide("proto", prost.PerFileTypeProvideKind, pkg, + label.New("", "C", cCrate)) + + // a.proto imports c.proto so c is on the dep stack; b.proto imports + // a.proto so the merge graph touches every file. + resolver.Provide("proto", "depends", + "perfilemulti/pkg/a.proto", + label.New("", "perfilemulti/pkg", "c.proto")) + resolver.Provide("proto", "depends", + "perfilemulti/pkg/b.proto", + label.New("", "perfilemulti/pkg", "a.proto")) + + // Build a merged rule owning a.proto + b.proto. c.proto is not ours. + r := rule.NewRule("proto_library", "ab_merged") + libA := protoc.NewOtherProtoLibrary(nil, + makeLibraryRule("a_proto", "perfilemulti/pkg", []string{"a.proto"}), + protoc.NewFile("perfilemulti/pkg", "a.proto")) + libB := protoc.NewOtherProtoLibrary(nil, + makeLibraryRule("b_proto", "perfilemulti/pkg", []string{"b.proto"}), + protoc.NewFile("perfilemulti/pkg", "b.proto")) + r.SetPrivateAttr(protoc.MergedProtoLibrariesKey, []protoc.ProtoLibrary{libA, libB}) + + from := label.New("", "perfilemulti/pkg", "ab_merged") + + got := prost.ResolveTransitiveExternPaths(r, from) + sort.Strings(got) + + // Only C should appear (A and B are ours). The rust_path includes the + // trailing `::C` so prost emits `::::C` rather than `::` + // when substituting type references. + want := []string{ + "extern_path=.perfilemulti.pkg.C=::perfilemulti_pkg__c::C", + } + if !reflect.DeepEqual(got, want) { + t.Errorf("ResolveTransitiveExternPaths:\n got: %v\nwant: %v", got, want) + } +} + +// TestResolveTransitiveExternPaths_MergedLibrariesOwn covers the merged-library +// case: proto_compile/proto_compiled_sources collapses two proto_libraries into +// one rule via the protos= attribute. Before the fix, only the first library's +// srcs went into ownFiles, so files from the second library were treated as +// external in the dep walk and produced a self-referential extern_path entry +// like .google.api=::google_api. The fix is for the rule to carry the full +// []ProtoLibrary set under MergedProtoLibrariesKey, and ResolveTransitive- +// ExternPaths to iterate all of them. +func TestResolveTransitiveExternPaths_MergedLibrariesOwn(t *testing.T) { + resolver := protoc.GlobalResolver() + + // Two libraries sharing one proto package (mergetest.merged) — emulates + // e.g. google/api's annotations_proto + http_proto pair, both declaring + // `package google.api;`. + resolver.Provide("proto", "prost_extern", + "mergetest/merged/a.proto", + label.New("", "mergetest.merged", "mergetest_merged")) + resolver.Provide("proto", "prost_extern", + "mergetest/merged/b.proto", + label.New("", "mergetest.merged", "mergetest_merged")) + + // a.proto imports b.proto — the dep walk would reach b.proto from a.proto. + resolver.Provide("proto", "depends", + "mergetest/merged/a.proto", + label.New("", "mergetest/merged", "b.proto")) + + // Build a single merged rule directly: instead of ProtoLibraryKey only, + // set MergedProtoLibrariesKey with both backing libraries. + r := rule.NewRule("proto_library", "merged_proto") + libA := protoc.NewOtherProtoLibrary(nil, + makeLibraryRule("a_proto", "mergetest/merged", []string{"a.proto"}), + protoc.NewFile("mergetest/merged", "a.proto")) + libB := protoc.NewOtherProtoLibrary(nil, + makeLibraryRule("b_proto", "mergetest/merged", []string{"b.proto"}), + protoc.NewFile("mergetest/merged", "b.proto")) + r.SetPrivateAttr(protoc.MergedProtoLibrariesKey, []protoc.ProtoLibrary{libA, libB}) + + from := label.New("", "mergetest/merged", "merged_proto") + + got := prost.ResolveTransitiveExternPaths(r, from) + for _, opt := range got { + if opt == "extern_path=.mergetest.merged=::mergetest_merged" { + t.Errorf("got self-referential extern_path for merged-in own library, all options: %v", got) + } + } + if len(got) != 0 { + t.Errorf("expected no extern_path entries (all files are own), got: %v", got) + } +} diff --git a/pkg/plugin/neoeinstein/prost/needs_cwkt_test.go b/pkg/plugin/neoeinstein/prost/needs_cwkt_test.go new file mode 100644 index 000000000..984b284ac --- /dev/null +++ b/pkg/plugin/neoeinstein/prost/needs_cwkt_test.go @@ -0,0 +1,35 @@ +package prost + +import "testing" + +func TestNeedsCompileWellKnownTypes(t *testing.T) { + cases := []struct { + name string + opts []string + want bool + }{ + {"empty", nil, false}, + {"unrelated extern", []string{"extern_path=.foo.bar=::foo_bar"}, false}, + { + "per-package google.protobuf", + []string{"extern_path=.google.protobuf=::google_protobuf__descriptor"}, + true, + }, + { + "per-type google.protobuf.Empty", + []string{"extern_path=.google.protobuf.Empty=::google_protobuf__empty::Empty"}, + true, + }, + { + // `google.protobufabc` is a different package, must not match. + "prefix-but-not-google.protobuf", + []string{"extern_path=.google.protobufabc=::google_protobufabc"}, + false, + }, + } + for _, c := range cases { + if got := needsCompileWellKnownTypes(c.opts, nil); got != c.want { + t.Errorf("%s: got %v, want %v", c.name, got, c.want) + } + } +} diff --git a/pkg/plugin/neoeinstein/prost/nested_type_path_test.go b/pkg/plugin/neoeinstein/prost/nested_type_path_test.go new file mode 100644 index 000000000..951d53cff --- /dev/null +++ b/pkg/plugin/neoeinstein/prost/nested_type_path_test.go @@ -0,0 +1,35 @@ +package prost + +import "testing" + +func TestProtoTypePathToRustPath(t *testing.T) { + cases := []struct{ in, want string }{ + {"MyMessage", "MyMessage"}, + {"PIKInfo", "PikInfo"}, + // Nested: every non-final segment becomes snake_cased module name, + // the leaf stays UpperCamel. + {"UstOrderStatus.Confirmed", "ust_order_status::Confirmed"}, + {"Outer.Inner.Leaf", "outer::inner::Leaf"}, + {"PIKInfo.SubType", "pik_info::SubType"}, + } + for _, c := range cases { + if got := protoTypePathToRustPath(c.in); got != c.want { + t.Errorf("protoTypePathToRustPath(%q): got %q, want %q", c.in, got, c.want) + } + } +} + +func TestToSnakeFromCamel(t *testing.T) { + cases := []struct{ in, want string }{ + {"MyType", "my_type"}, + {"PIKInfo", "pik_info"}, + {"URLLoader", "url_loader"}, + {"already_snake", "already_snake"}, + {"X", "x"}, + } + for _, c := range cases { + if got := toSnakeFromCamel(c.in); got != c.want { + t.Errorf("toSnakeFromCamel(%q): got %q, want %q", c.in, got, c.want) + } + } +} diff --git a/pkg/plugin/neoeinstein/prost/protoc-gen-prost.go b/pkg/plugin/neoeinstein/prost/protoc-gen-prost.go index 72b260cc4..8b5a66fb3 100644 --- a/pkg/plugin/neoeinstein/prost/protoc-gen-prost.go +++ b/pkg/plugin/neoeinstein/prost/protoc-gen-prost.go @@ -3,6 +3,9 @@ package prost import ( "path" "sort" + "strings" + + eproto "github.com/emicklei/proto" "github.com/bazelbuild/bazel-gazelle/label" "github.com/bazelbuild/bazel-gazelle/rule" @@ -32,45 +35,162 @@ func (p *ProtocGenProstPlugin) Configure(ctx *protoc.PluginContext) *protoc.Plug return nil } - outputs := p.outputs(ctx.ProtoLibrary) + perFile := ctx.IsProtoFileMode() + outputs := p.outputs(ctx.ProtoLibrary, perFile) if len(outputs) == 0 { return nil } - p.registerExternPaths(ctx.ProtoLibrary) + p.registerExternPaths(ctx.ProtoLibrary, perFile) + + options := ctx.PluginConfig.GetOptions() + if perFile { + // In per-file mode the plugin needs to be told to emit one .rs + // per .proto file with the file-stemmed naming convention. + // `per_file=true` triggers the matching branch in our vendored + // protoc-gen-prost; see bazel_tools/rust/vendor/README.md in the + // downstream repo for the rationale. + options = append(options, "per_file=true") + } return &protoc.PluginConfiguration{ Label: label.New("build_stack_rules_proto", "plugin/neoeinstein/prost", "protoc-gen-prost"), Outputs: outputs, - Options: ctx.PluginConfig.GetOptions(), + Options: options, } } -// ResolvePluginOptions implements the PluginOptionsResolver interface. -// It computes extern_path options based on transitive proto file dependencies. +// ResolvePluginOptions implements the PluginOptionsResolver interface. It +// computes extern_path options based on transitive proto file dependencies, +// and prepends compile_well_known_types=true whenever: +// +// - one of the library's own proto packages is google.protobuf (the library +// is itself compiling the well-known types — without this flag prost +// skips them and emits a stub), or +// - the resolved extern_path set references .google.protobuf (the library +// consumes well-known types via a foreign crate; prost-build registers a +// default extern path to ::prost_types unless this flag clears it, which +// would collide with our extern_path entry and error out as "duplicate +// extern Protobuf path"). +// +// Only the prost plugin needs this — protoc-gen-prost-serde (pbjson-build) +// doesn't consult the flag and its codegen doesn't hit the collision path. func (p *ProtocGenProstPlugin) ResolvePluginOptions(cfg *protoc.PluginConfiguration, r *rule.Rule, from label.Label) []string { - return ResolveExternPathOptions(cfg, r, from) + opts := ResolveExternPathOptions(cfg, r, from) + if needsCompileWellKnownTypes(opts, ownProtoPackages(r, from)) { + opts = append([]string{"compile_well_known_types=true"}, opts...) + } + return opts } -// shouldApply returns true if the library has files with messages or enums. +const wellKnownTypesProtoPackage = "google.protobuf" + +// needsCompileWellKnownTypes reports whether the prost plugin should emit +// compile_well_known_types=true for a library based on its computed options +// and own-package set. See ResolvePluginOptions for the rationale. +func needsCompileWellKnownTypes(opts []string, ownPackages map[string]bool) bool { + if ownPackages[wellKnownTypesProtoPackage] { + return true + } + // Match any extern_path entry targeting `.google.protobuf` or any + // `.google.protobuf.` — both forms would collide with the + // defaults prost-build registers when prost_types is on, so + // `compile_well_known_types=true` must be set to suppress them. + pkgPrefix := "extern_path=." + wellKnownTypesProtoPackage + for _, opt := range opts { + if !strings.HasPrefix(opt, pkgPrefix) { + continue + } + // Next char after the prefix must be either `=` (per-package + // `extern_path=.google.protobuf=...`) or `.` (per-type + // `extern_path=.google.protobuf.Foo=...`). + next := opt[len(pkgPrefix):] + if strings.HasPrefix(next, "=") || strings.HasPrefix(next, ".") { + return true + } + } + return false +} + +// shouldApply returns true if the library has any file with messages, enums, +// or services. Services are included because protoc-gen-tonic's generated +// code is inserted into prost's {package}.rs at the @@protoc_insertion_point +// marker (see protoc-gen-prost's append_to_file mechanism). If prost isn't +// invoked, the file doesn't exist and tonic's insert fails with +// "Tried to insert into file that doesn't exist". func (p *ProtocGenProstPlugin) shouldApply(lib protoc.ProtoLibrary) bool { for _, f := range lib.Files() { - if f.HasMessages() || f.HasEnums() { + if f.HasMessages() || f.HasEnums() || f.HasServices() { return true } } return false } -// outputs computes the output files for the plugin. Prost generates one .rs -// file per proto package, named {proto_package}.rs. The path includes the -// file's directory so that mergeSources can handle the rel stripping. -func (p *ProtocGenProstPlugin) outputs(lib protoc.ProtoLibrary) []string { - seen := make(map[string]bool) +// HasGeneratableContent reports whether `f` defines any top-level construct +// that would cause prost / pbjson / tonic to emit non-trivial output. +// +// Specifically: real messages (not `extend` blocks — emicklei/proto reports +// `extend` as a `proto.Message` with `IsExtend=true`, but downstream +// generators emit nothing for them), enums, or services. +// +// Used by the per-file output computations to avoid declaring a serde/tonic +// output for a file that only contains `extend` blocks: pbjson-build / +// tonic-build would produce no file, and the missing artifact later trips +// proto_compile.bzl's `mv` step. +func HasGeneratableContent(f *protoc.File) bool { + if f.HasEnums() || f.HasServices() { + return true + } + for _, msg := range f.Messages() { + if !msg.IsExtend { + return true + } + } + return false +} + +// outputs computes the output files for the plugin. +// +// In per-package mode (the default), prost generates one .rs file per proto +// package, named {proto_package}.rs. +// +// In per-file mode (used when the surrounding bazel package opts into +// `gazelle:proto file`), prost generates one .rs file per .proto file in +// `file_to_generate`, named `..rs` so that +// multiple files in the same package don't collide. The vendored +// protoc-gen-prost honours this naming when `per_file=true` is set on +// its options. +// +// Service-only files (no messages/enums) are included — prost still emits +// a stub .rs containing the @@protoc_insertion_point(module) marker, which +// tonic relies on to inject client/server code via append_to_file. +func (p *ProtocGenProstPlugin) outputs(lib protoc.ProtoLibrary, perFile bool) []string { outputs := make([]string, 0) + if perFile { + for _, f := range lib.Files() { + if !(f.HasMessages() || f.HasEnums() || f.HasServices()) { + continue + } + pkg := f.Package() + if pkg.Name == "" { + continue + } + stem := strings.TrimSuffix(f.Basename, ".proto") + filename := stem + "." + pkg.Name + ".rs" + if f.Dir != "" { + filename = path.Join(f.Dir, filename) + } + outputs = append(outputs, filename) + } + sort.Strings(outputs) + return outputs + } + + seen := make(map[string]bool) for _, f := range lib.Files() { - if !(f.HasMessages() || f.HasEnums()) { + if !(f.HasMessages() || f.HasEnums() || f.HasServices()) { continue } pkg := f.Package() @@ -94,28 +214,132 @@ func (p *ProtocGenProstPlugin) outputs(lib protoc.ProtoLibrary) []string { } // registerExternPaths records prost extern_path data in the GlobalResolver for -// each proto file in the library. This data is later consumed by -// ResolveTransitiveExternPaths when computing extern_path options for dependent -// packages. +// each proto file in the library. The data is later consumed by +// ResolveTransitiveExternPaths when computing extern_path options for +// dependent packages. +// +// Two flavours are registered: // -// The label encodes: Pkg = proto package name, Name = crate name. The crate -// name comes from protoc.RustCrateName so it matches the rust_library target -// name produced by RustLibrary.Name() — without this alignment, downstream -// extern_path entries would point at a non-existent crate and rustc would -// fail to resolve types. -func (p *ProtocGenProstPlugin) registerExternPaths(lib protoc.ProtoLibrary) { +// - "prost_extern" / → (proto pkg, **façade** crate name). +// Used to map a file-import to the rust crate that consumers should depend +// on. Always the per-package façade — not a per-file crate — because +// cross-package extern_path entries are prefix-matched by prost-build, +// which then runs `to_snake_case` on each segment of the rust_path. That +// normalization collapses `__` → `_`, so a per-file rust_path like +// `::__` would land as the nonexistent `::_`. +// The façade transparently re-exports the per-file crates' contents +// (see proto_rust_library.bzl's facade emission), so the consumer's +// code path stays identical. +// +// - "prost_extern_type" / → (type name, **per-file** crate +// name). Only populated in per-file mode. Used by +// ResolveTransitiveExternPaths to emit per-type extern_path entries for +// same-package cross-file references — these are EXACT-match resolved by +// prost-build (no to_snake) so the double-underscored per-file crate name +// survives intact. Without these, prost emits a relative `super::...` +// path that won't resolve once each .proto lives in its own crate. +// +// Crate-name choices: per-package mode uses `RustCrateName(pkg.Name)` — +// e.g. `omnistac.spok.message` → `omnistac_spok_message`. The per-file +// crates (registered only under PerFileTypeProvideKind) add a `__` +// suffix — `omnistac_spok_message__order`. That suffix matches the +// proto_rust_library names the gazelle plugin emits for per-file crates. +func (p *ProtocGenProstPlugin) registerExternPaths(lib protoc.ProtoLibrary, perFile bool) { + resolver := protoc.GlobalResolver() for _, f := range lib.Files() { pkg := f.Package() if pkg.Name == "" { continue } + facadeCrate := protoc.RustCrateName(pkg.Name) protoFile := path.Join(f.Dir, f.Basename) - protoc.GlobalResolver().Provide( + + resolver.Provide( "proto", "prost_extern", protoFile, - label.New("", pkg.Name, protoc.RustCrateName(pkg.Name)), + label.New("", pkg.Name, facadeCrate), ) + + // In per-file mode, also register per-type entries so sibling per-file + // crates in the same proto package can map their cross-file type + // references to the correct sibling crate. Without this, prost emits + // a relative path like `Other` assuming nested-module layout, which + // fails to resolve once Other lives in a different crate. + // + // The key is the proto package name (not the fully-qualified type + // path), so that ResolveTransitiveExternPaths can enumerate every + // type in a package by a single Resolve() call. The label's `Pkg` + // holds the type name; `Name` holds the crate that defines it. + if !perFile { + continue + } + stem := strings.TrimSuffix(f.Basename, ".proto") + perFileCrate := facadeCrate + "__" + stem + + // Register top-level AND nested messages/enums. Why nested: + // prost-build's `resolve_ident` tries an exact match first and + // falls back to prefix matching only when no exact key exists. + // Prefix-match re-applies `to_snake_case` to every rust_path + // segment, which collapses our per-file crate's `__` to + // `_` and produces a non-existent crate reference. By + // registering the nested type with an exact key, the + // exact-match branch fires and the rust_path is returned + // verbatim — `__` survives. + for _, msg := range f.Messages() { + registerNestedTypes(resolver, pkg.Name, msg, msg.Name, perFileCrate) + } + for _, enum := range f.Enums() { + resolver.Provide( + "proto", + PerFileTypeProvideKind, + pkg.Name, + label.New("", enum.Name, perFileCrate), + ) + } } } + +// registerNestedTypes walks a message's nested messages and enums (recursively) +// and registers each under PerFileTypeProvideKind with a dotted type-path key +// rooted at the supplied `prefix` (the message's own name on first call). +// +// Each registration's Label.Pkg holds the full dotted type path +// (e.g. `Outer.Inner` or `Outer.Inner.MoreNested`); perFileSiblingTypeExternPaths +// later converts the dots to `::` and upper-camels each segment when emitting +// the extern_path rust_path. +func registerNestedTypes( + resolver protoc.ImportResolver, + protoPackage string, + msg eproto.Message, + prefix string, + perFileCrate string, +) { + resolver.Provide( + "proto", + PerFileTypeProvideKind, + protoPackage, + label.New("", prefix, perFileCrate), + ) + for _, el := range msg.Elements { + switch e := el.(type) { + case *eproto.Message: + registerNestedTypes(resolver, protoPackage, *e, prefix+"."+e.Name, perFileCrate) + case *eproto.Enum: + resolver.Provide( + "proto", + PerFileTypeProvideKind, + protoPackage, + label.New("", prefix+"."+e.Name, perFileCrate), + ) + } + } +} + +// PerFileTypeProvideKind is the GlobalResolver `kind` arg under which +// per-file mode registers each message/enum a per-file proto_library +// contributes. Keyed by proto package, the resulting Resolve() call returns +// the (type name, crate name) for every type declared anywhere in that +// proto package across sibling per-file libraries. +const PerFileTypeProvideKind = "prost_extern_per_file_type" diff --git a/pkg/plugin/neoeinstein/prost/protoc-gen-prost_test.go b/pkg/plugin/neoeinstein/prost/protoc-gen-prost_test.go index 01a796f6c..c841d4984 100644 --- a/pkg/plugin/neoeinstein/prost/protoc-gen-prost_test.go +++ b/pkg/plugin/neoeinstein/prost/protoc-gen-prost_test.go @@ -42,6 +42,39 @@ func TestProtocGenProstPlugin(t *testing.T) { ), SkipIntegration: true, }, + "service only - emits stub .rs for tonic to insert into": { + // Without this case the protoc plugin chain breaks: tonic + // appends its client/server code at the @@protoc_insertion_point + // in {package}.rs, and protoc errors with + // "Tried to insert into file that doesn't exist" if prost + // hasn't created that file first. + Input: "package example.v1;\nservice Greeter { rpc Hello(Req) returns (Resp); }\nmessage Req {}\nmessage Resp {}", + Directives: plugintest.WithDirectives( + "proto_plugin", "protoc-gen-prost implementation neoeinstein:prost:protoc-gen-prost", + ), + PluginName: "protoc-gen-prost", + Configuration: plugintest.WithConfiguration( + plugintest.WithLabel(t, "@build_stack_rules_proto//plugin/neoeinstein/prost:protoc-gen-prost"), + plugintest.WithOutputs("example.v1.rs"), + ), + SkipIntegration: true, + }, + "service only - no messages": { + // Same as above but the proto has no messages/enums of its own + // (could be e.g. a separate proto_library that imports its + // request/response types from a sibling). Prost still has to + // emit the .rs stub so tonic's append succeeds. + Input: "package example.v1;\nimport \"google/protobuf/empty.proto\";\nservice Pinger { rpc Ping(google.protobuf.Empty) returns (google.protobuf.Empty); }", + Directives: plugintest.WithDirectives( + "proto_plugin", "protoc-gen-prost implementation neoeinstein:prost:protoc-gen-prost", + ), + PluginName: "protoc-gen-prost", + Configuration: plugintest.WithConfiguration( + plugintest.WithLabel(t, "@build_stack_rules_proto//plugin/neoeinstein/prost:protoc-gen-prost"), + plugintest.WithOutputs("example.v1.rs"), + ), + SkipIntegration: true, + }, "no package - skipped": { Input: "message Foo {}", Directives: plugintest.WithDirectives( diff --git a/pkg/plugin/neoeinstein/prost/register_nested_test.go b/pkg/plugin/neoeinstein/prost/register_nested_test.go new file mode 100644 index 000000000..38aba062e --- /dev/null +++ b/pkg/plugin/neoeinstein/prost/register_nested_test.go @@ -0,0 +1,57 @@ +package prost + +import ( + "sort" + "strings" + "testing" + + "github.com/stackb/rules_proto/v4/pkg/protoc" +) + +func TestRegisterNestedTypes_RegistersNestedPaths(t *testing.T) { + resolver := protoc.NewImportResolver(&protoc.ImportResolverOptions{}) + + const src = `syntax = "proto2"; +package nested.test; +message Outer { + message Inner { + message Leaf {} + } + enum NestedEnum { A = 0; } +} +message TopLevel {} +` + f := protoc.NewFile("nested/test", "x.proto") + if err := f.ParseReader(strings.NewReader(src)); err != nil { + t.Fatalf("parse: %v", err) + } + + for _, msg := range f.Messages() { + registerNestedTypes(resolver, "nested.test", msg, msg.Name, "perfile_crate") + } + + var keys []string + for _, ent := range resolver.Resolve("proto", PerFileTypeProvideKind, "nested.test") { + keys = append(keys, ent.Label.Pkg) + } + sort.Strings(keys) + + for _, want := range []string{ + "Outer", + "Outer.Inner", + "Outer.Inner.Leaf", + "Outer.NestedEnum", + "TopLevel", + } { + found := false + for _, k := range keys { + if k == want { + found = true + break + } + } + if !found { + t.Errorf("missing nested registration %q; got: %v", want, keys) + } + } +} diff --git a/pkg/plugin/neoeinstein/prost_serde/protoc-gen-prost-serde.go b/pkg/plugin/neoeinstein/prost_serde/protoc-gen-prost-serde.go index e39fdda30..1883d3fe1 100644 --- a/pkg/plugin/neoeinstein/prost_serde/protoc-gen-prost-serde.go +++ b/pkg/plugin/neoeinstein/prost_serde/protoc-gen-prost-serde.go @@ -3,6 +3,7 @@ package prost_serde import ( "path" "sort" + "strings" "github.com/bazelbuild/bazel-gazelle/label" "github.com/bazelbuild/bazel-gazelle/rule" @@ -31,15 +32,26 @@ func (p *ProtocGenProstSerdePlugin) Configure(ctx *protoc.PluginContext) *protoc return nil } - outputs := p.outputs(ctx.ProtoLibrary) + perFile := ctx.IsProtoFileMode() + outputs := p.outputs(ctx.ProtoLibrary, perFile) if len(outputs) == 0 { return nil } + options := ctx.PluginConfig.GetOptions() + if perFile { + // Must match the prost plugin's per_file=true so pbjson emits one + // `..serde.rs` per file AND points its `insert_include` + // pragma at the matching `..rs` (which prost wrote); + // the unpatched form inserts into `.rs`, a filename that no + // longer exists in per-file mode. + options = append(options, "per_file=true") + } + return &protoc.PluginConfiguration{ Label: label.New("build_stack_rules_proto", "plugin/neoeinstein/prost-serde", "protoc-gen-prost-serde"), Outputs: outputs, - Options: ctx.PluginConfig.GetOptions(), + Options: options, } } @@ -63,15 +75,46 @@ func (p *ProtocGenProstSerdePlugin) shouldApply(lib protoc.ProtoLibrary) bool { return false } -// outputs computes the output files for the plugin. Prost-serde generates one -// .serde.rs file per proto package. The path includes the file's directory so -// that mergeSources can handle the rel stripping. -func (p *ProtocGenProstSerdePlugin) outputs(lib protoc.ProtoLibrary) []string { - seen := make(map[string]bool) +// outputs computes the output files for the plugin. +// +// Per-package mode (default): one `.serde.rs` per proto package. +// +// Per-file mode (the surrounding bazel package opts into `gazelle:proto file`): +// one `..serde.rs` per .proto file that defines messages or +// enums. The vendored protoc-gen-prost-serde honours this naming convention +// when `per_file=true` is in the options block. +func (p *ProtocGenProstSerdePlugin) outputs(lib protoc.ProtoLibrary, perFile bool) []string { outputs := make([]string, 0) + if perFile { + for _, f := range lib.Files() { + if !prost.HasGeneratableContent(f) { + // Same rationale as the per-package branch below: pbjson + // emits no file for extend-only protos, and a declared + // output would trip proto_compile.bzl's `mv` rename. + continue + } + pkg := f.Package() + if pkg.Name == "" { + continue + } + stem := strings.TrimSuffix(f.Basename, ".proto") + filename := stem + "." + pkg.Name + ".serde.rs" + if f.Dir != "" { + filename = path.Join(f.Dir, filename) + } + outputs = append(outputs, filename) + } + sort.Strings(outputs) + return outputs + } + + seen := make(map[string]bool) for _, f := range lib.Files() { - if !(f.HasMessages() || f.HasEnums()) { + if !prost.HasGeneratableContent(f) { + // Skip files with no serializable types. pbjson-build emits + // nothing for extend-only files, and the declared output would + // later fail proto_compile.bzl's `mv` rename. continue } pkg := f.Package() diff --git a/pkg/plugin/neoeinstein/tonic/protoc-gen-tonic.go b/pkg/plugin/neoeinstein/tonic/protoc-gen-tonic.go index 7caefef82..38bc4ab98 100644 --- a/pkg/plugin/neoeinstein/tonic/protoc-gen-tonic.go +++ b/pkg/plugin/neoeinstein/tonic/protoc-gen-tonic.go @@ -3,6 +3,7 @@ package tonic import ( "path" "sort" + "strings" "github.com/bazelbuild/bazel-gazelle/label" "github.com/bazelbuild/bazel-gazelle/rule" @@ -31,15 +32,25 @@ func (p *ProtocGenTonicPlugin) Configure(ctx *protoc.PluginContext) *protoc.Plug return nil } - outputs := p.outputs(ctx.ProtoLibrary) + perFile := ctx.IsProtoFileMode() + outputs := p.outputs(ctx.ProtoLibrary, perFile) if len(outputs) == 0 { return nil } + options := ctx.PluginConfig.GetOptions() + if perFile { + // Must match the prost plugin's per_file=true so tonic emits one + // `..tonic.rs` per file AND inserts its module entry + // into the matching `..rs` rather than the per-package + // `.rs` (which doesn't exist in per-file mode). + options = append(options, "per_file=true") + } + return &protoc.PluginConfiguration{ Label: label.New("build_stack_rules_proto", "plugin/neoeinstein/tonic", "protoc-gen-tonic"), Outputs: outputs, - Options: ctx.PluginConfig.GetOptions(), + Options: options, } } @@ -63,13 +74,39 @@ func (p *ProtocGenTonicPlugin) shouldApply(lib protoc.ProtoLibrary) bool { return false } -// outputs computes the output files for the plugin. Tonic generates one -// .tonic.rs file per proto package that has services. The path includes the -// file's directory so that mergeSources can handle the rel stripping. -func (p *ProtocGenTonicPlugin) outputs(lib protoc.ProtoLibrary) []string { - seen := make(map[string]bool) +// outputs computes the output files for the plugin. +// +// Per-package mode (default): one `.tonic.rs` per proto package that +// declares services. +// +// Per-file mode (the surrounding bazel package opts into `gazelle:proto file`): +// one `..tonic.rs` per .proto file that declares services. +// The vendored protoc-gen-tonic honours this naming convention when +// `per_file=true` is in the options block. +func (p *ProtocGenTonicPlugin) outputs(lib protoc.ProtoLibrary, perFile bool) []string { outputs := make([]string, 0) + if perFile { + for _, f := range lib.Files() { + if !f.HasServices() { + continue + } + pkg := f.Package() + if pkg.Name == "" { + continue + } + stem := strings.TrimSuffix(f.Basename, ".proto") + filename := stem + "." + pkg.Name + ".tonic.rs" + if f.Dir != "" { + filename = path.Join(f.Dir, filename) + } + outputs = append(outputs, filename) + } + sort.Strings(outputs) + return outputs + } + + seen := make(map[string]bool) for _, f := range lib.Files() { if !f.HasServices() { continue diff --git a/pkg/protoc/BUILD.bazel b/pkg/protoc/BUILD.bazel index 126c3231e..2f26414c0 100644 --- a/pkg/protoc/BUILD.bazel +++ b/pkg/protoc/BUILD.bazel @@ -40,6 +40,7 @@ go_library( deps = [ "@bazel_gazelle//config", "@bazel_gazelle//label", + "@bazel_gazelle//language/proto", "@bazel_gazelle//resolve", "@bazel_gazelle//rule", "@com_github_bazelbuild_buildtools//build", diff --git a/pkg/protoc/plugin_context.go b/pkg/protoc/plugin_context.go index 7c2847c7e..6745b17d2 100644 --- a/pkg/protoc/plugin_context.go +++ b/pkg/protoc/plugin_context.go @@ -1,5 +1,9 @@ package protoc +import ( + gproto "github.com/bazelbuild/bazel-gazelle/language/proto" +) + // PluginContext represents the environment available to the plugin when // invoked. type PluginContext struct { @@ -15,4 +19,19 @@ type PluginContext struct { Plugin Plugin } +// IsProtoFileMode reports whether the surrounding bazel package is using +// the standard gazelle proto language's `file` mode (`# gazelle:proto file`), +// which generates one proto_library per .proto file. Plugins use this to +// switch between per-package and per-file output schemes. +func (ctx *PluginContext) IsProtoFileMode() bool { + if ctx == nil || ctx.PackageConfig.Config == nil { + return false + } + cfg := gproto.GetProtoConfig(ctx.PackageConfig.Config) + if cfg == nil { + return false + } + return cfg.Mode == gproto.FileMode +} + // type PluginContextResolver func(c *config.Config, ix *resolve.RuleIndex, r *rule.Rule, imports []string, from label.Label) diff --git a/pkg/protoc/proto_compile.go b/pkg/protoc/proto_compile.go index bb0b7bcd3..f38b81499 100644 --- a/pkg/protoc/proto_compile.go +++ b/pkg/protoc/proto_compile.go @@ -14,8 +14,19 @@ import ( ) const ( - // ProtoLibraryKey stores the ProtoLibrary implementation for a rule. + // ProtoLibraryKey stores the ProtoLibrary implementation for a rule. When a + // proto_compile / proto_compiled_sources rule merges multiple proto_libraries + // into its `protos` attribute, this key still references only the first + // library — preserved for compatibility with callers that index a single + // ProtoLibrary. See MergedProtoLibrariesKey for the full set. ProtoLibraryKey = "_proto_library" + // MergedProtoLibrariesKey stores the full []ProtoLibrary backing a merged + // proto_compile / proto_compiled_sources rule. Always contains at least one + // library; equal to []{ProtoLibraryKey} when no merge occurred. Plugin option + // resolvers must consult this — relying on ProtoLibraryKey alone misses the + // post-merge libraries and lets their .proto files leak into the dep-walk's + // "external" classification. + MergedProtoLibrariesKey = "_merged_proto_libraries" ) func init() { @@ -132,6 +143,11 @@ func (s *protoCompileRule) Rule(otherGen ...*rule.Rule) *rule.Rule { existingProtos = append(existingProtos, s.config.Library.Name()) other.SetAttr("protos", DeduplicateAndSort(existingProtos)) + // Track the full backing-library set for downstream plugin option + // resolution. See MergedProtoLibrariesKey docs. + existingMerged, _ := other.PrivateAttr(MergedProtoLibrariesKey).([]ProtoLibrary) + other.SetPrivateAttr(MergedProtoLibrariesKey, append(existingMerged, s.config.Library)) + // Merge plugins otherPlugins := other.AttrStrings("plugins") otherPlugins = append(otherPlugins, GetPluginLabels(s.config.Plugins)...) @@ -162,6 +178,7 @@ func (s *protoCompileRule) Rule(otherGen ...*rule.Rule) *rule.Rule { newRule.SetAttr(s.outputsAttrName, outputs) newRule.SetAttr("plugins", GetPluginLabels(s.config.Plugins)) newRule.SetAttr("proto", s.config.Library.Name()) + newRule.SetPrivateAttr(MergedProtoLibrariesKey, []ProtoLibrary{s.config.Library}) if s.config.LanguageConfig.Protoc != "" { newRule.SetAttr("protoc", s.config.LanguageConfig.Protoc) diff --git a/pkg/protoc/protoc_configuration.go b/pkg/protoc/protoc_configuration.go index 8047817fd..db042aa7f 100644 --- a/pkg/protoc/protoc_configuration.go +++ b/pkg/protoc/protoc_configuration.go @@ -3,6 +3,8 @@ package protoc import ( "path" "strings" + + gproto "github.com/bazelbuild/bazel-gazelle/language/proto" ) // ProtocConfiguration represents the complete configuration and source @@ -61,6 +63,26 @@ func (c *ProtocConfiguration) GetPluginOutputs(implementationName string) []stri return plugin.Outputs } +// IsProtoFileMode reports whether the surrounding bazel package is using +// the standard gazelle proto language's `file` mode (`# gazelle:proto file`), +// which generates one proto_library per .proto file. This is the signal the +// rust plugin uses to decide whether to emit one rust crate per .proto file +// (with a per-package façade that re-exports each sibling), rather than the +// default one-crate-per-package convention. +// +// Returns false when the config can't be reached (e.g. unit tests that +// construct a ProtocConfiguration without a parent PackageConfig). +func (c *ProtocConfiguration) IsProtoFileMode() bool { + if c == nil || c.PackageConfig == nil || c.PackageConfig.Config == nil { + return false + } + cfg := gproto.GetProtoConfig(c.PackageConfig.Config) + if cfg == nil { + return false + } + return cfg.Mode == gproto.FileMode +} + // mergeSources computes the source files that are generated by the rule and any // necessary mappings. func mergeSources(workDir, rel string, plugins []*PluginConfiguration, stripImportPrefix string) ([]string, map[string]string) { diff --git a/pkg/protoc/rust_keywords.go b/pkg/protoc/rust_keywords.go index f6202952f..b64bfbbfc 100644 --- a/pkg/protoc/rust_keywords.go +++ b/pkg/protoc/rust_keywords.go @@ -62,6 +62,32 @@ var rustKeywords = map[string]bool{ "yield": true, } +// RustProtocOutputDir returns the directory that protoc-gen-prost (and its +// siblings) will write outputs into for a given proto package, with Rust +// keyword segments escaped via the r# prefix. +// +// Examples: +// - "google.type" → "google/r#type" +// - "trumid.common.auth" → "trumid/common/auth" +// - "" → "" +// +// Note this is independent of the bazel package location — protoc-gen-prost +// always derives the output path from the proto package name unless given +// flat_output_dir=true. Use the result to compare against pc.Rel and decide +// whether output_mappings are needed. +func RustProtocOutputDir(pkg string) string { + if pkg == "" { + return "" + } + segments := strings.Split(pkg, ".") + for i, seg := range segments { + if rustKeywords[seg] { + segments[i] = "r#" + seg + } + } + return strings.Join(segments, "/") +} + // RustKeywordEscapeMappings computes output mappings needed when // protoc-gen-prost escapes Rust keywords with the r# prefix in directory paths. // @@ -69,17 +95,17 @@ var rustKeywords = map[string]bool{ // "google/r#type/" instead of "google/type/". This function returns a mapping // from each declared output filename to the actual prost output path. // -// Returns an empty map if no package segments are Rust keywords. +// Returns an empty map if no package segments are Rust keywords. Callers who +// also need to handle the more general case of proto-package path differing +// from the bazel package path should use RustProtocOutputDir directly. func RustKeywordEscapeMappings(pkg string, outputs []string) map[string]string { if pkg == "" || len(outputs) == 0 { return nil } - segments := strings.Split(pkg, ".") - // Check if any segment is a Rust keyword. needsEscape := false - for _, seg := range segments { + for _, seg := range strings.Split(pkg, ".") { if rustKeywords[seg] { needsEscape = true break @@ -89,17 +115,7 @@ func RustKeywordEscapeMappings(pkg string, outputs []string) map[string]string { return nil } - // Build the escaped directory path. - escaped := make([]string, len(segments)) - for i, seg := range segments { - if rustKeywords[seg] { - escaped[i] = "r#" + seg - } else { - escaped[i] = seg - } - } - escapedDir := strings.Join(escaped, "/") - + escapedDir := RustProtocOutputDir(pkg) mappings := make(map[string]string, len(outputs)) for _, output := range outputs { base := path.Base(output) @@ -109,14 +125,15 @@ func RustKeywordEscapeMappings(pkg string, outputs []string) map[string]string { } // RustCrateName returns the canonical Rust crate name for a proto package. -// The proto package's dots are replaced with underscores and an "_rs" suffix -// is appended so the resulting identifier is unambiguously a Rust target, -// not a proto_library (e.g. "trumid.common.utils.state.snapshot.proto" → -// "trumid_common_utils_state_snapshot_proto_rs"). Returns the empty string -// for an empty input. +// The proto package's dots are replaced with underscores; the crate name IS +// the namespace, so no language suffix is appended (e.g. +// "trumid.common.utils.state.snapshot.proto" → +// "trumid_common_utils_state_snapshot_proto"). The proto_rust_library macro +// is expected to expose all generated types at the crate root, matching this +// flat convention. Returns the empty string for an empty input. func RustCrateName(protoPackage string) string { if protoPackage == "" { return "" } - return strings.ReplaceAll(protoPackage, ".", "_") + "_rs" + return strings.ReplaceAll(protoPackage, ".", "_") } diff --git a/pkg/protoc/rust_keywords_test.go b/pkg/protoc/rust_keywords_test.go index 8b3b396c4..1c06328e2 100644 --- a/pkg/protoc/rust_keywords_test.go +++ b/pkg/protoc/rust_keywords_test.go @@ -85,11 +85,11 @@ func TestRustCrateName(t *testing.T) { want string }{ "empty": {pkg: "", want: ""}, - "single segment": {pkg: "foo", want: "foo_rs"}, - "trailing proto": {pkg: "trumid.common.utils.state.snapshot.proto", want: "trumid_common_utils_state_snapshot_proto_rs"}, + "single segment": {pkg: "foo", want: "foo"}, + "trailing proto": {pkg: "trumid.common.utils.state.snapshot.proto", want: "trumid_common_utils_state_snapshot_proto"}, "sub-package": {pkg: "trumid.common.utils.state.snapshot.proto.example", - want: "trumid_common_utils_state_snapshot_proto_example_rs"}, - "keywords are not escaped here": {pkg: "google.type", want: "google_type_rs"}, + want: "trumid_common_utils_state_snapshot_proto_example"}, + "keywords are not escaped here": {pkg: "google.type", want: "google_type"}, } { t.Run(name, func(t *testing.T) { if got := RustCrateName(tc.pkg); got != tc.want { diff --git a/pkg/rule/rules_rust/BUILD.bazel b/pkg/rule/rules_rust/BUILD.bazel index 8f8ebe5e6..b1181459c 100644 --- a/pkg/rule/rules_rust/BUILD.bazel +++ b/pkg/rule/rules_rust/BUILD.bazel @@ -25,6 +25,7 @@ go_test( "//pkg/protoc", "@bazel_gazelle//config", "@bazel_gazelle//label", + "@bazel_gazelle//language/proto", "@bazel_gazelle//rule", "@com_github_google_go_cmp//cmp", ], diff --git a/pkg/rule/rules_rust/proto_rust_library.go b/pkg/rule/rules_rust/proto_rust_library.go index 448590d29..33130b276 100644 --- a/pkg/rule/rules_rust/proto_rust_library.go +++ b/pkg/rule/rules_rust/proto_rust_library.go @@ -1,6 +1,7 @@ package rules_rust import ( + "path" "strings" "github.com/bazelbuild/bazel-gazelle/label" @@ -57,16 +58,32 @@ func (s *protoRustLibrary) ProvideRule(cfg *protoc.LanguageRuleConfig, pc *proto return nil } - // Compute Rust keyword escape mappings for proto packages containing - // Rust reserved keywords (e.g., "google.type" → prost writes to - // "google/r#type/" instead of "google/type/"). + // Compute output_mappings whenever the directory protoc-gen-prost writes + // to (derived from the proto package name, with Rust keyword segments + // r#-escaped) differs from the bazel package the rule lives in. Two + // distinct causes: + // + // 1. Rust keyword escapes — proto package "google.type" → prost writes + // to "google/r#type/" while the bazel pkg is "google/type". + // + // 2. Proto package path simply differs from bazel package path — + // e.g. proto package "trumid.common.auth" living at bazel pkg + // "trumid/common/auth/proto", or "grpc.health.v1" living at + // "thirdparty/protobuf/grpc/src/main/protobuf/grpc/health/v1". + // Without a mapping, proto_compile.bzl's rename step looks for the + // output at //.rs and fails with + // `mv: ... No such file or directory`. if files := pc.Library.Files(); len(files) > 0 { pkg := files[0].Package().Name - for output, escapedPath := range protoc.RustKeywordEscapeMappings(pkg, outputs) { + protocDir := protoc.RustProtocOutputDir(pkg) + if protocDir != "" && protocDir != pc.Rel { if pc.Mappings == nil { pc.Mappings = make(map[string]string) } - pc.Mappings[output] = escapedPath + for _, output := range outputs { + base := path.Base(output) + pc.Mappings[base] = path.Join(protocDir, base) + } } } diff --git a/pkg/rule/rules_rust/proto_rust_library_test.go b/pkg/rule/rules_rust/proto_rust_library_test.go index 0798992c9..4af924f41 100644 --- a/pkg/rule/rules_rust/proto_rust_library_test.go +++ b/pkg/rule/rules_rust/proto_rust_library_test.go @@ -6,6 +6,7 @@ import ( "github.com/bazelbuild/bazel-gazelle/config" "github.com/bazelbuild/bazel-gazelle/label" + gproto "github.com/bazelbuild/bazel-gazelle/language/proto" "github.com/bazelbuild/bazel-gazelle/rule" "github.com/google/go-cmp/cmp" "github.com/stackb/rules_proto/v4/pkg/protoc" @@ -51,9 +52,8 @@ func TestProtoRustLibraryRule(t *testing.T) { }, want: ` proto_rust_library( - name = "google_api_rs", + name = "google_api", srcs = ["google.api.rs"], - pkg = "google.api", deps = [ "@crates//:pbjson", "@crates//:prost", @@ -77,12 +77,11 @@ proto_rust_library( }, want: ` proto_rust_library( - name = "trumid_common_proto_rs", + name = "trumid_common_proto", srcs = [ "trumid.common.proto.rs", "trumid.common.proto.serde.rs", ], - pkg = "trumid.common.proto", deps = [ "@crates//:pbjson", "@crates//:prost", @@ -107,9 +106,8 @@ proto_rust_library( }, want: ` proto_rust_library( - name = "example_wkt_rs", + name = "example_wkt", srcs = ["example.wkt.rs"], - pkg = "example.wkt", deps = [ "@crates//:pbjson", "@crates//:prost", @@ -134,12 +132,11 @@ proto_rust_library( }, want: ` proto_rust_library( - name = "example_grpc_rs", + name = "example_grpc", srcs = [ "example.grpc.rs", "example.grpc.tonic.rs", ], - pkg = "example.grpc", deps = [ "@crates//:pbjson", "@crates//:prost", @@ -227,3 +224,96 @@ func formatRules(rules ...*rule.Rule) string { } return string(file.Format()) } + +// TestProtoRustLibraryRule_PerFileMode_OneRulePerFile is the core regression +// test for the per-file scheme: in `gazelle:proto file` mode each per-file +// proto_library must produce a uniquely-named `proto_rust_library` rule, so +// the merge branch in `Rule()` is a no-op and consumer dep resolution can +// route every cross-file/cross-package reference to the specific per-file +// label (rather than collapsing through a per-package facade and re-closing +// the proto-package-level cycle). +// +// Name convention asserted: `__` where `` is the +// basename (minus `.proto`) of the first content-bearing file in the +// proto_library — `package.proto` co-includes are skipped so they don't +// steal the suffix. +func TestProtoRustLibraryRule_PerFileMode_OneRulePerFile(t *testing.T) { + c := &config.Config{Exts: map[string]interface{}{}} + c.Exts["proto"] = &gproto.ProtoConfig{Mode: gproto.FileMode} + pkgCfg := protoc.NewPackageConfig(c) + ruleCfg := protoc.NewLanguageRuleConfig(c, "rust") + + libA := protoc.NewOtherProtoLibrary(nil, rule.NewRule("proto_library", "a_proto"), + makeFile("p/k", "a.proto", `syntax = "proto3"; package p.k; message A {}`)) + libB := protoc.NewOtherProtoLibrary(nil, rule.NewRule("proto_library", "b_proto"), + makeFile("p/k", "b.proto", `syntax = "proto3"; package p.k; message B {}`)) + + pcA := &protoc.ProtocConfiguration{ + PackageConfig: pkgCfg, + Rel: "p/k", + Library: libA, + Plugins: []*protoc.PluginConfiguration{ + {Config: &protoc.LanguagePluginConfig{}, Outputs: []string{"a.p.k.rs"}}, + }, + } + pcB := &protoc.ProtocConfiguration{ + PackageConfig: pkgCfg, + Rel: "p/k", + Library: libB, + Plugins: []*protoc.PluginConfiguration{ + {Config: &protoc.LanguagePluginConfig{}, Outputs: []string{"b.p.k.rs"}}, + }, + } + + impl := protoRustLibrary{ + protoLibrariesByRule: make(map[label.Label][]protoc.ProtoLibrary), + } + rA := impl.ProvideRule(ruleCfg, pcA).Rule() + rB := impl.ProvideRule(ruleCfg, pcB).Rule(rA) + + if rA.Name() != "p_k__a" { + t.Errorf("first rule name: got %q, want %q", rA.Name(), "p_k__a") + } + if rB.Name() != "p_k__b" { + t.Errorf("second rule name: got %q, want %q", rB.Name(), "p_k__b") + } + if rA == rB { + t.Errorf("per-file rules must NOT merge — got the same *Rule pointer back from Rule(rA)") + } +} + +// TestProtoRustLibraryRule_PerFileMode_SkipsPackageProto verifies that when +// a per-file proto_library carries a shared `package.proto` co-include, the +// rule's `__` suffix is derived from the content-bearing file (not +// `package`). +func TestProtoRustLibraryRule_PerFileMode_SkipsPackageProto(t *testing.T) { + c := &config.Config{Exts: map[string]interface{}{}} + c.Exts["proto"] = &gproto.ProtoConfig{Mode: gproto.FileMode} + pkgCfg := protoc.NewPackageConfig(c) + ruleCfg := protoc.NewLanguageRuleConfig(c, "rust") + + // Note `package.proto` comes FIRST in the file list — the suffix must + // still resolve to `order`, the content-bearing file. + lib := protoc.NewOtherProtoLibrary(nil, rule.NewRule("proto_library", "order_proto"), + makeFile("p/k", "package.proto", `syntax = "proto3"; package p.k;`), + makeFile("p/k", "order.proto", `syntax = "proto3"; package p.k; message Order {}`), + ) + + pc := &protoc.ProtocConfiguration{ + PackageConfig: pkgCfg, + Rel: "p/k", + Library: lib, + Plugins: []*protoc.PluginConfiguration{ + {Config: &protoc.LanguagePluginConfig{}, Outputs: []string{"order.p.k.rs"}}, + }, + } + + impl := protoRustLibrary{ + protoLibrariesByRule: make(map[label.Label][]protoc.ProtoLibrary), + } + r := impl.ProvideRule(ruleCfg, pc).Rule() + if r.Name() != "p_k__order" { + t.Errorf("rule name: got %q, want %q", r.Name(), "p_k__order") + } +} + diff --git a/pkg/rule/rules_rust/rust_library.go b/pkg/rule/rules_rust/rust_library.go index 6335b73db..e62e3ca6e 100644 --- a/pkg/rule/rules_rust/rust_library.go +++ b/pkg/rule/rules_rust/rust_library.go @@ -27,6 +27,11 @@ var rustLibraryKindInfo = rule.KindInfo{ } // RustLibrary implements RuleProvider for 'proto_rust_library'-derived rules. +// +// In `gazelle:proto file` packages each per-file proto_library produces its +// own RustLibrary (see Name() — the rule name carries the file stem). The +// `proto_rust_library` Starlark macro is a plain rust_library wrapper; no +// per-file expansion happens at the macro layer. type RustLibrary struct { KindName string RuleNameSuffix string @@ -44,14 +49,42 @@ func (s *RustLibrary) Kind() string { } // Name implements part of the RuleProvider interface. +// +// In per-file mode the rule name carries the file stem suffix +// (`__`) so each per-file proto_library in the same proto +// package produces a uniquely-named rule. That makes the merge branch in +// `Rule()` a no-op for per-file callers and lets gazelle emit one +// rust_library per .proto file — the dep DAG then mirrors the +// (acyclic) proto-file import graph, dissolving the cross-package +// cycles that motivated the per-file scheme. +// +// In per-package mode the name is the proto package's sanitized crate +// name and merge across proto_libraries with the same package keeps +// working as before. func (s *RustLibrary) Name() string { - if pkg := s.Pkg(); pkg != "" { - return protoc.RustCrateName(pkg) + pkg := s.Pkg() + if pkg == "" { + return s.Config.Library.BaseName() + s.RuleNameSuffix } - return s.Config.Library.BaseName() + s.RuleNameSuffix + base := protoc.RustCrateName(pkg) + if !s.Config.IsProtoFileMode() { + return base + } + // Per-file proto_libraries typically contain one content-bearing + // .proto plus a shared co-included `package.proto` (no codegen). + // Suffix the rule name with the first content-bearing file's stem. + for _, f := range s.Config.Library.Files() { + if f.HasMessages() || f.HasEnums() || f.HasServices() { + return base + "__" + strings.TrimSuffix(f.Basename, ".proto") + } + } + return base } -// Pkg returns the proto package name from the first file in the library. +// Pkg returns the proto package name of the first file in the library, or "". +// Internal helper: drives the rule's crate name (via RustCrateName) and the +// reexport computation. Not propagated to the generated rule as an attribute — +// the consuming Starlark rule no longer accepts a `pkg` attr. func (s *RustLibrary) Pkg() string { files := s.Config.Library.Files() if len(files) == 0 { @@ -156,9 +189,6 @@ func (s *RustLibrary) Rule(otherGen ...*rule.Rule) *rule.Rule { newRule.SetPrivateAttr(config.GazelleImportsKey, imports) s.protoLibrariesByRule[s.id] = []protoc.ProtoLibrary{s.Config.Library} - if pkg := s.Pkg(); pkg != "" { - newRule.SetAttr("pkg", pkg) - } if len(deps) > 0 { newRule.SetAttr("deps", deps) } @@ -205,6 +235,16 @@ func (s *RustLibrary) Reexports() []string { // via the regular extern_path mechanism. continue } + // Skip parents that are in per-file mode. Their facade crate + // (impCrate) no longer exists, and the per-type extern_paths + // emitted by perFileCrossPackageTypeExternPaths route the + // parent-package references at the specific per-file crates + // — so the lib.rs `pub use ::::*;` re-export shim is + // neither needed nor satisfiable. Kind constant inlined to + // avoid a dep cycle on the prost plugin package. + if len(resolver.Resolve("proto", "prost_extern_per_file_type", impPkg)) > 0 { + continue + } entry := impCrate + "=" + impPkg if seen[entry] { continue diff --git a/rules/proto_compile.bzl b/rules/proto_compile.bzl index 793341ed4..451ef2bdc 100644 --- a/rules/proto_compile.bzl +++ b/rules/proto_compile.bzl @@ -159,8 +159,21 @@ def _proto_compile_impl(ctx): # const primary proto provider (for descriptor path resolution) primary_proto_info = proto_infos[0] - # const > plugins to be applied - plugins = [plugin[ProtoPluginInfo] for plugin in ctx.attr.plugins] + # const > plugins to be applied. Sort by plugin name + # so protoc invocation order is deterministic and independent of attr + # ordering. This matters when plugins use protoc's @@protoc_insertion_point + # mechanism — e.g. protoc-gen-tonic inserts client/server code at the + # `module` insertion point of protoc-gen-prost's {package}.rs output. If + # tonic runs before prost, the insertion target doesn't exist yet and + # protoc fails with "Tried to insert into file that doesn't exist" (and + # then "Tried to write the same file twice" when prost's subsequent + # create collides with the failed-insertion entry). Alphabetical sort + # puts protoc-gen-prost before protoc-gen-tonic, satisfying the + # producer-before-consumer ordering this protocol requires. + plugins = sorted( + [plugin[ProtoPluginInfo] for plugin in ctx.attr.plugins], + key = lambda p: p.name, + ) # const > outs = {_plugin_label_key(Label(k)): v for k, v in ctx.attr.outs.items()} @@ -195,17 +208,15 @@ def _proto_compile_impl(ctx): for plugin in plugins: ### Part 2.1: build protos list - # When using protos (plural), all ProtoInfo providers share the - # same package (that's why their outputs overlap). Only pass files - # from the first provider to protoc as file_to_generate — the - # descriptor sets from ALL providers are already included, giving - # the plugin full type information to generate the complete - # package output. This avoids duplicate CodeGeneratorResponse.File - # entries from package-level plugins like protoc-gen-prost. - gen_infos = [proto_infos[0]] if len(proto_infos) > 1 else proto_infos - - # add all protos unless excluded - for pi in gen_infos: + # All ProtoInfo providers (either the single `proto` attr or every + # entry in `protos`) contribute their direct_sources to protoc's + # file_to_generate list. protoc plugins emit code only for files + # explicitly listed there, so dropping providers here would silently + # truncate the generated output for package-level plugins like + # protoc-gen-prost (which generates per-file, not per-package). + # The inner `proto in protos` check deduplicates if a .proto somehow + # appears in multiple providers' direct_sources. + for pi in proto_infos: for proto in pi.direct_sources: if any([ proto.dirname.endswith(exclusion) or proto.path.endswith(exclusion) @@ -307,6 +318,15 @@ def _proto_compile_impl(ctx): replaced_args = _ctx_replace_args(ctx, _uniq(args)) final_args = ctx.actions.args() + + # protoc reads `@paramfile` with one arg per line and NO shell quoting. + # Bazel's default "shell" format wraps each arg in single quotes, which + # protoc treats as input filenames (the leading `'` defeats the `--flag` + # prefix match) and the action fails with "Missing output directives". + # The bug stays latent until the args list overflows the command line and + # bazel actually materializes the param file — typically only triggered + # once a rule accumulates many --xxx_opt= entries. + final_args.set_param_file_format("multiline") final_args.use_param_file("@%s", use_always = False) final_args.add_all(replaced_args)