From 68dc71cda69e0aa117aea4c62b16ccd2c5b745e9 Mon Sep 17 00:00:00 2001 From: bbaldino Date: Thu, 24 Apr 2025 10:10:26 -0700 Subject: [PATCH 1/9] change how StateSync trait is defined and make it a required supertrait of ParselyWrite --- impl/src/code_gen/gen_write.rs | 4 +++- impl/src/parsely_write.rs | 28 +++++++++++++++++++++------- src/lib.rs | 1 + 3 files changed, 25 insertions(+), 8 deletions(-) diff --git a/impl/src/code_gen/gen_write.rs b/impl/src/code_gen/gen_write.rs index eae4755..148d76f 100644 --- a/impl/src/code_gen/gen_write.rs +++ b/impl/src/code_gen/gen_write.rs @@ -30,6 +30,7 @@ fn generate_parsely_write_impl_struct( struct_alignment: Option, sync_args: Option, ) -> TokenStream { + // TODO: call sync on all fields let crate_name = get_crate_name(); let (context_assignments, context_types) = if let Some(ref required_context) = required_context { @@ -159,7 +160,8 @@ fn generate_parsely_write_impl_struct( } } - impl StateSync<(#(#sync_args_types,)*)> for #struct_name { + impl StateSync for #struct_name { + type SyncCtx = (#(#sync_args_types,)*); fn sync(&mut self, (#(#sync_args_variables,)*): (#(#sync_args_types,)*)) -> ParselyResult<()> { #(#sync_field_calls)* diff --git a/impl/src/parsely_write.rs b/impl/src/parsely_write.rs index 7bd7ee6..ad54407 100644 --- a/impl/src/parsely_write.rs +++ b/impl/src/parsely_write.rs @@ -2,14 +2,27 @@ use bits_io::prelude::*; use crate::error::ParselyResult; -pub trait StateSync: Sized { - // TODO: I think we should probably do a default impl of sync here that just returns Ok(()) - fn sync(&mut self, sync_ctx: SyncCtx) -> ParselyResult<()>; +/// A trait for syncing a field with any required context. In order to prevent accidental misses +/// of this trait, it's required for all `ParselyWrite` implementors. When generating the +/// `ParselyWrite` implementation, `sync` will be called on every field. +pub trait StateSync: Sized { + type SyncCtx; + + fn sync(&mut self, _sync_ctx: Self::SyncCtx) -> ParselyResult<()> { + Ok(()) + } +} + +#[macro_export] +macro_rules! impl_stateless_sync { + ($ty:ty) => { + impl StateSync for $ty { + type SyncCtx = (); + } + }; } -// TODO: should this be changed to require StateSync? I think so? Pretty sure we assume it exists, -// so it'll move errors sooner -pub trait ParselyWrite: Sized { +pub trait ParselyWrite: StateSync + Sized { fn write(&self, buf: &mut B, ctx: Ctx) -> ParselyResult<()>; } @@ -52,7 +65,8 @@ macro_rules! for_all { macro_rules! impl_state_sync_builtin { ($type:ty) => { - impl StateSync<()> for $type { + impl StateSync for $type { + type SyncCtx = (); fn sync(&mut self, _sync_ctx: ()) -> ParselyResult<()> { Ok(()) } diff --git a/src/lib.rs b/src/lib.rs index aa53263..552a6e2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,6 +1,7 @@ // TODO: these should be moved to a prelude file pub use parsely_impl::anyhow::{Context, anyhow, bail}; pub use parsely_impl::error::ParselyResult; +pub use parsely_impl::impl_stateless_sync; pub use parsely_impl::nsw_types::*; pub use parsely_impl::{BigEndian, ByteOrder, LittleEndian, NetworkOrder}; pub use parsely_impl::{BitBuf, BitBufExts, BitBufMut, BitBufMutExts, Bits, BitsMut}; From 32e0365a4399bba80942f2313f169a285cafa974 Mon Sep 17 00:00:00 2001 From: bbaldino Date: Thu, 24 Apr 2025 11:58:30 -0700 Subject: [PATCH 2/9] allow context/sync expressions to return a raw value or a result --- impl/src/code_gen/gen_read.rs | 8 ++++---- impl/src/code_gen/gen_write.rs | 2 +- impl/src/error.rs | 23 +++++++++++++++++++++++ impl/src/lib.rs | 33 +++++++++++++++++++++++++++------ impl/src/model_types.rs | 19 +++++++++++++++++-- src/lib.rs | 2 +- 6 files changed, 73 insertions(+), 14 deletions(-) diff --git a/impl/src/code_gen/gen_read.rs b/impl/src/code_gen/gen_read.rs index 1f6f88d..6bf4506 100644 --- a/impl/src/code_gen/gen_read.rs +++ b/impl/src/code_gen/gen_read.rs @@ -118,8 +118,8 @@ fn wrap_in_optional(when_expr: &syn::Expr, inner: TokenStream) -> TokenStream { /// 5. If an 'assertion' attribute is present, then generate code to assert on the read value using /// the given assertion function or closure. /// 6. After the code to perform the read has been generated, we check if the field is an option -/// type. If so, a 'when' attribute is required. This is an expression that determines when -/// the read should actually be done. +/// type. If so, a 'when' attribute is required. This is an expression that determines when the +/// read should actually be done. /// 7. Finally, if an 'alignment' attribute is present, code is added to detect and consume any /// padding after the read. fn generate_field_read(field_data: &ParselyReadFieldData) -> TokenStream { @@ -146,9 +146,9 @@ fn generate_field_read(field_data: &ParselyReadFieldData) -> TokenStream { } else { panic!("Collection field '{field_name}' must have either 'count' or 'while' attribute"); }; - output.extend(generate_collection_read(limit, read_type, context_values)); + output.extend(generate_collection_read(limit, read_type, &context_values)); } else { - output.extend(generate_plain_read(read_type, context_values)); + output.extend(generate_plain_read(read_type, &context_values)); } // println!("tokenstream: {}", output); diff --git a/impl/src/code_gen/gen_write.rs b/impl/src/code_gen/gen_write.rs index 148d76f..04d23cf 100644 --- a/impl/src/code_gen/gen_write.rs +++ b/impl/src/code_gen/gen_write.rs @@ -120,7 +120,7 @@ fn generate_parsely_write_impl_struct( // provided. quote! {} } else { - let sync_with = f.sync_with.expressions(); + let sync_with = f.sync_with_expressions(); quote! { self.#field_name.sync((#(#sync_with,)*)).with_context(|| format!("Syncing field '{}'", #field_name_string))?; } diff --git a/impl/src/error.rs b/impl/src/error.rs index 1f05531..c17246f 100644 --- a/impl/src/error.rs +++ b/impl/src/error.rs @@ -1 +1,24 @@ pub type ParselyResult = anyhow::Result; + +pub trait IntoParselyResult { + fn into_parsely_result(self) -> ParselyResult; +} + +impl IntoParselyResult for T { + fn into_parsely_result(self) -> ParselyResult { + Ok(self) + } +} + +impl IntoParselyResult for Result +where + E: Into, +{ + fn into_parsely_result(self) -> ParselyResult { + self.map_err(Into::into) + } +} + +pub fn wrap_in_parsely_result(value: impl IntoParselyResult) -> ParselyResult { + value.into_parsely_result() +} diff --git a/impl/src/lib.rs b/impl/src/lib.rs index 7b92803..a18754d 100644 --- a/impl/src/lib.rs +++ b/impl/src/lib.rs @@ -109,11 +109,16 @@ impl ParselyReadFieldData { } /// Get the context values that need to be passed to the read or write call for this field - pub(crate) fn context_values(&self) -> &[syn::Expr] { + pub(crate) fn context_values(&self) -> Vec { + let field_name = self + .ident + .as_ref() + .expect("Field must have a name") + .to_string(); if let Some(ref field_context) = self.common.context { - field_context.expressions() + field_context.expressions(&format!("Read context for field '{field_name}'")) } else { - &[] + vec![] } } } @@ -154,13 +159,29 @@ impl ParselyWriteFieldData { } /// Get the context values that need to be passed to the read or write call for this field - pub(crate) fn context_values(&self) -> &[syn::Expr] { + pub(crate) fn context_values(&self) -> Vec { + let field_name = self + .ident + .as_ref() + .expect("Field must have a name") + .to_string(); if let Some(ref field_context) = self.common.context { - field_context.expressions() + field_context.expressions(&format!("Write context for field '{field_name}'")) } else { - &[] + vec![] } } + + /// Get the context values that need to be passed to the read or write call for this field + pub(crate) fn sync_with_expressions(&self) -> Vec { + let field_name = self + .ident + .as_ref() + .expect("Field must have a name") + .to_string(); + self.sync_with + .expressions(&format!("Sync context for field '{field_name}'")) + } } #[derive(Debug, FromDeriveInput)] diff --git a/impl/src/model_types.rs b/impl/src/model_types.rs index 65bcf83..9dcce0f 100644 --- a/impl/src/model_types.rs +++ b/impl/src/model_types.rs @@ -67,8 +67,23 @@ impl FromMeta for TypedFnArgList { pub(crate) struct Context(Vec); impl Context { - pub(crate) fn expressions(&self) -> &[syn::Expr] { - &self.0 + /// Get the context arguments as a vector of expressions, with an erorr context including the + /// given `context` value. + pub(crate) fn expressions(&self, context: &str) -> Vec { + // We support Context expressions that return a ParselyResult or a raw value. So now wrap + // all the expressions with code that will normalize all of the results into + // ParselyResults. + self.0 + .iter() + .cloned() + .enumerate() + .map(|(idx, e)| { + syn::parse2(quote! { + (#e).into_parsely_result().with_context(|| format!("{}: expression {}", #context, #idx))? + }) + .unwrap() + }) + .collect() } pub(crate) fn is_empty(&self) -> bool { diff --git a/src/lib.rs b/src/lib.rs index 552a6e2..2415c69 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,6 +1,6 @@ // TODO: these should be moved to a prelude file pub use parsely_impl::anyhow::{Context, anyhow, bail}; -pub use parsely_impl::error::ParselyResult; +pub use parsely_impl::error::{IntoParselyResult, ParselyResult}; pub use parsely_impl::impl_stateless_sync; pub use parsely_impl::nsw_types::*; pub use parsely_impl::{BigEndian, ByteOrder, LittleEndian, NetworkOrder}; From 4c9e467130e968b3f4fcf0f1b76b07f1fb12b0b0 Mon Sep 17 00:00:00 2001 From: bbaldino Date: Thu, 24 Apr 2025 14:16:35 -0700 Subject: [PATCH 3/9] refactor: rename sync_func -> sync_expr --- impl/src/code_gen/gen_write.rs | 7 +++---- impl/src/lib.rs | 6 +++--- tests/ui/pass/sync.rs | 4 ++-- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/impl/src/code_gen/gen_write.rs b/impl/src/code_gen/gen_write.rs index 04d23cf..52236a5 100644 --- a/impl/src/code_gen/gen_write.rs +++ b/impl/src/code_gen/gen_write.rs @@ -30,7 +30,6 @@ fn generate_parsely_write_impl_struct( struct_alignment: Option, sync_args: Option, ) -> TokenStream { - // TODO: call sync on all fields let crate_name = get_crate_name(); let (context_assignments, context_types) = if let Some(ref required_context) = required_context { @@ -105,14 +104,14 @@ fn generate_parsely_write_impl_struct( .iter() // 'sync_with' attirbutes mean this field's 'sync' method needs to be called with some data // Iterate over all fields and either: - // a) call the sync function provided in the sync_func attribute + // a) execute the expression provided in the sync_expr attribute // b) call the sync function on that type with any provided sync_with arguments .map(|f| { let field_name = f.ident.as_ref().expect("Field has a name"); let field_name_string = field_name.to_string(); - if let Some(ref sync_func) = f.sync_func { + if let Some(ref sync_expr) = f.sync_expr { quote! { - self.#field_name = #sync_func.with_context(|| format!("Syncing field '{}'", #field_name_string))?; + self.#field_name = (#sync_expr).into_parsely_result().with_context(|| format!("Syncing field '{}'", #field_name_string))?; } } else if f.sync_with.is_empty() && f.ty.is_wrapped() { // We'll allow this combination to skip a call to sync: for types like Option or diff --git a/impl/src/lib.rs b/impl/src/lib.rs index a18754d..2054ce4 100644 --- a/impl/src/lib.rs +++ b/impl/src/lib.rs @@ -133,9 +133,9 @@ pub struct ParselyWriteFieldData { #[darling(flatten)] common: ParselyCommonFieldData, - /// An optional function or closure that will be called to synchronize this field based on some - /// external data - sync_func: Option, + /// An expression or function call that will be used to update this field in the generated + /// `StateSync` implementation for its parent type. + sync_expr: Option, /// An list of expressions that should be passed as context to this field's sync method. The /// sync method provides an opportunity to synchronize "linked" fields, where one field's value diff --git a/tests/ui/pass/sync.rs b/tests/ui/pass/sync.rs index 98de080..6a27736 100644 --- a/tests/ui/pass/sync.rs +++ b/tests/ui/pass/sync.rs @@ -7,10 +7,10 @@ use parsely_rs::*; struct Header { version: u8, packet_type: u8, - // sync_func can refer to an expression or a function and will be used to update the annotated + // sync_expr can refer to an expression or a function and will be used to update the annotated // field, it should evaluate to ParselyResult where T is the type of the field. You can // refer to variables defined in sync_args - #[parsely_write(sync_func = "ParselyResult::Ok(payload_length_bytes + 4)")] + #[parsely_write(sync_expr = "payload_length_bytes + 4")] length_bytes: u16, } From a32f2d159f7f4cc4191e9abbe19e7b409f1e7774 Mon Sep 17 00:00:00 2001 From: bbaldino Date: Thu, 24 Apr 2025 14:17:21 -0700 Subject: [PATCH 4/9] wip: add Assertion newtype to clean things up a bit --- impl/src/code_gen/gen_read.rs | 25 +++--------------- impl/src/code_gen/gen_write.rs | 21 ++++++++++----- impl/src/lib.rs | 4 +-- impl/src/model_types.rs | 42 ++++++++++++++++++++++++++++++ tests/expand/alignment.expanded.rs | 3 ++- tests/expand/assertion.rs | 7 +++++ 6 files changed, 71 insertions(+), 31 deletions(-) create mode 100644 tests/expand/assertion.rs diff --git a/impl/src/code_gen/gen_read.rs b/impl/src/code_gen/gen_read.rs index 6bf4506..f1ec16e 100644 --- a/impl/src/code_gen/gen_read.rs +++ b/impl/src/code_gen/gen_read.rs @@ -73,23 +73,6 @@ fn generate_map_read(field_name: &syn::Ident, map_fn: &FuncOrClosure) -> TokenSt } } -/// Generate an assertion 'block' that can be appended to a [`Result`] type by embedding it in an -/// `and_then` block. Note that we take a [`syn::Expr`] for the assertion, but it needs to -/// effectively be a function (or a closure) which accepts the value type and returns a boolean. -fn generate_assertion(field_name: &syn::Ident, assertion: &FuncOrClosure) -> TokenStream { - let assertion_string = quote! { #assertion }.to_string(); - let field_name_string = field_name.to_string(); - quote! { - .and_then(|actual_value| { - let assertion_func = #assertion; - if !assertion_func(&actual_value) { - bail!("Assertion failed: value of field '{}' ('{:?}') didn't pass assertion: '{}'", #field_name_string, actual_value, #assertion_string) - } - Ok(actual_value) - }) - } -} - fn wrap_in_optional(when_expr: &syn::Expr, inner: TokenStream) -> TokenStream { quote! { if #when_expr { @@ -115,7 +98,7 @@ fn wrap_in_optional(when_expr: &syn::Expr, inner: TokenStream) -> TokenStream { /// elements should be read. /// 4. If none of the above are the case, do a 'plain' read where we just read the type directly /// from the buffer. -/// 5. If an 'assertion' attribute is present, then generate code to assert on the read value using +/// 5. If an 'assertion' attribute is present then generate code to assert on the read value using /// the given assertion function or closure. /// 6. After the code to perform the read has been generated, we check if the field is an option /// type. If so, a 'when' attribute is required. This is an expression that determines when the @@ -127,6 +110,7 @@ fn generate_field_read(field_data: &ParselyReadFieldData) -> TokenStream { .ident .as_ref() .expect("Only named fields supported"); + let field_name_str = field_name.to_string(); let read_type = field_data.buffer_type(); // Context values that we need to pass to this field's ParselyRead::read method let context_values = field_data.context_values(); @@ -151,10 +135,9 @@ fn generate_field_read(field_data: &ParselyReadFieldData) -> TokenStream { output.extend(generate_plain_read(read_type, &context_values)); } - // println!("tokenstream: {}", output); - if let Some(ref assertion) = field_data.common.assertion { - output.extend(generate_assertion(field_name, assertion)); + assertion.to_read_assertion_tokens(&field_name_str, &mut output); + // output.extend(generate_assertion(field_name, assertion)); } let error_context = format!("Reading field '{field_name}'"); output.extend(quote! { .with_context(|| #error_context)?}); diff --git a/impl/src/code_gen/gen_write.rs b/impl/src/code_gen/gen_write.rs index 52236a5..5a3edea 100644 --- a/impl/src/code_gen/gen_write.rs +++ b/impl/src/code_gen/gen_write.rs @@ -23,6 +23,19 @@ pub fn generate_parsely_write_impl(data: ParselyWriteData) -> TokenStream { } } +// Given the data associated with a field, generate the code for properly writing it to a buffer. +// +// The attributes set in the [`ParselyWriteFieldData`] all shape the logic necessary in order to +// properly write this field. Roughly, the processing is as follows: +// +// 1. If an 'assertion' attribute is present then generate the code to assert on the field's current +// value using the given function or closure. +// fn generate_field_write(field_data: &ParselyWriteFieldData) -> TokenStream { +// let mut output = TokenStream::new(); +// +// output +// } + fn generate_parsely_write_impl_struct( struct_name: syn::Ident, fields: darling::ast::Fields, @@ -50,13 +63,7 @@ fn generate_parsely_write_impl_struct( let mut field_write_output = TokenStream::new(); if let Some(ref assertion) = f.common.assertion { - let assertion_string = quote! { #assertion }.to_string(); - field_write_output.extend(quote! { - let assertion_func = #assertion; - if !assertion_func(&self.#field_name) { - bail!("Assertion failed: value of field '{}' ('{:?}') didn't pass assertion: '{}'", #field_name_string, self.#field_name, #assertion_string) - } - }) + assertion.to_write_assertion_tokens(&field_name_string, &mut field_write_output); } if let Some(ref map_fn) = f.common.map { diff --git a/impl/src/lib.rs b/impl/src/lib.rs index 2054ce4..a5ec689 100644 --- a/impl/src/lib.rs +++ b/impl/src/lib.rs @@ -26,7 +26,7 @@ pub mod anyhow { use code_gen::{gen_read::generate_parsely_read_impl, gen_write::generate_parsely_write_impl}; use darling::{ast, FromDeriveInput, FromField, FromMeta}; -use model_types::{Context, ExprOrFunc, FuncOrClosure, TypedFnArgList}; +use model_types::{Assertion, Context, ExprOrFunc, FuncOrClosure, TypedFnArgList}; use proc_macro2::TokenStream; use syn::DeriveInput; use syn_helpers::TypeExts; @@ -57,7 +57,7 @@ pub struct ParselyCommonFieldData { // See https://github.com/TedDriggs/darling/issues/330 // generics: Option, - assertion: Option, + assertion: Option, /// Values that need to be passed as context to this fields read or write method context: Option, diff --git a/impl/src/model_types.rs b/impl/src/model_types.rs index 9dcce0f..b0b3528 100644 --- a/impl/src/model_types.rs +++ b/impl/src/model_types.rs @@ -240,6 +240,48 @@ impl FromMeta for FuncOrClosure { } } +#[derive(Debug)] +pub(crate) struct Assertion(FuncOrClosure); + +impl Parse for Assertion { + fn parse(input: syn::parse::ParseStream) -> syn::Result { + Ok(Self(FuncOrClosure::parse(input)?)) + } +} + +impl FromMeta for Assertion { + fn from_string(value: &str) -> darling::Result { + Ok(Self(FuncOrClosure::from_string(value)?)) + } +} + +impl Assertion { + pub(crate) fn to_read_assertion_tokens(&self, field_name: &str, tokens: &mut TokenStream) { + let assertion = &self.0; + tokens.extend(quote! { + .and_then(|read_value| { + let assertion_func = #assertion; + if !assertion_func(&read_value) { + bail!("Assertion failed: value of field '{}' ('{:?}') didn't pass assertion: '{}'", #field_name, read_value, #assertion) + } + Ok(read_value) + }) + }); + } + + pub(crate) fn to_write_assertion_tokens(&self, field_name: &str, tokens: &mut TokenStream) { + let assertion = &self.0; + let assertion_func_ident = format_ident!("__{}_assertion_func", field_name); + let field_name_ident = format_ident!("{field_name}"); + tokens.extend(quote! { + let #assertion_func_ident = #assertion; + if !#assertion_func_ident(&self.#field_name_ident) { + bail!("Assertion failed: value of field '{}' ('{:?}') didn't pass assertion: '{}'", #field_name, self.#field_name_ident, #assertion) + } + }) + } +} + pub(crate) fn wrap_read_with_padding_handling( element_ident: &syn::Ident, alignment: usize, diff --git a/tests/expand/alignment.expanded.rs b/tests/expand/alignment.expanded.rs index a8f6b02..1cd44c5 100644 --- a/tests/expand/alignment.expanded.rs +++ b/tests/expand/alignment.expanded.rs @@ -42,7 +42,8 @@ impl ::parsely_rs::ParselyWrite for Foo { Ok(()) } } -impl StateSync<()> for Foo { +impl StateSync for Foo { + type SyncCtx = (); fn sync(&mut self, (): ()) -> ParselyResult<()> { self.one .sync(()) diff --git a/tests/expand/assertion.rs b/tests/expand/assertion.rs new file mode 100644 index 0000000..0ff4b54 --- /dev/null +++ b/tests/expand/assertion.rs @@ -0,0 +1,7 @@ +use parsely_rs::*; + +#[derive(ParselyRead, ParselyWrite)] +struct Foo { + #[parsely(assertion = "|v: &u8| *v % 2 == 0")] + value: u8, +} From 52c7344603d9a359a0be64ef92326b70e188420e Mon Sep 17 00:00:00 2001 From: bbaldino Date: Thu, 24 Apr 2025 22:57:38 -0700 Subject: [PATCH 5/9] wip: refactoring map expression. generated code is broken right now --- Notes.md | 119 +++++++++++++++++++++++++++++++-- impl/src/code_gen/gen_read.rs | 20 +----- impl/src/code_gen/gen_write.rs | 20 +----- impl/src/error.rs | 2 + impl/src/lib.rs | 4 +- impl/src/model_types.rs | 47 +++++++++++++ tests/ui/pass/map.rs | 17 ++--- 7 files changed, 180 insertions(+), 49 deletions(-) diff --git a/Notes.md b/Notes.md index c3ed7fa..3eca824 100644 --- a/Notes.md +++ b/Notes.md @@ -81,6 +81,92 @@ takes a fair bit of special handling (including additional attributes like these cases, which seems like it may make sense. Will probably play with some implementations of that and see how they feel. +An interesting scenario came up with regards to the 'write' map path vs the 'read': + +Both sides want to take advantage of type inference to avoid having to make the +caller explicitly state types. Assuming a struct like this: + +```rust +#[derive(ParselyRead, ParselyWrite)] +struct Foo { + #[parsely_read(map = "|v: u8| { v.to_string() }")] + #[parsely_write(map = "|v: &str| { v.parse::() }")] + value: String, +} +``` + +the generated read code can look like: + +```rust + let original_value = ::parsely_rs::ParselyRead::read::(buf, ()).with_context(...)?; + (|v: u8| { v.to_string() })(original_value) + .with_context(...)?; + +``` + +In this case the `read` map function isn't ambiguous about its type, but even +if it was it would be inferred ok because eventually we're going to assign it +to the field which _does_ have an explicit type, so it can be inferred from +there. The write side is trickier, but mainly because i didn't want to have to +force the caller to explicitly return a result, i wanted to be able to handle a +function that returned either a raw value or a result that could be converted +into a ParselyResult. The write code looks like this: + +```rust + let mapped_value = (|v: &str| { v.parse() })(&self.value) + .with_context(...)?; + ::parsely_rs::ParselyWrite::write::(&mapped_value, buf, ()) + .with_context(...)?; +``` + +We need to somehow wrap the result of the function in such a way that we end up with a `ParselyResult` regardless of whether it returned some raw type T or some kind of Result. + +The first solution that comes to mind to try and do that is this: + +```rust +pub trait IntoParselyResult { + fn into_parsely_result(self) -> ParselyResult; +} + +impl IntoParselyResult for Result +where + E: Into, +{ + fn into_parsely_result(self) -> ParselyResult { + self.map_err(Into::into) + } +} + +impl IntoParselyResult for T { + fn into_parsely_result(self) -> ParselyResult { + Ok(self) + } +} +``` + +The problem is that for the case where the expression returns `Result` +there's ambiguity: it can't tell if what we want is `ParselyResult` or +`ParselyResult>` so the compiler complains about both +implementations being possible. I tried a bunch of different wrappers and +macros to try and work around this but couldn't come up with anything. Finally +my only idea was to tweak the implementation for `T` slightly: + +```rust +impl IntoParselyResult for T where T: ParselyWrite { + fn into_parsely_result(self) -> ParselyResult { + Ok(self) + } +} +``` + +Since we don't impl `ParselyWrite` for any `Result` type (and I don't think +we'd have a need to) this disambiguates the two cases enough that there's no +issue trying to infer which one touse. + +In order to do this, though, we'll need to change the definition of +`ParselyWrite` to use associated types instead of generics, but I think that +may actually make more sense anyway. + is there an inherent "ordering" to all the attributes that we could always apply in a consistent way? context reads (assigning the context tuple variables into named variables defined by the 'required_context' attribute): I think these can always be first @@ -253,7 +339,8 @@ I want to do that. So maybe the call to `sync` will have to be manual and it will just be generated to do the right things? Going with that for now. TODO: - can we get rid of the ambiguity of a plain "Ok" in sync_func? Could we make it such that plain (no Ok needed) would also work? + can we get rid of the ambiguity of a plain "Ok" in sync_func? Could we make +it such that plain (no Ok needed) would also work? Follow-up on this: I think that, when writing, the generated code should call 'sync' on every field, that way if the user forgets to set the 'sync_with' @@ -270,7 +357,31 @@ the context for writing? I don't think context is needed for writing (I've seen no use cases of it so far, and it seems like any write context), but logically it seems like a use-case could exist for it... -### The buffer type y +Follow-up: +I've separated out the sync code into a trait (`StateSync`). I've also moved +the arguments to an associated type, which means we can make `StateSync` a +supertrait of `ParselyWrite`. The idea is that, when generating the write, +`sync` will be called on every field so there's no chance of it being missed. + +One issue with this is built-in types: they need a default impl so that the +compiler doesn't complain about calling `sync` on them. But sometimes we need +to sync fields that are built-in types. For example, the length field of the +RtcpHeader needs to be sync'd using the sync args. + +There are 3 aspect to synchronizing fields: + +* `sync_args`: This is attached to a type definition and it describes the names +and types of the arguments that will be expected in that type's `StateSync` +impl. +* `sync_with`: This is attached to a field, and describes the values that will be +used when calling that field's `sync` method. +* `sync_func`: This is attached to a field and describes how _that field_ +should be updated in the `sync` method for its surrounding type using the args +passed to it via `sync_args` + +The last one is confusing. Maybe it's better named 'sync_expr'? + +### The buffer type Currently, ParselyRead takes any buffer that is BitRead and ParselyWrite takes any buffer that is BitWrite. The issue here is that BitRead and BitWrite are @@ -306,8 +417,8 @@ struct MyStruct { ... } Some thoughts: -- I think a crate would have to be very consistent with their use of this: mixing and matching could lead to problems -- Is it possible to write an 'alias' for a macro attribute? +* I think a crate would have to be very consistent with their use of this: mixing and matching could lead to problems +* Is it possible to write an 'alias' for a macro attribute? ### Post hooks diff --git a/impl/src/code_gen/gen_read.rs b/impl/src/code_gen/gen_read.rs index f1ec16e..76805ea 100644 --- a/impl/src/code_gen/gen_read.rs +++ b/impl/src/code_gen/gen_read.rs @@ -3,9 +3,7 @@ use quote::{format_ident, quote}; use crate::{ get_crate_name, - model_types::{ - wrap_read_with_padding_handling, CollectionLimit, FuncOrClosure, TypedFnArgList, - }, + model_types::{wrap_read_with_padding_handling, CollectionLimit, TypedFnArgList}, syn_helpers::TypeExts, ParselyReadData, ParselyReadFieldData, }; @@ -61,18 +59,6 @@ fn generate_collection_read( } } -fn generate_map_read(field_name: &syn::Ident, map_fn: &FuncOrClosure) -> TokenStream { - let field_name_string = field_name.to_string(); - quote! { - { - let original_value = ParselyRead::read::(buf, ()).with_context(|| format!("Reading raw value for field '{}'", #field_name_string))?; - let mapped_value = (#map_fn)(original_value).with_context(|| format!("Mapping raw value for field '{}'", #field_name_string))?; - - ParselyResult::<_>::Ok(mapped_value) - } - } -} - fn wrap_in_optional(when_expr: &syn::Expr, inner: TokenStream) -> TokenStream { quote! { if #when_expr { @@ -120,8 +106,8 @@ fn generate_field_read(field_data: &ParselyReadFieldData) -> TokenStream { output.extend(quote! { ParselyResult::<_>::Ok(#assign_expr) }) - } else if let Some(ref map_fn) = field_data.common.map { - output.extend(generate_map_read(field_name, map_fn)); + } else if let Some(ref map_expr) = field_data.common.map { + map_expr.to_read_map_tokens(field_name, &mut output); } else if field_data.ty.is_collection() { let limit = if let Some(ref count) = field_data.count { CollectionLimit::Count(count.clone()) diff --git a/impl/src/code_gen/gen_write.rs b/impl/src/code_gen/gen_write.rs index 5a3edea..6f9189e 100644 --- a/impl/src/code_gen/gen_write.rs +++ b/impl/src/code_gen/gen_write.rs @@ -23,19 +23,6 @@ pub fn generate_parsely_write_impl(data: ParselyWriteData) -> TokenStream { } } -// Given the data associated with a field, generate the code for properly writing it to a buffer. -// -// The attributes set in the [`ParselyWriteFieldData`] all shape the logic necessary in order to -// properly write this field. Roughly, the processing is as follows: -// -// 1. If an 'assertion' attribute is present then generate the code to assert on the field's current -// value using the given function or closure. -// fn generate_field_write(field_data: &ParselyWriteFieldData) -> TokenStream { -// let mut output = TokenStream::new(); -// -// output -// } - fn generate_parsely_write_impl_struct( struct_name: syn::Ident, fields: darling::ast::Fields, @@ -66,11 +53,8 @@ fn generate_parsely_write_impl_struct( assertion.to_write_assertion_tokens(&field_name_string, &mut field_write_output); } - if let Some(ref map_fn) = f.common.map { - field_write_output.extend(quote! { - let mapped_value = (#map_fn)(&self.#field_name).with_context(|| format!("Mapping raw value for field '{}'", #field_name_string))?; - ParselyWrite::write::(&mapped_value, buf, ()).with_context(|| format!("Writing mapped value for field '{}'", #field_name_string))?; - }); + if let Some(ref map_expr) = f.common.map { + map_expr.to_write_map_tokens(field_name, &mut field_write_output); } else if f.ty.is_option() { field_write_output.extend(quote! { if let Some(ref v) = self.#field_name { diff --git a/impl/src/error.rs b/impl/src/error.rs index c17246f..d66710d 100644 --- a/impl/src/error.rs +++ b/impl/src/error.rs @@ -4,6 +4,8 @@ pub trait IntoParselyResult { fn into_parsely_result(self) -> ParselyResult; } +// TODO: change this to be bound by T: ParselyWrite once ParselyWrite is changed to use associated +// types impl IntoParselyResult for T { fn into_parsely_result(self) -> ParselyResult { Ok(self) diff --git a/impl/src/lib.rs b/impl/src/lib.rs index a5ec689..ed702ee 100644 --- a/impl/src/lib.rs +++ b/impl/src/lib.rs @@ -26,7 +26,7 @@ pub mod anyhow { use code_gen::{gen_read::generate_parsely_read_impl, gen_write::generate_parsely_write_impl}; use darling::{ast, FromDeriveInput, FromField, FromMeta}; -use model_types::{Assertion, Context, ExprOrFunc, FuncOrClosure, TypedFnArgList}; +use model_types::{Assertion, Context, ExprOrFunc, MapExpr, TypedFnArgList}; use proc_macro2::TokenStream; use syn::DeriveInput; use syn_helpers::TypeExts; @@ -63,7 +63,7 @@ pub struct ParselyCommonFieldData { context: Option, /// An optional mapping that will be applied to the read value - map: Option, + map: Option, /// An optional indicator that this field is or needs to be aligned to the given byte alignment /// via padding. diff --git a/impl/src/model_types.rs b/impl/src/model_types.rs index b0b3528..462f756 100644 --- a/impl/src/model_types.rs +++ b/impl/src/model_types.rs @@ -3,6 +3,8 @@ use proc_macro2::TokenStream; use quote::{format_ident, quote, ToTokens}; use syn::parse::Parse; +use crate::get_crate_name; + pub(crate) enum CollectionLimit { Count(syn::Expr), While(syn::Expr), @@ -240,6 +242,51 @@ impl FromMeta for FuncOrClosure { } } +/// A map expression that can be applied to a value after reading or before writing +#[derive(Debug)] +pub(crate) struct MapExpr(FuncOrClosure); + +impl FromMeta for MapExpr { + fn from_string(value: &str) -> darling::Result { + Ok(Self(FuncOrClosure::from_string(value)?)) + } +} + +impl MapExpr { + pub(crate) fn to_read_map_tokens(&self, field_name: &syn::Ident, tokens: &mut TokenStream) { + let crate_name = get_crate_name(); + let field_name_string = field_name.to_string(); + let map_expr = &self.0; + // TODO: is there a case where context might be required for reading the 'buffer_type' + // value? + tokens.extend(quote! { + { + let original_value = ::#crate_name::ParselyRead::read::(buf, ()) + .with_context(|| format!("Reading raw value for field '{}'", #field_name_string))?; + (#map_expr)(original_value).into_parsely_result() + .with_context(|| format!("Mapping raw value for field '{}'", #field_name_string)) + } + }) + } + + pub(crate) fn to_write_map_tokens(&self, field_name: &syn::Ident, tokens: &mut TokenStream) { + let crate_name = get_crate_name(); + let field_name_string = field_name.to_string(); + let map_expr = &self.0; + tokens.extend(quote! { + { + let mapped_value = (#map_expr)(&self.#field_name).into_parsely_result() + .with_context(|| format!("Mapping raw value for field '{}'", #field_name_string))?; + ::#crate_name::ParselyWrite::write::(&mapped_value, buf, ()) + .with_context(|| format!("Writing mapped value for field '{}'", #field_name_string))?; + } + }) + } +} + + + +/// An assertion that can be used after reading a value or before writing one #[derive(Debug)] pub(crate) struct Assertion(FuncOrClosure); diff --git a/tests/ui/pass/map.rs b/tests/ui/pass/map.rs index cc59ff2..e0a77de 100644 --- a/tests/ui/pass/map.rs +++ b/tests/ui/pass/map.rs @@ -1,24 +1,25 @@ -use bitvec::prelude::*; use parsely_rs::*; #[derive(ParselyRead, ParselyWrite)] struct Foo { - #[parsely_read(map = "|v: u1| -> ParselyResult { Ok(v > 0) }")] - #[parsely_write(map = "|v: &bool| -> ParselyResult { Ok(u1::from(*v)) }")] - one: bool, + #[parsely_read(map = "|v: u8| { v.to_string() }")] + #[parsely_write(map = "|v: &str| { v.parse() }")] + value: String, } fn main() { - let mut bits = Bits::from_static_bytes(&[0b10101010]); + let mut bits = Bits::from_static_bytes(&[42]); let foo = Foo::read::(&mut bits, ()).expect("successful parse"); - assert!(foo.one); + assert_eq!(foo.value, "42"); let mut bits_mut = BitsMut::new(); - let foo = Foo { one: true }; + let foo = Foo { + value: String::from("42"), + }; foo.write::(&mut bits_mut, ()) .expect("successful write"); - assert_eq!(&bits_mut[..], bits![u8, Msb0; 1]); + assert_eq!(bits.get_u8().unwrap(), 42); } From 4d88ad1a8e593a587171a504a5093e300e887c98 Mon Sep 17 00:00:00 2001 From: bbaldino Date: Fri, 25 Apr 2025 12:18:30 -0700 Subject: [PATCH 6/9] wip the current code shows a half-completed implementation of moving generics out of ParselyWrite's type and into associated types. Then ParselyWrite can be used as a trait bound, which is useful when trying to coerce types correctly when writing (e.g. the 'map' ambiguity problem). Doing this requires an additional type (WriteAdaptor) which doesn't work in its current form: instead of a single impl defined in parsely we'd need to generate impls dynamically for every type otherwise we run into "can't-define-foreign-trait-on-foreign-type" problems when we generate impls in a calling crate. --- impl/src/code_gen/gen_write.rs | 16 ++++-- impl/src/error.rs | 7 ++- impl/src/parsely_write.rs | 91 +++++++++++++++++++++++++++++- src/lib.rs | 3 +- tests/expand/assertion.expanded.rs | 80 ++++++++++++++++++++++++++ tests/expand/map.expanded.rs | 72 +++++++++++++++++++++++ tests/expand/map.rs | 8 +++ tests/ui/pass/basic_write.rs | 17 ++++++ tests/ui/pass/map.rs | 4 +- 9 files changed, 287 insertions(+), 11 deletions(-) create mode 100644 tests/expand/assertion.expanded.rs create mode 100644 tests/expand/map.expanded.rs create mode 100644 tests/expand/map.rs create mode 100644 tests/ui/pass/basic_write.rs diff --git a/impl/src/code_gen/gen_write.rs b/impl/src/code_gen/gen_write.rs index 6f9189e..634d759 100644 --- a/impl/src/code_gen/gen_write.rs +++ b/impl/src/code_gen/gen_write.rs @@ -58,18 +58,21 @@ fn generate_parsely_write_impl_struct( } else if f.ty.is_option() { field_write_output.extend(quote! { if let Some(ref v) = self.#field_name { - #write_type::write::(v, buf, (#(#context_values,)*)).with_context(|| format!("Writing field '{}'", #field_name_string))?; + ::#crate_name::WriteAdaptor::new(v).write::(buf, (#(#context_values,)*)).with_context(|| format!("Writing field '{}'", #field_name_string))?; + // #write_type::write::(v, buf, (#(#context_values,)*)).with_context(|| format!("Writing field '{}'", #field_name_string))?; } }); } else if f.ty.is_collection() { field_write_output.extend(quote! { self.#field_name.iter().enumerate().map(|(idx, v)| { - #write_type::write::(v, buf, (#(#context_values,)*)).with_context(|| format!("Index {idx}")) + ::#crate_name::WriteAdaptor::new(v).write::(buf, (#(#context_values,)*)).with_context(|| format!("Index {idx}")) + // #write_type::write::(v, buf, (#(#context_values,)*)).with_context(|| format!("Index {idx}")) }).collect::>>().with_context(|| format!("Writing field '{}'", #field_name_string))?; }); } else { field_write_output.extend(quote! { - #write_type::write::(&self.#field_name, buf, (#(#context_values,)*)).with_context(|| format!("Writing field '{}'", #field_name_string))?; + ::#crate_name::WriteAdaptor::new(&self.value.#field_name).write::(buf, (#(#context_values,)*)).with_context(|| format!("Writing field '{}'", #field_name_string))?; + // #write_type::write::(&self.#field_name, buf, (#(#context_values,)*)).with_context(|| format!("Writing field '{}'", #field_name_string))?; }); } @@ -140,8 +143,11 @@ fn generate_parsely_write_impl_struct( }; quote! { - impl ::#crate_name::ParselyWrite for #struct_name { - fn write(&self, buf: &mut B, ctx: (#(#context_types,)*)) -> ::#crate_name::ParselyResult<()> { + impl<'a, B: BitBufMut> ::#crate_name::ParselyWrite for ::#crate_name::WriteAdaptor<'a, #struct_name, B> { + type Buf = B; + type Ctx = (#(#context_types,)*); + + fn write(&self, buf: &mut Self::Buf, _: Self::Ctx) -> ParselyResult<()> { #(#context_assignments)* #body diff --git a/impl/src/error.rs b/impl/src/error.rs index d66710d..3c1b988 100644 --- a/impl/src/error.rs +++ b/impl/src/error.rs @@ -1,3 +1,5 @@ +use crate::parsely_write::{ParselyWrite, ParselyWrite2}; + pub type ParselyResult = anyhow::Result; pub trait IntoParselyResult { @@ -6,7 +8,10 @@ pub trait IntoParselyResult { // TODO: change this to be bound by T: ParselyWrite once ParselyWrite is changed to use associated // types -impl IntoParselyResult for T { +impl IntoParselyResult for T +where + T: ParselyWrite, +{ fn into_parsely_result(self) -> ParselyResult { Ok(self) } diff --git a/impl/src/parsely_write.rs b/impl/src/parsely_write.rs index ad54407..a37c1f9 100644 --- a/impl/src/parsely_write.rs +++ b/impl/src/parsely_write.rs @@ -1,3 +1,5 @@ +use std::marker::PhantomData; + use bits_io::prelude::*; use crate::error::ParselyResult; @@ -22,31 +24,108 @@ macro_rules! impl_stateless_sync { }; } -pub trait ParselyWrite: StateSync + Sized { +pub trait ParselyWrite2: StateSync + Sized { fn write(&self, buf: &mut B, ctx: Ctx) -> ParselyResult<()>; } +pub trait ParselyWrite: StateSync + Sized { + type Buf: BitBufMut; + type Ctx; + fn write(&self, buf: &mut Self::Buf, ctx: Self::Ctx) -> ParselyResult<()>; +} + +pub struct WriteAdaptor<'a, T, B> { + pub value: &'a T, + _buf: PhantomData, +} + +impl<'a, T, B> WriteAdaptor<'a, T, B> { + pub fn new(value: &'a T) -> Self { + Self { + value, + _buf: PhantomData, + } + } +} + +// The `WriteAdaptor` type enables two things: +// 1. We can now use `ParselyWrite` as a trait bounds, which is useful to allow type inference for +// scenarios like the `map` functionality on the write path, where we don't have a concrete type +// to infer the result of the map function to, other than something that can be written. +// 2. Allows us to avoid having to define a concrete associated type in the impls of ParselyWrite: +// since we can use the generics on WriteAdaptor to set as the associated typed in the +// ParselyWrite impl. +// +// One downside of it, though, is that now a `WriteAdaptor` instance needs to be created to do +// writes. This isn't a performance concern, but is a bit awkward from an API perspective + +pub trait ParselyWritableExt { + fn write_with<'a, B, BO>( + &'a self, + buf: &mut B, + ctx: as ParselyWrite>::Ctx, + ) -> ParselyResult<()> + where + Self: Sized, + B: BitBufMut, + BO: ByteOrder, + WriteAdaptor<'a, Self, B>: ParselyWrite; +} + +impl ParselyWritableExt for T { + fn write_with<'a, B, BO>( + &'a self, + buf: &mut B, + ctx: as ParselyWrite>::Ctx, + ) -> ParselyResult<()> + where + B: BitBufMut, + BO: ByteOrder, + WriteAdaptor<'a, Self, B>: ParselyWrite, + { + WriteAdaptor::<_, B>::new(self).write::(buf, ctx) + } +} + macro_rules! impl_parsely_write_builtin { ($type:ty) => { - impl ParselyWrite for $type { + impl ParselyWrite2 for $type { fn write(&self, buf: &mut B, _: ()) -> ParselyResult<()> { ::paste::paste! { Ok(buf.[](*self)?) } } } + impl<'a, B: BitBufMut> ParselyWrite for WriteAdaptor<'a, $type, B> { + type Buf = B; + type Ctx = (); + fn write(&self, buf: &mut Self::Buf, _: Self::Ctx) -> ParselyResult<()> { + ::paste::paste! { + Ok(buf.[](*self.value)?) + } + } + } }; } macro_rules! impl_parsely_write_builtin_bo { ($type:ty) => { - impl ParselyWrite for $type { + impl ParselyWrite2 for $type { fn write(&self, buf: &mut B, _: ()) -> ParselyResult<()> { ::paste::paste! { Ok(buf.[]::(*self)?) } } } + impl<'a, B: BitBufMut> ParselyWrite for WriteAdaptor<'a, $type, B> { + type Buf = B; + type Ctx = (); + fn write(&self, buf: &mut Self::Buf, _: Self::Ctx) -> ParselyResult<()> { + ::paste::paste! { + Ok(buf.[]::(*self.value)?) + } + } + } }; } @@ -71,6 +150,12 @@ macro_rules! impl_state_sync_builtin { Ok(()) } } + impl<'a, B: BitBufMut> StateSync for WriteAdaptor<'a, $type, B> { + type SyncCtx = (); + fn sync(&mut self, _sync_ctx: ()) -> ParselyResult<()> { + Ok(()) + } + } }; } diff --git a/src/lib.rs b/src/lib.rs index 2415c69..4a5c2e4 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -7,7 +7,8 @@ pub use parsely_impl::{BigEndian, ByteOrder, LittleEndian, NetworkOrder}; pub use parsely_impl::{BitBuf, BitBufExts, BitBufMut, BitBufMutExts, Bits, BitsMut}; pub use parsely_impl::{BitCursor, BitRead, BitWrite}; pub use parsely_impl::{ - parsely_read::ParselyRead, parsely_write::ParselyWrite, parsely_write::StateSync, + parsely_read::ParselyRead, parsely_write::ParselyWritableExt, parsely_write::ParselyWrite, + parsely_write::ParselyWrite2, parsely_write::StateSync, parsely_write::WriteAdaptor, }; pub use parsely_macro::{ParselyRead, ParselyWrite}; diff --git a/tests/expand/assertion.expanded.rs b/tests/expand/assertion.expanded.rs new file mode 100644 index 0000000..2ba2fd5 --- /dev/null +++ b/tests/expand/assertion.expanded.rs @@ -0,0 +1,80 @@ +use parsely_rs::*; +struct Foo { + #[parsely(assertion = "|v: &u8| *v % 2 == 0")] + value: u8, +} +impl ::parsely_rs::ParselyRead for Foo { + fn read( + buf: &mut B, + _ctx: (), + ) -> ::parsely_rs::ParselyResult { + let value = u8::read::(buf, ()) + .and_then(|read_value| { + let assertion_func = |v: &u8| *v % 2 == 0; + if !assertion_func(&read_value) { + return ::anyhow::__private::Err( + ::anyhow::Error::msg( + ::alloc::__export::must_use({ + let res = ::alloc::fmt::format( + format_args!( + "Assertion failed: value of field \'{0}\' (\'{1:?}\') didn\'t pass assertion: \'{2}\'", + "value", read_value, | v : & u8 | * v % 2 == 0, + ), + ); + res + }), + ), + ); + } + Ok(read_value) + }) + .with_context(|| "Reading field 'value'")?; + Ok(Self { value }) + } +} +impl ::parsely_rs::ParselyWrite for Foo { + fn write( + &self, + buf: &mut B, + ctx: (), + ) -> ::parsely_rs::ParselyResult<()> { + let __value_assertion_func = |v: &u8| *v % 2 == 0; + if !__value_assertion_func(&self.value) { + return ::anyhow::__private::Err( + ::anyhow::Error::msg( + ::alloc::__export::must_use({ + let res = ::alloc::fmt::format( + format_args!( + "Assertion failed: value of field \'{0}\' (\'{1:?}\') didn\'t pass assertion: \'{2}\'", + "value", self.value, | v : & u8 | * v % 2 == 0, + ), + ); + res + }), + ), + ); + } + u8::write::(&self.value, buf, ()) + .with_context(|| ::alloc::__export::must_use({ + let res = ::alloc::fmt::format( + format_args!("Writing field \'{0}\'", "value"), + ); + res + }))?; + Ok(()) + } +} +impl StateSync for Foo { + type SyncCtx = (); + fn sync(&mut self, (): ()) -> ParselyResult<()> { + self.value + .sync(()) + .with_context(|| ::alloc::__export::must_use({ + let res = ::alloc::fmt::format( + format_args!("Syncing field \'{0}\'", "value"), + ); + res + }))?; + Ok(()) + } +} diff --git a/tests/expand/map.expanded.rs b/tests/expand/map.expanded.rs new file mode 100644 index 0000000..ddba43e --- /dev/null +++ b/tests/expand/map.expanded.rs @@ -0,0 +1,72 @@ +use parsely_rs::*; +struct Foo { + #[parsely_read(map = "|v: u8| { v.to_string() }")] + #[parsely_write(map = "|v: &str| { v.parse::() }")] + value: String, +} +impl ::parsely_rs::ParselyRead for Foo { + fn read( + buf: &mut B, + _ctx: (), + ) -> ::parsely_rs::ParselyResult { + let value = { + let original_value = ::parsely_rs::ParselyRead::read::(buf, ()) + .with_context(|| ::alloc::__export::must_use({ + let res = ::alloc::fmt::format( + format_args!("Reading raw value for field \'{0}\'", "value"), + ); + res + }))?; + (|v: u8| { v.to_string() })(original_value) + .into_parsely_result() + .with_context(|| ::alloc::__export::must_use({ + let res = ::alloc::fmt::format( + format_args!("Mapping raw value for field \'{0}\'", "value"), + ); + res + })) + } + .with_context(|| "Reading field 'value'")?; + Ok(Self { value }) + } +} +impl ::parsely_rs::ParselyWrite for Foo { + fn write( + &self, + buf: &mut B, + ctx: (), + ) -> ::parsely_rs::ParselyResult<()> { + { + let mapped_value = (|v: &str| { v.parse::() })(&self.value) + .into_parsely_result() + .with_context(|| ::alloc::__export::must_use({ + let res = ::alloc::fmt::format( + format_args!("Mapping raw value for field \'{0}\'", "value"), + ); + res + }))?; + ::parsely_rs::ParselyWrite::write::(&mapped_value, buf, ()) + .with_context(|| ::alloc::__export::must_use({ + let res = ::alloc::fmt::format( + format_args!("Writing mapped value for field \'{0}\'", "value"), + ); + res + }))?; + } + Ok(()) + } +} +impl StateSync for Foo { + type SyncCtx = (); + fn sync(&mut self, (): ()) -> ParselyResult<()> { + self.value + .sync(()) + .with_context(|| ::alloc::__export::must_use({ + let res = ::alloc::fmt::format( + format_args!("Syncing field \'{0}\'", "value"), + ); + res + }))?; + Ok(()) + } +} diff --git a/tests/expand/map.rs b/tests/expand/map.rs new file mode 100644 index 0000000..b88cd5b --- /dev/null +++ b/tests/expand/map.rs @@ -0,0 +1,8 @@ +use parsely_rs::*; + +#[derive(ParselyRead, ParselyWrite)] +struct Foo { + #[parsely_read(map = "|v: u8| { v.to_string() }")] + #[parsely_write(map = "|v: &str| { v.parse::() }")] + value: String, +} diff --git a/tests/ui/pass/basic_write.rs b/tests/ui/pass/basic_write.rs new file mode 100644 index 0000000..bcc423f --- /dev/null +++ b/tests/ui/pass/basic_write.rs @@ -0,0 +1,17 @@ +use parsely_rs::*; + +#[derive(ParselyWrite)] +struct Foo { + one: bool, + two: u3, +} + +fn main() { + let mut bits_mut = BitsMut::new(); + let foo = Foo { + one: true, + two: u3::new(4), + }; + + Foo::write::(&mut bits_mut, ()).unwrap(); +} diff --git a/tests/ui/pass/map.rs b/tests/ui/pass/map.rs index e0a77de..cdad357 100644 --- a/tests/ui/pass/map.rs +++ b/tests/ui/pass/map.rs @@ -2,8 +2,10 @@ use parsely_rs::*; #[derive(ParselyRead, ParselyWrite)] struct Foo { + // #[parsely_read(map = "|v: u8| -> ParselyResult { Ok(v.to_string()) }")] + // #[parsely_write(map = "|v: &str| -> ParselyResult { Ok(v.parse()?) }")] #[parsely_read(map = "|v: u8| { v.to_string() }")] - #[parsely_write(map = "|v: &str| { v.parse() }")] + #[parsely_write(map = "|v: &str| { v.parse::() }")] value: String, } From 28f0aefdaa391d7416fb64c83d246326f4b700b9 Mon Sep 17 00:00:00 2001 From: bbaldino Date: Fri, 25 Apr 2025 12:48:08 -0700 Subject: [PATCH 7/9] wip: good progress on moving the generic values to the 'write' method instead: it looks like this accomplishes enabling ParselyWrite as a trait bound but without requiring the 'WriteAdaptor' type to still enable a generic buffer type as opposed to a concrete one. I still think the 'Ctx' type might work best as an associated type and we don't need generics there, so need to try that. --- impl/src/code_gen/gen_write.rs | 20 ++++--- impl/src/error.rs | 38 +++++++++--- impl/src/model_types.rs | 4 +- impl/src/parsely_write.rs | 103 +++++---------------------------- src/lib.rs | 5 +- 5 files changed, 59 insertions(+), 111 deletions(-) diff --git a/impl/src/code_gen/gen_write.rs b/impl/src/code_gen/gen_write.rs index 634d759..b5ff472 100644 --- a/impl/src/code_gen/gen_write.rs +++ b/impl/src/code_gen/gen_write.rs @@ -58,21 +58,24 @@ fn generate_parsely_write_impl_struct( } else if f.ty.is_option() { field_write_output.extend(quote! { if let Some(ref v) = self.#field_name { - ::#crate_name::WriteAdaptor::new(v).write::(buf, (#(#context_values,)*)).with_context(|| format!("Writing field '{}'", #field_name_string))?; + // ::#crate_name::WriteAdaptor::new(v).write::(buf, (#(#context_values,)*)).with_context(|| format!("Writing field '{}'", #field_name_string))?; // #write_type::write::(v, buf, (#(#context_values,)*)).with_context(|| format!("Writing field '{}'", #field_name_string))?; + #write_type::write::(v, buf, (#(#context_values,)*)).with_context(|| format!("Writing field '{}'", #field_name_string))?; } }); } else if f.ty.is_collection() { field_write_output.extend(quote! { self.#field_name.iter().enumerate().map(|(idx, v)| { - ::#crate_name::WriteAdaptor::new(v).write::(buf, (#(#context_values,)*)).with_context(|| format!("Index {idx}")) + // ::#crate_name::WriteAdaptor::new(v).write::(buf, (#(#context_values,)*)).with_context(|| format!("Index {idx}")) // #write_type::write::(v, buf, (#(#context_values,)*)).with_context(|| format!("Index {idx}")) + #write_type::write::(v, buf, (#(#context_values,)*)).with_context(|| format!("Index {idx}")) }).collect::>>().with_context(|| format!("Writing field '{}'", #field_name_string))?; }); } else { field_write_output.extend(quote! { - ::#crate_name::WriteAdaptor::new(&self.value.#field_name).write::(buf, (#(#context_values,)*)).with_context(|| format!("Writing field '{}'", #field_name_string))?; + // ::#crate_name::WriteAdaptor::new(&self.value.#field_name).write::(buf, (#(#context_values,)*)).with_context(|| format!("Writing field '{}'", #field_name_string))?; // #write_type::write::(&self.#field_name, buf, (#(#context_values,)*)).with_context(|| format!("Writing field '{}'", #field_name_string))?; + #write_type::write::(&self.#field_name, buf, (#(#context_values,)*)).with_context(|| format!("Writing field '{}'", #field_name_string))?; }); } @@ -143,11 +146,12 @@ fn generate_parsely_write_impl_struct( }; quote! { - impl<'a, B: BitBufMut> ::#crate_name::ParselyWrite for ::#crate_name::WriteAdaptor<'a, #struct_name, B> { - type Buf = B; - type Ctx = (#(#context_types,)*); - - fn write(&self, buf: &mut Self::Buf, _: Self::Ctx) -> ParselyResult<()> { + impl ::#crate_name::ParselyWrite for #struct_name { + fn write( + &self, + buf: &mut B, + ctx: Ctx, + ) -> ParselyResult<()> { #(#context_assignments)* #body diff --git a/impl/src/error.rs b/impl/src/error.rs index 3c1b988..97ac835 100644 --- a/impl/src/error.rs +++ b/impl/src/error.rs @@ -1,14 +1,16 @@ -use crate::parsely_write::{ParselyWrite, ParselyWrite2}; +use crate::parsely_write::ParselyWrite; pub type ParselyResult = anyhow::Result; -pub trait IntoParselyResult { +/// Helper trait to coerce values of both `T: ParselyWrite` and `Result: E: +/// Into` into `ParselyResult`. We need a trait specifically for writing because +/// if we don't bound the impl for `T` in some way there's ambiguity: the compiler doesn't know if +/// we want `ParselyResult` or `ParselyResult>`. +pub trait IntoWritableParselyResult { fn into_parsely_result(self) -> ParselyResult; } -// TODO: change this to be bound by T: ParselyWrite once ParselyWrite is changed to use associated -// types -impl IntoParselyResult for T +impl IntoWritableParselyResult for T where T: ParselyWrite, { @@ -17,7 +19,7 @@ where } } -impl IntoParselyResult for Result +impl IntoWritableParselyResult for Result where E: Into, { @@ -26,6 +28,26 @@ where } } -pub fn wrap_in_parsely_result(value: impl IntoParselyResult) -> ParselyResult { - value.into_parsely_result() +/// When we need to convert an expression that may or may not be wrapped in a Result on the _read_ +/// path, we can rely on the fact that we'll eventually be assigning the value to a field with a +/// concrete type and we can rely on type inference in order to figure out what that should be. +/// Because of that we don't want/need the `ParselyWrite` trait bounds on the impl like we have +/// above for the writable side, so we need a different trait here. +pub trait IntoParselyResult { + fn into_parsely_result_read(self) -> ParselyResult; +} + +impl IntoParselyResult for T { + fn into_parsely_result_read(self) -> ParselyResult { + Ok(self) + } +} + +impl IntoParselyResult for Result +where + E: Into, +{ + fn into_parsely_result_read(self) -> ParselyResult { + self.map_err(Into::into) + } } diff --git a/impl/src/model_types.rs b/impl/src/model_types.rs index 462f756..5b781bd 100644 --- a/impl/src/model_types.rs +++ b/impl/src/model_types.rs @@ -263,7 +263,7 @@ impl MapExpr { { let original_value = ::#crate_name::ParselyRead::read::(buf, ()) .with_context(|| format!("Reading raw value for field '{}'", #field_name_string))?; - (#map_expr)(original_value).into_parsely_result() + (#map_expr)(original_value).into_parsely_result_read() .with_context(|| format!("Mapping raw value for field '{}'", #field_name_string)) } }) @@ -277,7 +277,7 @@ impl MapExpr { { let mapped_value = (#map_expr)(&self.#field_name).into_parsely_result() .with_context(|| format!("Mapping raw value for field '{}'", #field_name_string))?; - ::#crate_name::ParselyWrite::write::(&mapped_value, buf, ()) + ::#crate_name::ParselyWrite::write::(&mapped_value, buf, ()) .with_context(|| format!("Writing mapped value for field '{}'", #field_name_string))?; } }) diff --git a/impl/src/parsely_write.rs b/impl/src/parsely_write.rs index a37c1f9..0d07644 100644 --- a/impl/src/parsely_write.rs +++ b/impl/src/parsely_write.rs @@ -1,5 +1,3 @@ -use std::marker::PhantomData; - use bits_io::prelude::*; use crate::error::ParselyResult; @@ -24,108 +22,39 @@ macro_rules! impl_stateless_sync { }; } -pub trait ParselyWrite2: StateSync + Sized { - fn write(&self, buf: &mut B, ctx: Ctx) -> ParselyResult<()>; -} - pub trait ParselyWrite: StateSync + Sized { - type Buf: BitBufMut; - type Ctx; - fn write(&self, buf: &mut Self::Buf, ctx: Self::Ctx) -> ParselyResult<()>; -} - -pub struct WriteAdaptor<'a, T, B> { - pub value: &'a T, - _buf: PhantomData, -} - -impl<'a, T, B> WriteAdaptor<'a, T, B> { - pub fn new(value: &'a T) -> Self { - Self { - value, - _buf: PhantomData, - } - } -} - -// The `WriteAdaptor` type enables two things: -// 1. We can now use `ParselyWrite` as a trait bounds, which is useful to allow type inference for -// scenarios like the `map` functionality on the write path, where we don't have a concrete type -// to infer the result of the map function to, other than something that can be written. -// 2. Allows us to avoid having to define a concrete associated type in the impls of ParselyWrite: -// since we can use the generics on WriteAdaptor to set as the associated typed in the -// ParselyWrite impl. -// -// One downside of it, though, is that now a `WriteAdaptor` instance needs to be created to do -// writes. This isn't a performance concern, but is a bit awkward from an API perspective - -pub trait ParselyWritableExt { - fn write_with<'a, B, BO>( - &'a self, - buf: &mut B, - ctx: as ParselyWrite>::Ctx, - ) -> ParselyResult<()> - where - Self: Sized, - B: BitBufMut, - BO: ByteOrder, - WriteAdaptor<'a, Self, B>: ParselyWrite; -} - -impl ParselyWritableExt for T { - fn write_with<'a, B, BO>( - &'a self, - buf: &mut B, - ctx: as ParselyWrite>::Ctx, - ) -> ParselyResult<()> - where - B: BitBufMut, - BO: ByteOrder, - WriteAdaptor<'a, Self, B>: ParselyWrite, - { - WriteAdaptor::<_, B>::new(self).write::(buf, ctx) - } + fn write(&self, buf: &mut B, ctx: Ctx) -> ParselyResult<()>; } macro_rules! impl_parsely_write_builtin { ($type:ty) => { - impl ParselyWrite2 for $type { - fn write(&self, buf: &mut B, _: ()) -> ParselyResult<()> { + impl ParselyWrite for $type { + fn write( + &self, + buf: &mut B, + _: Ctx, + ) -> ParselyResult<()> { ::paste::paste! { Ok(buf.[](*self)?) } } } - impl<'a, B: BitBufMut> ParselyWrite for WriteAdaptor<'a, $type, B> { - type Buf = B; - type Ctx = (); - fn write(&self, buf: &mut Self::Buf, _: Self::Ctx) -> ParselyResult<()> { - ::paste::paste! { - Ok(buf.[](*self.value)?) - } - } - } }; } macro_rules! impl_parsely_write_builtin_bo { ($type:ty) => { - impl ParselyWrite2 for $type { - fn write(&self, buf: &mut B, _: ()) -> ParselyResult<()> { + impl ParselyWrite for $type { + fn write( + &self, + buf: &mut B, + _: Ctx, + ) -> ParselyResult<()> { ::paste::paste! { Ok(buf.[]::(*self)?) } } } - impl<'a, B: BitBufMut> ParselyWrite for WriteAdaptor<'a, $type, B> { - type Buf = B; - type Ctx = (); - fn write(&self, buf: &mut Self::Buf, _: Self::Ctx) -> ParselyResult<()> { - ::paste::paste! { - Ok(buf.[]::(*self.value)?) - } - } - } }; } @@ -150,12 +79,6 @@ macro_rules! impl_state_sync_builtin { Ok(()) } } - impl<'a, B: BitBufMut> StateSync for WriteAdaptor<'a, $type, B> { - type SyncCtx = (); - fn sync(&mut self, _sync_ctx: ()) -> ParselyResult<()> { - Ok(()) - } - } }; } diff --git a/src/lib.rs b/src/lib.rs index 4a5c2e4..bef3a92 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,14 +1,13 @@ // TODO: these should be moved to a prelude file pub use parsely_impl::anyhow::{Context, anyhow, bail}; -pub use parsely_impl::error::{IntoParselyResult, ParselyResult}; +pub use parsely_impl::error::{IntoParselyResult, IntoWritableParselyResult, ParselyResult}; pub use parsely_impl::impl_stateless_sync; pub use parsely_impl::nsw_types::*; pub use parsely_impl::{BigEndian, ByteOrder, LittleEndian, NetworkOrder}; pub use parsely_impl::{BitBuf, BitBufExts, BitBufMut, BitBufMutExts, Bits, BitsMut}; pub use parsely_impl::{BitCursor, BitRead, BitWrite}; pub use parsely_impl::{ - parsely_read::ParselyRead, parsely_write::ParselyWritableExt, parsely_write::ParselyWrite, - parsely_write::ParselyWrite2, parsely_write::StateSync, parsely_write::WriteAdaptor, + parsely_read::ParselyRead, parsely_write::ParselyWrite, parsely_write::StateSync, }; pub use parsely_macro::{ParselyRead, ParselyWrite}; From 75b2aab68a8cdc93f9a06384b46a852214516773 Mon Sep 17 00:00:00 2001 From: bbaldino Date: Fri, 25 Apr 2025 13:41:12 -0700 Subject: [PATCH 8/9] i think this completes the core work to try and improve the ergonomics around expressions that may or may not return a ParselyResult. there's still cleanup to do (arguably the ParselyRead trait should be tweaked to match ParselyWrite now) --- impl/src/code_gen/gen_write.rs | 19 ++++++++----------- impl/src/model_types.rs | 8 +++++--- impl/src/parsely_write.rs | 14 +++++++++----- tests/expand/alignment.expanded.rs | 11 ++++++----- tests/expand/assertion.expanded.rs | 15 ++++++++------- tests/expand/map.expanded.rs | 13 +++++++------ tests/ui/pass/alignment.rs | 2 +- tests/ui/pass/basic_write.rs | 2 +- tests/ui/pass/map.rs | 5 ++--- 9 files changed, 47 insertions(+), 42 deletions(-) diff --git a/impl/src/code_gen/gen_write.rs b/impl/src/code_gen/gen_write.rs index b5ff472..9e018cb 100644 --- a/impl/src/code_gen/gen_write.rs +++ b/impl/src/code_gen/gen_write.rs @@ -53,29 +53,25 @@ fn generate_parsely_write_impl_struct( assertion.to_write_assertion_tokens(&field_name_string, &mut field_write_output); } + // TODO: these write calls should be qualified. Something like <#write_type as + // ParselyWrite>::write if let Some(ref map_expr) = f.common.map { map_expr.to_write_map_tokens(field_name, &mut field_write_output); } else if f.ty.is_option() { field_write_output.extend(quote! { if let Some(ref v) = self.#field_name { - // ::#crate_name::WriteAdaptor::new(v).write::(buf, (#(#context_values,)*)).with_context(|| format!("Writing field '{}'", #field_name_string))?; - // #write_type::write::(v, buf, (#(#context_values,)*)).with_context(|| format!("Writing field '{}'", #field_name_string))?; - #write_type::write::(v, buf, (#(#context_values,)*)).with_context(|| format!("Writing field '{}'", #field_name_string))?; + #write_type::write::<_, T>(v, buf, (#(#context_values,)*)).with_context(|| format!("Writing field '{}'", #field_name_string))?; } }); } else if f.ty.is_collection() { field_write_output.extend(quote! { self.#field_name.iter().enumerate().map(|(idx, v)| { - // ::#crate_name::WriteAdaptor::new(v).write::(buf, (#(#context_values,)*)).with_context(|| format!("Index {idx}")) - // #write_type::write::(v, buf, (#(#context_values,)*)).with_context(|| format!("Index {idx}")) - #write_type::write::(v, buf, (#(#context_values,)*)).with_context(|| format!("Index {idx}")) + #write_type::write::<_, T>(v, buf, (#(#context_values,)*)).with_context(|| format!("Index {idx}")) }).collect::>>().with_context(|| format!("Writing field '{}'", #field_name_string))?; }); } else { field_write_output.extend(quote! { - // ::#crate_name::WriteAdaptor::new(&self.value.#field_name).write::(buf, (#(#context_values,)*)).with_context(|| format!("Writing field '{}'", #field_name_string))?; - // #write_type::write::(&self.#field_name, buf, (#(#context_values,)*)).with_context(|| format!("Writing field '{}'", #field_name_string))?; - #write_type::write::(&self.#field_name, buf, (#(#context_values,)*)).with_context(|| format!("Writing field '{}'", #field_name_string))?; + #write_type::write::<_, T>(&self.#field_name, buf, (#(#context_values,)*)).with_context(|| format!("Writing field '{}'", #field_name_string))?; }); } @@ -147,10 +143,11 @@ fn generate_parsely_write_impl_struct( quote! { impl ::#crate_name::ParselyWrite for #struct_name { - fn write( + type Ctx = (#(#context_types,)*); + fn write( &self, buf: &mut B, - ctx: Ctx, + ctx: Self::Ctx, ) -> ParselyResult<()> { #(#context_assignments)* diff --git a/impl/src/model_types.rs b/impl/src/model_types.rs index 5b781bd..19e605c 100644 --- a/impl/src/model_types.rs +++ b/impl/src/model_types.rs @@ -277,7 +277,7 @@ impl MapExpr { { let mapped_value = (#map_expr)(&self.#field_name).into_parsely_result() .with_context(|| format!("Mapping raw value for field '{}'", #field_name_string))?; - ::#crate_name::ParselyWrite::write::(&mapped_value, buf, ()) + ::#crate_name::ParselyWrite::write::(&mapped_value, buf, ()) .with_context(|| format!("Writing mapped value for field '{}'", #field_name_string))?; } }) @@ -305,11 +305,12 @@ impl FromMeta for Assertion { impl Assertion { pub(crate) fn to_read_assertion_tokens(&self, field_name: &str, tokens: &mut TokenStream) { let assertion = &self.0; + let assertion_string = quote! { #assertion }.to_string(); tokens.extend(quote! { .and_then(|read_value| { let assertion_func = #assertion; if !assertion_func(&read_value) { - bail!("Assertion failed: value of field '{}' ('{:?}') didn't pass assertion: '{}'", #field_name, read_value, #assertion) + bail!("Assertion failed: value of field '{}' ('{:?}') didn't pass assertion: '{}'", #field_name, read_value, #assertion_string) } Ok(read_value) }) @@ -318,12 +319,13 @@ impl Assertion { pub(crate) fn to_write_assertion_tokens(&self, field_name: &str, tokens: &mut TokenStream) { let assertion = &self.0; + let assertion_string = quote! { #assertion }.to_string(); let assertion_func_ident = format_ident!("__{}_assertion_func", field_name); let field_name_ident = format_ident!("{field_name}"); tokens.extend(quote! { let #assertion_func_ident = #assertion; if !#assertion_func_ident(&self.#field_name_ident) { - bail!("Assertion failed: value of field '{}' ('{:?}') didn't pass assertion: '{}'", #field_name, self.#field_name_ident, #assertion) + bail!("Assertion failed: value of field '{}' ('{:?}') didn't pass assertion: '{}'", #field_name, self.#field_name_ident, #assertion_string) } }) } diff --git a/impl/src/parsely_write.rs b/impl/src/parsely_write.rs index 0d07644..b45ec11 100644 --- a/impl/src/parsely_write.rs +++ b/impl/src/parsely_write.rs @@ -23,16 +23,19 @@ macro_rules! impl_stateless_sync { } pub trait ParselyWrite: StateSync + Sized { - fn write(&self, buf: &mut B, ctx: Ctx) -> ParselyResult<()>; + type Ctx; + fn write(&self, buf: &mut B, ctx: Self::Ctx) -> ParselyResult<()>; } macro_rules! impl_parsely_write_builtin { ($type:ty) => { impl ParselyWrite for $type { - fn write( + type Ctx = (); + + fn write( &self, buf: &mut B, - _: Ctx, + _: Self::Ctx, ) -> ParselyResult<()> { ::paste::paste! { Ok(buf.[](*self)?) @@ -45,10 +48,11 @@ macro_rules! impl_parsely_write_builtin { macro_rules! impl_parsely_write_builtin_bo { ($type:ty) => { impl ParselyWrite for $type { - fn write( + type Ctx = (); + fn write( &self, buf: &mut B, - _: Ctx, + _: Self::Ctx, ) -> ParselyResult<()> { ::paste::paste! { Ok(buf.[]::(*self)?) diff --git a/tests/expand/alignment.expanded.rs b/tests/expand/alignment.expanded.rs index 1cd44c5..bcfb7e0 100644 --- a/tests/expand/alignment.expanded.rs +++ b/tests/expand/alignment.expanded.rs @@ -19,14 +19,15 @@ impl ::parsely_rs::ParselyRead for Foo { Ok(Self { one }) } } -impl ::parsely_rs::ParselyWrite for Foo { - fn write( +impl ::parsely_rs::ParselyWrite for Foo { + type Ctx = (); + fn write( &self, buf: &mut B, - ctx: (), - ) -> ::parsely_rs::ParselyResult<()> { + ctx: Self::Ctx, + ) -> ParselyResult<()> { let __bytes_remaining_start = buf.remaining_mut_bytes(); - u8::write::(&self.one, buf, ()) + u8::write::<_, T>(&self.one, buf, ()) .with_context(|| ::alloc::__export::must_use({ let res = ::alloc::fmt::format( format_args!("Writing field \'{0}\'", "one"), diff --git a/tests/expand/assertion.expanded.rs b/tests/expand/assertion.expanded.rs index 2ba2fd5..cc10fe1 100644 --- a/tests/expand/assertion.expanded.rs +++ b/tests/expand/assertion.expanded.rs @@ -18,7 +18,7 @@ impl ::parsely_rs::ParselyRead for Foo { let res = ::alloc::fmt::format( format_args!( "Assertion failed: value of field \'{0}\' (\'{1:?}\') didn\'t pass assertion: \'{2}\'", - "value", read_value, | v : & u8 | * v % 2 == 0, + "value", read_value, "| v : & u8 | * v % 2 == 0", ), ); res @@ -32,12 +32,13 @@ impl ::parsely_rs::ParselyRead for Foo { Ok(Self { value }) } } -impl ::parsely_rs::ParselyWrite for Foo { - fn write( +impl ::parsely_rs::ParselyWrite for Foo { + type Ctx = (); + fn write( &self, buf: &mut B, - ctx: (), - ) -> ::parsely_rs::ParselyResult<()> { + ctx: Self::Ctx, + ) -> ParselyResult<()> { let __value_assertion_func = |v: &u8| *v % 2 == 0; if !__value_assertion_func(&self.value) { return ::anyhow::__private::Err( @@ -46,7 +47,7 @@ impl ::parsely_rs::ParselyWrite for Foo { let res = ::alloc::fmt::format( format_args!( "Assertion failed: value of field \'{0}\' (\'{1:?}\') didn\'t pass assertion: \'{2}\'", - "value", self.value, | v : & u8 | * v % 2 == 0, + "value", self.value, "| v : & u8 | * v % 2 == 0", ), ); res @@ -54,7 +55,7 @@ impl ::parsely_rs::ParselyWrite for Foo { ), ); } - u8::write::(&self.value, buf, ()) + u8::write::<_, T>(&self.value, buf, ()) .with_context(|| ::alloc::__export::must_use({ let res = ::alloc::fmt::format( format_args!("Writing field \'{0}\'", "value"), diff --git a/tests/expand/map.expanded.rs b/tests/expand/map.expanded.rs index ddba43e..3c3e8b4 100644 --- a/tests/expand/map.expanded.rs +++ b/tests/expand/map.expanded.rs @@ -18,7 +18,7 @@ impl ::parsely_rs::ParselyRead for Foo { res }))?; (|v: u8| { v.to_string() })(original_value) - .into_parsely_result() + .into_parsely_result_read() .with_context(|| ::alloc::__export::must_use({ let res = ::alloc::fmt::format( format_args!("Mapping raw value for field \'{0}\'", "value"), @@ -30,12 +30,13 @@ impl ::parsely_rs::ParselyRead for Foo { Ok(Self { value }) } } -impl ::parsely_rs::ParselyWrite for Foo { - fn write( +impl ::parsely_rs::ParselyWrite for Foo { + type Ctx = (); + fn write( &self, buf: &mut B, - ctx: (), - ) -> ::parsely_rs::ParselyResult<()> { + ctx: Self::Ctx, + ) -> ParselyResult<()> { { let mapped_value = (|v: &str| { v.parse::() })(&self.value) .into_parsely_result() @@ -45,7 +46,7 @@ impl ::parsely_rs::ParselyWrite for Foo { ); res }))?; - ::parsely_rs::ParselyWrite::write::(&mapped_value, buf, ()) + ::parsely_rs::ParselyWrite::write::(&mapped_value, buf, ()) .with_context(|| ::alloc::__export::must_use({ let res = ::alloc::fmt::format( format_args!("Writing mapped value for field \'{0}\'", "value"), diff --git a/tests/ui/pass/alignment.rs b/tests/ui/pass/alignment.rs index ddecd9a..dd82e08 100644 --- a/tests/ui/pass/alignment.rs +++ b/tests/ui/pass/alignment.rs @@ -15,6 +15,6 @@ fn main() { let mut bits_mut = BitsMut::new(); - Foo::write::(&foo, &mut bits_mut, ()).unwrap(); + Foo::write::<_, NetworkOrder>(&foo, &mut bits_mut, ()).unwrap(); assert_eq!(bits_mut.len_bytes(), 4); } diff --git a/tests/ui/pass/basic_write.rs b/tests/ui/pass/basic_write.rs index bcc423f..25aeeeb 100644 --- a/tests/ui/pass/basic_write.rs +++ b/tests/ui/pass/basic_write.rs @@ -13,5 +13,5 @@ fn main() { two: u3::new(4), }; - Foo::write::(&mut bits_mut, ()).unwrap(); + Foo::write::<_, NetworkOrder>(&foo, &mut bits_mut, ()).unwrap(); } diff --git a/tests/ui/pass/map.rs b/tests/ui/pass/map.rs index cdad357..27224dd 100644 --- a/tests/ui/pass/map.rs +++ b/tests/ui/pass/map.rs @@ -2,8 +2,6 @@ use parsely_rs::*; #[derive(ParselyRead, ParselyWrite)] struct Foo { - // #[parsely_read(map = "|v: u8| -> ParselyResult { Ok(v.to_string()) }")] - // #[parsely_write(map = "|v: &str| -> ParselyResult { Ok(v.parse()?) }")] #[parsely_read(map = "|v: u8| { v.to_string() }")] #[parsely_write(map = "|v: &str| { v.parse::() }")] value: String, @@ -21,7 +19,8 @@ fn main() { value: String::from("42"), }; - foo.write::(&mut bits_mut, ()) + foo.write::<_, NetworkOrder>(&mut bits_mut, ()) .expect("successful write"); + let mut bits = bits_mut.freeze(); assert_eq!(bits.get_u8().unwrap(), 42); } From 5cd49b3ada2cc0b9aa3a33a46c78375943cf3184 Mon Sep 17 00:00:00 2001 From: bbaldino Date: Fri, 25 Apr 2025 13:58:59 -0700 Subject: [PATCH 9/9] ci: update workflow file/fix some formatting issues --- .github/workflows/rust.yml | 20 +++++++++++++++----- impl/src/code_gen/gen_write.rs | 2 +- impl/src/model_types.rs | 4 +--- 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 7a53108..c7273da 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -16,11 +16,21 @@ jobs: steps: - uses: actions/checkout@v4 + - uses: dtolnay/rust-toolchain@master + with: + toolchain: stable + components: clippy + - name: Install cargo expand + run: cargo install cargo-expand - name: Build - run: cargo build --verbose + run: cargo build --verbose --all-features + - name: Run clippy + run: cargo clippy -- --D warnings - name: Run tests - run: cargo test --verbose - - uses: actions-rs/toolchain@v1 + run: cargo test --verbose --all-features + - uses: dtolnay/rust-toolchain@master with: - toolchain: stable - override: true + toolchain: nightly + components: rustfmt + - name: Run fmt check + run: cargo +nightly fmt --check diff --git a/impl/src/code_gen/gen_write.rs b/impl/src/code_gen/gen_write.rs index 9e018cb..296c402 100644 --- a/impl/src/code_gen/gen_write.rs +++ b/impl/src/code_gen/gen_write.rs @@ -56,7 +56,7 @@ fn generate_parsely_write_impl_struct( // TODO: these write calls should be qualified. Something like <#write_type as // ParselyWrite>::write if let Some(ref map_expr) = f.common.map { - map_expr.to_write_map_tokens(field_name, &mut field_write_output); + map_expr.to_write_map_tokens(field_name, &mut field_write_output); } else if f.ty.is_option() { field_write_output.extend(quote! { if let Some(ref v) = self.#field_name { diff --git a/impl/src/model_types.rs b/impl/src/model_types.rs index 19e605c..9e01d43 100644 --- a/impl/src/model_types.rs +++ b/impl/src/model_types.rs @@ -75,7 +75,7 @@ impl Context { // We support Context expressions that return a ParselyResult or a raw value. So now wrap // all the expressions with code that will normalize all of the results into // ParselyResults. - self.0 + self.0 .iter() .cloned() .enumerate() @@ -284,8 +284,6 @@ impl MapExpr { } } - - /// An assertion that can be used after reading a value or before writing one #[derive(Debug)] pub(crate) struct Assertion(FuncOrClosure);