Skip to content

Commit 6061e0e

Browse files
th0114ndclaude
andcommitted
fix: skip view generation on FooView name collision instead of erroring (#32)
When a user-defined message `FooView` exists alongside `Foo`, the generated `FooView<'a>` view type for `Foo` would collide. Previously this was a hard `ViewNameConflict` error blocking the entire crate. Now the codegen skips generating the view for `Foo` and continues. The user-defined `FooView` message (and its own view `FooViewView`) are generated normally. Downstream code that references `Foo`'s view will get a targeted compile error rather than a blanket codegen failure. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Made-with: Cursor
1 parent b065d14 commit 6061e0e

File tree

3 files changed

+69
-54
lines changed

3 files changed

+69
-54
lines changed

buffa-codegen/src/lib.rs

Lines changed: 24 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -424,47 +424,47 @@ fn check_module_name_conflicts(file: &FileDescriptorProto) -> Result<(), CodeGen
424424
check_siblings(&file.message_type, package)
425425
}
426426

427-
/// Check that no message named `FooView` collides with the generated view
428-
/// type for a sibling message `Foo`.
429-
fn check_view_name_conflicts(file: &FileDescriptorProto) -> Result<(), CodeGenError> {
427+
/// Collect proto FQNs of messages whose generated `{Name}View` type would
428+
/// collide with a sibling message of that name. Views for these messages
429+
/// are skipped rather than erroring, so the rest of the crate still compiles.
430+
fn collect_view_skip_fqns(file: &FileDescriptorProto) -> std::collections::HashSet<String> {
430431
use std::collections::HashSet;
431432

432-
fn check_siblings(
433+
fn collect_siblings(
433434
messages: &[crate::generated::descriptor::DescriptorProto],
434435
scope: &str,
435-
) -> Result<(), CodeGenError> {
436-
// Collect all message names at this level.
436+
out: &mut HashSet<String>,
437+
) {
437438
let names: HashSet<&str> = messages.iter().filter_map(|m| m.name.as_deref()).collect();
438439

439-
// For each message Foo, check if FooView also exists.
440440
for msg in messages {
441441
let name = msg.name.as_deref().unwrap_or("");
442442
let view_name = format!("{}View", name);
443443
if names.contains(view_name.as_str()) {
444-
return Err(CodeGenError::ViewNameConflict {
445-
scope: scope.to_string(),
446-
owned_msg: name.to_string(),
447-
view_msg: view_name,
448-
});
444+
let fqn = if scope.is_empty() {
445+
name.to_string()
446+
} else {
447+
format!("{}.{}", scope, name)
448+
};
449+
out.insert(fqn);
449450
}
450451
}
451452

452-
// Recurse into nested messages.
453453
for msg in messages {
454454
let name = msg.name.as_deref().unwrap_or("");
455455
let child_scope = if scope.is_empty() {
456456
name.to_string()
457457
} else {
458458
format!("{}.{}", scope, name)
459459
};
460-
check_siblings(&msg.nested_type, &child_scope)?;
460+
collect_siblings(&msg.nested_type, &child_scope, out);
461461
}
462-
463-
Ok(())
464462
}
465463

466464
let package = file.package.as_deref().unwrap_or("");
467-
check_siblings(&file.message_type, package)
465+
let mut skip = HashSet::new();
466+
collect_siblings(&file.message_type, package, &mut skip);
467+
skip
468468
}
469469

470470
/// Generate Rust source for a single `.proto` file.
@@ -475,9 +475,11 @@ fn generate_file(
475475
// Validate descriptors before generating code.
476476
check_reserved_field_names(file)?;
477477
check_module_name_conflicts(file)?;
478-
if ctx.config.generate_views {
479-
check_view_name_conflicts(file)?;
480-
}
478+
let view_skip_fqns = if ctx.config.generate_views {
479+
collect_view_skip_fqns(file)
480+
} else {
481+
std::collections::HashSet::new()
482+
};
481483

482484
let resolver = imports::ImportResolver::for_file(file);
483485
let mut tokens = resolver.generate_use_block();
@@ -520,6 +522,7 @@ fn generate_file(
520522
&proto_fqn,
521523
&features,
522524
&resolver,
525+
&view_skip_fqns,
523526
)?;
524527
tokens.extend(msg_top);
525528
// Nested extension const paths are relative to the message's module
@@ -537,7 +540,7 @@ fn generate_file(
537540
reg.json_any.extend(msg_reg.json_any);
538541
reg.text_any.extend(msg_reg.text_any);
539542

540-
let view_mod = if ctx.config.generate_views {
543+
let view_mod = if ctx.config.generate_views && !view_skip_fqns.contains(&proto_fqn) {
541544
let (view_top, view_mod) = view::generate_view(
542545
ctx,
543546
message_type,
@@ -692,17 +695,6 @@ pub enum CodeGenError {
692695
oneof_name: String,
693696
attempted: String,
694697
},
695-
/// A message named `FooView` collides with the generated view type for
696-
/// message `Foo`.
697-
#[error(
698-
"name conflict in '{scope}': message '{view_msg}' collides with \
699-
the generated view type for message '{owned_msg}'"
700-
)]
701-
ViewNameConflict {
702-
scope: String,
703-
owned_msg: String,
704-
view_msg: String,
705-
},
706698
/// The input contains a message with `option message_set_wire_format = true`
707699
/// but [`CodeGenConfig::allow_message_set`] was not set.
708700
#[error(

buffa-codegen/src/message.rs

Lines changed: 22 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@ impl RegistryPaths {
5656
/// to the per-message `__*_JSON_ANY` / `__*_TEXT_ANY` consts (relative to
5757
/// the struct's scope) and per-extension `__*_JSON_EXT` / `__*_TEXT_EXT`
5858
/// consts (relative to the `module_items` scope) for `register_types`.
59+
#[allow(clippy::too_many_arguments)]
5960
pub fn generate_message(
6061
ctx: &CodeGenContext,
6162
msg: &DescriptorProto,
@@ -64,6 +65,7 @@ pub fn generate_message(
6465
proto_fqn: &str,
6566
features: &ResolvedFeatures,
6667
resolver: &crate::imports::ImportResolver,
68+
view_skip_fqns: &std::collections::HashSet<String>,
6769
) -> Result<(TokenStream, TokenStream, RegistryPaths), CodeGenError> {
6870
let scope = MessageScope {
6971
ctx,
@@ -72,14 +74,15 @@ pub fn generate_message(
7274
features,
7375
nesting: 0,
7476
};
75-
generate_message_with_nesting(scope, msg, rust_name, resolver)
77+
generate_message_with_nesting(scope, msg, rust_name, resolver, view_skip_fqns)
7678
}
7779

7880
fn generate_message_with_nesting(
7981
scope: MessageScope<'_>,
8082
msg: &DescriptorProto,
8183
rust_name: &str,
8284
resolver: &crate::imports::ImportResolver,
85+
view_skip_fqns: &std::collections::HashSet<String>,
8386
) -> Result<(TokenStream, TokenStream, RegistryPaths), CodeGenError> {
8487
let MessageScope {
8588
ctx,
@@ -138,6 +141,7 @@ fn generate_message_with_nesting(
138141
nested,
139142
nested_proto_name,
140143
resolver,
144+
view_skip_fqns,
141145
)
142146
})
143147
.collect::<Result<Vec<_>, _>>()?;
@@ -516,23 +520,26 @@ fn generate_message_with_nesting(
516520
reg_paths.text_any.push(quote! { #mod_ident :: #p });
517521
}
518522

519-
// Also generate views for nested messages if enabled.
520-
// view_top (struct + impls) goes alongside the owned struct in the
521-
// parent module; view_mod (oneof view enums) goes in the sub-module.
523+
// Also generate views for nested messages if enabled and not skipped
524+
// due to a sibling name collision (e.g. FooView message exists).
522525
let view_mod_items = if ctx.config.generate_views {
523526
let nested_name = nested_desc.name.as_deref().unwrap_or("");
524527
let nested_fqn = format!("{}.{}", proto_fqn, nested_name);
525-
let (view_top, view_mod) = crate::view::generate_view_with_nesting(
526-
MessageScope {
527-
proto_fqn: &nested_fqn,
528-
nesting: nesting + 1,
529-
..scope
530-
},
531-
nested_desc,
532-
nested_name,
533-
)?;
534-
nested_items.extend(view_top);
535-
view_mod
528+
if view_skip_fqns.contains(&nested_fqn) {
529+
quote! {}
530+
} else {
531+
let (view_top, view_mod) = crate::view::generate_view_with_nesting(
532+
MessageScope {
533+
proto_fqn: &nested_fqn,
534+
nesting: nesting + 1,
535+
..scope
536+
},
537+
nested_desc,
538+
nested_name,
539+
)?;
540+
nested_items.extend(view_top);
541+
view_mod
542+
}
536543
} else {
537544
quote! {}
538545
};

buffa-codegen/src/tests/naming.rs

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -533,8 +533,9 @@ fn test_sibling_oneofs_get_distinct_names() {
533533
}
534534

535535
#[test]
536-
fn test_view_name_conflict_detected() {
537-
// Messages "Foo" and "FooView" — Foo's view type collides with FooView struct.
536+
fn test_view_name_collision_skips_view_generation() {
537+
// Messages "Foo" and "FooView" — Foo's view type would collide with the
538+
// FooView struct, so Foo's view is skipped rather than erroring.
538539
let mut file = proto3_file("test.proto");
539540
file.package = Some("pkg".to_string());
540541
file.message_type = vec![
@@ -550,12 +551,27 @@ fn test_view_name_conflict_detected() {
550551

551552
let config = CodeGenConfig::default(); // views enabled by default
552553
let result = generate(&[file], &["test.proto".to_string()], &config);
553-
assert!(result.is_err());
554-
let err = result.unwrap_err().to_string();
555-
assert!(err.contains("Foo"), "should mention owned message: {err}");
554+
let files = result.expect("view collision should be handled gracefully");
555+
let content = &files[0].content;
556+
assert!(
557+
content.contains("pub struct Foo"),
558+
"owned Foo struct must exist: {content}"
559+
);
560+
assert!(
561+
content.contains("pub struct FooView"),
562+
"FooView message struct must exist: {content}"
563+
);
564+
// The generated view for Foo would be `pub struct FooView<'a>` with a
565+
// `MessageView` impl — but that collides with the user-defined FooView
566+
// message, so the view must be skipped. The user-defined FooView
567+
// message's *own* view (`FooViewView<'a>`) should still exist.
568+
assert!(
569+
content.contains("FooViewView"),
570+
"FooView message's own view (FooViewView) should still be generated: {content}"
571+
);
556572
assert!(
557-
err.contains("FooView"),
558-
"should mention view collision: {err}"
573+
!content.contains("impl<'a> ::buffa::MessageView<'a> for FooView<'a>"),
574+
"Foo's view impl should be skipped to avoid collision: {content}"
559575
);
560576
}
561577

0 commit comments

Comments
 (0)