naming: dedup top-level names per package, not per file - #44
Merged
Conversation
The namespace a top-level name lands in is per PACKAGE, but index_file gave each FILE its own dedup scope. Two files sharing a package were indexed independently, so an escape in one could take a name the other legitimately declares -- `enum decode` sanitizes to `decode_` (the decoders expose a static decode()) beside a real `message decode_` -- and both were emitted as `p::decode_`. Any translation unit including the two headers then failed to compile (6 errors on gcc-13, 20 on clang-20). The scope is now keyed on the package namespace and shared across the resolved file set. Keyed on the PACKAGE namespace and not the message one, deliberately: top-level enums go in the shared common header, so an id that differed between the arena and streaming runs would make the two models' commons disagree. A first pass lets every name that needs no escape claim its id before any escaped name is placed. Without it the fix traded a collision for an order dependence -- whoever was indexed first took the contested id, so `rapidprotoc x.proto y.proto` and `y.proto x.proto` gave one schema two different sets of C++ names. It also puts the rename on the right side: the literal identifier keeps its spelling and the escape moves to `decode__`. That swap applies to single-file schemas too, which compiled before and still compile -- a generated-API change, so the CHANGELOG says to regenerate. Two PACKAGES that sanitize to one C++ namespace (`p.decode` and `p.decode_`) are literal-vs-literal and stay order-decided; the output compiles either way. escdedup_a/b.proto pin the shape, and the arenagen compile smoke #includes the generated header -- which is the real test, since the symptom is a header that does not compile. Confirmed failing at the parent: 2 unit assertions, 1 golden, and the compile smoke once the goldens are regenerated there. protoc has the same class of bug for names IT escapes (`message class` beside `message class_` gives two `p::class_`), so this is inherent to mangling rather than ours alone. What makes it worth fixing is the asymmetry: we reserve six non-keyword names protoc does not, so schemas protoc handles fine broke here. The dedup can only see one invocation's file set -- two separate generator runs over the same package can still collide, as they can with protoc.
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The namespace a top-level name lands in is per PACKAGE, but index_file gave each FILE its own dedup scope. Two files sharing a package were indexed independently, so an escape in one could take a name the other legitimately declares --
enum decodesanitizes todecode_(the decoders expose a static decode()) beside a realmessage decode_-- and both were emitted asp::decode_. Any translation unit including the two headers then failed to compile (6 errors on gcc-13, 20 on clang-20).The scope is now keyed on the package namespace and shared across the resolved file set. Keyed on the PACKAGE namespace and not the message one, deliberately: top-level enums go in the shared common header, so an id that differed between the arena and streaming runs would make the two models' commons disagree.
A first pass lets every name that needs no escape claim its id before any escaped name is placed. Without it the fix traded a collision for an order dependence -- whoever was indexed first took the contested id, so
rapidprotoc x.proto y.protoandy.proto x.protogave one schema two different sets of C++ names. It also puts the rename on the right side: the literal identifier keeps its spelling and the escape moves todecode__. That swap applies to single-file schemas too, which compiled before and still compile -- a generated-API change, so the CHANGELOG says to regenerate. Two PACKAGES that sanitize to one C++ namespace (p.decodeandp.decode_) are literal-vs-literal and stay order-decided; the output compiles either way.escdedup_a/b.proto pin the shape, and the arenagen compile smoke #includes the generated header -- which is the real test, since the symptom is a header that does not compile. Confirmed failing at the parent: 2 unit assertions, 1 golden, and the compile smoke once the goldens are regenerated there.
protoc has the same class of bug for names IT escapes (
message classbesidemessage class_gives twop::class_), so this is inherent to mangling rather than ours alone. What makes it worth fixing is the asymmetry: we reserve six non-keyword names protoc does not, so schemas protoc handles fine broke here. The dedup can only see one invocation's file set -- two separate generator runs over the same package can still collide, as they can with protoc.