-
Notifications
You must be signed in to change notification settings - Fork 146
macros: Add support for #[scylla(transparent)] attribute #1506
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds support for the #[scylla(transparent)] attribute to the SerializeValue and DeserializeValue derive macros. This attribute enables wrapper types (newtypes) to transparently serialize and deserialize as their inner type, similar to serde's transparent attribute. The implementation handles structs with single fields (both named and unnamed/tuple structs) and enums with a single variant containing one field.
Key Changes:
- Added transparent attribute support to SerializeValue derive macro with validation for single-field structs and single-variant enums
- Added transparent attribute support to DeserializeValue derive macro with corresponding validation and proper lifetime handling
- Both implementations delegate serialization/deserialization to the inner field's type implementation
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| scylla-macros/src/serialize/value.rs | Adds transparent attribute field and implements transparent serialization logic for single-field structs and single-variant enums by delegating to the inner field's SerializeValue implementation |
| scylla-macros/src/deserialize/value.rs | Adds transparent attribute field and implements transparent deserialization logic through a new derive_transparent function that properly handles lifetimes and delegates to the inner field's DeserializeValue implementation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
9139086 to
4dc482e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let input: syn::DeriveInput = syn::parse(tokens_input)?; | ||
|
|
||
| let attrs = StructAttrs::from_attributes(&input.attrs)?; | ||
| if attrs.transparent { | ||
| return derive_transparent(&input, &attrs); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the non-transparent case this will cause the struct attrs to be parsed twice: first time here, and then in StructDesc::new below.
One way to fix that that comes to my mind is to move this if below line let s = StructDesc::new... and use s.attrs instead of attrs. I did not test that, so it may not work. Is there a reason you chose the current approach? Do you see any downsides of my proposed approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried moving the check after StructDesc::new, but StructDesc strictly requires named fields during initialization. Since transparent structs are often tuple structs, StructDesc::new fails for them.
| typ: &#crate_path::ColumnType, | ||
| ) -> ::std::result::Result<(), #crate_path::TypeCheckError> { | ||
| <#inner_type as #crate_path::DeserializeValue<#frame_lifetime, #metadata_lifetime>>::type_check(typ) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mapped the type name in error in deserialize below, but not here. Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scylla-cql does not export a helper function to replace the Rust name in TypeCheckError (like value_deser_error_replace_rust_name for DeserializationError).
scylla-macros/src/serialize/value.rs
Outdated
| return Ok(parse_quote! { | ||
| #[automatically_derived] | ||
| impl #impl_generics #implemented_trait for #struct_name #ty_generics #where_clause { | ||
| fn serialize<'b>( | ||
| &self, | ||
| typ: &#crate_path::ColumnType, | ||
| writer: #crate_path::CellWriter<'b>, | ||
| ) -> ::std::result::Result<#crate_path::WrittenCellProof<'b>, #crate_path::SerializationError> { | ||
| #crate_path::SerializeValue::serialize(&self.#field_accessor, typ, writer) | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should error be mapped to correct name here too?
| match self { | ||
| #pattern_match => #crate_path::SerializeValue::serialize(inner, typ, writer) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto about name mapping
3d47496 to
d21a98a
Compare
|
@ziara1 Please squash fixes into corresponding commits. |
d21a98a to
24dd668
Compare
|
@wprzytula done |
Motivation
Users often employ the newtype pattern (i.e., define single-field structs) to wrap the driver's types that implement
{De,S}erialize{Value,Row}. In such cases, they are currently forced to manually implement (trivially) the respective de/ser traits for those new types, which results in unnecessary boilerplate code.Solution
Add support for a new item-level attribute,
#[scylla(transparent)].It is supported on:
The semantics are simple: delegate the trait method's implementation (serialization, deserialization, and type checking) to the only field/variant of the struct/enum.
What's done
Implemented transparent logic in DeserializeValue derive
Refactored
deserialize_value_deriveto delegate to a newderive_transparentfunction when the attribute is present.The implementation ensures that:
type_checkdelegates to the inner type.deserializedelegates to the inner type and wraps the result in the newtype constructor.whereclauses are handled correctly by merging generated predicates with existing user predicates usingmake_where_clause().Implemented transparent logic in SerializeValue derive
Modified
serialize_value_deriveto handle thetransparentattribute.Updated Attribute Parsing
Updated
StructAttrsandAttributesdefinitions in both deserialize and serialize modules to parse the#[scylla(transparent)]boolean flag viadarling.What has been tested
Tests
I added a comprehensive suite of tests covering:
struct Wrapper(i32)struct Wrapper { val: i32 }Outer(Inner(i32))to verify deep delegation.type_checkcorrectly propagates errors (e.g., trying to read anintcolumn astextthrough a wrapper fails).Fixes: #1293
Pre-review checklist
./docs/source/.Fixes:annotations to PR description.