Repository: Ralith/hecs
Pull request #104 (2020-10-28 14:40:01)

Rewrite and test the Bundle derive macro
This PR adds tests for the derive macro, makes it hygienic and fixes it not working for tuple structs and unit structs(though I guess unit structs are kind of pointless to use?).

I wanted to tackle adding generics but figured adding proper testing first would make more sense.

Veykril: Alright, the history should be cleaner now
Veykril: Ah dammit i pushed the generics test file by accident now 


Patch:
diff --git a/Cargo.toml b/Cargo.toml
index 21a40c5..8b71141 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -30,6 +30,7 @@ lazy_static = { version = "1.4.0", optional = true, features = ["spin_no_std"] }
 [dev-dependencies]
 bencher = "0.1.5"
 rand = "0.7.3"
+trybuild = "1.0.23"
 
 [[bench]]
 name = "bench"
@@ -38,3 +39,6 @@ required-features = ["macros"]
 
 [profile.release]
 debug = true
+
+[workspace]
+members = ["macros"]
\ No newline at end of file
diff --git a/macros/src/lib.rs b/macros/src/lib.rs
index 5b88d18..f4dfec4 100644
--- a/macros/src/lib.rs
+++ b/macros/src/lib.rs
@@ -14,10 +14,13 @@
 
 extern crate proc_macro;
 
+use std::borrow::Cow;
+
 use proc_macro::TokenStream;
 use proc_macro2::Span;
+use proc_macro2::TokenStream as TokenStream2;
 use quote::quote;
-use syn::{parse_macro_input, DeriveInput};
+use syn::{parse_macro_input, DeriveInput, Error, Result};
 
 /// Implement `Bundle` for a monomorphic struct
 ///
@@ -29,60 +32,98 @@ use syn::{parse_macro_input, DeriveInput};
 #[proc_macro_derive(Bundle)]
 pub fn derive_bundle(input: TokenStream) -> TokenStream {
     let input = parse_macro_input!(input as DeriveInput);
+    match derive_bundle_(input) {
+        Ok(ts) => ts,
+        Err(e) => e.to_compile_error(),
+    }
+    .into()
+}
+
+fn derive_bundle_(input: DeriveInput) -> Result<TokenStream2> {
+    let ident = input.ident;
     if !input.generics.params.is_empty() {
-        return TokenStream::from(
-            quote! { compile_error!("derive(Bundle) does not support generics"); },
-        );
+        return Err(Error::new_spanned(
+            input.generics,
+            "derive(Bundle) does not support generics",
+        ));
     }
     let data = match input.data {
         syn::Data::Struct(s) => s,
         _ => {
-            return TokenStream::from(
-                quote! { compile_error!("derive(Bundle) only supports structs"); },
-            )
+            return Err(Error::new_spanned(
+                ident,
+                "derive(Bundle) does not support enums or unions",
+            ))
         }
     };
-    let ident = input.ident;
-    let (tys, fields) = struct_fields(&data.fields);
+    let (tys, field_members) = struct_fields(&data.fields);
+    let field_idents = member_as_idents(&field_members);
+
+    let dyn_bundle_code = gen_dynamic_bundle_impl(&ident, &field_members, &tys);
+    let num_tys = tys.len();
+    let bundle_code = if num_tys == 0 {
+        gen_unit_struct_bundle_impl(ident)
+    } else {
+        gen_bundle_impl(&ident, &field_members, &field_idents, &tys)
+    };
+    let mut ts = dyn_bundle_code;
+    ts.extend(bundle_code);
+    Ok(ts)
+}
 
