Merge pull request #607 from nicholasbishop/bishop-protocol-macro-3

Add `unsafe_protocol` macro and drop use of the unstable `negative_impls` feature

Author: phip1611

CHANGELOG.md

@@ -3,6 +3,7 @@
 ## uefi - [Unreleased]
 
 ### Added
+
 - Implementations for the trait `EqStrUntilNul` now allow `?Sized` inputs. This means that
   you can write `some_cstr16.eq_str_until_nul("test")` instead of
   `some_cstr16.eq_str_until_nul(&"test")` now.
@@ -14,14 +15,22 @@
 - Added an `core::error::Error` implementation for `Error` to ease
   integration with error-handling crates. (requires the **unstable** feature)
 - Added partial support for the TCG protocols for TPM devices under `uefi::proto::tcg`.
+- Added the `unsafe_protocol` macro to provide a slightly nicer way to
+  implement protocols.
 
 ### Changed
 
 - `UnalignedSlice` now implements `Clone`, and the `Debug` impl now
   prints the elements instead of the internal fields.
+- The unstable `negative_impls` feature is no longer required to use this library.
 
 ### Removed
 
+- The `unsafe_guid` attribute macro and `Protocol` derive macro have
+  been removed. For implementing protocols, use the `unsafe_protocol`
+  macro instead. For any other implementations of the `Identify` trait,
+  implement it directly.
+
 ## uefi-macros - [Unreleased]
 
 ## uefi-services - [Unreleased]

CONTRIBUTING.md

@@ -81,7 +81,7 @@ in the order the UEFI spec requires.
 Each protocol also has a Globally Unique Identifier (in the C API, they're usually
 found in a `EFI_*_PROTOCOL_GUID` define). In Rust, we store the GUID as an associated
 constant, by implementing the unsafe trait `uefi::proto::Identify`. For convenience,
-this is done through the `unsafe_guid` macro.
+this is done through the `unsafe_protocol` macro.
 
 Finally, you should derive the `Protocol` trait. This is a marker trait,
 extending `Identify`, which is used as a generic bound in the functions which retrieve
@@ -92,8 +92,7 @@ An example protocol declaration:
 ```rust
 /// Protocol which does something.
 #[repr(C)]
-#[unsafe_guid("abcdefgh-1234-5678-9012-123456789abc")]
-#[derive(Protocol)]
+#[unsafe_protocol("abcdefgh-1234-5678-9012-123456789abc")]
 pub struct NewProtocol {
   some_entry_point: extern "efiapi" fn(
     this: *const NewProtocol,

uefi-macros/src/lib.rs

@@ -7,36 +7,10 @@ use proc_macro::TokenStream;
 use proc_macro2::{TokenStream as TokenStream2, TokenTree};
 use quote::{quote, ToTokens, TokenStreamExt};
 use syn::{
-    parse::{Parse, ParseStream},
-    parse_macro_input, parse_quote,
-    spanned::Spanned,
-    DeriveInput, Error, FnArg, Generics, Ident, ItemFn, ItemType, LitStr, Pat, Visibility,
+    parse_macro_input, parse_quote, spanned::Spanned, Error, Fields, FnArg, Ident, ItemFn,
+    ItemStruct, LitStr, Pat, Visibility,
 };
 
-/// Parses a type definition, extracts its identifier and generic parameters
-struct TypeDefinition {
-    ident: Ident,
-    generics: Generics,
-}
-
-impl Parse for TypeDefinition {
-    fn parse(input: ParseStream) -> syn::Result<Self> {
-        if let Ok(d) = DeriveInput::parse(input) {
-            Ok(Self {
-                ident: d.ident,
-                generics: d.generics,
-            })
-        } else if let Ok(t) = ItemType::parse(input) {
-            Ok(Self {
-                ident: t.ident,
-                generics: t.generics,
-            })
-        } else {
-            Err(input.error("Input is not an alias, enum, struct or union definition"))
-        }
-    }
-}
-
 macro_rules! err {
     ($span:expr, $message:expr $(,)?) => {
         Error::new($span.span(), $message).to_compile_error()
@@ -46,28 +20,70 @@ macro_rules! err {
     };
 }
 
-/// `unsafe_guid` attribute macro, implements the `Identify` trait for any type
-/// (mostly works like a custom derive, but also supports type aliases)
+/// Attribute macro for marking structs as UEFI protocols.
+///
+/// The macro takes one argument, a GUID string.
+///
+/// The macro can only be applied to a struct, and the struct must have
+/// named fields (i.e. not a unit or tuple struct). It implements the
+/// [`Protocol`] trait and the `unsafe` [`Identify`] trait for the
+/// struct. It also adds a hidden field that causes the struct to be
+/// marked as [`!Send` and `!Sync`][send-and-sync].
+///
+/// # Safety
+///
+/// The caller must ensure that the correct GUID is attached to the
+/// type. An incorrect GUID could lead to invalid casts and other
+/// unsound behavior.
+///
+/// # Example
+///
+/// ```
+/// use uefi::{Identify, guid};
+/// use uefi::proto::unsafe_protocol;
+///
+/// #[unsafe_protocol("12345678-9abc-def0-1234-56789abcdef0")]
+/// struct ExampleProtocol {}
+///
+/// assert_eq!(ExampleProtocol::GUID, guid!("12345678-9abc-def0-1234-56789abcdef0"));
+/// ```
+///
+/// [`Identify`]: https://docs.rs/uefi/latest/uefi/trait.Identify.html
+/// [`Protocol`]: https://docs.rs/uefi/latest/uefi/proto/trait.Protocol.html
+/// [send-and-sync]: https://doc.rust-lang.org/nomicon/send-and-sync.html
 #[proc_macro_attribute]
