Skip to content

codegen: escape a oneof enum name that PascalCases to Self - #465

Merged
iainmcgin merged 2 commits into
anthropics:mainfrom
dfedoryshchev:codegen-escape-oneof-enum-ident
Sep 25, 2026
Merged

iainmcgin merged 2 commits into
anthropics:mainfrom
dfedoryshchev:codegen-escape-oneof-enum-ident

Conversation

@dfedoryshchev

Copy link
Copy Markdown
Contributor

A oneof named self makes code generation fail outright. PascalCasing the name produces Self, which is a reserved Rust identifier, so the generator emits pub enum Self, the generated file does not parse, and generate returns an error that names no oneof at all. The same escape is already applied one function below to the oneof's variant names, and to the struct field, so this applies it to the enum's own name as well and the three finally agree.

oneof_enum_ident (buffa-codegen/src/oneof.rs:932) builds the identifier with a bare format_ident!. self, self_ and _self all reach it, because to_pascal_case (oneof.rs:972) splits on _ and capitalises each part, and Self is the only reserved Rust identifier with a leading capital, so it is the only reachable case. The ident is produced once in resolve_oneof_idents (oneof.rs:954) and consumed for both the owned enum (oneof.rs:679) and the view enum (view.rs:1105). format_tokens (buffa-codegen/src/lib.rs:4332) runs syn::parse2::<syn::File> before prettyplease sees the tokens, so the result is CodeGenError::InvalidSyntax("generated code failed to parse as Rust: ...") for a perfectly legal .proto.

oneof_variant_ident (oneof.rs:967) already routes through make_field_ident, and its doc comment spells out this exact failure: it "would otherwise produce pub enum Foo { Self(...) } and fail to parse". CodeGenContext::oneof_ident (context.rs:534) escapes the struct field too. Only the enum's own name was left raw, so for a oneof named self_ the generated field is self_ and the type it points at is Self.

No fixture puts a keyword on the oneof itself, which is why this survived: buffa-test/protos/keywords.proto covers keyword package, message, field and enum-value names, including string self = 4; at line 35, but its one oneof is named value, and the #47 regression test (buffa-codegen/src/tests/naming.rs:1147) puts the keyword on a variant while naming the oneof identity.

The fix routes the enum name through make_field_ident (idents.rs:67), exactly as the variant name already is, so Self becomes Self_ and matches the #47 convention. It is a no-op for every other name: make_field_ident only rewrites when is_rust_keyword matches, PascalCasing always yields a leading capital, and Self is the only capitalised entry in that table (idents.rs:181). The checked-in generated trees are unaffected, since the only oneof in either is google.protobuf.Value.kind yielding Kind, so nothing needs regenerating. format_ident! had no other use in oneof.rs, so its import goes with it; that is the only other line in the diff.

test_oneof_named_self_escapes_its_enum_to_self_underscore sits beside the #47 test it mirrors: it generates a message with a oneof named self_ and asserts the enum is Self_ while the non-keyword variant is untouched. On unpatched source it is the generate(...).expect(...) that fails, because generation errors out before any assertion runs.

cargo fmt --check -p buffa-codegen is clean on the pinned 1.95.0 toolchain. I have not run the test suite on this machine, so CI is the check on the new test.

The changelog fragment is .changes/unreleased/fixed-20260919-210200.yaml. It does not cite a PR number yet; happy to amend that in once this has one.

@github-actions

github-actions Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@dfedoryshchev

Copy link
Copy Markdown
Contributor Author

I have read the CLA Document and I hereby sign the CLA

github-actions Bot added a commit that referenced this pull request Sep 22, 2026
The unit test checks that the generated file parses. Add a oneof named
self_ to the keywords fixture with a round-trip test, so the Self_ enum
and the self_ field are also type-checked together.

The doc comment on oneof_enum_ident no longer claims sibling oneofs
cannot share an ident; names that PascalCase alike do, undiagnosed. The
changelog fragment is one paragraph and cites the PR.
@iainmcgin

Copy link
Copy Markdown
Collaborator

[claude code] Thanks for this, and for the careful write-up. The diagnosis is right: resolve_oneof_idents is the only place the enum name is derived, the owned, view and lazy-view paths all take its ident map, and Self is the only capitalised entry in the keyword table, so nothing else can reach the escape. With the one-line change reverted, your new test fails on the expect with InvalidSyntax, as you predicted; with it, all 714 buffa-codegen tests pass.

I pushed one maintainer commit (123f9d6) on top:

  • An end-to-end fixture. Your test checks that the generated file parses; buffa-test/protos/keywords.proto now has a oneof self_, with a round-trip test that names keywords::__buffa::oneof::identity::Self_, so the enum and the self_ field are type-checked together. It also compiled with views, JSON and text switched on for that fixture locally.
  • The doc comment on oneof_enum_ident said two sibling oneofs produce the same ident only if they share a proto name. That was already untrue (foo_bar and foo__bar), and self beside self_ is another case, so it now says such names collide and are not diagnosed. That is unchanged by this PR: both became Self before.
  • The changelog fragment is one paragraph and cites (#465), as you offered.

@iainmcgin
iainmcgin added this pull request to the merge queue Sep 25, 2026
Merged via the queue into anthropics:main with commit b47ac1c Sep 25, 2026
11 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 25, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants