Skip to content

descriptor: resolve a type name without a leading dot against its enclosing scopes - #811

Open
dfedoryshchev wants to merge 1 commit into
anthropics:mainfrom
dfedoryshchev:fix/relative-type-name
Open

dfedoryshchev wants to merge 1 commit into
anthropics:mainfrom
dfedoryshchev:fix/relative-type-name

Conversation

@dfedoryshchev

Copy link
Copy Markdown
Contributor

Closes #635.

#797 targets the same issue. As I noted there, it resolves a dotted name whole at every scope, while protoc resolves only the first component against the scopes; this one follows protoc. If you would rather take #797 with that changed, I am happy to close this.

DescriptorPool strips an optional leading dot and looks the rest up as one fully-qualified name, so on a hand-built descriptor type_name: "M" on a field of a.Holder fails with UnresolvedTypeName, or links to a root-level M from an imported file with no package where protoc and protobuf-go link a.M.

I took the first option from the issue: a new lookup_type_name follows LookupSymbolNoPlaceholder. A leading dot means fully qualified; otherwise each scope enclosing the field, extension or method is tried innermost first, then the name as written. A compound name A.B stops at the innermost scope that defines A, and a match in a file that is not imported is passed over for one further out, as protoc's FindSymbol does. resolve_singular and resolve_message_type_name both go through it, so type_name, extendee, input_type and output_type all get it.

Names with a leading dot, which is everything protoc writes, take the same single map lookup as before. The test that pinned "Holder" as unresolved now uses names no type has, and the docs on UnresolvedTypeName and add_file_descriptor_set say what the pool does now.

One place I went simpler than protoc: the pool has no package symbols, so for a compound name a scope counts as a package if any registered symbol sits under it. A package that declares nothing cannot be the target of a lookup anyway, so I don't think the difference shows, but say if you'd rather I register packages properly.

New tests in pool_e2e.rs cover the issue's case, a nested and a compound name, a dotless full name, method types, the protoc Bar.Baz example that must not resolve, a partly qualified name through the package, and the pass-over of a non-imported match. On main, three of the four new tests fail; the Bar.Baz one passes there too, since it guards against resolving too much. With the fix, cargo test -p buffa-descriptor --features reflect --test pool_e2e passes (168), and rustfmt and clippy are clean on the crate. I have not run the full workspace or conformance locally.

@github-actions

Copy link
Copy Markdown

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

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

descriptor: a type_name without a leading dot is looked up as fully qualified instead of being resolved against the enclosing scope

1 participant