-pub fn unsafe_guid(args: TokenStream, input: TokenStream) -> TokenStream {
-    // Parse the arguments and input using Syn
+pub fn unsafe_protocol(args: TokenStream, input: TokenStream) -> TokenStream {
+    // Parse `args` as a GUID string.
     let (time_low, time_mid, time_high_and_version, clock_seq_and_variant, node) =
         match parse_guid(parse_macro_input!(args as LitStr)) {
             Ok(data) => data,
             Err(tokens) => return tokens.into(),
         };
 
-    let mut result: TokenStream2 = input.clone().into();
+    let item_struct = parse_macro_input!(input as ItemStruct);
 
-    let type_definition = parse_macro_input!(input as TypeDefinition);
-
-    // At this point, we know everything we need to implement Identify
-    let ident = &type_definition.ident;
-    let (impl_generics, ty_generics, where_clause) = type_definition.generics.split_for_impl();
+    let ident = &item_struct.ident;
+    let struct_attrs = &item_struct.attrs;
+    let struct_vis = &item_struct.vis;
+    let struct_fields = if let Fields::Named(struct_fields) = &item_struct.fields {
+        &struct_fields.named
+    } else {
+        return err!(item_struct, "Protocol struct must used named fields").into();
+    };
+    let struct_generics = &item_struct.generics;
+    let (impl_generics, ty_generics, where_clause) = item_struct.generics.split_for_impl();
+
+    quote! {
+        #(#struct_attrs)*
+        #struct_vis struct #ident #struct_generics {
+            // Add a hidden field with `PhantomData` of a raw
+            // pointer. This has the implicit side effect of making the
+            // struct !Send and !Sync.
+            _no_send_or_sync: ::core::marker::PhantomData<*const u8>,
+            #struct_fields
+        }
 
-    result.append_all(quote! {
         unsafe impl #impl_generics ::uefi::Identify for #ident #ty_generics #where_clause {
-            #[doc(hidden)]
             const GUID: ::uefi::Guid = ::uefi::Guid::from_values(
                 #time_low,
                 #time_mid,
@@ -76,8 +92,10 @@ pub fn unsafe_guid(args: TokenStream, input: TokenStream) -> TokenStream {
                 #node,
             );
         }
-    });
-    result.into()
+
+        impl #impl_generics ::uefi::proto::Protocol for #ident #ty_generics #where_clause {}
+    }
+    .into()
 }
 
 /// Create a `Guid` at compile time.
