fix: always fully-qualify Option to prevent cross-file prelude shadowing#67
Merged
iainmcgin merged 1 commit intoanthropics:mainfrom Apr 27, 2026
Merged
Conversation
th0114nd
force-pushed
the
fix/cross-file-option-shadow
branch
from
46bbb4e to
6b6f8a9
Compare
iainmcgin
approved these changes
Apr 27, 2026
iainmcgin
approved these changes
Apr 27, 2026
Collaborator
iainmcgin
left a comment
iainmcgin
left a comment
There was a problem hiding this comment.
[claude code] Correct fix for cross-file prelude shadowing — per-file detection cannot catch this since each file is generated independently and merged later by the stitcher. Unconditional ::core::option::Option makes message.rs consistent with the rest of the codegen. Integration test in prelude_shadow_sibling.proto proves it end-to-end. Thanks for the thorough analysis and fix.
…ing (anthropics#67) The per-package stitcher (include!) combines all .proto files from one package into a single Rust module scope. If any file in the package defines `message Option`, it shadows core::option::Option for the entire module — including code generated from sibling files that do not define Option themselves. The previous ImportResolver per-file detection cannot catch this since each file is generated independently. Rather than extending the resolver with sibling-awareness (which adds complexity and breaks per-file plugin invocation), unconditionally emit ::core::option::Option everywhere. This makes message.rs consistent with view.rs/enumeration.rs/ impl_message.rs which already qualify unconditionally. ImportResolver becomes a stateless unit struct retained as the single source of truth for type-path emission. Adds prelude_shadow_sibling.proto (shares package with prelude_shadow.proto) as an end-to-end integration test. Fixes anthropics#64. Co-authored-by: Tim Holland <th0114nd@users.noreply.github.com> Co-authored-by: Iain McGinniss <309153+iainmcgin@users.noreply.github.com>
iainmcgin
force-pushed
the
fix/cross-file-option-shadow
branch
from
193ea23 to
1427936
Compare
Collaborator
|
[claude code] Thanks for the contribution and the thorough cross-file analysis — this was a real gap. |
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 #64.
The per-package stitcher (
include!) combines all.protofiles from one package into a single Rust module scope. If any file in the package definesmessage Option, it shadowscore::option::Optionfor the entire module — including code generated from sibling files that don't defineOptionthemselves.The previous
ImportResolver::for_fileonly checked names within a single file, so it missed cross-file shadowing. Rather than extending the resolver with sibling-awareness (which adds complexity and could still miss edge cases in future include patterns), this PR unconditionally emits::core::option::Optioneverywhere. Generated code readability is not a concern, and this eliminates the entire class of prelude-shadowing bugs.Changes
imports.rs: Removed collision-detection machinery (PRELUDE_NAMES,check_names_for_prelude_collisions,blockedset,for_file,for_file_with_siblings,child_for_message,is_available).ImportResolver::option()now unconditionally returns::core::option::Option.message.rs: Removedchild_resolver(no longer needed since qualification is unconditional).lib.rs: UseImportResolver::new()instead offor_file.test_cross_file_message_named_option_shadows_preludereproducing the bug. Updated 9 assertions across unit and integration tests to expect::core::option::Option<...>.Reproduction
Two
.protofiles in the same package:message.proto: definesmessage Option { string text = 1; }session.proto: definesmessage Wrapper { oneof kind { string a = 1; } }Before this fix,
session.rsemittedpub kind: Option<Kind>— the bareOptionresolved tomessage.proto'spub struct Optioninstead ofcore::option::Option.Test plan
test_cross_file_message_named_option_shadows_preludefails before fix, passes afterMade with Cursor