-    let n = tys.len();
-    let code = quote! {
+fn gen_dynamic_bundle_impl(
+    ident: &syn::Ident,
+    field_members: &[syn::Member],
+    tys: &[&syn::Type],
+) -> TokenStream2 {
+    quote! {
         impl ::hecs::DynamicBundle for #ident {
-            fn with_ids<T>(&self, f: impl FnOnce(&[std::any::TypeId]) -> T) -> T {
-                Self::with_static_ids(f)
+            fn with_ids<T>(&self, f: impl ::std::ops::FnOnce(&[::std::any::TypeId]) -> T) -> T {
+                <Self as ::hecs::Bundle>::with_static_ids(f)
             }
 
-            fn type_info(&self) -> Vec<::hecs::TypeInfo> {
-                Self::static_type_info()
+            fn type_info(&self) -> ::std::vec::Vec<::hecs::TypeInfo> {
+                <Self as ::hecs::Bundle>::static_type_info()
             }
 
             #[allow(clippy::forget_copy)]
-            unsafe fn put(mut self, mut f: impl FnMut(*mut u8, ::hecs::TypeInfo)) {
+            unsafe fn put(mut self, mut f: impl ::std::ops::FnMut(*mut u8, ::hecs::TypeInfo)) {
                 #(
-                    f((&mut self.#fields as *mut #tys).cast::<u8>(), ::hecs::TypeInfo::of::<#tys>());
-                    std::mem::forget(self.#fields);
+                    f((&mut self.#field_members as *mut #tys).cast::<u8>(), ::hecs::TypeInfo::of::<#tys>());
+                    ::std::mem::forget(self.#field_members);
                 )*
             }
         }
+    }
+}
 
+fn gen_bundle_impl(
+    ident: &syn::Ident,
+    field_members: &[syn::Member],
+    field_idents: &[Cow<syn::Ident>],
+    tys: &[&syn::Type],
+) -> TokenStream2 {
+    let num_tys = tys.len();
+    quote! {
         impl ::hecs::Bundle for #ident {
-            fn with_static_ids<T>(f: impl FnOnce(&[std::any::TypeId]) -> T) -> T {
-                use std::any::TypeId;
-                use std::mem;
-
+            fn with_static_ids<T>(f: impl ::std::ops::FnOnce(&[::std::any::TypeId]) -> T) -> T {
                 ::hecs::lazy_static::lazy_static! {
-                    static ref ELEMENTS: [TypeId; #n] = {
-                        let mut dedup = std::collections::HashSet::new();
-                        for &(ty, name) in [#((std::any::TypeId::of::<#tys>(), std::any::type_name::<#tys>())),*].iter() {
+                    static ref ELEMENTS: [::std::any::TypeId; #num_tys] = {
+                        let mut dedup = ::std::collections::HashSet::new();
+                        for &(ty, name) in [#((::std::any::TypeId::of::<#tys>(), ::std::any::type_name::<#tys>())),*].iter() {
                             if !dedup.insert(ty) {
-                                panic!("{} has multiple {} fields; each type must occur at most once!", stringify!(#ident), name);
+                                ::std::panic!("{} has multiple {} fields; each type must occur at most once!", stringify!(#ident), name);
                             }
                         }
 
-                        let mut tys = [#((mem::align_of::<#tys>(), TypeId::of::<#tys>())),*];
-                        tys.sort_unstable_by(|x, y| x.0.cmp(&y.0).reverse().then(x.1.cmp(&y.1)));
-                        let mut ids = [TypeId::of::<()>(); #n];
-                        for (id, info) in ids.iter_mut().zip(tys.iter()) {
+                        let mut tys = [#((::std::mem::align_of::<#tys>(), ::std::any::TypeId::of::<#tys>())),*];
+                        tys.sort_unstable_by(|x, y| {
+                            ::std::cmp::Ord::cmp(&x.0, &y.0)
+                                .reverse()
+                                .then(::std::cmp::Ord::cmp(&x.1, &y.1))
+                        });
+                        let mut ids = [::std::any::TypeId::of::<()>(); #num_tys];
+                        for (id, info) in ::std::iter::Iterator::zip(ids.iter_mut(), tys.iter()) {
                             *id = info.1;
                         }
                         ids
@@ -92,41 +133,76 @@ pub fn derive_bundle(input: TokenStream) -> TokenStream {
                 f(&*ELEMENTS)
             }
 
-            fn static_type_info() -> Vec<::hecs::TypeInfo> {
-                let mut info = vec![#(::hecs::TypeInfo::of::<#tys>()),*];
+            fn static_type_info() -> ::std::vec::Vec<::hecs::TypeInfo> {
+                let mut info = ::std::vec![#(::hecs::TypeInfo::of::<#tys>()),*];
                 info.sort_unstable();
                 info
             }
 
             unsafe fn get(
-                mut f: impl FnMut(::hecs::TypeInfo) -> Option<std::ptr::NonNull<u8>>,
-            ) -> Result<Self, ::hecs::MissingComponent> {
+                mut f: impl ::std::ops::FnMut(::hecs::TypeInfo) -> ::std::option::Option<::std::ptr::NonNull<u8>>,
+            ) -> ::std::result::Result<Self, ::hecs::MissingComponent> {
                 #(
-                    let #fields = f(::hecs::TypeInfo::of::<#tys>())
+                    let #field_idents = f(::hecs::TypeInfo::of::<#tys>())
                             .ok_or_else(::hecs::MissingComponent::new::<#tys>)?
                             .cast::<#tys>()
-                        .as_ptr();
+                            .as_ptr();
                 )*
-                Ok(Self { #( #fields: #fields.read(), )* })
+                ::std::result::Result::Ok(Self { #( #field_members: #field_idents.read(), )* })
             }
         }
-    };
-    TokenStream::from(code)
+    }
 }
 
-fn struct_fields(fields: &syn::Fields) -> (Vec<&syn::Type>, Vec<syn::Ident>) {
+// no reason to generate a static for unit structs
+fn gen_unit_struct_bundle_impl(ident: syn::Ident) -> TokenStream2 {
+    quote! {
+        impl ::hecs::Bundle for #ident {
+            fn with_static_ids<T>(f: impl ::std::ops::FnOnce(&[::std::any::TypeId]) -> T) -> T { f(&[]) }
+            fn static_type_info() -> ::std::vec::Vec<::hecs::TypeInfo> { ::std::vec::Vec::new() }
+
+            unsafe fn get(
+                mut f: impl ::std::ops::FnMut(::hecs::TypeInfo) -> Option<::std::ptr::NonNull<u8>>,
+            ) -> Result<Self, ::hecs::MissingComponent> {
+                Ok(Self {/* for some reason this works for all unit struct variations */})
+            }
+        }
+    }
+}
+
+fn struct_fields(fields: &syn::Fields) -> (Vec<&syn::Type>, Vec<syn::Member>) {
     match fields {
         syn::Fields::Named(ref fields) => fields
             .named
             .iter()
-            .map(|f| (&f.ty, f.ident.clone().unwrap()))
+            .map(|f| (&f.ty, syn::Member::Named(f.ident.clone().unwrap())))
             .unzip(),
         syn::Fields::Unnamed(ref fields) => fields
             .unnamed
             .iter()
             .enumerate()
-            .map(|(i, f)| (&f.ty, syn::Ident::new(&i.to_string(), Span::call_site())))
+            .map(|(i, f)| {
+                (
+                    &f.ty,
+                    syn::Member::Unnamed(syn::Index {
+                        index: i as u32,
+                        span: Span::call_site(),
+                    }),
+                )
+            })
             .unzip(),
         syn::Fields::Unit => (Vec::new(), Vec::new()),
     }
 }
+
+fn member_as_idents<'a>(members: &'a [syn::Member]) -> Vec<Cow<'a, syn::Ident>> {
+    members
+        .iter()
+        .map(|member| match member {
+            syn::Member::Named(ident) => Cow::Borrowed(ident),
+            &syn::Member::Unnamed(syn::Index { index, span }) => {
+                Cow::Owned(syn::Ident::new(&format!("tuple_field_{}", index), span))
+            }
+        })
+        .collect()
+}


Test patch:
diff --git a/tests/derive.rs b/tests/derive.rs
new file mode 100644
index 0000000..c843459
--- /dev/null
+++ b/tests/derive.rs
@@ -0,0 +1,22 @@
+//!
+
+#[cfg(feature = "macros")]
+#[cfg_attr(miri, ignore)]
+#[test]
+fn derive() {
+    const TEST_DIR: &str = "tests/derive";
+    let t = trybuild::TestCases::new();
+    let failures = &["enum.rs", "union.rs"];
+    let successes = &[
+        "unit_structs.rs",
+        "tuple_structs.rs",
+        "named_structs.rs",
+        "no_prelude.rs",
+    ];
+    for &passing_test in successes {
+        t.pass(&format!("{}/{}", TEST_DIR, passing_test));
+    }
+    for &failing_test in failures {
+        t.compile_fail(&format!("{}/{}", TEST_DIR, failing_test));
+    }
+}
diff --git a/tests/derive/enum.rs b/tests/derive/enum.rs
new file mode 100644
index 0000000..569f1d7
--- /dev/null
+++ b/tests/derive/enum.rs
@@ -0,0 +1,6 @@
+use hecs::Bundle;
+
+#[derive(Bundle)]
+enum Foo {}
+
+fn main() {}
diff --git a/tests/derive/enum.stderr b/tests/derive/enum.stderr
new file mode 100644
index 0000000..bec72c6
--- /dev/null
+++ b/tests/derive/enum.stderr
@@ -0,0 +1,5 @@
+error: derive(Bundle) does not support enums or unions
+ --> $DIR/enum.rs:4:6
+  |
+4 | enum Foo {}
+  |      ^^^
diff --git a/tests/derive/named_structs.rs b/tests/derive/named_structs.rs
new file mode 100644
index 0000000..9ef9b26
--- /dev/null
+++ b/tests/derive/named_structs.rs
@@ -0,0 +1,21 @@
+use hecs::Bundle;
+
+#[derive(Bundle)]
+struct Foo {
+    foo: i32,
+}
+
+#[derive(Bundle)]
+struct Bar {
+    foo: i32,
+    bar: String,
+}
+
+#[derive(Bundle)]
+struct Baz {
+    foo: i32,
+    bar: String,
+    baz: &'static str,
+}
+
+fn main() {}
diff --git a/tests/derive/no_prelude.rs b/tests/derive/no_prelude.rs
new file mode 100644
index 0000000..6d6fd38
--- /dev/null
+++ b/tests/derive/no_prelude.rs
@@ -0,0 +1,8 @@
+#![no_implicit_prelude]
+
+#[derive(::hecs::Bundle)]
+struct Foo {
+    foo: (),
+}
+
+fn main() {}
diff --git a/tests/derive/tuple_structs.rs b/tests/derive/tuple_structs.rs
new file mode 100644
index 0000000..68729b1
--- /dev/null
+++ b/tests/derive/tuple_structs.rs
@@ -0,0 +1,12 @@
+use hecs::Bundle;
+
+#[derive(Bundle)]
+struct Foo(i32);
+
+#[derive(Bundle)]
+struct Bar(i32, String);
+
+#[derive(Bundle)]
+struct Baz(i32, String, &'static str);
+
+fn main() {}
diff --git a/tests/derive/union.rs b/tests/derive/union.rs
new file mode 100644
index 0000000..3c2f1d5
--- /dev/null
+++ b/tests/derive/union.rs
@@ -0,0 +1,8 @@
+use hecs::Bundle;
+
+#[derive(Bundle)]
+union Foo {
+    u8: u8,
+}
+
+fn main() {}
diff --git a/tests/derive/union.stderr b/tests/derive/union.stderr
new file mode 100644
index 0000000..87ba7b0
--- /dev/null
+++ b/tests/derive/union.stderr
@@ -0,0 +1,5 @@
+error: derive(Bundle) does not support enums or unions
+ --> $DIR/union.rs:4:7
+  |
+4 | union Foo {
+  |       ^^^
diff --git a/tests/derive/unit_structs.rs b/tests/derive/unit_structs.rs
new file mode 100644
index 0000000..7dd6314
--- /dev/null
+++ b/tests/derive/unit_structs.rs
@@ -0,0 +1,12 @@
+use hecs::Bundle;
+
+#[derive(Bundle)]
+struct NamedUnit {}
+
+#[derive(Bundle)]
+struct TupleUnit();
+
+#[derive(Bundle)]
+struct Unit;
+
+fn main() {}