Macros ignore if condition and read wrong methods

I have a macros which generates dynamic struct and allow to pass options to this struct. My issue is that my macros try to read wrong methods from generated struct.

So, this is how I use the macro:

packet! {
    @option[world_opcode=100]
    @option[dynamic_fields: ["nickname"]; deps=["nickname_size"]]
    struct Income {
        name: String,
        age: u64,
        nickname_size: u32,
        nickname: String,
    }
    
    impl Income {
        pub fn should_read_nickname(dep_values: Income) -> bool {
            dep_values.nickname_size > 0
        }
    }
}

and inside macro I consider that only Self::should_read_nickname will be called. But actually macro call method for each field in struct, so I got an errors like below:

error[E0599]: no function or associated item named `should_read_name` found for struct `Income` in the current scope
   --> src/main.rs:182:46
    |
103 | /         $vis struct $PacketStruct {
104 | |             $($field_vis $field_name: $field_type),*
105 | |         }
    | |_________- function or associated item `should_read_name` not found for this struct
...
182 |                                       if Self::[<should_read_ $field_name>](dep_values) {
    |                                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |                                                |
    |                                                function or associated item not found in `Income`
    |                                                help: there is an associated function with a similar name: `should_read_nickname`
...
267 | / packet! {
268 | |     @option[world_opcode=100]
269 | |     @option[dynamic_fields: ["nickname"]; deps=["nickname_size"]]
270 | |     struct Income {
...   |
281 | |     }
282 | | }
    | |_- in this macro invocation
    |
    = note: this error originates in the macro `packet` (in Nightly builds, run with -Z macro-backtrace for more info)

This is my macro:

#[macro_export]
macro_rules! packet {
    (
        $(@option[login_opcode=$login_opcode:expr])?
        $(@option[world_opcode=$world_opcode:expr])?
        $(@option[compressed_if:$compressed_if:expr])?
        $(@option[
            dynamic_fields:$($dynamic_fields:expr),*
            $(; deps=$($dependencies:expr),*)?
        ])?

        $(#[$outer:meta])*
        $vis:vis struct $PacketStruct:ident {
            $($field_vis:vis $field_name:ident: $field_type:ty),*$(,)?
        }

        $($PacketStructImpl: item)*
    ) => {
        $(#[$outer])*
        #[derive(Clone, Debug, Default)]
        $vis struct $PacketStruct {
            $($field_vis $field_name: $field_type),*
        }

        $($PacketStructImpl)*

        impl $PacketStruct {
            #[allow(dead_code)]
            pub fn from_binary(buffer: &Vec<u8>) -> Self {
                #![allow(unused_mut)]
                #![allow(unused_variables)]
                #![allow(unused_assignments)]
                let mut omit_bytes: usize = INCOMING_HEADER_LENGTH;
                $(
                    omit_bytes = ($login_opcode as u8).to_le_bytes().len();
                )?

                let mut is_compressed = false;
                $(
                    let mut reader = std::io::Cursor::new(buffer[2..].to_vec());
                    let opcode = byteorder::ReadBytesExt::read_u16::<byteorder::LittleEndian>(
                        &mut reader
                    ).unwrap();

                    if $compressed_if == opcode {
                        // 4 bytes uncompressed + 2 bytes used by zlib
                        omit_bytes += 6;
                        is_compressed = true;
                    }
                )?

                let mut internal_buffer: Vec<u8> = Vec::new();
                if is_compressed {
                    let data = &buffer[omit_bytes..];
                    let mut decoder = flate2::read::DeflateDecoder::new(data);
                    std::io::Read::read_to_end(&mut decoder, &mut internal_buffer).unwrap();
                }

                let buffer = if internal_buffer.is_empty() {
                    buffer[omit_bytes..].to_vec()
                } else {
                    internal_buffer
                };
                let mut reader = std::io::Cursor::new(&buffer);

                let mut dynamic_fields: Vec<&str> = vec![];
                $(
                    $(
                        for index in 0..$dynamic_fields.len() {
                            dynamic_fields.push($dynamic_fields[index]);
                        }
                    )*
                )?

                let mut deps: Vec<&str> = vec![];
                $(
                    $(
                        $(
                            for index in 0..$dependencies.len() {
                                deps.push($dependencies[index]);
                            }
                        )*
                    )?
                )?

                struct DepValues {
                    $($field_name: $field_type),*
                }
                let dep_values = DepValues {
                    $(
                        $field_name: <$field_type>::default()
                    ),*
                };

                let initial = Self {
                    $(
                        $field_name: {
                            if dynamic_fields.contains(&stringify!($field_name)) {
                                paste::paste! {
                                    if Self::[<should_read_ $field_name>](dep_values) {
                                        BinaryConverter::read_from(
                                            &mut reader
                                        ).unwrap()
                                    } else {
                                        <$field_type>::default()
                                    }
                                }
                            } else {
                                let value = BinaryConverter::read_from(
                                    &mut reader
                                ).unwrap();

                                if deps.contains(&stringify!($field_name)) {
                                    dep_values.$field_name = value;
                                }

                                value
                            }
                        }
                    ),*
                };

                // initial is true until first dynamic field

                initial
            }

            #[allow(dead_code)]
            pub fn to_binary(&mut self) -> Vec<u8> {
                #![allow(unused_mut)]
                let mut body = Vec::new();
                $(
                    BinaryConverter::write_into(
                        &mut self.$field_name,
                        &mut body
                    ).unwrap();
                )*
                let header = Self::_build_header(&body);
                [header, body].concat()
            }

            #[allow(unused_variables)]
            fn _build_header(body: &Vec<u8>) -> Vec<u8> {
                #![allow(unused_mut)]
                let mut header: Vec<u8> = Vec::new();
                $(
                    byteorder::WriteBytesExt::write_u8(
                        &mut header,
                        $login_opcode as u8
                    ).unwrap();
                )?
                $(
                    let size = body.len() + OUTCOMING_OPCODE_LENGTH;

                    byteorder::WriteBytesExt::write_u16::<byteorder::BigEndian>(
                        &mut header,
                        size as u16,
                    ).unwrap();
                    byteorder::WriteBytesExt::write_u32::<byteorder::LittleEndian>(
                        &mut header,
                        $world_opcode as u32
                    ).unwrap();
                )?

                header
           }

            $(
                #[allow(dead_code)]
                pub fn unpack(&mut self) -> PacketOutcome {
                    ($login_opcode as u32, self.to_binary())
                }
            )?

            $(
                #[allow(dead_code)]
                pub fn unpack(&mut self) -> PacketOutcome {
                    ($world_opcode as u32, self.to_binary())
                }
            )?
        }
    };
}

and this is the place in macro where I try to call Self::should_read_nickname:

let initial = Self {
	$(
		$field_name: {
			if dynamic_fields.contains(&stringify!($field_name)) {
				paste::paste! {
					if Self::[<should_read_ $field_name>](dep_values) {
						BinaryConverter::read_from(
							&mut reader
						).unwrap()
					} else {
						<$field_type>::default()
					}
				}
			} else {
				let value = BinaryConverter::read_from(
					&mut reader
				).unwrap();

				if deps.contains(&stringify!($field_name)) {
					dep_values.$field_name = value;
				}

				value
			}
		}
	),*
};

For me it's unclear why macro try to read methods for another fields if I use the check:

if dynamic_fields.contains(&stringify!($field_name)) {

This is sandbox to reproduce the issue.

Could somebody explain why this issue happen and how to fix this ?

The issue lies with the fact that you are trying to do something which is unclear what is supposed to achieve.

I'm not 100% sure what have you expected here but what is actually happening is the following:

  1. First macro expands everything and code if dynamic_fields.contains(&stringify!($field_name)) { is generated for every field, uncoditionally
  2. That code is compiled for every field, uncoditionally
  3. Then, when you start the program, then and only then, if is executed and code in that if is executed (or not executed).

I couldn't help you with fixing that code because I couldn't even understand what you are trying to do. It looks to me that you have mixed steps #1, #2, and #3 and thus I'm not even sure that what you are trying to achieve is achievable in principle.

Frankly, I'm almost 100% sure it's a bad idea for you to even use macro_rules!.

I'm not saying it's actually impossible, but I think proc_macro2, syn, etc would be easier to understand and reason about.

At least there you would actually be able to compare two strings in a straightforward fashion.

3 Likes

Or phrased differently -- and assuming you understand what code is being generated here -- not even provably dead code is allowed to call non-existent functions.

Example.

2 Likes

The goal of my macro is next. When response come from server, it should be deserialized into struct. Some of the fields in struct are conditional, or in other words that fields depends on other fields. So, in case of code example if nickname_size is great than 0 nickname field will be read from Cursor(&buffer). This especially useful if dynamic field exists in the middle of field sequence, so I cannot just omit it as I would do using just Cursor (please see another example below, this is example from main branch of my project where I do not use the macro yet. The code below in particular is what I trying to replace):

let mut reader = Cursor::new(input.data.as_ref().unwrap()[4..].to_vec());
let message_type = reader.read_u8()?;
let _language = reader.read_u32::<LittleEndian>()?;

let sender_guid = reader.read_u64::<LittleEndian>()?;
reader.read_u32::<LittleEndian>()?;

let mut channel_name = Vec::new();
if message_type == MessageType::CHANNEL {
    // dynamic field depends on message_type here
    reader.read_until(0, &mut channel_name)?;
}

let _target_guid = reader.read_u64::<LittleEndian>()?;
let size = reader.read_u32::<LittleEndian>()?;
// dynamic field depends on size here
let mut message = vec![0u8; (size - 1) as usize];
reader.read_exact(&mut message)?;

so in case of macro the code above should looks like this:

packet! {
    @option[world_opcode=Opcode::SMSG_MESSAGECHAT]
    @option[
        dynamic_fields: ["message", "channel_name"];
        deps=["message_type", "size"]
    ]
    struct Income {
        message_type: u8,
        language: u32,
        sender_guid: u64,
        channel_name: String,
        target_guid: u64,
        size: u32,
        message: String,
    }

    impl Income {
        pub fn should_read_message(dep_values: Income) -> bool {
            dep_values.size > 0
        }

        pub fn should_read_channel_name(dep_values: Income) -> bool {
            dep_values.message_type == MessageType::CHANNEL
        }
    }
}

I want to add what was my initial idea to implement desired behavior. I wanted to use some kind of attribute for each field I want to be dynamic. So this would look like as below:

@option[world_opcode=Opcode::SMSG_MESSAGECHAT]
struct Income {
	message_type: u8,
	language: u32,
	sender_guid: u64,
	@dynamic[if message_type == MessageType::CHANNEL]
	channel_name: String,
	target_guid: u64,
	size: u32,
	@dynamic[size if size > 0]
	message: String,
}

for this purpose @dynamic attribute will need to have access to fields that were read before. The implementation like above is much appropriate for me and I this is what I want to achieve. But I am not sure for now how to do this so I started from afar to check what the limitation of Rust syntax extending.

Could somebody point me in right direction ?

I took a quick pass at porting your macro into a derive procedural macro here

It's very much not finished, in part because I'm actually not clear on how parts of it were supposed to work.

You have fields which appear to depend on other fields to get their size, but there doesn't appear to be anyway for the actual parsing to receive that size. So I'm not sure how you intended that to work.

You also appear to be using the 0 as a string terminator in impl BinaryConverter for String, but UTF-8 strings can technically contain a zero byte which could potentially cause problems. You also don't have any way to support parsing a sized string instead, even though your example of the macro invocation seems to imply you wanted to be able to do that.

The macro invocation looks like this

use packet_codegen::Packet;

#[derive(Packet)]
#[packet(world_opcode = 100)]
#[packet(login_opcode = 12)]
struct Income {
    name: String,
    age: u64,
    nickname_size: u32,
    #[packet(dynamic = [nickname_size])]
    nickname: String,
}

Though you may of course want to change how I set the attributes up, I was kind of just guessing about how they should be set up and parsed based on usage and naming.

I didn't set up the should_read stuff, because didn't look like it could work the way you had it. Since you can pretty easily track specific dependencies with this setup, you might just be able to pass tuples to the should_read methods instead of creating a function local struct that contains all the deps fields.

2 Likes

thank you very much for your effort ! I will need some time to investigate more about proc macros and derive.

could you tell if there any difference when declare use on most top level and inside a function (I mean if there any performance issues could be) ? Like you did on this line packet-proc-macro/lib.rs at develop · semicoleon/packet-proc-macro · GitHub

No, the only difference is scoping. I didn't want the enum variants to be imported for the whole file, just the place where I was using them.

1 Like

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.