Improve ergonomics of defining GUIDs about uefi-rs HOT 5 CLOSED

Sweet ! Alex Crichton just wrote the blog post I needed to get started with this more easily : https://blog.rust-lang.org/2018/12/21/Procedural-Macros-in-Rust-2018.html .

from uefi-rs.

As a Christmas gift, I now have a decent prototype of this, which I will submit as a WIP MR after cleaning it up a bit. At this point, I think I got a good enough grasp of the procedural macro design space for us to discuss whether we want to do this, and if so how we want to do it.

As it turns out, simple custom derives like those we initially discussed are not terribly difficult to write, because the Syn crate provides excellent support for this use case. Using it, I can fit in a reasonable 100-ish LOC budget an Identify and Protocol derive which handle lifetime arguments and report errors much more cleanly than our previous macro_rules-based solution.

Compile times do get significantly worse as a result of using syn/quote/proc_macro2, but only when the new uefi-derive crate is compiled, not when it is used. Since people should switch nightlies at most once a day and should not need to cargo clean often, I think that is an acceptable cost.

Given the reduced hackiness (less repetitive, more general), the greatly improved error reporting, the simplicity of the implementation, and the overall low impact on everyday compile times, I would now argue that procedural macros are superior to macro_rules for this use case.

The current proposal takes a GUID in its canonical representation, like this:

#[derive(Identify, Protocol)]
#[unsafe_guid(0x1234_5678, 0x9abc, 0xdef0, 0xfedc, 0xba98_7654_3210)]
struct Dummy {}

As an alternative syntax, if we wanted to get even closer to RFC 4122, it wouldn't be much more difficult to take the GUID in its standard textual form:

#[derive(Identify, Protocol)]
#[unsafe_guid = "12345678-9abc-def0-fedc-ba9876543210"]
struct Dummy {}

To ease this port, the underlying Guid::from_values constructor has also been modified to take the GUID in canonical representation, which I think makes more sense than UEFI's bizarre binary representation from a standard compliance point of view.

However, there are other things that we cannot do, such as taking the node identifier as an array like Guid::from_values does:

#[derive(Identify, Protocol)]
// BAD: Passing an array literal as an attribute argument violates the Rust grammar
#[unsafe_guid(0x1234_5678, 0x9abc, 0xdef0, 0xfedc, [0xba, 0x98, 0x76, 0x54, 0x32, 0x10])]
struct Dummy {}

Conversely, we cannot modify Guid::from_values to take its last argument as an u64 and assert that it is 48-bit after the fact, like the custom derive does, because that would require some language features which are not yet supported in a const evaluation context:

impl Guid {
    /// Creates a new GUID from its canonical representation
    pub const fn from_values(time_low: u32,
                             time_mid: u16,
                             time_high_and_version: u16,
                             clock_seq_and_variant: u16,
                             node: u64) -> Self {
        // BAD: Assertions in const fns would require const conditionals and panics
        assert!(node.leading_zeros() >= 16, "Node ID must be a 48-bit integer");

        // ...
    }
}

Since we cannot have a homogeneous syntax for Guid::from_values and #[derive(Identify)], an obvious temptation would be to use #[derive(Identify)] all the time. However, we would then face the fact that custom derive does not support typedefs, which we use for FileProtocolInfo:

// BAD: Cannot use a custom derive on a typedef
#[derive(Identify)]
#[unsafe_guid(0x1234_5678, 0x9abc, 0xdef0, 0xfedc, 0xba98_7654_3210)]
pub type FileInfo = NamedFileProtocolInfo<FileInfoHeader>;

We could work around this by making unsafe_guid a full-blown attribute macro, which would also make the code more compact, but the implementation would also be more complex as we would need to leave behind Syn's handy conveniences for custom derive writers:

#[unsafe_guid(0x1234_5678, 0x9abc, 0xdef0, 0xfedc, 0xba98_7654_3210)]
pub type FileInfo = NamedFileProtocolInfo<FileInfoHeader>;

Another drawback of a full-blown attribute macro is that we would need to enable more of syn's feature flags, which will likely hurt cold compile times a bit more. I'm not sure what the impact on warm compile times will be, it should be tested.

So, time for design questions!

1. Overall, do you think that this procedural macro path is worth exploring further?
2. If so, do you prefer the #[unsafe_guid(0x1234_5678, 0x9abc, 0xdef0, 0xfedc, 0xba98_7654_3210)] syntax or the #[unsafe_guid = "12345678-9abc-def0-fedc-ba9876543210"] one?
3. Should we have a #[derive(Identify)] attribute with an unsafe_guid helper (simplest implementation, but does not support all use cases) or a full-blown #[unsafe_guid] attribute macro (allows consistent Identify derivation for all types, but more complex & costly at compile time)?

Meanwhile, I'll be submitting a WIP code prototype shortly.

from uefi-rs.

This approach is looking great! Unfortunately there's not much to be done about compile times so I wouldn't worry about if for now.

I think the unsafe_guid = "..." form works better, since the extra , and 0x weren't improving readability.

As for how to handle the custom derive, the #[unsafe_guid] as a stand-alone attribute macro sounds like the better idea. We want to encourage people to use this macro instead of manually hard-coding a GUID with an impl Identify.

I don't see why using a full-blown attribute macro would affect compile times much, since most of the parsing code is already there. We'd basically change:

quote! {
    unsafe impl #impl_generics crate::Identify for #ident #ty_generics #where_clause {

to something like:

quote! {
  #[derive(Identify)]
  #structure_declaration

  unsafe impl ...

from uefi-rs.

Thanks for your suggestions! I'll update the PR accordingly.

Regarding compile times of custom derive vs full attribute macros, I expect to see a hit in Syn because the parser must support the full Rust syntax instead of just the custom derive input subset. But I agree that the code generation part should have similar overhead.

I'll try to go down the custom attribute path and do a quick compile time comparison to see how much of a problem this is in practice.

from uefi-rs.

Well, the compile time hit turned out to be small with respect to the crate's cold build time (2s / 37s on my machine), so I'd say it doesn't matter here. PR is ready for review!

from uefi-rs.