Skip to content

fix(tonic-prost-build): avoid super::primitive for wrapper RPC types#2534

Closed
mingley wants to merge 9 commits intohyperium:masterfrom
mingley:mingley/fix-wkt-wrapper-super-primitive
Closed

fix(tonic-prost-build): avoid super::primitive for wrapper RPC types#2534
mingley wants to merge 9 commits intohyperium:masterfrom
mingley:mingley/fix-wkt-wrapper-super-primitive

Conversation

@mingley
Copy link

@mingley mingley commented Mar 3, 2026

Summary

  • Fix tonic-prost-build wrapper WKT RPC type generation by classifying google types from input_proto_type / output_proto_type (not resolved Rust type fields).
  • Preserve existing behavior for dotted protobuf paths, extern paths, and non-path allowlist handling.
  • Add regression tests for both observed shapes:
    • prost-resolved primitive shape (BoolValue -> bool, UInt32Value -> u32)
    • legacy dotted wrapper shape.
  • Expand tests/wellknown wrapper fixture coverage (BoolValue, BytesValue, DoubleValue, FloatValue, Int32Value, Int64Value, UInt32Value, UInt64Value, StringValue).

Root Cause

This regressed in #2321 (969408e, prost extraction).

  • Before that change, classification effectively used protobuf type info.
  • After that change, classification relied on input_type / output_type.
  • For wrappers, those are already primitive Rust types, so google-WKT handling was skipped and fallback path rendering produced invalid types like super::bool.

Fix

  • In request_response_name, classify with input_proto_type / output_proto_type.
  • If non-compiled google WKT:
    • emit resolved Rust primitive directly when available (bool, u32, etc.)
    • otherwise map dotted wrapper/WKT paths to the correct Rust/prost types.
  • Keep existing path handling behavior unchanged for non-google and extern types.

Risk / Compatibility

  • Scope is limited to request/response type tokenization in tonic-prost-build.
  • Existing extern-path and non-wrapper behavior is preserved by existing tests.
  • compile_well_known_types(true) behavior remains unchanged.

Testing Done

  • cargo fmt
  • cargo test -p tonic-prost-build -p wellknown
  • cargo clippy -p tonic-prost-build -p wellknown --all-targets --all-features -- -D warnings
  • Wrapper-specific regression check:
    • cargo test -p tonic-prost-build wrapper_types

Reverse-dependency spot checks for crates depending on tonic-prost-build:

  • Pass: tonic-prost-build, codegen, examples, test-compile, compression, default_stubs, deprecated_methods, disable-comments, my_application, integration-tests, test_web, wellknown-compiled, wellknown
  • interop: locally blocked by macOS arm64 linker error in tonic-protobuf-build; interop is covered in CI.

Resolves

Future Testing Enhancement (Follow-on)

  • Should we add a wrapper-focused compile_well_known_types(true) regression test in tonic-prost-build to explicitly lock that behavior for future refactors?

Michael Ingley added 6 commits March 3, 2026 12:53
Use input_proto_type/output_proto_type for google.protobuf detection and keep rust_type emission for already-resolved wrapper primitives from prost-build.

Add a unit test that reproduces the super::bool regression shape and extend the wellknown compile fixture with BoolValue RPC coverage.
Map wrapper WKTs to primitives even when a dotted protobuf wrapper name is present, and add regression coverage for that legacy shape.
Extract google dotted-type mapping into a helper and flatten convert_type with early returns while preserving behavior.
@mingley
Copy link
Author

mingley commented Mar 13, 2026

Closing this out because upstream PR #2544 already merged the same issue family into master at 3cd155f.

I verified the overlap locally while resolving the branch conflict against current master: both changes address the well-known-wrapper request/response type failure in tonic-prost-build. The implementations differ, but #2544 covers the upstream fix path, so this PR is no longer needed.

@mingley mingley closed this Mar 13, 2026
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.

1 participant