Skip to content

feat(compiler): reject non-thread-safe payload types in Rust compiler#3732

Closed
BaldDemian wants to merge 1 commit into
apache:mainfrom
BaldDemian:rust-sync-send
Closed

feat(compiler): reject non-thread-safe payload types in Rust compiler#3732
BaldDemian wants to merge 1 commit into
apache:mainfrom
BaldDemian:rust-sync-send

Conversation

@BaldDemian
Copy link
Copy Markdown
Contributor

@BaldDemian BaldDemian commented Jun 2, 2026

Why?

This is a prerequisite PR for Rust gRPC code generation.

In the Rust gRPC code generation requirement, we chose tonic as the network transport layer for gRPC, so the generated Rust code needs to be compatible with tonic.

In tonic, a gRPC payload (a request or a response) type must be both Send and Sync, e.g.

https://docs.rs/tonic/latest/tonic/client/struct.Grpc.html#method.unary

However, in Fory, users can define the following types, which cause the generated Rust types to be neither Send nor Sync:

  • Non-thread-safe refs
  • any

In addition, Fory currently generates an UnknownCase by default for a union type.

lines.append(" #[fory(unknown)]")
lines.append(" Unknown(::fory::UnknownCase),")

However, the type of its value field is Arc<dyn Any>, rather than something like Arc<dyn Any + Send + Sync>.

#[derive(Clone)]
pub struct UnknownCase {
case_id: u32,
type_id: u32,
// Keep resolver TypeInfo/Rc out of the carrier. Generated unions can outlive or move
// independently from the resolver context, so the carrier stores only stable metadata
// plus the dynamic payload owned by Rust's existing polymorphic Arc path.
value: Arc<dyn Any>,
}

What does this PR do?

In this PR, I made the following changes:

  1. Before generating Rust gRPC stub code, all types used by gRPC payloads are checked via graph traversal. If their fields contain non-thread-safe refs or any, an error will be raised directly with a corresponding diagnostic message for the user.
  2. UnknownCase is not generated for unions used by gRPC payloads. Currently, I use a special macro attribute #[fory(no_unknown_case)] to make the derive macro logic handle this case specially, rather than directly panicking when encountering a union without an UnknownCase.
  3. Add testcases accordingly.

It is worth noting that, for simplicity, if a gRPC payload type contains imported types, the current implementation will directly raise an error and exit.

Related issues

#3266

AI Contribution Checklist

  • Substantial AI assistance was used in this PR: no
  • If yes, I included a completed AI Contribution Checklist in this PR description and the required AI Usage Disclosure.
  • If yes, my PR description includes the required ai_review summary and screenshot evidence of the final clean AI review results from both fresh reviewers on the current PR diff or current HEAD after the latest code changes.

Does this PR introduce any user-facing change?

All unions in gRPC payload types will not generate UnknownCase.

Benchmark

N/A.

@BaldDemian
Copy link
Copy Markdown
Contributor Author

To address the Send and Sync issue, I have tried several other approaches. However, the current implementation appears to be the simplest one, although it may slightly affect the original design intent of UnknownCase. Happy to hear your feedback or advice @chaokunyang

chaokunyang added a commit that referenced this pull request Jun 3, 2026
)

## Why?



## What does this PR do?



## Related issues

Closes #3732

## AI Contribution Checklist



- [ ] Substantial AI assistance was used in this PR: `yes` / `no`
- [ ] If `yes`, I included a completed [AI Contribution
Checklist](https://github.com/apache/fory/blob/main/AI_POLICY.md#9-contributor-checklist-for-ai-assisted-prs)
in this PR description and the required `AI Usage Disclosure`.
- [ ] If `yes`, my PR description includes the required `ai_review`
summary and screenshot evidence of the final clean AI review results
from both fresh reviewers on the current PR diff or current HEAD after
the latest code changes.



## Does this PR introduce any user-facing change?



- [ ] Does this PR introduce any public API change?
- [ ] Does this PR introduce any binary protocol compatibility change?

## Benchmark
@BaldDemian
Copy link
Copy Markdown
Contributor Author

@chaokunyang Thanks for addressing the issue, this really saved my day 🚀

chaokunyang added a commit that referenced this pull request Jun 4, 2026
## Why?



## What does this PR do?



## Related issues

#3732 
#3736

## AI Contribution Checklist



- [ ] Substantial AI assistance was used in this PR: `yes` / `no`
- [ ] If `yes`, I included a completed [AI Contribution
Checklist](https://github.com/apache/fory/blob/main/AI_POLICY.md#9-contributor-checklist-for-ai-assisted-prs)
in this PR description and the required `AI Usage Disclosure`.
- [ ] If `yes`, my PR description includes the required `ai_review`
summary and screenshot evidence of the final clean AI review results
from both fresh reviewers on the current PR diff or current HEAD after
the latest code changes.



## Does this PR introduce any user-facing change?



- [ ] Does this PR introduce any public API change?
- [ ] Does this PR introduce any binary protocol compatibility change?

## Benchmark
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