feat: 添加 SonicDeserialize derive - #240
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #240 +/- ##
==========================================
+ Coverage 71.16% 72.30% +1.14%
==========================================
Files 42 43 +1
Lines 9762 9778 +16
==========================================
+ Hits 6947 7070 +123
+ Misses 2815 2708 -107 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f18676a3ea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let serde_path = container | ||
| .serde_path | ||
| .clone() | ||
| .unwrap_or_else(|| syn::parse_str("::serde").expect("valid serde path")); |
There was a problem hiding this comment.
When a downstream crate enables sonic-rs/derive but does not also declare serde directly, every expansion fails because ::serde is resolved in that downstream crate, where sonic-rs's transitive dependency is not in the extern prelude. This breaks the natural advertised usage of the re-exported sonic_rs::SonicDeserialize; expose Serde through sonic-rs's private derive support and use that as the default path, while retaining the override for renamed crates.
Useful? React with 👍 / 👎.
| for _ in &field.names { | ||
| entries.push((fields_index, id)); | ||
| fields_index += 1; | ||
| } |
There was a problem hiding this comment.
When a non-self-describing deserializer supplies numeric struct-field identifiers and a field has aliases, this loop allocates an index for every alias. Serde's derive assigns numeric indices per deserializable field and treats aliases only as additional string spellings, so for fields first (alias alias) and second, numeric key 1 should select second but this implementation selects first. Increment the numeric index once per field and map only that index to the field.
Useful? React with 👍 / 👎.
| let canonical = attrs | ||
| .deserialize_name | ||
| .clone() | ||
| .unwrap_or_else(|| ident.to_string()); |
There was a problem hiding this comment.
When a struct declares an unrenamed raw identifier such as r#type, Ident::to_string() produces r#type, but Serde's external field name is type. Consequently normal input like {"type": 1} is treated as unknown and the derive reports the r#type field missing. Normalize raw identifiers before constructing the canonical wire name.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🟡 Changes recommended
The current derive codegen has confirmed behavioral mismatches with Serde semantics for sequence skipping and numeric field identifier mapping that can break correctness/compatibility.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces an optional SonicDeserialize derive (via a new sonic-derive proc-macro crate) that generates serde::Deserialize<'de> implementations optimized for wide named structs by using compile-time PHF-based field dispatch while keeping the public deserialization entrypoints unchanged.
Changes:
- Added
sonic-deriveproc-macro crate implementing#[derive(SonicDeserialize)]with an MVP Serde attribute subset and fail-closed behavior for unsupported semantics. - Added root
derivefeature that gatessonic_derive::SonicDeserializere-export and exposes a hidden__privatemodule for downstream macro expansions (including amissing_fieldhelper andphfre-export). - Added integration tests/bench-style ignored tests comparing behavior with Serde derive and measuring dispatch overhead.
File summaries
| File | Description |
|---|---|
| tests/sonic_deserialize.rs | Integration tests for SonicDeserialize correctness and a local benchmark for wide-struct dispatch. |
| tests/deserialize_with_benchmark.rs | Ignored benchmark-style test focusing on deserialize_with overhead comparisons. |
| src/lib.rs | Adds self-crate aliasing, gates __private, and re-exports SonicDeserialize behind the derive feature. |
| src/__private.rs | Hidden support module for macro expansions (PHF re-export and missing-field helper). |
| sonic-derive/src/model.rs | Builds a struct model from syn input and validates MVP constraints. |
| sonic-derive/src/lib.rs | Proc-macro entrypoint for SonicDeserialize and compile-time rejection tests. |
| sonic-derive/src/codegen.rs | Generates optimized Deserialize impls with PHF/match dispatch and map/seq handling. |
| sonic-derive/src/attr.rs | Parses supported serde/sonic attributes into a simplified model. |
| sonic-derive/Cargo.toml | Defines the new proc-macro crate and dependencies. |
| Cargo.toml | Adds optional phf + sonic-derive deps and the new derive feature. |
Review details
Suppressed comments (1)
sonic-derive/src/codegen.rs:397
numeric_field_entriescurrently assigns numeric indices once per (canonical+alias) name, which makesvisit_u64depend on how many aliases a field has. Serde's derivedvisit_u64expects numeric identifiers to index canonical fields only; aliases should not affect numeric mapping.
fn numeric_field_entries(fields: &[FieldInfo]) -> Vec<(u64, usize)> {
let mut fields_index = 0u64;
let mut entries = Vec::new();
for field in fields {
if let Some(id) = field.de_id {
for _ in &field.names {
entries.push((fields_index, id));
fields_index += 1;
}
}
}
entries
- Files reviewed: 10/10 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } else { | ||
| let missing = skipped_expr(field, &container.default); | ||
| quote!(let #var = #missing;) | ||
| } |
| let field_names: Vec<_> = model | ||
| .fields | ||
| .iter() | ||
| .filter(|field| field.de_id.is_some()) | ||
| .flat_map(|field| field.names.iter()) | ||
| .map(|name| LitStr::new(name, Span::call_site())) | ||
| .collect(); | ||
| let numeric_entries = numeric_field_entries(&model.fields); |
| #[test] | ||
| fn numeric_field_indexes_include_alias_entries() { | ||
| let input = vec![(2_u64, 20_i64)].into_iter(); | ||
| let fast = FastNumericFields::deserialize(serde::de::value::MapDeserializer::< |
| #[test] | ||
| fn sequence_representation_matches_serde() { | ||
| let control: ControlRequired = sonic_rs::from_str("[null,9]").unwrap(); | ||
| let fast: FastRequired = sonic_rs::from_str("[null,9]").unwrap(); | ||
| assert_eq!(control.optional, fast.optional); | ||
| assert_eq!(control.required, fast.required); | ||
| } |
背景
Serde derive 为 named struct 生成字段名分派逻辑时，宽结构体会产生较大的字符串匹配开销。sonic-rs 的 token parser 已经很快，但在字段很多的 typed deserialization 场景里，
FieldVisitor::visit_str仍可能成为主要成本。这个 PR 增加一个可选的
SonicDeserializederive，用于在保持官方serde::Deserialize<'de>接口不变的前提下，为宽 named struct 生成更快的字段分派代码。改动方案
sonic-deriveproc-macro crate，提供#[derive(SonicDeserialize)]。derivefeature，并在开启 feature 时 re-exportsonic_derive::SonicDeserialize。match；对宽 struct 使用编译期 PHF 表。rename、deserialize-siderename、alias、字段/容器default、deserialize_with、skip_deserializing、deny_unknown_fields、map/seq 两种表示。没有引入新的 deserialization trait，也没有新增
from_str_fast/from_slice_fast这类平行入口；生成代码仍实现官方serde::Deserialize<'de>。影响范围
derive时不暴露新宏，也不依赖phf/sonic-derive。derive后新增 hidden__private模块，供 proc-macro expansion 在 downstream crate 中引用。deserialize_with继续使用 wrapper 形态调用，避免改变现有 custom decoder 语义。#[derive(Deserialize)]。发布注意：root crate 依赖新的 proc-macro crate。正式发布时需要先发布
sonic-derive，再发布开启可选依赖的 root crate；否则 root crate 的 package/publish 检查会在 registry 找不到 derive crate。测试与验证
RUSTUP_TOOLCHAIN=nightly-x86_64-unknown-linux-gnu rustfmt --check sonic-derive/src/lib.rs sonic-derive/src/attr.rs sonic-derive/src/model.rs sonic-derive/src/codegen.rs src/__private.rs tests/sonic_deserialize.rs tests/deserialize_with_benchmark.rsRUSTUP_TOOLCHAIN=nightly-x86_64-unknown-linux-gnu cargo test --manifest-path sonic-derive/Cargo.tomlRUSTUP_TOOLCHAIN=nightly-x86_64-unknown-linux-gnu cargo test --release --features derive --test sonic_deserialize -- --nocaptureRUSTUP_TOOLCHAIN=nightly-x86_64-unknown-linux-gnu cargo test --release --features derive --test deserialize_with_benchmark -- --nocaptureRUSTUP_TOOLCHAIN=nightly-x86_64-unknown-linux-gnu cargo check --no-default-featuresRUSTUP_TOOLCHAIN=nightly-x86_64-unknown-linux-gnu cargo package --manifest-path sonic-derive/Cargo.toml --allow-dirty --no-verifyThe integration tests cover alias/rename/default/skip/
deserialize_with/unknown/sequence/missing-field behavior and compare generated output with Serde derive on synthetic wide structs.