codegen: fix module redefinition when a message and sub-package share a name (#135)#145
Merged
Conversation
A message with nested types emits its members in a snake_case(Name) submodule at the package root. Protobuf is case-sensitive, so `message Oof` and a sibling `package foo.oof` legally coexist, but both map to `mod oof` in Rust — producing an E0428 "module redefined" error in the stitched output (issue #135). Deconflict the message-nesting module: when snake_case(Name) collides with a sub-package segment in the same scope, append `_` until the name is unique against the occupied set (sub-package segments, sibling message modules, the `__buffa` sentinel). The message struct (`Oof`) and the sub-package module (`oof`) keep their natural names; only the nested-types module moves (`oof_`). The computation lives at the single FQN->path chokepoint (the context type_map) and is reused by the emission site, so references and the emitted module always agree. It activates only on a real collision, so all currently-compiling schemas are byte-for-byte unchanged (verified: the WKT and bootstrap descriptor regen, incl. google.protobuf + the google.protobuf.compiler sub-package, produces no diff). Tests: codegen integration assertions (collision -> oof_, no-collision -> oof) and a buffa-test module that compiles the nested layout and checks both paths.
… rule Address review findings on the nested-module deconfliction: - The owned Any-registry paths (json_any/text_any) bubbled from a nested message were prefixed with the raw module name, but a top-level message's module is wrapped with the deconflicted name — so a deconflicted message built with generate_json/text emitted `super::oof::__INNER_JSON_ANY` while the module is `oof_`, an E0432. Prefix with the deconflicted wrap name. - Skip extern-mapped packages in the collision pre-pass: a sub-package routed to another crate via extern_path emits no local module, so it must not trigger a spurious rename of a same-named message's nested module. - Use the deconflicted name in the root re-export occupied set so a future re-export candidate can't shadow the renamed module. - Emit a doc-comment on the deconflicted module explaining the trailing `_`. Docs: correct DESIGN.md (which claimed collisions were structurally impossible with no suffix-escalation) and add a CHANGELOG entry. Tests: build the modcollide regression with generate_json so the Any-registry path through the deconflicted module is compiled; cover a nested message inside the sub-package; harden the no-collision assertion against formatter spacing.
|
All contributors have signed the CLA ✍️ ✅ |
The per-message deconfliction only checked sibling messages' *raw* module names, so two messages whose deconflicted candidates converge could still collide: `message Oof` (oof) and `message Oof_` (oof_) alongside sub-packages `foo.oof` and `foo.oof_` both resolved to `oof__` — a fresh E0428. Compute a package's nested-module names as a batch with a shared `taken` set seeded with the sub-package segments, the sentinel, and every message's raw module name, reserving each deconflicted name as it is assigned. The two messages above now resolve to `oof__` and `oof___`. The `_`-append loop already handled the single-message repeated-append case (sub-packages `oof` and `oof_` -> `oof__`); this closes the multi-message gap. Names are deterministic for a given declaration order and collision-free regardless of order. Default output is unchanged (deconfliction still fires only on a real collision; WKT + bootstrap regen produce no diff). Tests: unit tests for the pure helper (no-collision, single, repeated-append, two-message race, sentinel-avoidance, child-segment extraction) and an end-to-end codegen-integration test for the racing case.
Re-review polish (all Low findings): - Assign colliding nested-module names in a stable order (sorted by base name), so the per-message suffix no longer depends on file/message declaration order — reordering inputs can't swap which message gets `oof__` vs `oof___`. - Broaden the generated module doc-comment (the collision source can be a sibling message, not only a sub-package) and fix the stale "snake_case" wording in the root re-export occupied-set comment. - CHANGELOG/DESIGN: note that suffixes can grow past one `_`, and that the colliding message and sub-package must be generated in the same compile() (deconfliction can't span separate descriptor sets). Tests: add an order-independence unit test and an end-to-end buffa-test module (`modrace`) that compiles the multi-message race layout — `Oof`/`Oof_` nested modules (`oof__`/`oof___`) coexisting with sub-packages `oof`/`oof_`.
asacamano
previously approved these changes
May 22, 2026
| std::fs::write( | ||
| dir.path().join("foo.proto"), | ||
| "syntax = \"proto3\";\npackage foo;\n\ | ||
| message Oof { message Inner { int32 x = 1; } Inner inner = 1; }\n\ |
…llision # Conflicts: # buffa-codegen/tests/codegen_integration.rs
asacamano
approved these changes
May 23, 2026
iainmcgin
added a commit
that referenced
this pull request
May 24, 2026
Propagates the origin/main merge (idiomatic enum aliases #152, module collision fix #145) up the stack. Sole conflict in codegen lib.rs: keep the two-function generate / generate_with_diagnostics split from view-vtable, but retain owned-vtable's relaxed vtable check (vtable reflection requires generate_reflection only, not views) and move it into generate_with_diagnostics. Doc # Errors updated to match.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #135 — a message with nested types and a sibling sub-package of the same name produced uncompilable generated code.
Protobuf is case-sensitive, so
message Oofandpackage foo.ooflegally coexist. But buffa emits a message's nested types in asnake_case(MessageName)submodule, so both wantedfoo::oofand collided —error[E0428]: the name 'oof' is defined multiple times.Fix
When a message's nested-types module name collides with a sub-package module in the same scope, the nested-types module is deconflicted by appending
_(repeated until unique against the sub-package segments, sibling message modules, and the__buffasentinel). The message struct and the sub-package keep their natural names:The deconflicted name is computed once at the FQN→Rust-path chokepoint (
CodeGenContext) and reused by the emission site, so references and the emitted module always agree. Why trailing-underscore-with-occupied-set rather than a fixed marker: proto package segments and snake_cased message names share the identifier space, so no fixed suffix is collision-proof against a pathological schema — verifying against the actual occupied set is collision-free by construction and reserves no identifier space.Compatibility
This activates only on a collision that previously failed to compile, so every schema that builds today is byte-for-byte unchanged (verified: regenerating the WKT and bootstrap descriptor types — which include the real
google.protobuf+google.protobuf.compilerpair — produces zero diff). Forward note in the CHANGELOG: if you add a sub-package that collides with an existing message's nested-types module, paths to those nested types move fromooftooof_.Extern-mapped sub-packages (
extern_path) are excluded — they emit no local module, so they don't trigger a rename.Testing
oof_, no-collision →oof)buffa-testmodule compiling the full nested layout withgenerate_json, exercising binary round-trips and the Any-registry paths that bubble through the deconflicted module (super::oof_::__INNER_JSON_ANY), plus a nested message inside the sub-package to confirm deconfliction doesn't leakcargo test --workspace+clippy -D warningscleanDESIGN.md is updated (it previously claimed collisions were structurally impossible with no suffix-escalation).