Derive on structs with generics

[kitlith]

Still a work in progress, this mostly consists of threading generics through to the right place in each relevant macro (and i have several more macros to go), and generating the relevant PhantomData field to allow the generics to stick around.

I'm a little nervous about my potential fix for the bitsize check -- all I know so far is that it still allows it to compile, but I haven't checked if it still functions as intended yet.

[kitlith]

huh, apparently I caused rust to say "you might be interested in the trivial_bounds feature" and that broke the UI test. Not sure if that means I should update the UI test or try and filter out where clauses where they probably aren't necessary.

[kitlith]

Ah. I should probably switch to just requiring that the generic parameters implement the traits for the same reasons as in rust-lang/rust#26925 and dtolnay/syn#370 , even though those constraints can actually be incorrect (as those issues note)

[pickx]

thank you for your work, it is appreciated. for now, just a quick thing I've noticed, and I'll take a more thorough look later.

[pickx]

so overall this looks quite good to me. thanks again.

one unfortunate side effect here is that we're back to having an implementation detail const in the impl block of the generated struct, and now there's an additional PhantomData field as well.
fortunately these two don't need to be pub. but even non-pub things are still in scope for the same module, since this struct is generated in the user's crate.

[pickx]

is it correct that adding a PhantomData here is only needed when generics are involved?

if so, do you think it's possible (and practical) to not generate a _phantom field unless it's actually needed, just by looking at item.generics?

[kitlith]

It should be possible, yes, but it would create additional conditions at the construction sites (there are 4 in this PR) and potentially any external derive macros. We could refactor those to use a common constructor method, but I figured I'd go for the approach that requires the least conditional code.

I'm fine either way, though.

[kitlith]

RE: the implementation detail const: we can generate an un-namable trait that has an associated const, and implement it.

struct Example;

// macro generated
const _: () = {
    trait Assertion {
        const TEST: ();
    }

    impl Assertion for Example {
        const TEST: () = assert!(true); // try changing to false
    }
};

fn main() {
    // A::TEST // compile error
}

should probably make sure that this still works with generic impls, though.

[pickx]

so your example here made me realize that the code as it currently is, doesn't actually panic when the bitsizes differ.
the const isn't evaluated until it's explicitly accessed:

use bilge::prelude::*;

#[bitsize(2)]
struct Foo(u1);

// won't panic at compile time
// fn main() { }

// panics
fn main() {
    let _ = Foo::_BITSIZE_CHECK;
}

RE: the implementation detail const: we can generate an un-namable trait that has an associated const, and implement it.

you're absolutely right. I think this is a good idea:

quote! {
    #item

    const _: () = {
        // doesn't seem like the trait name needs to be mangled here:
        // both the impl block and the assert, appear to resolve `Assertion` to the trait defined here
        trait Assertion {
            const SIZE_CHECK: bool;
        }

        impl #impl_generics Assertion for #ident #ty_generics #where_clause {
            const SIZE_CHECK: bool = (#computed_bitsize) == (#declared_bitsize);
        }

        assert!(<something as Assertion>::SIZE_CHECK, "insert message here")
    };
}

...except, what do we put instead of something in the case where the struct has generics? how do we name this?
and again, even if the const assert is moved to the trait itself like in your example (which solves the naming problem), it won't automatically evaluate. we would then need to be able to name the const assert itself, from outside the trait impl block.

[kitlith]

I did some experimentation with that -- indeed, we can't get evaluation without an access, but we can embed an access into relevant public methods.

This would mean that we'd need to also generate the impl blocks that we want to prevent the user from calling inside the anonymous block, instead of outside. The modular nature of bilge will mean that we can't necessarily block all methods, unless the other implementations go through methods that we have blocked using this method.

I.e., using the formulation of the trait I specified it'd be

impl<T> Example<T> {
    pub fn something_public() -> Self {
        Self::TEST;
        todo!();
    }
}

which will fail to compile in the case where: the assert would fail, and the user makes a call to something_public.

(apologies for the close/reopen, on my phone and had an accidental tap while writing this message)

[kitlith]

hm. Am I reading this correctly in that bitsize.rs only handles the assert, while bitsize_internal.rs handles everything else? That messes with the above idea a little bit.

[kitlith]

Alright, I have a possible solution to the compile time error thing, and that's to make the consts on Bitsized rely on the assertion. This means that using any method that uses either const on the Bitsized trait will produce a compiler error if the sizes mismatch.

