From 06be737cacba6aa6973a068aa3602e0b3704750e Mon Sep 17 00:00:00 2001 From: Iain McGinniss <309153+iainmcgin@users.noreply.github.com> Date: Wed, 27 May 2026 03:37:34 +0000 Subject: [PATCH] codegen: fix silently-ignored per-type extern_path mappings (#111) (#153) extern_path entries naming a single type FQN (the prost/tonic idiom, e.g. .google.protobuf.Timestamp = ::pbjson_types::Timestamp) parsed but never matched, because resolution only considered package prefixes at file registration. Resolution now happens per type: an exact type-FQN entry wins over the internal descriptor.proto routing, then the longest matching package prefix, then local generation. Nested types inherit an enclosing message's override. Package-prefix mappings produce byte-identical output to before (regression-tested). Also documents the per-type form across the guide, prost migration notes, plugin help text, and Config::extern_path rustdoc, and adds the CHANGELOG entry. Co-authored-by: Omar Garcia <12629920+ogarciarevett@users.noreply.github.com> Co-authored-by: Iain McGinniss <309153+iainmcgin@users.noreply.github.com> --- CHANGELOG.md | 14 + buffa-build/src/lib.rs | 53 +- buffa-codegen/src/context.rs | 487 ++++++++++++++++-- buffa-codegen/src/lib.rs | 21 +- buffa-test/build.rs | 12 + buffa-test/protos/cross_package_pertype.proto | 19 + buffa-test/src/lib.rs | 5 + buffa-test/src/tests/cross_ref.rs | 30 ++ docs/guide.md | 26 +- docs/migration-from-prost.md | 2 +- protoc-gen-buffa/src/main.rs | 7 +- 11 files changed, 600 insertions(+), 76 deletions(-) create mode 100644 buffa-test/protos/cross_package_pertype.proto diff --git a/CHANGELOG.md b/CHANGELOG.md index ee863a1..e20dd5b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -81,6 +81,20 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), `buffa_build::Config::compile()` call — deconfliction cannot span separate compilations, since each only sees its own descriptor set. +- **Per-type `extern_path` mappings were silently ignored (#111).** An + `extern_path` entry naming a single type FQN (e.g. + `.extern_path(".google.protobuf.Timestamp", "::my_types::Timestamp")`, the + prost/tonic idiom) parsed but never matched, because resolution only + considered package prefixes. Type references now resolve per-type: an exact + type-FQN entry wins over the internal `descriptor.proto` routing, which wins + over the longest matching package prefix, which wins over local generation. + Nested types inherit an enclosing message's override, resolving to the + override's parent module plus the usual `snake_case(MessageName)` + nested-types module. Note that entries which previously had no effect now + take effect: a type-FQN entry (including a typo'd one) that was a silent + no-op before will now change the generated reference, and a wrong target + surfaces as a compile error in the generated code. + ## [0.6.0] - 2026-05-15 ### Added diff --git a/buffa-build/src/lib.rs b/buffa-build/src/lib.rs index 14bf2d1..dd573e2 100644 --- a/buffa-build/src/lib.rs +++ b/buffa-build/src/lib.rs @@ -451,21 +451,56 @@ impl Config { /// Declare an external type path mapping. /// - /// Types under the given protobuf path prefix will reference the specified - /// Rust module path instead of being generated. This allows shared proto - /// packages to be compiled once in a dedicated crate and referenced from - /// others. - /// - /// `proto_path` is a fully-qualified protobuf package path, e.g., - /// `".my.common"` or `"my.common"` (the leading dot is optional and will - /// be added automatically). `rust_path` is the Rust module path where - /// those types are accessible (e.g., `"::common_protos"`). + /// The matched types reference the specified Rust path instead of being + /// generated. This allows shared proto packages to be compiled once in a + /// dedicated crate and referenced from others. + /// + /// `proto_path` is a fully-qualified protobuf path — either a **package** + /// (`".my.common"`, mapping every type under it to a Rust module root) or a + /// single **type FQN** (`".google.protobuf.Timestamp"`, mapping just that + /// type, the prost/tonic idiom). The leading dot is optional and is added + /// automatically. As in prost, the most specific entry wins: an exact type + /// FQN beats a covering package prefix, which in turn beats a shorter + /// prefix. + /// + /// `rust_path` is where the type(s) are accessible — a module root for a + /// package mapping (e.g. `"::common_protos"`) or a full type path for a + /// per-type mapping (e.g. `"::pbjson_types::Timestamp"`). It must be an + /// absolute path (starting with `::` or `crate::`); any other value is + /// emitted into the generated code verbatim and will fail to resolve there. + /// + /// **Nested types** inherit an enclosing message's per-type override: + /// mapping `.my.pkg.Outer` to `::ext::Outer` resolves `.my.pkg.Outer.Inner` + /// to `::ext::outer::Inner` — the override's parent module plus buffa's + /// usual `snake_case(MessageName)` nested-types module (snake case of the + /// *proto* message name, regardless of the override's final segment). This + /// matches the layout of another buffa-generated crate; for a target crate + /// laid out differently, add explicit per-type entries for the nested types + /// as well. + /// + /// # Limitations + /// + /// An extern type that is referenced by a generated **view** must map to + /// another buffa-generated crate — the view path is composed as + /// `::__buffa::view::…`, which a non-buffa crate (e.g. + /// `pbjson_types`) does not provide. Map per-type to a buffa crate, or + /// disable views ([`generate_views(false)`](Self::generate_views)), for + /// such types. + /// + /// A misconfigured mapping (a typo'd FQN target, a non-absolute + /// `rust_path`, or a view-referenced type mapped to a non-buffa crate) is + /// not diagnosed at generation time; it surfaces as an unresolved-path + /// error when the generated code is compiled. /// /// # Example /// /// ```rust,ignore /// buffa_build::Config::new() + /// // Whole-package mapping. /// .extern_path(".my.common", "::common_protos") + /// // Per-type mapping (issue #111) — overrides the package prefix for + /// // just this type. + /// .extern_path(".google.protobuf.Timestamp", "::common_protos::well_known::Timestamp") /// .files(&["proto/my_service.proto"]) /// .includes(&["proto/"]) /// .compile() diff --git a/buffa-codegen/src/context.rs b/buffa-codegen/src/context.rs index fe8b3cd..ceac3f2 100644 --- a/buffa-codegen/src/context.rs +++ b/buffa-codegen/src/context.rs @@ -203,18 +203,23 @@ impl<'a> CodeGenContext<'a> { /// /// `file_extern_paths` maps a `.proto` file's full path (as reported in /// `FileDescriptorProto.name`, e.g. `"google/protobuf/descriptor.proto"`) - /// to a Rust module root. It takes priority over package-level - /// `effective_extern_paths`, which lets two files in the same proto + /// to a Rust module root. It takes priority over a package-level + /// `effective_extern_paths` match, which lets two files in the same proto /// package (`google.protobuf`) resolve to different external crates — /// `descriptor.proto` types to `buffa-descriptor`, every other /// `google.protobuf` WKT to `buffa-types`. Without this split a single /// package-keyed mapping would route everything to one crate or the /// other. /// + /// Per-type resolution priority (issue #111), most specific first: an + /// **exact** `extern_path` entry for a type's FQN, then the **file-level** + /// mapping above, then a **package/prefix** `extern_path` match, then the + /// **local** package path. See [`resolve_type_path`]. + /// /// File-level mappings are an internal mechanism used for the /// auto-injected descriptor-types routing. They are not part of the - /// public `CodeGenConfig` API; user-facing `extern_path` entries remain - /// package-prefix keyed. + /// public `CodeGenConfig` API; user-facing `extern_path` entries are keyed + /// by proto package *or* type FQN. pub(crate) fn with_extern_resolution( files: &'a [FileDescriptorProto], config: &'a CodeGenConfig, @@ -248,6 +253,20 @@ impl<'a> CodeGenContext<'a> { all_packages.insert(package.to_string()); for msg in &file.message_type { if let Some(name) = &msg.name { + // A per-type `extern_path` override (issue #111) makes the + // message extern: it emits no local module, so it must not + // reserve a module name in sub-package deconfliction (#135). + let fqn = if package.is_empty() { + format!(".{name}") + } else { + format!(".{package}.{name}") + }; + if effective_extern_paths + .iter() + .any(|(proto, _)| proto == &fqn) + { + continue; + } pkg_message_names .entry(package.to_string()) .or_default() @@ -283,48 +302,55 @@ impl<'a> CodeGenContext<'a> { format!(".{}.", package) }; - // Resolution priority: file-level extern → package-level extern → - // local module path. The file-level check must come first so two + // The file-level extern root (the internal descriptor.proto → + // buffa-descriptor split) and the local package module. Per-type + // resolution (issue #111) layers exact/longest-prefix `extern_path` + // matching on top, via `resolve_type_path`, which preserves the + // historic priority: per-type exact → file-level → package prefix → + // local. The file-level check still outranks a package prefix so two // files in the same proto package can route to different crates // (descriptor.proto → buffa-descriptor, timestamp.proto → // buffa-types). - let rust_module = if let Some(rust_root) = file + let file_root = file .name .as_deref() - .and_then(|n| resolve_file_extern(n, file_extern_paths)) - { - rust_root.to_string() - } else if let Some(rust_root) = resolve_extern_prefix(package, effective_extern_paths) { - rust_root - } else { - package.replace('.', "::") - }; + .and_then(|n| resolve_file_extern(n, file_extern_paths)); + let local_module = package.replace('.', "::"); // Register top-level messages for msg in &file.message_type { if let Some(name) = &msg.name { let fqn = format!("{}{}", proto_prefix, name); - let rust_path = if rust_module.is_empty() { - name.clone() + let (rust_path, is_extern) = resolve_type_path( + &fqn, + name, + file_root, + &local_module, + effective_extern_paths, + ); + + // The module the message's nested types live in. For a local + // message it is `::`, where the module name + // is deconflicted against sub-package modules (issue #135), + // precomputed above and looked up here so emission and + // references share the same value. For an extern/overridden + // message no local module is emitted, so the nested module is + // the resolved path's parent plus the plain `snake_case` name. + let parent_mod = if is_extern { + match rust_path.rsplit_once("::") { + Some((parent, _)) => format!("{parent}::{}", to_snake_case(name)), + None => to_snake_case(name), + } } else { - format!("{}::{}", rust_module, name) + let snake = nested_module_names + .get(&fqn) + .cloned() + .unwrap_or_else(|| to_snake_case(name)); + join_mod(&local_module, &snake) }; + type_map.insert(fqn.clone(), rust_path); package_of.insert(fqn.clone(), package.to_string()); - - // Register nested messages using module-qualified paths. - // The parent module name is deconflicted against sub-package - // modules (issue #135), precomputed above and looked up here - // so emission and references share the same value. - let snake = nested_module_names - .get(&fqn) - .cloned() - .unwrap_or_else(|| to_snake_case(name)); - let parent_mod = if rust_module.is_empty() { - snake - } else { - format!("{}::{}", rust_module, snake) - }; register_nested_types( &mut type_map, &mut package_of, @@ -332,6 +358,7 @@ impl<'a> CodeGenContext<'a> { &fqn, &parent_mod, msg, + effective_extern_paths, ); register_nested_enum_closedness( &mut enum_closedness, @@ -346,11 +373,13 @@ impl<'a> CodeGenContext<'a> { for enum_type in &file.enum_type { if let Some(name) = &enum_type.name { let fqn = format!("{}{}", proto_prefix, name); - let rust_path = if rust_module.is_empty() { - name.clone() - } else { - format!("{}::{}", rust_module, name) - }; + let (rust_path, _) = resolve_type_path( + &fqn, + name, + file_root, + &local_module, + effective_extern_paths, + ); type_map.insert(fqn.clone(), rust_path); package_of.insert(fqn.clone(), package.to_string()); register_enum_closedness(&mut enum_closedness, &fqn, &file_features, enum_type); @@ -598,18 +627,19 @@ impl<'a> CodeGenContext<'a> { }; // within_proto is dotted (e.g. "Outer.Inner"); within full_path // it's `outer::Inner` (snake_case modules + final PascalCase). - // Count the segments and strip that many from full_path. + // Count the segments and strip that many from full_path to recover + // the module the type lives in. + // + // For paths buffa builds itself (`::`) and for + // per-type `extern_path` overrides whose target mirrors the proto + // nesting (the sensible case — e.g. mapping to another + // buffa-generated crate), `full_segs.len() >= within_segs`. A + // pathological override that maps a deeply-nested type to a shorter + // Rust path can't have a matching `__buffa::` view/oneof tree + // anyway, so we clamp with `saturating_sub` rather than panic and + // let the (unresolvable) reference surface as a normal compile error. let within_segs = within_proto.split('.').count(); let full_segs: Vec<&str> = full_path.split("::").collect(); - // Invariant: `full_path` was built by `CodeGenContext::new` as - // `::`, so it always has at least - // `within_segs` trailing segments. If this fires the type_map - // and package_of maps are out of sync. - debug_assert!( - full_segs.len() >= within_segs, - "extern path '{full_path}' has fewer segments than \ - within-package proto path '{within_proto}'" - ); let cut = full_segs.len().saturating_sub(within_segs); full_segs[..cut].join("::") } else { @@ -939,10 +969,118 @@ pub(crate) fn resolve_extern_prefix( Some(format!("{}::{}", rust_prefix, suffix)) } +/// Resolve a fully-qualified proto **type** name against `extern_paths`, +/// mirroring prost's `resolve_ident` (issue #111). +/// +/// Unlike [`resolve_extern_prefix`] — which only matches a file's *package* and +/// returns the package's Rust module — this matches the type's whole FQN: +/// +/// 1. An **exact** `extern_path` entry for the FQN wins (a per-type override, +/// e.g. `.google.protobuf.Timestamp = ::pbjson_types::Timestamp`). +/// 2. Otherwise the **longest dotted-prefix** entry (a package or an enclosing +/// type) applies, with the proto segments past that prefix rendered as +/// `snake_case` modules and the final segment kept as the Rust type name — +/// exactly the path [`CodeGenContext::new`] would otherwise build from +/// [`resolve_extern_prefix`] plus the type name, so package-prefix mappings +/// resolve identically to before. +/// +/// `fqn` is the leading-dot form (e.g. `.google.protobuf.Timestamp`). Returns +/// the full Rust type path, or `None` when nothing matches (the caller falls +/// back to the local package path). +fn resolve_extern_type(fqn: &str, extern_paths: &[(String, String)]) -> Option { + // 1. Exact per-type entry — the mapping *is* the full Rust path. + if let Some((_, rust)) = extern_paths.iter().find(|(proto, _)| proto == fqn) { + return Some(rust.clone()); + } + + // 2. Longest dotted-prefix entry (package or enclosing type). + let mut best: Option<(&str, &str, usize)> = None; + for (proto_prefix, rust_prefix) in extern_paths { + let matches = proto_prefix == "." + || fqn + .strip_prefix(proto_prefix.as_str()) + .is_some_and(|rest| rest.starts_with('.')); + if matches && best.is_none_or(|(_, _, best_len)| proto_prefix.len() > best_len) { + best = Some((proto_prefix, rust_prefix, proto_prefix.len())); + } + } + + let (proto_prefix, rust_prefix, _) = best?; + // The catch-all `.` leaves the whole FQN (minus its leading dot); any other + // prefix leaves the `.`-separated remainder after the matched boundary. + let rest = if proto_prefix == "." { + fqn.strip_prefix('.').unwrap_or(fqn) + } else { + fqn.strip_prefix(proto_prefix) + .and_then(|r| r.strip_prefix('.')) + .unwrap_or("") + }; + let mut segments = rest.split('.').collect::>(); + // The final segment is the type name (kept verbatim); the rest are modules. + let type_name = segments.pop()?; + let mut path = rust_prefix.to_string(); + for module in segments { + path.push_str("::"); + path.push_str(&to_snake_case(module)); + } + path.push_str("::"); + path.push_str(type_name); + Some(path) +} + +/// Join a Rust module path and a type name, handling the empty (no-package) +/// module by returning the bare type name. +fn join_mod(module: &str, name: &str) -> String { + if module.is_empty() { + name.to_string() + } else { + format!("{module}::{name}") + } +} + +/// Resolve a registered type's Rust path, applying per-type `extern_path` +/// overrides (issue #111). +/// +/// Priority, most specific first: an **exact** per-type FQN entry, then a +/// **file-level** mapping (the internal `descriptor.proto` → `buffa-descriptor` +/// split, which must outrank a package prefix), then the **longest +/// dotted-prefix** entry (package or enclosing type, via [`resolve_extern_type`]), +/// then the **local** package path. +/// +/// Returns `(rust_path, is_extern)`; `is_extern` is `false` only for the local +/// fallback, telling the caller whether the type emits a local module (and thus +/// participates in sub-package deconfliction, issue #135). +fn resolve_type_path( + fqn: &str, + name: &str, + file_root: Option<&str>, + local_module: &str, + extern_paths: &[(String, String)], +) -> (String, bool) { + // The exact check is done here (not left to `resolve_extern_type`) so an + // exact per-type entry outranks the file-level mapping below; once it has + // failed, `resolve_extern_type`'s own exact pass can only fall through to + // its prefix logic. + if let Some((_, rust)) = extern_paths.iter().find(|(proto, _)| proto == fqn) { + (rust.clone(), true) + } else if let Some(root) = file_root { + (join_mod(root, name), true) + } else if let Some(path) = resolve_extern_type(fqn, extern_paths) { + (path, true) + } else { + (join_mod(local_module, name), false) + } +} + /// Recursively register nested messages and enums with module-qualified paths. /// /// Each nested message `Parent.Child` maps to `parent_mod::Child` in Rust, /// where `parent_mod` is the snake_case module path of the enclosing message. +/// +/// A per-type `extern_path` override (issue #111) on a nested type's own FQN +/// takes priority over the inherited `parent_mod` path; otherwise the nested +/// type simply lives in `parent_mod` — which already reflects any override of an +/// enclosing type, so a parent override cascades to its descendants. fn register_nested_types( type_map: &mut HashMap, package_of: &mut HashMap, @@ -950,24 +1088,52 @@ fn register_nested_types( parent_fqn: &str, parent_mod: &str, msg: &crate::generated::descriptor::DescriptorProto, + extern_paths: &[(String, String)], ) { for nested in &msg.nested_type { if let Some(name) = &nested.name { let fqn = format!("{}.{}", parent_fqn, name); - let rust_path = format!("{}::{}", parent_mod, name); + // An exact per-type override wins; the child module is then the + // override's parent plus the plain snake_case name. Otherwise the + // type lives in `parent_mod`. + let (rust_path, child_mod) = match extern_paths.iter().find(|(proto, _)| proto == &fqn) + { + Some((_, rust)) => { + let child = match rust.rsplit_once("::") { + Some((parent, _)) => format!("{parent}::{}", to_snake_case(name)), + None => to_snake_case(name), + }; + (rust.clone(), child) + } + None => ( + format!("{parent_mod}::{name}"), + format!("{parent_mod}::{}", to_snake_case(name)), + ), + }; type_map.insert(fqn.clone(), rust_path); package_of.insert(fqn.clone(), package.to_string()); // Recurse: nested-of-nested goes in a deeper module. - let child_mod = format!("{}::{}", parent_mod, to_snake_case(name)); - register_nested_types(type_map, package_of, package, &fqn, &child_mod, nested); + register_nested_types( + type_map, + package_of, + package, + &fqn, + &child_mod, + nested, + extern_paths, + ); } } for enum_type in &msg.enum_type { if let Some(name) = &enum_type.name { let fqn = format!("{}.{}", parent_fqn, name); - let rust_path = format!("{}::{}", parent_mod, name); + let rust_path = extern_paths + .iter() + .find(|(proto, _)| proto == &fqn) + .map(|(_, rust)| rust.clone()) + .unwrap_or_else(|| format!("{parent_mod}::{name}")); type_map.insert(fqn.clone(), rust_path); package_of.insert(fqn, package.to_string()); } @@ -1292,6 +1458,129 @@ mod tests { assert_eq!(ctx.rust_type(".ns.b.MsgB"), Some("ns::b::MsgB")); } + // ── Per-type FQN extern_path resolution (issue #111) ────────────────── + // + // `extern_path` historically matched only at the package-prefix level, so + // a mapping naming a concrete type FQN (e.g. + // `.google.protobuf.Timestamp=::pbjson_types::Timestamp`, a normal + // prost/tonic idiom) was silently ignored. These tests pin the prost + // `resolve_ident` behavior: an exact type FQN entry wins, otherwise the + // longest dotted-prefix (package or enclosing type) applies. + + #[test] + fn test_extern_path_exact_per_type_match() { + let files = [make_file( + "test.proto", + "test.pkg", + vec![msg("Msg")], + vec![], + )]; + let config = CodeGenConfig { + extern_paths: vec![(".test.pkg.Msg".into(), "::ext_crate::Msg".into())], + ..Default::default() + }; + let ctx = CodeGenContext::new(&files, &config, &config.extern_paths); + // An exact per-type FQN mapping must be honored, not silently dropped. + assert_eq!(ctx.rust_type(".test.pkg.Msg"), Some("::ext_crate::Msg")); + } + + #[test] + fn test_extern_path_per_type_overrides_package_prefix() { + // Package-level mapping covers the whole package; an exact per-type + // entry overrides it for that one type (exact-match-first), while a + // sibling still resolves via the package prefix. + let files = [make_file( + "test.proto", + "test.pkg", + vec![msg("Msg"), msg("Other")], + vec![], + )]; + let config = CodeGenConfig { + extern_paths: vec![ + (".test.pkg".into(), "::pkg_crate".into()), + (".test.pkg.Msg".into(), "::ext_crate::Msg".into()), + ], + ..Default::default() + }; + let ctx = CodeGenContext::new(&files, &config, &config.extern_paths); + assert_eq!(ctx.rust_type(".test.pkg.Msg"), Some("::ext_crate::Msg")); + assert_eq!(ctx.rust_type(".test.pkg.Other"), Some("::pkg_crate::Other")); + } + + #[test] + fn test_extern_path_nested_type_inherits_per_type_override() { + // A per-type override of a parent message cascades to its nested + // types via the snake_case module of the overridden path. + let outer = msg_with_nested("Outer", vec![msg("Inner")]); + let files = [make_file("test.proto", "test.pkg", vec![outer], vec![])]; + let config = CodeGenConfig { + extern_paths: vec![(".test.pkg.Outer".into(), "::ext::Outer".into())], + ..Default::default() + }; + let ctx = CodeGenContext::new(&files, &config, &config.extern_paths); + assert_eq!(ctx.rust_type(".test.pkg.Outer"), Some("::ext::Outer")); + assert_eq!( + ctx.rust_type(".test.pkg.Outer.Inner"), + Some("::ext::outer::Inner") + ); + } + + #[test] + fn test_extern_path_exact_per_type_enum() { + let files = [make_file( + "test.proto", + "test.pkg", + vec![], + vec![enum_desc("Status")], + )]; + let config = CodeGenConfig { + extern_paths: vec![(".test.pkg.Status".into(), "::ext_crate::Status".into())], + ..Default::default() + }; + let ctx = CodeGenContext::new(&files, &config, &config.extern_paths); + assert_eq!( + ctx.rust_type(".test.pkg.Status"), + Some("::ext_crate::Status") + ); + } + + #[test] + fn test_extern_path_package_prefix_still_resolves() { + // Guard: package-prefix mappings (the only historically-supported + // form) must keep resolving exactly as before. + let files = [make_file( + "test.proto", + "test.pkg", + vec![msg("Msg")], + vec![], + )]; + let config = CodeGenConfig { + extern_paths: vec![(".test.pkg".into(), "::pkg_crate".into())], + ..Default::default() + }; + let ctx = CodeGenContext::new(&files, &config, &config.extern_paths); + assert_eq!(ctx.rust_type(".test.pkg.Msg"), Some("::pkg_crate::Msg")); + } + + #[test] + fn test_extern_path_per_type_does_not_affect_unmapped_type() { + // Guard: a per-type entry must not leak onto an unrelated type. + let files = [make_file( + "test.proto", + "test.pkg", + vec![msg("Msg"), msg("Other")], + vec![], + )]; + let config = CodeGenConfig { + extern_paths: vec![(".test.pkg.Msg".into(), "::ext_crate::Msg".into())], + ..Default::default() + }; + let ctx = CodeGenContext::new(&files, &config, &config.extern_paths); + assert_eq!(ctx.rust_type(".test.pkg.Msg"), Some("::ext_crate::Msg")); + // `Other` has no entry — resolves locally. + assert_eq!(ctx.rust_type(".test.pkg.Other"), Some("test::pkg::Other")); + } + #[test] fn test_keyword_package_segment_in_type_map() { // Proto package `google.type` — the type map stores plain string paths. @@ -1655,6 +1944,70 @@ mod tests { assert_eq!(result, Some("crate::proto::google::type".into())); } + // ── resolve_extern_type — per-type FQN resolution (issue #111) ───────── + + #[test] + fn test_resolve_extern_type_exact_match() { + // An exact entry for the type FQN is the full Rust path verbatim. + let result = resolve_extern_type( + ".google.protobuf.Timestamp", + &[( + ".google.protobuf.Timestamp".into(), + "::pbjson_types::Timestamp".into(), + )], + ); + assert_eq!(result, Some("::pbjson_types::Timestamp".into())); + } + + #[test] + fn test_resolve_extern_type_exact_wins_over_prefix() { + // Exact-match-first: the per-type entry beats a covering package entry. + let result = resolve_extern_type( + ".my.pkg.Msg", + &[ + (".my.pkg".into(), "::pkg_crate".into()), + (".my.pkg.Msg".into(), "::ext_crate::Msg".into()), + ], + ); + assert_eq!(result, Some("::ext_crate::Msg".into())); + } + + #[test] + fn test_resolve_extern_type_package_prefix_appends_type() { + // A package prefix yields `::::`, + // matching what resolve_extern_prefix + the type name would build. + let result = resolve_extern_type( + ".my.common.sub.Msg", + &[(".my.common".into(), "::common_protos".into())], + ); + assert_eq!(result, Some("::common_protos::sub::Msg".into())); + } + + #[test] + fn test_resolve_extern_type_catchall() { + let result = resolve_extern_type(".greet.v1.Hello", &[(".".into(), "crate::proto".into())]); + assert_eq!(result, Some("crate::proto::greet::v1::Hello".into())); + } + + #[test] + fn test_resolve_extern_type_no_match() { + let result = resolve_extern_type( + ".other.pkg.Msg", + &[(".my.common".into(), "::common_protos".into())], + ); + assert_eq!(result, None); + } + + #[test] + fn test_resolve_extern_type_partial_name_no_match() { + // ".my.common" must not match ".my.commonext.Msg" (dot-boundary). + let result = resolve_extern_type( + ".my.commonext.Msg", + &[(".my.common".into(), "::common_protos".into())], + ); + assert_eq!(result, None); + } + // ── rust_type_relative_split — extern branch ──────────────────────── #[test] @@ -1710,6 +2063,38 @@ mod tests { assert_eq!(split.within_package, "value::Inner"); } + #[test] + fn test_split_per_type_extern_override() { + // Per-type override (issue #111): the `__buffa::` boundary recovery must + // still split the override path at the package/within-package seam, so + // callers compose `::__buffa::view::` + // correctly. Both the overridden type and its nested children are + // exercised. + let outer = msg_with_nested("Outer", vec![msg("Inner")]); + let files = [make_file("custom.proto", "my.pkg", vec![outer], vec![])]; + let config = CodeGenConfig { + extern_paths: vec![(".my.pkg.Outer".into(), "::ext::custom::Outer".into())], + ..Default::default() + }; + let ctx = CodeGenContext::new(&files, &config, &config.extern_paths); + + let split = ctx + .rust_type_relative_split(".my.pkg.Outer", "other.pkg", 2) + .expect("overridden type resolves"); + assert!(split.is_extern); + assert_eq!(split.to_package, "::ext::custom"); + assert_eq!(split.within_package, "Outer"); + + // The nested type inherits the override's module, and the seam is + // recovered the same way (2 within-package segments stripped). + let nested = ctx + .rust_type_relative_split(".my.pkg.Outer.Inner", "other.pkg", 0) + .expect("nested type resolves"); + assert!(nested.is_extern); + assert_eq!(nested.to_package, "::ext::custom"); + assert_eq!(nested.within_package, "outer::Inner"); + } + #[test] fn test_extern_path_top_level_message() { let files = [make_file( diff --git a/buffa-codegen/src/lib.rs b/buffa-codegen/src/lib.rs index f924798..f439b41 100644 --- a/buffa-codegen/src/lib.rs +++ b/buffa-codegen/src/lib.rs @@ -307,11 +307,15 @@ pub struct CodeGenConfig { pub generate_arbitrary: bool, /// External type path mappings. /// - /// Each entry maps a fully-qualified protobuf path prefix (e.g., - /// `".my.common"`) to a Rust module path (e.g., `"::common_protos"`). - /// Types under the proto prefix will reference the extern Rust path - /// instead of being generated, allowing shared proto packages to be - /// compiled once in a dedicated crate and referenced from others. + /// Each entry maps either a fully-qualified protobuf package prefix + /// (e.g., `".my.common"`) to a Rust module path (e.g., + /// `"::common_protos"`), or a single type FQN (e.g., + /// `".my.common.Shared"`) to a full Rust type path (e.g., + /// `"::shared_types::Shared"`). Matched types reference the extern Rust + /// path instead of being generated, allowing shared proto packages to be + /// compiled once in a dedicated crate and referenced from others. An + /// exact type-FQN entry wins over a covering package prefix; otherwise + /// the longest matching prefix wins. /// /// Well-known types (`google.protobuf.*`) are automatically mapped to /// `::buffa_types::google::protobuf::*` without needing an explicit @@ -704,9 +708,10 @@ pub(crate) fn effective_extern_paths( /// must resolve to the local module, not externally. /// /// Currently internal-only — there is no `CodeGenConfig` field for -/// user-provided file-level mappings. The user-facing `extern_path` API -/// remains package-prefix keyed; per-file or per-type overrides may be added -/// later as a public feature if a concrete need arises. +/// user-provided *file-level* mappings. The user-facing `extern_path` API is +/// keyed by proto package *or* type FQN (per-type overrides, issue #111); +/// per-file overrides may be added later as a public feature if a concrete +/// need arises. pub(crate) fn effective_file_extern_paths( files_to_generate: &[String], config: &CodeGenConfig, diff --git a/buffa-test/build.rs b/buffa-test/build.rs index 10d29db..cff6863 100644 --- a/buffa-test/build.rs +++ b/buffa-test/build.rs @@ -182,6 +182,18 @@ fn main() { .compile() .expect("buffa_build failed for cross_package.proto"); + // Per-type extern_path references (issue #111) — maps individual type + // FQNs (.basic.Person, .basic.Status) to crate::basic, rather than the + // whole `.basic` package. Exercises exact-FQN resolution end-to-end. + buffa_build::Config::new() + .files(&["protos/cross_package_pertype.proto"]) + .includes(&["protos/"]) + .extern_path(".basic.Person", "crate::basic::Person") + .extern_path(".basic.Status", "crate::basic::Status") + .generate_views(false) + .compile() + .expect("buffa_build failed for cross_package_pertype.proto"); + // Cross-syntax import: proto2 file using a proto3-declared enum. // Spec: enum closedness follows the DECLARING file's syntax, so the // proto3 enum stays open even when referenced from proto2. diff --git a/buffa-test/protos/cross_package_pertype.proto b/buffa-test/protos/cross_package_pertype.proto new file mode 100644 index 0000000..22f99fa --- /dev/null +++ b/buffa-test/protos/cross_package_pertype.proto @@ -0,0 +1,19 @@ +// Per-type `extern_path` resolution (issue #111). Unlike cross_package.proto +// — which maps whole packages (`.basic` -> crate::basic) — this file's build +// config maps individual type FQNs (`.basic.Person`, `.basic.Status`) to the +// already-generated `crate::basic` types. Historically those per-type mappings +// were silently ignored and the references resolved to non-existent local +// modules; now they resolve to the mapped Rust paths. + +syntax = "proto3"; + +package test.cross_pertype; + +import "basic.proto"; + +message PerTypeComposite { + // Mapped by exact type FQN to crate::basic::Person. + basic.Person person = 1; + // Enum mapped by exact type FQN to crate::basic::Status. + basic.Status status = 2; +} diff --git a/buffa-test/src/lib.rs b/buffa-test/src/lib.rs index b75edf7..75ff634 100644 --- a/buffa-test/src/lib.rs +++ b/buffa-test/src/lib.rs @@ -69,6 +69,11 @@ pub mod cross_syntax { buffa::include_proto!("test.cross_syntax"); } +#[allow(clippy::derivable_impls, clippy::match_single_binding)] +pub mod cross_pertype { + buffa::include_proto!("test.cross_pertype"); +} + #[allow(clippy::derivable_impls, clippy::match_single_binding)] pub mod collisions { buffa::include_proto!("test.collisions"); diff --git a/buffa-test/src/tests/cross_ref.rs b/buffa-test/src/tests/cross_ref.rs index c1c5ed3..c695e04 100644 --- a/buffa-test/src/tests/cross_ref.rs +++ b/buffa-test/src/tests/cross_ref.rs @@ -30,6 +30,36 @@ fn test_cross_package_round_trip() { assert_eq!(decoded.tree.name, "root"); } +#[test] +fn test_per_type_extern_path_round_trip() { + // Per-type `extern_path` (issue #111): the build maps `.basic.Person` and + // `.basic.Status` by exact FQN to crate::basic. If those mappings were + // ignored (the old behavior) the generated references would not compile; + // that this builds and round-trips proves per-type resolution works. + use crate::cross_pertype::PerTypeComposite; + // The field types below are crate::basic::Person / crate::basic::Status: + // this would not compile if the per-type mappings resolved to a local + // (non-existent) module instead. + let msg = PerTypeComposite { + person: buffa::MessageField::some(Person { + id: 7, + name: "Bob".into(), + status: buffa::EnumValue::Known(Status::ACTIVE), + ..Default::default() + }), + status: buffa::EnumValue::Known(Status::INACTIVE), + ..Default::default() + }; + let decoded = round_trip(&msg); + assert_eq!(decoded.person.id, 7); + assert_eq!(decoded.person.name, "Bob"); + assert_eq!( + decoded.person.status, + buffa::EnumValue::Known(Status::ACTIVE) + ); + assert_eq!(decoded.status, buffa::EnumValue::Known(Status::INACTIVE)); +} + #[test] fn test_cross_syntax_proto3_enum_in_proto2_is_open() { // Spec (protobuf.dev/programming-guides/enum): enum closedness diff --git a/docs/guide.md b/docs/guide.md index c585454..6fd3a5c 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -181,7 +181,7 @@ The macro pulls in `OUT_DIR/.mod.rs`, which in turn includes the per | `.generate_arbitrary(bool)` | `false` | Emit `#[derive(arbitrary::Arbitrary)]` gated behind the `arbitrary` feature (for fuzzing) | | `.gate_impls_on_crate_features(bool)` | `false` | Wrap json/views/text impls in `#[cfg(feature = ...)]` for library crates whose generated code is a public dependency surface | | `.strict_utf8_mapping(bool)` | `false` | Map `utf8_validation = NONE` string fields to `Vec` / `&[u8]` instead of `String` (see [Skipping UTF-8 validation](#skipping-utf-8-validation)) | -| `.extern_path(proto, rust)` | — | Map a proto package to an external Rust crate (see below) | +| `.extern_path(proto, rust)` | — | Map a proto package or a single type to an external Rust path (see below) | | `.use_bytes_type()` | — | Use `bytes::Bytes` for all bytes fields | | `.use_bytes_type_in(&[...])` | — | Use `bytes::Bytes` for matching bytes fields | | `.use_buf()` | — | Use `buf build` instead of `protoc` for descriptor generation | @@ -252,9 +252,25 @@ buffa_build::Config::new() With this configuration, any reference to a type like `my.common.SharedMessage` in `my_service.proto` will generate `::common_protos::SharedMessage` instead of a locally-generated struct. -The proto path must start with `.` (fully qualified), though the leading dot is optional and will be added automatically. When multiple extern paths match, the longest prefix wins. +The proto path must start with `.` (fully qualified), though the leading dot is optional and will be added automatically. -**View types:** When view generation is enabled (the default), the codegen also expects a `FooView<'a>` type at `::__buffa::view::FooView` for each extern-mapped message `Foo`. If you're using extern_path to reference types from another buffa-generated crate, the views are already there. If you're mapping to [custom type implementations](#custom-type-implementations), see that section for how to provide the view type. +**Per-type mappings:** an entry may also name a single type instead of a package — the prost/tonic idiom for overriding individual types while the rest of the package generates (or routes) as usual: + +```rust,ignore +buffa_build::Config::new() + // Whole-package mapping. + .extern_path(".my.common", "::common_protos") + // Per-type mapping: just this type; other my.common types still come from common_protos. + .extern_path(".my.common.SharedMessage", "::shared_types::SharedMessage") + .files(&["proto/my_service.proto"]) + .includes(&["proto/"]) + .compile() + .unwrap(); +``` + +When several entries could match a reference, the most specific one wins: an exact type FQN beats a covering package prefix, and a longer package prefix beats a shorter one. Nested types inherit an enclosing message's per-type override — `my.common.SharedMessage.Inner` resolves to `::shared_types::shared_message::Inner`, i.e. the override's parent module plus buffa's usual `snake_case(MessageName)` nested-types module. That layout matches another buffa-generated crate; if the target crate lays out nested types differently, add explicit per-type entries for the nested types as well. + +**View types:** When view generation is enabled (the default), the codegen also expects a `FooView<'a>` type at `::__buffa::view::FooView` for each extern-mapped message `Foo`. If you're using extern_path to reference types from another buffa-generated crate, the views are already there. If you're mapping to [custom type implementations](#custom-type-implementations), see that section for how to provide the view type. This applies to per-type mappings too: a message referenced by generated views must map to a buffa-generated crate, or view generation must be disabled (`.generate_views(false)`). ### Multi-package projects @@ -474,7 +490,7 @@ Passed via `opt:` (works for `remote:` and `local:`): | `arbitrary=true` | Emit `#[derive(arbitrary::Arbitrary)]` for fuzzing | | `gate_impls=true` | Wrap json/views/text impls in `#[cfg(feature = ...)]` for library crates whose generated code is a public dependency surface (default: emitted unconditionally) | | `with_setters=false` | Disable `with_()` builder-style setters for explicit-presence fields (default: emitted) | -| `extern_path=.pkg=::rust` | Map a proto package to an external Rust path | +| `extern_path=.pkg=::rust` | Map a proto package — or a single type, e.g. `extern_path=.pkg.Type=::rust::Type` — to an external Rust path | | `file_per_package=true` | Emit one `.rs` per package instead of per-proto-file content + a `.mod.rs` stitcher. Use this with the remote plugin when you don't want to install `protoc-gen-buffa-packaging` — see [Remote plugin only](#remote-plugin-only-no-local-install). Under `strategy: directory`, requires the input module to be `PACKAGE_DIRECTORY_MATCH`-clean. | #### BSR-generated SDKs @@ -503,7 +519,7 @@ If you prefer to use `protoc` without buf: ```sh protoc --buffa_out=. --plugin=protoc-gen-buffa my_service.proto -# With extern_path: +# With extern_path (package-level or per-type): protoc --buffa_out=. \ --buffa_opt=extern_path=.my.common=::common_protos \ --plugin=protoc-gen-buffa my_service.proto diff --git a/docs/migration-from-prost.md b/docs/migration-from-prost.md index 064e066..ed59adf 100644 --- a/docs/migration-from-prost.md +++ b/docs/migration-from-prost.md @@ -279,7 +279,7 @@ Features that prost supports but buffa does not (yet): |---------------|-------------| | `btree_map(&[...])` | Not supported. Maps always use `HashMap`. | | `bytes(&[...])` | Supported. `.use_bytes_type()` for all, or `.use_bytes_type_in(&[...])` for specific fields. | -| `extern_path(proto, rust)` | Supported. Same API: `.extern_path(".pkg", "::crate")`. | +| `extern_path(proto, rust)` | Supported. Same API, both package-level (`.extern_path(".pkg", "::crate")`) and per-type (`.extern_path(".google.protobuf.Timestamp", "::pbjson_types::Timestamp")`) mappings. A per-type mapping to a non-buffa crate requires `.generate_views(false)`, or map to a buffa-generated crate instead — see [External type paths](guide.md#external-type-paths). | | `type_attribute(path, attr)` | Not supported. Use `generate_json(true)` for serde. | | `field_attribute(path, attr)` | Not supported. | | `service_generator(...)` | Not supported. Services codegen is planned. | diff --git a/protoc-gen-buffa/src/main.rs b/protoc-gen-buffa/src/main.rs index 7e5ad1f..6f5dc41 100644 --- a/protoc-gen-buffa/src/main.rs +++ b/protoc-gen-buffa/src/main.rs @@ -161,8 +161,10 @@ struct PluginConfig { /// Parameters are comma-separated key=value pairs: /// --buffa_opt=views=true,unknown_fields=false,json=true /// -/// Extern paths use the format `extern_path==`: +/// Extern paths use the format `extern_path==`, where `` +/// is either a package or a single type FQN: /// --buffa_opt=extern_path=.my.common=::common_protos +/// --buffa_opt=extern_path=.my.common.Shared=::shared_types::Shared fn parse_config(params: &str) -> Result { let mut codegen = CodeGenConfig::default(); @@ -232,7 +234,8 @@ fn parse_config(params: &str) -> Result { } else { eprintln!( "protoc-gen-buffa: invalid extern_path format '{}', \ - expected 'extern_path=.proto.pkg=::rust::path'", + expected 'extern_path=.proto.pkg=::rust::path' \ + (or a type FQN, 'extern_path=.proto.pkg.Type=::rust::path::Type')", value ); }