@@ -164,28 +182,6 @@ fn parse_guid(guid_lit: LitStr) -> Result<(u32, u16, u16, u16, u64), TokenStream
     ))
 }
 
-/// Custom derive for the `Protocol` trait
-#[proc_macro_derive(Protocol)]
-pub fn derive_protocol(item: TokenStream) -> TokenStream {
-    // Parse the input using Syn
-    let item = parse_macro_input!(item as DeriveInput);
-
-    // Then implement Protocol
-    let ident = item.ident.clone();
-    let (impl_generics, ty_generics, where_clause) = item.generics.split_for_impl();
-    let result = quote! {
-        // Mark this as a `Protocol` implementation
-        impl #impl_generics ::uefi::proto::Protocol for #ident #ty_generics #where_clause {}
-
-        // Most UEFI functions expect to be called on the bootstrap processor.
-        impl #impl_generics !Send for #ident #ty_generics #where_clause {}
-
-        // Most UEFI functions do not support multithreaded access.
-        impl #impl_generics !Sync for #ident #ty_generics #where_clause {}
-    };
-    result.into()
-}
-
 /// Get the name of a function's argument at `arg_index`.
 fn get_function_arg_name(f: &ItemFn, arg_index: usize, errors: &mut TokenStream2) -> Option<Ident> {
     if let Some(FnArg::Typed(arg)) = f.sig.inputs.iter().nth(arg_index) {

uefi-macros/tests/ui/guid.rs

@@ -0,0 +1,7 @@
+use uefi::Guid;
+use uefi_macros::guid;
+
+// Error span should point to the second group.
+const BadHexGroup2: Guid = guid!("aaaaaaaa-Gaaa-aaaa-aaaa-aaaaaaaaaaaa");
+
+fn main() {}

uefi-macros/tests/ui/guid.stderr

@@ -0,0 +1,5 @@
+error: GUID component "Gaaa" is not a hexadecimal number
+ --> tests/ui/guid_bad_hex_group2.rs:5:44
+  |
+5 | const BadHexGroup2: Guid = guid!("aaaaaaaa-Gaaa-aaaa-aaaa-aaaaaaaaaaaa");
+  |                                            ^^^^

uefi-macros/tests/ui/guid_bad_hex_group2.rs

@@ -0,0 +1,7 @@
+use uefi::Guid;
+use uefi_macros::guid;
+
+// Error span should point to the fifth group.
+const BadHexGroup5: Guid = guid!("aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaG");
+
+fn main() {}

uefi-macros/tests/ui/guid_bad_hex_group2.stderr

@@ -0,0 +1,5 @@
+error: GUID component "aaaaaaaaaaaG" is not a hexadecimal number
+ --> tests/ui/guid_bad_hex_group5.rs:5:59
+  |
+5 | const BadHexGroup5: Guid = guid!("aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaG");
+  |                                                           ^^^^^^^^^^^^

uefi-macros/tests/ui/guid_bad_hex_group5.rs

@@ -0,0 +1,7 @@
+use uefi::Guid;
+use uefi_macros::guid;
+
+// Fail because the length is wrong.
+const TooShort: Guid = guid!("aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaa");
+
+fn main() {}

uefi-macros/tests/ui/guid_bad_hex_group5.stderr

@@ -0,0 +1,5 @@
+error: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaa" is not a canonical GUID string (expected 36 bytes, found 35)
+ --> tests/ui/guid_bad_length.rs:5:30
+  |
+5 | const TooShort: Guid = guid!("aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaa");
+  |                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

uefi-macros/tests/ui/guid_bad_length.rs

@@ -1,9 +1,9 @@
 use core::ffi::c_void;
 use core::ptr::{self, NonNull};
 
-use uefi::proto::Protocol;
+use uefi::proto::unsafe_protocol;
 use uefi::table::boot::{BootServices, EventType, SearchType, TimerTrigger, Tpl};
-use uefi::{unsafe_guid, Event, Identify};
+use uefi::{Event, Identify};
 
 pub fn test(bt: &BootServices) {
     info!("Testing timer...");
@@ -80,8 +80,7 @@ fn test_watchdog(bt: &BootServices) {
 }
 
 /// Dummy protocol for tests
-#[unsafe_guid("1a972918-3f69-4b5d-8cb4-cece2309c7f5")]
-#[derive(Protocol)]
+#[unsafe_protocol("1a972918-3f69-4b5d-8cb4-cece2309c7f5")]
 struct TestProtocol {}
 
 unsafe extern "efiapi" fn _test_notify(_event: Event, _context: Option<NonNull<c_void>>) {

uefi-macros/tests/ui/guid_bad_length.stderr

@@ -1,7 +1,6 @@
 #![no_std]
 #![no_main]
 #![feature(abi_efiapi)]
-#![feature(negative_impls)]
 
 #[macro_use]
 extern crate log;

uefi-test-runner/src/boot/misc.rs

@@ -114,35 +114,29 @@ impl fmt::Display for Guid {
 /// this trait is a building block to interface them in uefi-rs.
 ///
 /// You should never need to use the `Identify` trait directly, but instead go
-/// for more specific traits such as `Protocol` or `FileProtocolInfo`, which
+/// for more specific traits such as [`Protocol`] or [`FileProtocolInfo`], which
 /// indicate in which circumstances an `Identify`-tagged type should be used.
 ///
+/// For the common case of implementing this trait for a protocol, use
+/// the [`unsafe_protocol`] macro.
+///
 /// # Safety
 ///
 /// Implementing `Identify` is unsafe because attaching an incorrect GUID to a
 /// type can lead to type unsafety on both the Rust and UEFI side.
 ///
-/// You can derive `Identify` for a type using the `unsafe_guid` procedural
-/// macro, which is exported by this module. This macro mostly works like a
-/// custom derive, but also supports type aliases. It takes a GUID in canonical
-/// textual format as an argument, and is used in the following way:
-///
-/// ```
-/// use uefi::unsafe_guid;
-/// #[unsafe_guid("12345678-9abc-def0-1234-56789abcdef0")]
-/// struct Emptiness;
-/// ```
+/// [`Protocol`]: crate::proto::Protocol
+/// [`FileProtocolInfo`]: crate::proto::media::file::FileProtocolInfo
+/// [`unsafe_protocol`]: crate::proto::unsafe_protocol
 pub unsafe trait Identify {
     /// Unique protocol identifier.
     const GUID: Guid;
 }
 
-pub use uefi_macros::unsafe_guid;
-
 #[cfg(test)]
 mod tests {
     use super::*;
-    use uefi::{guid, unsafe_guid};
+    use uefi::guid;
 
     #[test]
     fn test_guid_display() {
@@ -163,17 +157,6 @@ mod tests {
         );
     }
 
-    #[test]
-    fn test_unsafe_guid_macro() {
-        #[unsafe_guid("12345678-9abc-def0-1234-56789abcdef0")]
-        struct X;
-
-        assert_eq!(
-            X::GUID,
-            Guid::from_values(0x12345678, 0x9abc, 0xdef0, 0x1234, 0x56789abcdef0)
-        );
-    }
-
     #[test]
     fn test_to_from_bytes() {
         #[rustfmt::skip]

uefi-test-runner/src/main.rs

@@ -121,7 +121,7 @@ pub type VirtualAddress = u64;
 
 mod guid;
 pub use self::guid::Guid;
-pub use self::guid::{unsafe_guid, Identify};
+pub use self::guid::Identify;
 
 pub mod chars;
 pub use self::chars::{Char16, Char8};