derive Joined to implement JoinedValue and BufferMapLayout

[koonpeng]

Status:

Updated to the new join api. Added derive helper attributes, but the only config now is to change the generated buffer struct name.

• With select_buffers there should no longer be a need to change the buffer type. Allowing to customize the types also lead to other issues as explained in derive Joined to implement JoinedValue and BufferMapLayout #53 (comment).
• It is not possible to change expose pub idents in a private span and vice versa (E0446). In derive macros, my guess is that it inherits the visibility of the target struct, attempting to generate structs with different visibility than the original will result in E0446.

---

For now only struct with named fields is supported, tuple struct or newtype struct doesn't make sense as a buffer map layout. enums are questionable but they will require more logic to validate and select which branch to use.

[mxgrey]

The latest version of #52 refactors the way JoinedValue is implemented to make it significantly simpler. Here are the highlights:

• BufferedMap has been removed. BufferMap remains as a generic container of named buffers.
• We no longer need Builder::join_into. Now the JoinedValues can be produced by the already existing Builder::join. The new Builder::try_join_into has been renamed to simply Builder::try_join.
• The JoinedValue trait implementation only needs to define the associated type field type Buffers.
• The JoinedValue::Buffers type now must implement BufferMapLayout and Joined where the Joined::Item must match the JoinedValue that it is associated with.
• I've renamed IncompatibleLayout::require_buffer into IncompatibleLayout::require_message_type and then added IncompatibleLayout::require_buffer_type. The latter allows users to require specialized buffer types like JsonBuffer. In the future we will likely add more like ProtoBuffer and/or RosBuffer.

The trait implementation examples for these changes can be found here. You should find that it's overall much simpler than the previous implementation.

I'd like to request a few additional features for the JoinedValue proc macro:

• We should have some attribute that lets the user override how private the _Buffers struct is (i.e. pub, pub(crate), private). By default the privacy should match the struct that is being derived.
• I recommend all the fields for the _Buffers struct should be pub. Let the privacy of the struct dictate the overall privacy.
• Provide an attribute that lets the user override the name of the _Buffers struct.
• Provide an attribute that lets the user override the expected buffer type for one of the fields in the JoinedValue, e.g. the user can apply #[buffer(JsonBuffer)] to a field in the JoinedValue struct so that the _Buffers struct uses a JsonBuffer for that field.

[koonpeng]

In this implementation, instead of allowing to customize the buffer type, there is a select_buffer function that works for any buffer types. I think this is better because of several problems I discovered while trying to implement customizing buffer types.

1. In some cases, it will lead to invalid code being generated.

e.g.

  #[derive(Clone, JoinedValue)]
  struct TestJoinedValue<T: Send + Sync + 'static + Clone> {
      integer: i64,
      float: f64,
      string: String,
      #[bevy_impulse(buffer_type = AnyBuffer)]
      generic: T,
  }

this will generate

#[derive(Clone)]
#[allow(non_camel_case_types)]
struct __bevy_impulse_TestJoinedValue_Buffers<T: Send + Sync + 'static + Clone> {
    integer: ::bevy_impulse::Buffer<i64>,
    float: ::bevy_impulse::Buffer<f64>,
    string: ::bevy_impulse::Buffer<String>,
    generic: AnyBuffer,
}

T is never used so we get a compile error. In order to workaround this, we would need to analyze all the fields and remove generics that is never used.

2. AnyBuffer does not impl as_any_buffer, so this code is invalid.

  fn buffer_list(&self) -> ::smallvec::SmallVec<[AnyBuffer; 8]> {
      use smallvec::smallvec;
      smallvec![
          self.integer.as_any_buffer(),
          self.float.as_any_buffer(),
          self.string.as_any_buffer(),
          self.generic.as_any_buffer(),
      ]
  }

An easy fix is to impl as_any_buffer.

3. Different buffers has different pull schematics

