Fix public import classification when paths differ in public-ness - #754
Open
bufdev wants to merge 1 commit into
Open
Fix public import classification when paths differ in public-ness#754bufdev wants to merge 1 commit into
bufdev wants to merge 1 commit into
Conversation
`imports.Recurse` classified each transitive import by looking only at whichever direct import happened to pull it in first, so a file reachable through several direct imports with differing public-ness could be misclassified. Two shapes were affected, both of which `protoc` accepts: // reexport.proto import public "plain_leaf.proto"; // imports leaf.proto non-publicly import public "public_leaf.proto"; // imports leaf.proto publicly `plain_leaf.proto` is visited first, so leaf.proto landed in the trailing non-public segment and reexport.proto stopped re-exporting it. #665 patched the `visible` half of this with `seenPublicImport`, which is why the file itself still compiled, but importers of it did not see leaf.proto. // reexport.proto import "leaf.proto"; import public "public_leaf.proto"; // imports leaf.proto publicly Here leaf.proto is re-exported, but it has to stay in a direct segment, and `Transitive` derived public-ness purely from the segment offsets. Classify every transitive import up front, quantifying over all direct imports, and record public-ness on `imported` rather than deriving it from the segment offsets. The offsets still order the table, but they are no longer the source of truth for `Transitive().Public`. Fixes bufbuild/buf#4633.
doriable
self-requested a review
August 4, 2026 19:45
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.
Fixes bufbuild/buf#4633. Since
bufv1.68.0 (which switched to the new compiler), a symbol re-exported viaimport publicis not always found by importers.protocand the legacy compiler accept the same files.Root cause
imports.Recurseclassified each transitive import by looking only at whichever direct import happened to pull it in first, so a file reachable through several direct imports with differing public-ness could be misclassified. Two shapes were affected, both of whichprotocaccepts.1. The public path is not visited first.
Public imports are ordered first in
Directs()precisely so that the public path wins, but that does not disambiguate when both direct imports are public.plain_leaf.protois visited first,file.Public && imp.Publicis false, andleaf.protolands in the trailing non-public segment, soreexport.protostops re-exporting it.#665 patched the
visiblehalf of this withseenPublicImport, which is whyreexport.protoitself still compiled. ButTransitive().Publicwas left wrong, so importers ofreexport.protocould not nameLeaf.2. A direct import that is also re-exported.
reexport.protore-exportsleaf.protoviapublic_leaf.proto, butleaf.protohas to stay in a direct segment, andTransitive()derived public-ness purely from the segment offsets, so it could not represent this at all.Fix
Classify every transitive import up front in
classifyTransitive, quantifying over all direct imports, and record public-ness as a bit onimportedrather than deriving it from the segment offsets. The offsets still order the table, but they are no longer the source of truth forTransitive().Public.This makes the classification match
protoc'sDescriptorBuilder::RecordPublicDependencies, which is a closure overpublic_dependencyedges and therefore order-independent.Testing
TestImportResolutiongains the two shapes above; both fail without the fix. It also now assertsImport.Visible, which had no coverage.imports/public_diamondandimports/public_shadowed; theirfds.yamloutput was checked byte-for-byte againstprotoc33.3, including thatpublic_dependencyis unchanged in shape 2.protoc: 700 random import DAGs of up to 12 files, each file referencing every symbolprotoc's rule says is visible (zero rejections), plus 838 pairwise accept/reject probes agreeing withprotocin both directions. The same fuzzer flags the bug on an unpatched build within ~30 seeds.