From 435ec10a968b3c7ca8932872657976824c8c7b11 Mon Sep 17 00:00:00 2001 From: Tim Holland Date: Sun, 5 Apr 2026 19:59:04 -0400 Subject: [PATCH 1/3] fix: qualify Option in nested scopes to prevent prelude shadowing (#36) When a proto message named `Option` is nested inside another message, the generated `pub struct Option` shadows `core::option::Option` in the parent module scope. This caused struct fields like `pub value: Option` to resolve to the proto struct (0 generic args) instead of the standard library type. Fix: extend ImportResolver with `child_for_message()` to propagate blocked prelude names through the `use super::*` module chain. Each recursive `generate_message` call now receives a child resolver that accounts for sibling types in its module scope. Also qualify bare `Option` in custom deserialize temporaries and `skip_serializing_if` predicates. Co-Authored-By: Claude Sonnet 4.6 Made-with: Cursor --- buffa-codegen/src/imports.rs | 56 ++++++++++------ buffa-codegen/src/message.rs | 14 ++-- buffa-codegen/src/oneof.rs | 4 +- buffa-codegen/src/tests/naming.rs | 104 ++++++++++++++++++++++++++++++ 4 files changed, 153 insertions(+), 25 deletions(-) diff --git a/buffa-codegen/src/imports.rs b/buffa-codegen/src/imports.rs index 47b68b8..8b33e5e 100644 --- a/buffa-codegen/src/imports.rs +++ b/buffa-codegen/src/imports.rs @@ -2,7 +2,7 @@ //! //! Controls whether types are emitted as short names (e.g. `Option`) or //! fully-qualified paths (e.g. `::core::option::Option`), by detecting -//! collisions with proto-defined type names in the current file. +//! collisions with proto-defined type names in the current scope. //! //! `core` prelude types (`Option`, `Default`, etc.) are in scope in both `std` //! and `no_std` contexts and can be emitted as bare names unless shadowed. @@ -14,7 +14,7 @@ use std::collections::HashSet; -use crate::generated::descriptor::FileDescriptorProto; +use crate::generated::descriptor::{DescriptorProto, FileDescriptorProto}; use proc_macro2::TokenStream; use quote::quote; @@ -27,7 +27,20 @@ use quote::quote; /// Those types are always emitted via `::buffa::alloc::*` re-exports. const PRELUDE_NAMES: &[&str] = &["Option"]; -/// Tracks which short names are safe to use in a generated file. +fn check_names_for_prelude_collisions<'a>( + names: impl Iterator, +) -> HashSet { + let prelude: HashSet<&str> = PRELUDE_NAMES.iter().copied().collect(); + let mut blocked = HashSet::new(); + for name in names { + if prelude.contains(name) { + blocked.insert(name.to_string()); + } + } + blocked +} + +/// Tracks which short names are safe to use in a generated scope. pub(crate) struct ImportResolver { /// Proto type names that collide with prelude names. blocked: HashSet, @@ -37,24 +50,29 @@ impl ImportResolver { /// Build a resolver for a single `.proto` file by checking top-level /// message and enum names against the set of short names we want to use. pub fn for_file(file: &FileDescriptorProto) -> Self { - let mut proto_names = HashSet::new(); - for msg in &file.message_type { - if let Some(name) = &msg.name { - proto_names.insert(name.as_str()); - } - } - for e in &file.enum_type { - if let Some(name) = &e.name { - proto_names.insert(name.as_str()); - } + let names = file + .message_type + .iter() + .filter_map(|m| m.name.as_deref()) + .chain(file.enum_type.iter().filter_map(|e| e.name.as_deref())); + Self { + blocked: check_names_for_prelude_collisions(names), } + } - let mut blocked = HashSet::new(); - for &name in PRELUDE_NAMES { - if proto_names.contains(name) { - blocked.insert(name.to_string()); - } - } + /// Build a child resolver for a message's `pub mod` scope. + /// + /// Each message module contains `use super::*`, so parent-scope blocked + /// names propagate. On top of those, the message's own nested types and + /// nested enums introduce additional names that can shadow prelude types. + pub fn child_for_message(&self, msg: &DescriptorProto) -> Self { + let mut blocked = self.blocked.clone(); + let child_names = msg + .nested_type + .iter() + .filter_map(|m| m.name.as_deref()) + .chain(msg.enum_type.iter().filter_map(|e| e.name.as_deref())); + blocked.extend(check_names_for_prelude_collisions(child_names)); Self { blocked } } diff --git a/buffa-codegen/src/message.rs b/buffa-codegen/src/message.rs index e792570..d8ec38b 100644 --- a/buffa-codegen/src/message.rs +++ b/buffa-codegen/src/message.rs @@ -118,6 +118,12 @@ fn generate_message_with_nesting( // Nested messages (skip map entry synthetics) — simple name, emitted // inside the message's module. + // + // The child resolver inherits parent-scope blocked names (via + // `use super::*`) and adds this message's nested types/enums, so that + // a nested message named `Option` causes `::core::option::Option` to + // be emitted in struct fields within this module scope. + let child_resolver = resolver.child_for_message(msg); let nested_msgs = msg .nested_type .iter() @@ -137,7 +143,7 @@ fn generate_message_with_nesting( scope.nested(&nested_fqn, &msg_features), nested, nested_proto_name, - resolver, + &child_resolver, ) }) .collect::, _>>()?; @@ -874,7 +880,7 @@ fn custom_deser_regular_field( quote! { #json_name => { #var_ident = Some(#deser_expr); } } }; - let var_decl = quote! { let mut #var_ident: Option<#rust_type> = None; }; + let var_decl = quote! { let mut #var_ident: ::core::option::Option<#rust_type> = None; }; // Overwrite only if present — missing fields keep the struct's Default // (which honours proto2 [default = X], unlike ::default()). let field_init = quote! { @@ -915,7 +921,7 @@ fn custom_deser_oneof_group( let field_ident = make_field_ident(oneof_name); // Oneof enum lives in the message's module. - let var_decl = quote! { let mut #var_ident: Option<#mod_ident::#enum_ident> = None; }; + let var_decl = quote! { let mut #var_ident: ::core::option::Option<#mod_ident::#enum_ident> = None; }; let mut arms = Vec::new(); for field in &msg.field { @@ -1631,7 +1637,7 @@ fn skip_serializing_predicate( } else if info.is_repeated { Some("::buffa::json_helpers::skip_if::is_empty_vec") } else if info.is_optional { - Some("Option::is_none") + Some("::core::option::Option::is_none") } else { singular_skip_predicate(field_type, features) } diff --git a/buffa-codegen/src/oneof.rs b/buffa-codegen/src/oneof.rs index f9e765e..35f68db 100644 --- a/buffa-codegen/src/oneof.rs +++ b/buffa-codegen/src/oneof.rs @@ -437,13 +437,13 @@ pub(crate) fn oneof_variant_deser_arm(input: &OneofVariantDeserInput<'_>) -> Tok #helper::deserialize(d) } } - let v: Option<#variant_type> = map.next_value_seed( + let v: ::core::option::Option<#variant_type> = map.next_value_seed( ::buffa::json_helpers::NullableDeserializeSeed(_DeserSeed) )?; } } else { quote! { - let v: Option<#variant_type> = map.next_value_seed( + let v: ::core::option::Option<#variant_type> = map.next_value_seed( ::buffa::json_helpers::NullableDeserializeSeed( ::buffa::json_helpers::DefaultDeserializeSeed::<#variant_type>::new() ) diff --git a/buffa-codegen/src/tests/naming.rs b/buffa-codegen/src/tests/naming.rs index e751369..4ff82b5 100644 --- a/buffa-codegen/src/tests/naming.rs +++ b/buffa-codegen/src/tests/naming.rs @@ -625,6 +625,110 @@ fn test_proto3_optional_field_name_matches_nested_enum_no_conflict() { ); } +#[test] +fn test_nested_message_named_option_does_not_shadow_prelude() { + // Reproduces gh#36: a nested message named `Option` shadows + // `core::option::Option`, causing `pub value: Option` to + // resolve to the proto struct instead of the standard library type. + // The codegen must emit `::core::option::Option<...>` in this scope. + let option_msg = DescriptorProto { + name: Some("Option".to_string()), + field: vec![ + make_field("title", 1, Label::LABEL_OPTIONAL, Type::TYPE_STRING), + { + let mut f = make_field("int_value", 2, Label::LABEL_OPTIONAL, Type::TYPE_UINT64); + f.oneof_index = Some(0); + f + }, + ], + oneof_decl: vec![OneofDescriptorProto { + name: Some("value".to_string()), + ..Default::default() + }], + ..Default::default() + }; + let picker_msg = DescriptorProto { + name: Some("Picker".to_string()), + field: vec![{ + let mut f = make_field("options", 1, Label::LABEL_REPEATED, Type::TYPE_MESSAGE); + f.type_name = Some(".test.option_shadow.Picker.Option".to_string()); + f + }], + nested_type: vec![option_msg], + ..Default::default() + }; + let mut file = proto3_file("option_shadow.proto"); + file.package = Some("test.option_shadow".to_string()); + file.message_type = vec![picker_msg]; + + let config = CodeGenConfig { + generate_views: false, + ..Default::default() + }; + let result = generate(&[file], &["option_shadow.proto".to_string()], &config); + let files = result.expect("nested Option message should not break codegen"); + let content = &files[0].content; + assert!( + content.contains("pub struct Option"), + "nested Option struct must exist: {content}" + ); + // The oneof field on Option must use the fully-qualified + // `::core::option::Option` to avoid resolving to the proto struct. + assert!( + !content.contains("pub value: Option<"), + "bare Option<> in struct field would shadow core::option::Option: {content}" + ); + assert!( + content.contains("::core::option::Option<"), + "must use fully-qualified ::core::option::Option: {content}" + ); +} + +#[test] +fn test_top_level_message_named_option_qualifies_option() { + // A top-level message named `Option` — file-level ImportResolver should + // detect this and qualify all Option type references in the file. + let mut file = proto3_file("option_top.proto"); + file.package = Some("pkg".to_string()); + file.message_type = vec![ + DescriptorProto { + name: Some("Option".to_string()), + ..Default::default() + }, + DescriptorProto { + name: Some("Wrapper".to_string()), + field: vec![{ + let mut f = make_field("tag", 1, Label::LABEL_OPTIONAL, Type::TYPE_STRING); + f.proto3_optional = Some(true); + f.oneof_index = Some(0); + f + }], + oneof_decl: vec![OneofDescriptorProto { + name: Some("_tag".to_string()), + ..Default::default() + }], + ..Default::default() + }, + ]; + + let config = CodeGenConfig { + generate_views: false, + ..Default::default() + }; + let result = generate(&[file], &["option_top.proto".to_string()], &config); + let files = result.expect("top-level Option should not break codegen"); + let content = &files[0].content; + // The Wrapper struct must use qualified Option for its optional field. + assert!( + content.contains("::core::option::Option<"), + "must use fully-qualified ::core::option::Option for optional field: {content}" + ); + assert!( + !content.contains("pub tag: Option<"), + "bare Option<> on Wrapper field would shadow core::option::Option: {content}" + ); +} + #[test] fn test_message_named_type_with_nested() { // Proto message named "Type" (a Rust keyword) with a nested message. From b6e812432c3208723dd79b515c678321e1d66ad8 Mon Sep 17 00:00:00 2001 From: Tim Holland Date: Mon, 20 Apr 2026 11:50:35 -0400 Subject: [PATCH 2/3] style: apply rustfmt formatting fixes Co-Authored-By: Claude Sonnet 4.6 (1M context) --- buffa-codegen/src/imports.rs | 4 +--- buffa-codegen/src/message.rs | 3 ++- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/buffa-codegen/src/imports.rs b/buffa-codegen/src/imports.rs index 8b33e5e..762caf8 100644 --- a/buffa-codegen/src/imports.rs +++ b/buffa-codegen/src/imports.rs @@ -27,9 +27,7 @@ use quote::quote; /// Those types are always emitted via `::buffa::alloc::*` re-exports. const PRELUDE_NAMES: &[&str] = &["Option"]; -fn check_names_for_prelude_collisions<'a>( - names: impl Iterator, -) -> HashSet { +fn check_names_for_prelude_collisions<'a>(names: impl Iterator) -> HashSet { let prelude: HashSet<&str> = PRELUDE_NAMES.iter().copied().collect(); let mut blocked = HashSet::new(); for name in names { diff --git a/buffa-codegen/src/message.rs b/buffa-codegen/src/message.rs index d8ec38b..9feb806 100644 --- a/buffa-codegen/src/message.rs +++ b/buffa-codegen/src/message.rs @@ -921,7 +921,8 @@ fn custom_deser_oneof_group( let field_ident = make_field_ident(oneof_name); // Oneof enum lives in the message's module. - let var_decl = quote! { let mut #var_ident: ::core::option::Option<#mod_ident::#enum_ident> = None; }; + let var_decl = + quote! { let mut #var_ident: ::core::option::Option<#mod_ident::#enum_ident> = None; }; let mut arms = Vec::new(); for field in &msg.field { From 6964025b1a927c95512e2677c2ababfbf6731544 Mon Sep 17 00:00:00 2001 From: Iain McGinniss <309153+iainmcgin@users.noreply.github.com> Date: Thu, 23 Apr 2026 16:54:01 +0000 Subject: [PATCH 3/3] test: add end-to-end regression coverage for nested Option shadowing The unit tests in tests/naming.rs only checked codegen output substrings. This adds: - buffa-test/protos/prelude_shadow.proto built with views + JSON, covering the gh#36 nested case, a sibling-propagation case (Outer.Option + Outer.Middle.Inner where Inner must qualify Option via two `use super::*` hops), and the top-level case under JSON. Compilation is the assertion. - Two runtime tests in tests/collision.rs that round-trip the generated types over binary and JSON. - A unit test in tests/naming.rs for the sibling-propagation case, verifying child_for_message inherits the parent blocked set. Audited all codegen modules: view.rs, enumeration.rs, impl_message.rs, extension.rs, impl_text.rs already emit ::core::option::Option uniformly, and Result/Default/Debug are universally ::core::-qualified, so no additional qualification sites were needed. Co-authored-by: Iain McGinniss <309153+iainmcgin@users.noreply.github.com> --- buffa-codegen/src/tests/naming.rs | 71 ++++++++++++++++++++++++++ buffa-test/build.rs | 10 ++++ buffa-test/protos/prelude_shadow.proto | 57 +++++++++++++++++++++ buffa-test/src/lib.rs | 5 ++ buffa-test/src/tests/collision.rs | 46 +++++++++++++++++ 5 files changed, 189 insertions(+) create mode 100644 buffa-test/protos/prelude_shadow.proto diff --git a/buffa-codegen/src/tests/naming.rs b/buffa-codegen/src/tests/naming.rs index 4ff82b5..0724a76 100644 --- a/buffa-codegen/src/tests/naming.rs +++ b/buffa-codegen/src/tests/naming.rs @@ -729,6 +729,77 @@ fn test_top_level_message_named_option_qualifies_option() { ); } +#[test] +fn test_nested_option_blocked_propagates_through_sibling_subtree() { + // `Outer { nested Option; nested Middle { nested Inner } }` — `Option` + // is declared in `mod outer`, so it shadows the prelude there AND in + // `mod outer::middle` via `use super::*`. The child resolver for + // `Middle` must inherit the parent's blocked set so that `Inner` + // (emitted inside `mod outer::middle`) qualifies its optional field. + let inner_msg = DescriptorProto { + name: Some("Inner".to_string()), + field: vec![{ + let mut f = make_field("x", 1, Label::LABEL_OPTIONAL, Type::TYPE_INT32); + f.proto3_optional = Some(true); + f.oneof_index = Some(0); + f + }], + oneof_decl: vec![OneofDescriptorProto { + name: Some("_x".to_string()), + ..Default::default() + }], + ..Default::default() + }; + let middle_msg = DescriptorProto { + name: Some("Middle".to_string()), + field: vec![{ + let mut f = make_field("note", 1, Label::LABEL_OPTIONAL, Type::TYPE_STRING); + f.proto3_optional = Some(true); + f.oneof_index = Some(0); + f + }], + oneof_decl: vec![OneofDescriptorProto { + name: Some("_note".to_string()), + ..Default::default() + }], + nested_type: vec![inner_msg], + ..Default::default() + }; + let outer_msg = DescriptorProto { + name: Some("Outer".to_string()), + nested_type: vec![ + DescriptorProto { + name: Some("Option".to_string()), + ..Default::default() + }, + middle_msg, + ], + ..Default::default() + }; + let mut file = proto3_file("option_deep.proto"); + file.package = Some("pkg".to_string()); + file.message_type = vec![outer_msg]; + + let config = CodeGenConfig { + generate_views: false, + ..Default::default() + }; + let files = generate(&[file], &["option_deep.proto".to_string()], &config) + .expect("nested Option sibling should not break codegen"); + let content = &files[0].content; + // `Middle.note` lives in `mod outer` (Option in scope); `Inner.x` lives + // in `mod outer::middle` (Option in scope via `use super::*`). Both must + // be qualified. + assert!( + !content.contains("pub note: Option<"), + "Middle.note must qualify Option (sibling collision): {content}" + ); + assert!( + !content.contains("pub x: Option<"), + "Inner.x must qualify Option (inherited via use super::*): {content}" + ); +} + #[test] fn test_message_named_type_with_nested() { // Proto message named "Type" (a Rust keyword) with a nested message. diff --git a/buffa-test/build.rs b/buffa-test/build.rs index 22c99fd..9863dbd 100644 --- a/buffa-test/build.rs +++ b/buffa-test/build.rs @@ -48,6 +48,16 @@ fn main() { .compile() .expect("buffa_build failed for name_collisions.proto"); + // Prelude shadowing (gh#36) — nested `message Option` with optional/oneof + // fields, built with views + JSON so all `Option<...>` emission paths are + // exercised. Compilation is the assertion. + buffa_build::Config::new() + .files(&["protos/prelude_shadow.proto"]) + .includes(&["protos/"]) + .generate_json(true) + .compile() + .expect("buffa_build failed for prelude_shadow.proto"); + // Proto2 with custom defaults, required fields, closed enums. buffa_build::Config::new() .files(&["protos/proto2_defaults.proto"]) diff --git a/buffa-test/protos/prelude_shadow.proto b/buffa-test/protos/prelude_shadow.proto new file mode 100644 index 0000000..5d90811 --- /dev/null +++ b/buffa-test/protos/prelude_shadow.proto @@ -0,0 +1,57 @@ +// Regression for gh#36: a proto message named `Option` in a nested scope +// shadows `core::option::Option` via the `use super::*` chain, breaking +// generated optional/oneof field types and the JSON deserialize path. +// +// Covers top-level, 1-level nested, and 3-level nested `Option` messages, +// each with an optional scalar and a oneof so the struct-field type, the +// `skip_serializing_if` predicate, and the custom-deserialize temporaries +// all emit `::core::option::Option<...>`. +// +// Built with views and JSON enabled — compilation alone is the assertion. + +syntax = "proto3"; + +package test.prelude_shadow; + +// Top-level — already detected by `ImportResolver::for_file` pre-gh#36, +// but exercises the JSON path here (skip_serializing_if, deser temps). +message Option { + optional string title = 1; + oneof value { + uint64 int_value = 2; + string str_value = 3; + } +} + +// 1-level nested — the original gh#36 reproduction. Requires +// `ImportResolver::child_for_message` to block `Option` inside +// `mod picker { use super::*; ... }`. +message Picker { + message Option { + optional string title = 1; + oneof value { + uint64 int_value = 2; + string str_value = 3; + } + } + repeated Option options = 1; + optional string label = 2; +} + +// Sibling propagation — `Option` and `Middle` are both nested in `Outer`, +// so `mod outer` declares `struct Option` (shadowing the prelude there and +// in `mod outer::middle` via `use super::*`). The child resolver must +// inherit the parent's blocked set so `Inner.x` is qualified. +message Outer { + message Option { + bool present = 1; + } + message Middle { + message Inner { + optional int32 x = 1; + } + optional Inner inner = 1; + optional string note = 2; + } + optional Middle middle = 1; +} diff --git a/buffa-test/src/lib.rs b/buffa-test/src/lib.rs index 917f375..aa2a4d9 100644 --- a/buffa-test/src/lib.rs +++ b/buffa-test/src/lib.rs @@ -53,6 +53,11 @@ pub mod collisions { include!(concat!(env!("OUT_DIR"), "/name_collisions.rs")); } +#[allow(clippy::derivable_impls, clippy::match_single_binding, dead_code)] +pub mod prelude_shadow { + include!(concat!(env!("OUT_DIR"), "/prelude_shadow.rs")); +} + #[allow( clippy::derivable_impls, clippy::match_single_binding, diff --git a/buffa-test/src/tests/collision.rs b/buffa-test/src/tests/collision.rs index c46b745..c757fca 100644 --- a/buffa-test/src/tests/collision.rs +++ b/buffa-test/src/tests/collision.rs @@ -124,3 +124,49 @@ fn test_container_references_collision_types() { assert_eq!(decoded.vec_field.items, vec![1]); assert_eq!(decoded.string_field.value, "v"); } + +#[test] +fn test_nested_option_message_round_trip() { + // gh#36: nested `message Option` shadows core::option::Option in the + // message's `pub mod { use super::*; }` scope. The proto is built with + // views + JSON enabled so all Option<...> emission paths compile. + use crate::prelude_shadow::{self, picker}; + + let msg = prelude_shadow::Picker { + options: vec![picker::Option { + title: Some("a".into()), + value: Some(picker::option::ValueOneof::IntValue(7)), + ..core::default::Default::default() + }], + label: Some("L".into()), + ..core::default::Default::default() + }; + let decoded = round_trip(&msg); + assert_eq!(decoded.options[0].title.as_deref(), Some("a")); + assert_eq!(decoded.label.as_deref(), Some("L")); + + // JSON round-trip exercises skip_serializing_if + custom-deser temporaries. + let json = serde_json::to_string(&msg).expect("serialize"); + let back: prelude_shadow::Picker = serde_json::from_str(&json).expect("deserialize"); + assert_eq!(back, msg); +} + +#[test] +fn test_nested_option_sibling_propagation_compiles() { + // `Outer.Option` shadows the prelude in `mod outer` AND in + // `mod outer::middle` (via `use super::*`). Verifies the child + // resolver propagates the blocked set through >1 hop. + use crate::prelude_shadow::outer::{self, middle}; + + let msg = outer::Middle { + inner: buffa::MessageField::some(middle::Inner { + x: Some(1), + ..core::default::Default::default() + }), + note: Some("n".into()), + ..core::default::Default::default() + }; + let decoded = round_trip(&msg); + assert_eq!(decoded.inner.x, Some(1)); + assert_eq!(decoded.note.as_deref(), Some("n")); +}