impl<T: Send + Sync + 'static + Clone> ::bevy_impulse::Joined
    for __bevy_impulse_TestJoinedValue_Buffers<T>
{
    type Item = TestJoinedValue<T>;
    fn pull(
        &self,
        session: ::bevy_ecs::prelude::Entity,
        world: &mut ::bevy_ecs::prelude::World,
    ) -> Result<Self::Item, ::bevy_impulse::OperationError> {
        let integer = self.integer.pull(session, world)?;
        let float = self.float.pull(session, world)?;
        let string = self.string.pull(session, world)?;
        let generic = self.generic.pull(session, world)?;
        Ok(Self::Item {
            integer,
            float,
            string,
            generic,
        })
    }
}

This fails as generic ends up as a AnyMessageBox. To get T, we need to either downcast it or convert AnyBuffer into Buffer<T>, I guess the latter is preferred, but we can't do the conversion as we don't have access to a Builder.

4. Buffer types are strict and fixed, so only 1 type of buffers can enjoy the convenience.

// assume `generic_buffer` is a `AnyBuffer`
let user_friendly  = __bevy_impulse_TestJoinedValue_Buffer {
  ...
  generic: generic_buffer,
}
// assume `generic_output` is a `Output<T>`
let not_friendly = __bevy_impulse_TestJoinedValue_Buffer {
  ...
  generic: generic_output.into_buffer().as_any_buffer(),
}
...

Because the buffer type is fixed, a conversion needs to be done to use other types of buffers.

5. Special buffer types will need to be converted into Buffer<T> anyway when we pull from it.

[koonpeng]

Tried to implement customizing the generated struct visibility but it seems that it is not possible to change expose pub idents in a private span and vice versa (E0446). In derive macros, my guess is that it inherits the visibility of the target struct, attempting to generate structs with different visibility than the original will result in E0446.

[mxgrey]

there is a select_buffer function that works for any buffer types.

Can you show how your approach would work for the TestJoinedValueJson case?

1. In some cases, it will lead to invalid code being generated.