This required moving the assertion generation into bitsize_internal.

[kitlith]

hm, might still need to add some usages of the consts from Bitsized into some of the other trait methods -- but at least we're not exposing an extra name for that i guess.

[pickx]

that's clever! congratulations on finding this workaround!

[pickx]

nice work.
I'd like to wait for @hecatia-elegua to take a look at this and give his opinion before we proceed. 👍

[hecatia-elegua]

Great work!

If you don't mind, could you add an example with your real usecase?
I would also have to experiment more on my own, when I get the time.

[nwtnni]

Hi all, thanks for your work on this crate :)

I've been looking for a bitfield implementation that would support the following use-case:

// examples/generic_wrapper.rs

use bilge::prelude::*;

/// Prefix `T` with an [ABA version counter][aba] for use in a lock-free data structure.
///
/// [aba]: https://en.wikipedia.org/wiki/ABA_problem
#[bitsize(64)]
#[derive(DebugBits, FromBits)]
struct Versioned<T> {
    version: u16,
    inner: T,
}

#[bitsize(48)]
#[derive(DebugBits, FromBits)]
struct Data {
    a: u32,
    b: u16,
}

fn main() {
    let mut versioned = Versioned::<Data>::from(0x0123_4567_89ab_cdefu64);
    println!("{versioned:?}");
    println!("{:#x?}", versioned.version());
    println!("{:#x?}", versioned.inner());
    println!("{:#x?}", versioned.value);

    versioned.set_version(27u16);
    println!("{:#x?}", versioned.version());
    println!("{:#x?}", versioned.inner());
    println!("{versioned:?}");

    versioned.set_inner(Data::from(u48::from(0xfedc_ba98_7654u64)));
    println!("{:#x?}", versioned.inner().a());
    println!("{:#x?}", versioned.inner().b());
    println!("{:#x?}", versioned.inner().value);
    println!("{versioned:?}");
}

If I understand this PR correctly, it seems like the generic types aren't actually used during (de-)serialization? I'm wondering if it would make sense to instead:

• Assume generics will be (de-)serialized by default.
• Introduce the equivalent of serde's skip, skip_serializing, and skip_deserializing attributes to control this behavior.

I can try to implement a proof of concept when I get the chance.

[kitlith]

@nwtnni This PR should be able to enable use-cases like that, yes, with the use of additional annotations. i.e.:

#[bitsize(64)]
#[derive(DebugBits, FromBits)]
struct Versioned<T: Bitsized<ArbitraryInt = u48> + Filled + From<u48>>
where
    u48: From<T>,
{
    version: u16,
    inner: T,
}

This, along with the substitution of u48::new in place of u48::from gets your example to run.

[hecatia-elegua]

@kitlith with substitution, do you mean in the macro code?

Honestly I'm sorry, I don't have any time to spend on this right now / it's too complicated to wrap my head around when I'm not working with bitfields anymore at this time. I can do some simple checks and publish this PR though, since we are not at a 1.0 version yet. I don't remember which parts I wanted to check more.

TLDR; I can't keep my own standards right now, but that's not too bad in this case.

[kitlith]

I don't have any time to spend on this right now / it's too complicated to wrap my head around when I'm not working with bitfields anymore at this time.

Extremely understandable. I'm in a similar boat, in that I'm not currently actively working with bitfields, and have not had the energy to spend time on things like this outside of work for awhile. Just look at how long it took for me to respond. >_>

with substitution, do you mean in the macro code?

Was referring to the example code they posted, because I didn't want to copy-paste the whole thing. They wrote versioned.set_inner(Data::from(u48::from(0xfedc_ba98_7654u64)));, but u48 does not implement From<u64> because a u48 is smaller. Instead, they want u48::new or u48::try_new, because those handle construction from the backing integer. Minor detail, probably due to writing it as a quick "this is what i think it would look like" rather than "sitting down with this fork and the compiler to hash out what details are missing".

I can do some simple checks and publish this PR though

I'm somewhat hesitant to advocate for that, because one of your stated issues is that it's too complicated for the amount of time you can dedicate to this right now, and I don't want this to cause issues for you down the line.

If I have the time/energy, I might try to look for anything "magic" and document it better/wrap it up in a better abstraction. The cleverness that pickx commented on, for instance. Can't guarantee anything though, as right now I'm mainly just trying to take time for myself.