Better expression ergonomics

.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

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::<u8>() }")]
+    value: String,
+}
+```
+
+the generated read code can look like:
+
+```rust
+  let original_value = ::parsely_rs::ParselyRead::read::<T>(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::<T>(&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<T>` regardless of whether it returned some raw type T or some kind of Result<T, E>.
+
+The first solution that comes to mind to try and do that is this:
+
+```rust
+pub trait IntoParselyResult<T> {
+    fn into_parsely_result(self) -> ParselyResult<T>;
+}
+
+impl<T, E> IntoParselyResult<T> for Result<T, E>
+where
+    E: Into<anyhow::Error>,
+{
+    fn into_parsely_result(self) -> ParselyResult<T> {
+        self.map_err(Into::into)
+    }
+}
+
+impl<T> IntoParselyResult<T> for T {
+    fn into_parsely_result(self) -> ParselyResult<T> {
+        Ok(self)
+    }
+}
+```
+
+The problem is that for the case where the expression returns `Result<T, E>`
+there's ambiguity: it can't tell if what we want is `ParselyResult<T>` or
+`ParselyResult<Result<T, E>>` 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<T> IntoParselyResult<T> for T where T: ParselyWrite {
+    fn into_parsely_result(self) -> ParselyResult<T> {
+        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
 

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,35 +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::<T>(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)
-        }
-    }
-}
-
-/// 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,18 +84,19 @@ 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 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 {
     let field_name = field_data
         .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();
@@ -136,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())
@@ -146,15 +116,14 @@ 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);
-
     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)?});

impl/src/code_gen/gen_write.rs

@@ -50,35 +50,28 @@ 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 {
-                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::<T>(&mapped_value, buf, ()).with_context(|| format!("Writing mapped value for field '{}'", #field_name_string))?;
-                });
+            // 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 {
-                        #write_type::write::<T>(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)| {
-                        #write_type::write::<T>(v, buf, (#(#context_values,)*)).with_context(|| format!("Index {idx}"))
+                        #write_type::write::<_, T>(v, buf, (#(#context_values,)*)).with_context(|| format!("Index {idx}"))
                     }).collect::<ParselyResult<Vec<_>>>().with_context(|| format!("Writing field '{}'", #field_name_string))?;
                 });
             } else {
                 field_write_output.extend(quote! {
-                    #write_type::write::<T>(&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))?;
                 });
             }
 
@@ -104,22 +97,22 @@ 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<T> or
                 // Vec<T>, synchronization is only going to make sense if a custom function was
                 // 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))?;
                 }
@@ -149,8 +142,13 @@ fn generate_parsely_write_impl_struct(
     };
 
     quote! {
-        impl<B: BitBufMut> ::#crate_name::ParselyWrite<B, (#(#context_types,)*)> for #struct_name {
-            fn write<T: ::#crate_name::ByteOrder>(&self, buf: &mut B, ctx: (#(#context_types,)*)) -> ::#crate_name::ParselyResult<()> {
+        impl ::#crate_name::ParselyWrite for #struct_name {
+            type Ctx = (#(#context_types,)*);
+            fn write<B: BitBufMut, T: ByteOrder>(
+                &self,
+                buf: &mut B,
+                ctx: Self::Ctx,
+            ) -> ParselyResult<()> {
                 #(#context_assignments)*
 
                 #body
@@ -159,7 +157,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)*