I think this should be easy to fix by adding a field along the lines of _ignore: ::std::marker::PhantomData<fn( #impl_generics )> into the _Buffers struct. It might be a little tricky to strip the trait bounds out of #impl_generics, but it should be doable by removing all substrings that start with : and end with either , or >.

2. AnyBuffer does not impl as_any_buffer, so this code is invalid.

There's no reason we can't implement as_any_buffer for AnyBuffer. It would just return self.

3. Different buffers has different pull schematics ... This fails as generic ends up as a AnyMessageBox.

This is exactly why I believe we need to allow the user to specify the buffer type for special cases. The original use case you showed of

#[bevy_impulse(buffer_type = "AnyBuffer")]
generic: T,

would be invalid and should produce a compilation error. The use case we actually want it for would be this:

#[bevy_impulse(buffer_type = "AnyBuffer")]
any_message: AnyMessageBox,

In this example, the any_message field is prepared to receive a message of any kind from any buffer. If we aren't able to specialize the buffer type, then it would only be able to receive messages from a Buffer<AnyMessageBox> type of buffer.

5. Special buffer types will need to be converted into Buffer anyway when we pull from it.

No, this is not the case. AnyBuffer can be pulled from to produce a AnyMessageBox and JsonBuffer can be pulled from to produce a JsonMessage. Both have implemented the Joined trait so you can use Joined::pull on them just like any concrete buffer type.

[mxgrey]

I'd rather not have builder as an argument. I think we should leave it to users to call output.into_buffer(builder) from outside of select_buffers and have select_buffers just take in the specific buffer types that are expected. This will especially matter for supporting specialized buffer types like AnyBuffer and JsonBuffer.

[koonpeng]

No, this is not the case. AnyBuffer can be pulled from to produce a AnyMessageBox and JsonBuffer can be pulled from to produce a JsonMessage. Both have implemented the Joined trait so you can use Joined::pull on them just like any concrete buffer type.

I thought the idea is to auto downcast AnyBuffer, but if that is not the case, then it would actually solve many of the invalid code generation. Though I think it would also be confusing the difference between AnyBuffer and Buffer<AnyMessageBox>.

I think this should be easy to fix by adding a field along the lines of _ignore: ::std::marker::PhantomData<fn( #impl_generics )> into the _Buffers struct. It might be a little tricky to strip the trait bounds out of #impl_generics, but it should be doable by removing all substrings that start with : and end with either , or >.

I didn't think of this, yeah it seems to be a good catch-all solution.

[mxgrey]

I thought the idea is to auto downcast AnyBuffer,

It's actually the opposite. The idea is if a user wants to obtain an AnyMessageBox in their JoinedValue struct, they should be able to receive that AnyMessageBox from any kind of buffer by upcasting the buffer into an AnyBuffer.

Like you said, trying to pull a concrete message from an AnyBuffer would require us to know how to convert it into the concrete message type, which can't always be done.

Though I think it would also be confusing the difference between AnyBuffer and Buffer<AnyMessageBox>.

I expect this won't be a very common use case, and only advanced users would even be interested in obtaining an AnyMessageBox.

I do think the JsonMessage / JsonBuffer use case will be more common, so we'll still need to deal with the problem of distinguishing a Buffer<JsonMessage> from a JsonBuffer. I'll think about how to give clear examples to illustrate the difference in the JsonBuffer documentation.

[mxgrey]

You may want to move this test into the json_buffer.rs module since JsonBuffer requires the diagram feature, but buffer_map.rs doesn't.

[koonpeng]

There is another problem with different buffer types.

The 2 ways IncompatibleLayout can check the buffer is either with require_message_type or require_buffer_type. The former doesn't work because it always return a Buffer<T>, the latter also doesn't work because it cannot convert other buffers to Buffer<T>, and since all buffers in buffer map is AnyBuffer, it only works if all buffers in the buffer struct is one of the "special" buffer types.

Another problem is that AnyMessageBox cannot impl Clone. I dropped it and the tests still works but I don't know how much of a blocker is it for actual usage.

[mxgrey]

I think it would make sense to pass a real value through this AnyBuffer and test that it comes out correctly on the other side instead of leaving it unused.

[mxgrey]

The 2 ways IncompatibleLayout can check the buffer is either with require_message_type or require_buffer_type. The former doesn't work because it always return a Buffer, the latter also doesn't work because it cannot convert other buffers to Buffer, and since all buffers in buffer map is AnyBuffer, it only works if all buffers in the buffer struct is one of the "special" buffer types.

I don't follow what the issue is here. There's a reference implementation of this for a struct that contains a JsonBuffer. The key is to use require_message_type::<T> in the normal case and then use require_buffer_type::<SpecialBufferType> when the user is overriding the buffer type.

I understand that procedural macros are easiest to write when everything follows a very consistent pattern, but I believe it should still be possible to generate a proc_macro2::TokenStream that has the correct line for each buffer and then insert that token stream into the function body.

[mxgrey]

Another problem is that AnyMessageBox cannot impl Clone. I dropped it and the tests still works but I don't know how much of a blocker is it for actual usage.

This is to be expected. We can't assume that all message types implement clone, and the uncloneability of some message types is accounted for throughout this library. I think we can save it for a future PR to support a CloneableMessageBox type that implements Any + Clone.

[koonpeng]

I'm not very in depth on the impl of the buffers but I think it should be possible to register a downcast Fn when a Buffer is converted to AnyBuffer? If so then we could allow require_buffer_type to be able to downcast back into the original Buffer.

37ee9b9 not sure if this is the right place to add it. But this does fix require_buffer_type not able to downcast to Buffer<T>.

[mxgrey]

We actually can't without using hacks, procedural macros work on tokens and does not have type information. AnyBuffer may not be ::bevy_impulse::AnyBuffer

Did you try doing it? If so can you show me what you tried and what went wrong with it?

I believe it will be able to work just like how serde(skip_serializing_if = "is_default") is able to work in the site editor without serde knowing the full path to the custom is_default function that we've defined.

The impl will exist within the same module as the struct that the derive is associated with, so whatever tokens exist in that scope should be usable by the macro. If a user does:

use bevy_impulse::JsonBuffer;

#[derive(Clone, JoinedValue)]
struct TestJoinedValueJson {
    integer: i64,
    float: f64,
    #[bevy_impulse(buffer = "JsonBuffer")]
    json: JsonMessage,
}

then the derive macro should be able to expand that into

#[derive(Clone)]
struct TestJoinedValueJsonBuffers {
    integer: Buffer<i64>,
    float: Buffer<f64>,
    json: JsonBuffer,
}

impl ::bevy_impulse::BufferMapLayout for TestJoinedValueJsonBuffers {
    fn try_from_buffer_map(buffers: &::bevy_impulse::BufferMap) -> Result<Self, ::bevy_impulse::IncompatibleLayout> {
        let mut compatibility = ::bevy_impulse::IncompatibleLayout::default();
        let integer = compatibility.require_buffer_type::<::bevy_impulse::Buffer<i64>>("integer", buffers);
        let float = compatibility.require_buffer_type::<::bevy_impulse::Buffer<f64>>("float", buffers);
        let json = compatibility.require_buffer_type::<JsonBuffer>("json", buffers);

        let Ok(integer) = integer else {
            return Err(compatibility);
        };
        let Ok(float) = float else {
            return Err(compatibility);
        };
        let Ok(json) = json else {
            return Err(compatibility);
        };

        Ok(Self {
            integer,
            float,
            json,
        })
    }
}

In fact the same logic works for all the types of the fields within the struct. The macro doesn't know the full path for any of those types but can still use them in the implementation.

[mxgrey]

37ee9b9 not sure if this is the right place to add it. But this does fix require_buffer_type not able to downcast to Buffer.

I hadn't thought of doing that, but it makes enough sense in order to fully generalize the .downcast_buffer<B> method. You picked the right place to put it.

[mxgrey]

downstream users may also implement their own special buffers, all of these would break the proc macro.

I don't agree with this statement and in fact we should support downstream specialized buffers. Everything else in the way that I've implemented AnyBuffer accommodates it, and the macro should be able to as well.

[mxgrey]

Something else that occurred to me, if we rule out the case of

#[bevy_impulse(buffer = "AnyBuffer")]
field: T,

then we no longer need to worry about introducing an

_ignore: ::std::marker::PhantomData<fn( #generics )>,

field to the _Buffers struct since specialized buffer types have to map to a concrete output value, making it impossible to have a specialized buffer that outputs a generic without that generic being part of the specialized buffer signature.

Simply put, I think we can totally forget about the need for PhantomData, at least until someone can come up with a concrete and legitimate example where it would be needed.

[koonpeng]

downstream users may also implement their own special buffers, all of these would break the proc macro.

I don't agree with this statement and in fact we should support downstream specialized buffers. Everything else in the way that I've implemented AnyBuffer accommodates it, and the macro should be able to as well.

What I mean is that if we do special case for AnyBuffer or JsonBuffer, then it won't work for custom special buffers that downstream creates. In order support to any special buffers, the generated code must be consistent and must not depend on assumptions and edge cases. Allowing downcast to Buffer<T> does allow us to have the generated code work in all cases (as long as the special buffer register the correct downcast handlers).

[koonpeng]

Something else that occurred to me, if we rule out the case of

#[bevy_impulse(buffer = "AnyBuffer")]
field: T,

then we no longer need to worry about introducing an

_ignore: ::std::marker::PhantomData<fn( #generics )>,

field to the _Buffers struct since specialized buffer types have to map to a concrete output value, making it impossible to have a specialized buffer that outputs a generic without that generic being part of the specialized buffer signature.

Simply put, I think we can totally forget about the need for PhantomData, at least until someone can come up with a concrete and legitimate example where it would be needed.

Yeah, in fact the latest code already does exactly that, though I left the code there because this logic of converting from generic tokens to a TypePath of PhantomData could be useful in the future?

bevy_impulse/macros/src/buffer.rs

Lines 63 to 77 in a00f09f

	/// Converts a list of generics to a [`PhantomData`] TypePath.
	/// e.g. `::std::marker::PhantomData<(T,)>`
	// Currently unused but could be used in the future
	fn _to_phantom_data(generics: &Generics) -> TypePath {
	let lifetimes: Vec<Type> = generics
	.lifetimes()
	.map(|lt| {
	let lt = &lt.lifetime;
	let ty: Type = parse_quote! { & #lt () };
	ty
	})
	.collect();
	let ty_params: Vec<&Ident> = generics.type_params().map(|ty| &ty.ident).collect();
	parse_quote! { ::std::marker::PhantomData<(#(#lifetimes,)* #(#ty_params,)*)> }
	}

[mxgrey]

I left the code there because this logic of converting from generic tokens to a TypePath of PhantomData could be useful in the future?

Some would say it's bad practice to leave dead code hanging around, but I don't mind letting it linger for a while just in case. I would just recommend making it ::std::marker::PhantomData<fn( #(#lifetimes,)* #(#ty_params,)*)> instead because the fn(...) allows the field to automatically implement Copy, Send, and Sync which for some reason doesn't work if it's a regular tuple.

[mxgrey]

What I mean is that if we do special case for AnyBuffer or JsonBuffer, then it won't work for custom special buffers that downstream creates.

I don't agree with this statement and I'll need you to concretely demonstrate a problem with the approach before I can believe it.

You should be able to take the tokens JsonBuffer directly out of the string inside of #[bevy_impulse(buffer = "JsonBuffer")] and make it the value of #buffer_type inside the token stream of compatibility.require_buffer_type::< #buffer_type >(#map_key, buffers).

Please give that a try and then show me how it's unable to work.

Users will be able to use this exactly as-is with downstream / third-party specialized buffers types.

[mxgrey]

Allowing downcast to Buffer does allow us to have the generated code work in all cases (as long as the special buffer register the correct downcast handlers).

What you're describing here doesn't fit the use case of the buffer specialization, as far as I can figure. Can you demonstrate how your approach would work for this test case?

[mxgrey]

What I mean is that if we do special case for AnyBuffer or JsonBuffer, then it won't work for custom special buffers that downstream creates.

I don't agree with this statement and I'll need you to concretely demonstrate a problem with the approach before I can believe it.

Actually I just did a review of the current code and it looks like you're already doing exactly what I've been asking for, so I'm now even more confused about why you've been saying it can't work.

[mxgrey]

After looking back over our conversation, I think I may have misunderstood what you think of as a "special case".

In my mind the "special case" is that we have to apply the #[...(buffer = "AnyBuffer")] attribute to the field that needs a specialized buffer type, so when you said that we can't have special cases I thought you were saying that we can't use attributes for specialized buffers.

I guess what you were actually saying is that we shouldn't have special treatment for internal specialized buffer types, and that's something I fully agree with. I never wanted us to treat AnyBuffer or JsonBuffer differently than any other special/custom buffer types.

I'll do a full review of the PR now that it looks like everything is being supported the way that I had been hoping for.

[koonpeng]

What I mean is that if we do special case for AnyBuffer or JsonBuffer, then it won't work for custom special buffers that downstream creates.

I don't agree with this statement and I'll need you to concretely demonstrate a problem with the approach before I can believe it.

You should be able to take the tokens JsonBuffer directly out of the string inside of #[bevy_impulse(buffer = "JsonBuffer")] and make it the value of #buffer_type inside the token stream of compatibility.require_buffer_type::< #buffer_type >(#map_key, buffers).

Please give that a try and then show me how it's unable to work.

Users will be able to use this exactly as-is with downstream / third-party specialized buffers types.

type FooBuffer = Buffer<Foo>;

#[derive(JoinedValue)]
struct TestWithCustomBuffer {
  normal_data: String,

  #[buffers(buffer_type = JsonBuffer)]
  json_data: JsonMessage,

  #[buffers(buffer_type = FooBuffer)]
  foo_data: Foo,
}

// somewhere in generated code
fn try_from_buffer_map(buffers: &::bevy_impulse::BufferMap) -> Result<Self, ::bevy_impulse::IncompatibleLayout> {
  // there is no customization of the buffer type, so the buffer type is `Buffer<String>`, `IncompatibleLayout::require_buffer_type`
  // cannot downcast to `Buffer<T>`, so we need to use `IncompatibleLayout::require_message_type`.
  let normal_data = if let Ok(buffer) = compatibility.require_message_type::<String>("normal_data", buffers) {
      buffer
  } else {
      return Err(compatibility);
  };

  // A custom buffer type is given, it is `JsonBuffer` so we assume that we can use `IncompatibleLayout::require_buffer_type`.
  let json_data = if let Ok(buffer) = compatibility.require_buffer_type::<JsonBuffer>("json_data", buffers) {
      buffer
  } else {
      return Err(compatibility);
  };

  // A custom buffer type is given, it is `FooBuffer`, we don't know what `FooBuffer`, but 
  // `IncompatibleLayout::require_buffer_type` can convert between "any" special buffers, so we should be able to use it.
  // However, this fails because `FooBuffer` is actually a `Buffer<Foo>` and we need to use `IncompatibleLayout::require_message_type`.
  let foo_data = if let Ok(buffer) = compatibility.require_buffer_type::<FooBuffer>("foo_data", buffers) {
      buffer
  } else {
      return Err(compatibility);
  };
}

The main problem is that require_buffer_type works for any type of buffers except Buffer<T>. If we generate different code based on the buffer type in the proc macro, then we would get situations where we use the wrong function to convert the buffers.

In order to support any buffers, I think we need to avoid generating code based on the buffer type, we must consider any buffer_type options we receive as opaque and it may even be a Buffer<T>. A solution so that we do not have to make assumptions is like serde where there is another option to give it a function that converts AnyBuffer to whatever buffer that is specified in buffer_type (and we must not consider AnyBuffer or JsonBuffer as special in anyway). But if require_buffer_type can downcast to Buffer<T>, then we wouldn't have this problem in the first place and it would work with any buffer that downstream defines. An option to customize the conversion function is still useful but imo not as critical now as downstream can ensure they register the correct handlers so their buffers work.

[koonpeng]

After looking back over our conversation, I think I may have misunderstood what you think of as a "special case".

In my mind the "special case" is that we have to apply the #[...(buffer = "AnyBuffer")] attribute to the field that needs a specialized buffer type, so when you said that we can't have special cases I thought you were saying that we can't use attributes for specialized buffers.

I guess what you were actually saying is that we shouldn't have special treatment for internal specialized buffer types, and that's something I fully agree with. I never wanted us to treat AnyBuffer or JsonBuffer differently than any other special/custom buffer types.

I'll do a full review of the PR now that it looks like everything is being supported the way that I had been hoping for.

Yeah, maybe I should have made it more clear. I think that if we treat AnyBuffer or JsonBuffer (or any other buffers) differently, then we would get a situation where the generated code is wrong due to type alias, downstream custom buffers etc.

[mxgrey]

I'm glad this has come out so nicely. It works exactly the way I was hoping it would. The __bevy_impulse_{}_Buffers technique for avoiding symbol collisions and/or generally hiding the struct is interesting. I suppose it does make sense to force users to explicitly name the struct if they want to be able to use the struct directly.

My only points of feedback are on some of the attribute names. I realize that attribute naming doesn't have any rigorous objective standards so it's going to be a very subjective topic, but I'd suggest a few tweaks to the current naming:

1. Instead of #[buffers(...)] I'd suggest we do #[join(...)] or #[joined(...)]. The struct that we're tagging is a joined value, not a set of buffers, so I think it's better to emphasize that the attributes are related to joining.

2. I'm worried that buffer_struct_name = "..." will make it sound like we're naming the struct of a buffer when we're actually naming a struct that will contain a collection of named buffers. I'd suggest changing that attribute name to either buffers_struct_name = "..." or buffer_map_name = "...".

3. Instead of buffer_type = "..." I think it would be enough to just have buffer = "...". Combined with (1), the user would write e.g. #[join(buffer = "JsonBuffer")]. That being said, I don't feel strongly about this one. If you think it should be #[join(buffer_type = "JsonBuffer")] I'd be fine with that.

[koonpeng]

I'm glad this has come out so nicely. It works exactly the way I was hoping it would. The __bevy_impulse_{}_Buffers technique for avoiding symbol collisions and/or generally hiding the struct is interesting. I suppose it does make sense to force users to explicitly name the struct if they want to be able to use the struct directly.

My only points of feedback are on some of the attribute names. I realize that attribute naming doesn't have any rigorous objective standards so it's going to be a very subjective topic, but I'd suggest a few tweaks to the current naming:

1. Instead of #[buffers(...)] I'd suggest we do #[join(...)] or #[joined(...)]. The struct that we're tagging is a joined value, not a set of buffers, so I think it's better to emphasize that the attributes are related to joining.
2. I'm worried that buffer_struct_name = "..." will make it sound like we're naming the struct of a buffer when we're actually naming a struct that will contain a collection of named buffers. I'd suggest changing that attribute name to either buffers_struct_name = "..." or buffer_map_name = "...".
3. Instead of buffer_type = "..." I think it would be enough to just have buffer = "...". Combined with (1), the user would write e.g. #[join(buffer = "JsonBuffer")]. That being said, I don't feel strongly about this one. If you think it should be #[join(buffer_type = "JsonBuffer")] I'd be fine with that.

I think most of these makes sense, only thing is that I think we shouldn't use "strings" in the attributes, e.g. #[join(buffer = "JsonBuffer")] instead of #[join(buffer = JsonBuffer)]. Using the ident directly allows for better compiler checks.

[mxgrey]

we shouldn't use "strings" in the attributes, e.g. #[join(buffer = "JsonBuffer")] instead of #[join(buffer = JsonBuffer)].

I agree that what you're suggesting looks nicer and probably allows code completion to work better, but I think the attribute = "value" convention exists to help deal with syntax that could create ambiguity for the attribute parser. For example, what if someone has a custom buffer type that takes two generic arguments, e.g.:

#[joined(buffer = CustomBuffer<A, B>)]

When the parser splits the attributes based on the comma, I think it'll get buffer = CustomBuffer<A and then B> as two separate attributes. Whereas if we use quotes then

#[joined(buffer = "CustomBuffer<A, B>")]

will definitely get parsed correctly.

[mxgrey]

what if someone has a custom buffer type that takes two generic arguments

I've tried this out now, and there's actually no parsing issue at all. It seems syn is smart enough to know the comma belongs to the < .. > section. Tbh I no longer understand why serde uses quotes for its attribute values, but it seems that it's not necessary.

I'll push a change that adds a unit test for a buffer with multiple generic arguments.

[mxgrey]

In b110e87 I added a test to make sure that everything works for buffer types with multiple generic arguments, and in fact it does.

At the same time I fixed how Clone is implemented for the generated buffer structs because the existing implementation would not have worked if any generic argument did not have the Clone trait, even though buffers can be cloned regardless of any generic arguments.

Also all of our internal buffer types implement Copy so I think we should implement Copy for the generated buffer structs by default. I added a noncopy_buffer attribute e.g. #[joined(buffer = CustomBufferType, noncopy_buffer)], which will disable generating the Copy trait whenever it's applied to at least one field in the struct.

[koonpeng]

Thanks for the improvements!