rustler icon indicating copy to clipboard operation
rustler copied to clipboard

Remove lifetime from Encoders

Open neosimsim opened this issue 3 years ago • 4 comments

Closes #470

neosimsim avatar Jul 04 '22 13:07 neosimsim

Something is still not working properly. I'll have to double check.

neosimsim avatar Jul 04 '22 13:07 neosimsim

What effect is the lifetime in Encoder supposed to have? It the lifetime there in case the type has a lifetime defined? If so, how would it working?

neosimsim avatar Jul 04 '22 15:07 neosimsim

Thanks for the PR. I think we need to fix lifetimes for good, see https://github.com/rusterlium/rustler/issues/428 as well.

What effect is the lifetime in Encoder supposed to have?

The lifetime should rename a pre-existing lifetime for a struct, if a struct has a lifetime. When rustler_codegen was introduced with 594afed3, handling a struct with an associated lifetime was handled with this:

pub fn gen_encoder(struct_name: &Ident, fields: &[Field], atom_defs: &Tokens, has_lifetime: bool) -> Tokens {
    let struct_type = if has_lifetime {
        quote! { #struct_name <'b> }
    } else {
        quote! { #struct_name }
    };
// ...
    quote! {
        impl<'b> rustler::NifEncoder for #struct_type {
// ...
}

So back then, the lifetime was renamed to 'b. We refactored that part in 6be843b6:

    let struct_type = &ctx.ident_with_lifetime;

The ident_with_lifetime is defined in context.rs:

        let ident_with_lifetime = if has_lifetime {
            quote! { #ident <'a> }
        } else {
            quote! { #ident }
        };

Now, that code is wrong: the lifetime needs to be named 'a for the Decoder, but 'b for the Encoder. This is what caused https://github.com/rusterlium/rustler/issues/428.

evnu avatar Jul 05 '22 13:07 evnu

@evnu It sounds to me the codegen only can handle exactly one or zero lifetime for structs, right?

https://github.com/rusterlium/rustler/blob/f946092552f8057dabdb13c66fb9364ce03120a3/rustler_codegen/src/context.rs#L41-L45

But it may be good if Rustler supports more flexible lifetimes for them. I think The approach of #430 may solve this problem by using ugly names for lifetimes for Rustler, but I cannot be sure is it active...

SeokminHong avatar Jul 06 '22 01:07 SeokminHong