Add simple_shard proc macro for simplified shard definitions #1214
Conversation
Introduces a new `#[simple_shard]` attribute macro that reduces boilerplate for simple transform shards by ~60-77%. Features: - ShardType trait for automatic Rust-to-Shards type mapping - Auto-generates struct, Default, and all Shard trait implementations - Parameters via #[param] (ClonedVar) and #[param_var] (ParamVar) - Automatically includes both base type and var type for params - Proper compose handling for context variable parameters - Memory management via ClonedVar for outputs Converts yaml.rs and argon.rs as examples of the new syntax.
Code Review: Add simple_shard proc macro for simplified shard definitionsThis is a well-designed macro that significantly reduces boilerplate for simple transform shards. The implementation is clean and the examples demonstrate good reduction in code (~60-77% as claimed). Here's my detailed review: ✅ Strengths
🔴 Issues to Address1. Naming Inconsistency in yaml.rs (line 26)#[simple_shard("Yaml.FromJson", "A shard that converts JSON to YAML.")]
fn json_to_yaml(...)The function is named
2. Missing Bounds Validation in argon.rs (lines 20-22)let memory_cost = u32::try_from(memory_cost).map_err(|_| "Invalid memory cost")?;No upper bound checking. Argon2 has limits (e.g., memory should be reasonable, not 3. Potential Panic in macro (lib.rs:1180)argon2::Params::new(memory_cost, time_cost, parallelism, None).unwrap()Wait, I see this was actually fixed in the new code with 4. Missing
|
PR Review: Add simple_shard proc macro for simplified shard definitionsThis is an excellent addition that significantly reduces boilerplate for simple transform shards. The macro design is clean and the automatic type inference via Summary👍 Overall: Good to merge with fixes The macro implementation is solid and the converted shards demonstrate the value proposition well (77% reduction in yaml.rs, 69% in argon.rs). Issues to Address1. Naming Inconsistency in yaml.rs (Bug)register_shard::<YamlToJsonShard>();
register_shard::<YamlFromJsonShard>(); // Should this be JsonToYamlShard?The function is named The shard name "Yaml.FromJson" is semantically confusing - it reads as "Yaml from JSON" but could be misread as "Yaml.FromJson outputs Yaml". Consider renaming to "Json.ToYaml" for clarity, matching the function name pattern. 2. Missing Required Parameter Validation (Bug)In #[param_var("Hash", "The Argon2id hash to verify against.")]
hash: &str,The original code had explicit validation in if self.hash.is_none() {
return Err("Hash parameter is required");
}The new macro doesn't generate this validation. When Fix: Add compose-time validation for required 3. Minor: Cleanup calls signature mismatchIn the macro-generated code: fn cleanup(&mut self, context: std::option::Option<&shards::types::Context>) -> std::result::Result<(), &str> {
if let Some(context) = context {
#(#param_cleanups)*
}
// ...
}But Suggestions for Improvement1. Error Messages Lose ContextThe original argon.rs had error handling that could be improved: argon2::Params::new(memory_cost, time_cost, parallelism, None).unwrap()The new version properly handles this: argon2::Params::new(memory_cost, time_cost, parallelism, None)
.map_err(|_| "Invalid Argon2 parameters")?👍 Good improvement! 2. Consider Adding
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a #[simple_shard] procedural macro that dramatically reduces boilerplate for defining simple transform shards in Rust. The macro automatically generates struct definitions, trait implementations, and parameter handling code from annotated functions. It achieves this through a new ShardType trait that maps Rust types to Shards type system types, enabling automatic type inference.
Key changes:
- New
simple_shardmacro in rust_macro/src/lib.rs (~512 lines) that generates complete shard implementations from annotated functions - ShardType trait in types/shard_type.rs providing bidirectional Rust⟷Shards type mappings for primitives, strings, tuples, and bytes
- Conversion of yaml.rs and argon.rs shards to the new syntax, demonstrating 60-77% reduction in boilerplate code
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| shards/rust_macro/src/lib.rs | Implements the simple_shard macro with parameter parsing, type extraction, and code generation for complete shard implementations |
| shards/rust/src/types/shard_type.rs | Adds ShardType trait with implementations for common Rust types, enabling automatic type inference in the macro |
| shards/rust/src/types/mod.rs | Re-exports ShardType trait for use with simple_shard macro |
| shards/rust/src/lib.rs | Re-exports simple_shard macro at the library level |
| shards/modules/crypto/src/argon.rs | Converts Argon2id shards to use simple_shard macro, demonstrating cleaner syntax and better error handling |
| shards/modules/core/src/yaml.rs | Converts YAML conversion shards to use simple_shard macro, showing significant reduction in boilerplate |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
shards/rust_macro/src/lib.rs
Outdated
| struct SimplShardAttrArgs { | ||
| name: LitStr, | ||
| help: LitStr, | ||
| } | ||
|
|
||
| impl syn::parse::Parse for SimplShardAttrArgs { |
There was a problem hiding this comment.
Typo in struct name: SimplShardAttrArgs should be SimpleShardAttrArgs for consistency with the naming pattern used elsewhere (e.g., SimpleParamInfo, SimpleParamAttr).
| struct SimplShardAttrArgs { | |
| name: LitStr, | |
| help: LitStr, | |
| } | |
| impl syn::parse::Parse for SimplShardAttrArgs { | |
| struct SimpleShardAttrArgs { | |
| name: LitStr, | |
| help: LitStr, | |
| } | |
| impl syn::parse::Parse for SimpleShardAttrArgs { |
shards/rust_macro/src/lib.rs
Outdated
| } | ||
| } | ||
|
|
||
| fn generate_simple_shard(args: SimplShardAttrArgs, func: syn::ItemFn) -> Result<TokenStream, Error> { |
There was a problem hiding this comment.
Typo in struct name: SimplShardAttrArgs should be SimpleShardAttrArgs for consistency with the naming pattern used elsewhere (e.g., SimpleParamInfo, SimpleParamAttr).
shards/rust_macro/src/lib.rs
Outdated
| /// trait implementations. | ||
| #[proc_macro_attribute] | ||
| pub fn simple_shard(attr: TokenStream, item: TokenStream) -> TokenStream { | ||
| let args = syn::parse_macro_input!(attr as SimplShardAttrArgs); |
There was a problem hiding this comment.
Typo in struct name: SimplShardAttrArgs should be SimpleShardAttrArgs for consistency with the naming pattern used elsewhere (e.g., SimpleParamInfo, SimpleParamAttr).
| let args = syn::parse_macro_input!(attr as SimplShardAttrArgs); | |
| let args = syn::parse_macro_input!(attr as SimpleShardAttrArgs); |
| // Rest are parameters - parse #[param(...)] or #[param_var(...)] attribute | ||
| let mut param_name = arg_name.to_string(); | ||
| let mut param_desc = String::new(); | ||
| let mut param_default: Option<syn::Expr> = None; | ||
| let mut is_var = false; | ||
|
|
||
| for attr in &pat_type.attrs { | ||
| if attr.path().is_ident("param") { | ||
| let parsed: SimpleParamAttr = attr.parse_args()?; | ||
| param_name = parsed.name; | ||
| param_desc = parsed.description; | ||
| param_default = parsed.default; | ||
| } else if attr.path().is_ident("param_var") { | ||
| // For context variable parameters | ||
| let parsed: SimpleParamAttr = attr.parse_args()?; | ||
| param_name = parsed.name; | ||
| param_desc = parsed.description; | ||
| param_default = parsed.default; | ||
| is_var = true; | ||
| } | ||
| } | ||
|
|
||
| params.push(SimpleParamInfo { | ||
| name: param_name, | ||
| rust_name: arg_name, | ||
| rust_type: arg_type, | ||
| description: param_desc, | ||
| default: param_default, | ||
| is_var, | ||
| }); |
There was a problem hiding this comment.
[nitpick] Parameters without #[param] or #[param_var] attributes are silently processed with default metadata (empty description, arg name as parameter name). This could lead to confusing behavior. Consider requiring one of these attributes for all parameters after the input argument, or at minimum documenting this behavior.
shards/rust_macro/src/lib.rs
Outdated
| struct SimplShardAttrArgs { | ||
| name: LitStr, | ||
| help: LitStr, | ||
| } | ||
|
|
||
| impl syn::parse::Parse for SimplShardAttrArgs { |
There was a problem hiding this comment.
Typo in struct name: SimplShardAttrArgs should be SimpleShardAttrArgs for consistency with the naming pattern used elsewhere (e.g., SimpleParamInfo, SimpleParamAttr).
| struct SimplShardAttrArgs { | |
| name: LitStr, | |
| help: LitStr, | |
| } | |
| impl syn::parse::Parse for SimplShardAttrArgs { | |
| struct SimpleShardAttrArgs { | |
| name: LitStr, | |
| help: LitStr, | |
| } | |
| impl syn::parse::Parse for SimpleShardAttrArgs { |
- Add unit type () input support for generator shards - Support plain return types (non-Result) for infallible shards - Add ShardType for i64/f64 vector types and [u8; 16] - Add From<Vec<u8>>, From<[u8; 16]>, From<String> for ClonedVar - Fix typo SimplShardAttrArgs -> SimpleShardAttrArgs - Add #[inline] to generated inner functions - Convert uuid.rs shards to simple_shard (82% reduction)
PR Review: Add simple_shard proc macro for simplified shard definitionsThis PR introduces a well-designed Strengths
Issues to AddressCritical
Moderate
Minor
Suggestions for Future Enhancement
Questions
VerdictThis is a well-implemented feature that will significantly improve developer experience when writing simple shards. The converted examples (UUID, YAML, Argon2) demonstrate the macro works correctly for real-world use cases. Recommend merge after addressing:
The other items are improvements that could be addressed in follow-up PRs. 🤖 Generated with Claude Code |
sinkingsugar
force-pushed
the
feature/simple-shard-macro
branch
from
2461801 to
2d12923
Compare
PR Review: Add simple_shard proc macro for simplified shard definitionsThis is a well-designed ergonomic improvement that significantly reduces boilerplate for simple transform shards. The implementation demonstrates good Rust macro design patterns. Strengths1. Excellent Developer Experience
2. Good Code Quality
3. Solid Examples
Issues to Address1. Missing Required Parameter Validation (Important)In #[simple_shard("Argon2id.Verify", "Verifies a password against an Argon2id hash.")]
fn argon2id_verify(
password: &str,
#[param_var("Hash", "The Argon2id hash to verify against.")]
hash: &str,
) -> Result<bool, &'static str> {The old implementation had: fn compose(&mut self, data: &InstanceData) -> Result<Type, &str> {
if self.hash.is_none() {
return Err("Hash parameter is required");
}
// ...
}The simple_shard macro doesn't generate this validation. Users will get a confusing runtime error when Recommendation: Add support for a 2. Inconsistent Shard Name in Registration (Bug)In pub fn register_shards() {
register_shard::<YamlToJsonShard>();
register_shard::<YamlFromJsonShard>(); // Function is json_to_yaml, shard name is "Yaml.FromJson"
}The generated struct for 3.
|
sinkingsugar
force-pushed
the
feature/simple-shard-macro
branch
from
2d12923 to
1023525
Compare
PR Review: Add simple_shard proc macro for simplified shard definitionsThis PR introduces a ✅ Strengths
🔴 Critical Issues1. Bug in Argon2id.Verify - Missing required parameter validation
The old implementation validated that if self.hash.is_none() {
return Err("Hash parameter is required");
}The new
2. Cleanup inconsistency - context may be None
fn cleanup(&mut self, context: std::option::Option<&shards::types::Context>) -> std::result::Result<(), &str> {
if let Some(context) = context {
#(#param_cleanups)*
}But the cleanup calls generated use quote! { self.#name.cleanup(Some(context)); }This would pass quote! { self.#name.cleanup(context); }Where 3. Regression: UUID.ToString help text changedThe old help text was "Reads an UUID" (grammatically correct for spelled-out "U-U-I-D") but the new one says "Reads a UUID" - minor but this is inconsistent with the original. 🟡 Potential Issues1. Error handling is less informative
Original: Consider preserving error details. The macro could support 2. Unnecessary type wrapping for
|
Avoid extra allocations by returning typed ClonedVar wrappers directly. The macro detects these types and extracts the inner ClonedVar without calling .into() on intermediate allocations.
sinkingsugar
force-pushed
the
feature/simple-shard-macro
branch
from
1023525 to
49d5cd1
Compare
PR Review: Add simple_shard proc macro for simplified shard definitionsSummaryThis PR introduces a ✅ Strengths
🔴 Critical Issues1. Memory allocation in cleanup for output resetLocation: fn cleanup(&mut self, context: std::option::Option<&shards::types::Context>) -> std::result::Result<(), &str> {
if let Some(context) = context {
#(#param_cleanups)*
}
self.output = shards::types::ClonedVar::default(); // Potential issue
Ok(())
}This unconditionally allocates a new default 2. Missing required parameter validation in compose for
|
PR Review: Add simple_shard proc macro for simplified shard definitionsOverviewThis PR introduces a new Positives ✅
Issues to Address1. Critical: Missing required parameter validation in
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## devel #1214 +/- ##
==========================================
- Coverage 77.34% 77.30% -0.05%
==========================================
Files 386 385 -1
Lines 60709 60215 -494
==========================================
- Hits 46958 46548 -410
+ Misses 13751 13667 -84 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Introduces a new
#[simple_shard]attribute macro that reduces boilerplate for simple transform shards by ~60-77%. Features:Converts yaml.rs and argon.rs as examples of the new syntax.
Note
Introduces the #[simple_shard] macro with ShardType support and refactors YAML, UUID/NanoID, and Argon2 shards to the new pattern, plus small runtime/type additions and a deps cleanup.
#[simple_shard]macro inshards/rust_macrogenerating full shard boilerplate from functions (params via#[param]/#[param_var], automatic type/var handling, compose/warmup/cleanup, result handling).shards::types::ShardTypetrait (types/shard_type.rs) and re-export; new output wrappersStringOut/BytesOut; extraClonedVarFromimpls; exportsimple_shardinshards/rust/src/lib.rs.modules/core/src/uuid.rs): implementUUID,UUID.ToString,UUID.ToBytes,NanoIDviasimple_shard; keepUUID.Convertlegacy; update registrations; minor type path fixes inSnowflake.modules/core/src/yaml.rs): rewriteYaml.ToJsonandYaml.FromJsonusingsimple_shard; update registration.modules/crypto/src/argon.rs): rewriteArgon2id.HashandArgon2id.Verifyusingsimple_shard, with defaults andHashas context var.add_subdirectory(FTXUI)fromdeps/CMakeLists.txt..gitignoreto ignoreshards/rust_macro/Cargo.lock.Written by Cursor Bugbot for commit 49d5cd1. This will update automatically on new commits. Configure here.