feat(schema): support generic bounds in structs & enums
Changes
- Support generic bounds in struct & enum definitions that use
HasSchemaderive macro- Required for jumpy#942
- Move the
static S: OnceLock<parking_lot::RwLock<HashMap<TypeId, &'static Schema>>>for generics intobones_schemaso that user crates don't have to take a dependency onparking_lot
Given the following rust code:
use bones_schema::HasSchema;
#[derive(HasSchema, Clone, Default)]
#[repr(C)]
struct HasGenericWithBounds<T: Default>(T);
The HasSchema derive macro used to generate:
#[repr(C)]
#[allow(dead_code)]
struct HasGenericWithBounds<T: Default>(T);
unsafe impl<T: HasSchema + Clone> bones_schema::HasSchema
for HasGenericWithBounds<T: Default> {
// ^^^^^^^^^^ error[E0229]: associated item constraints are not allowed here
fn schema() -> &'static bones_schema::Schema {
use ::std::sync::OnceLock;
use ::std::any::TypeId;
use bones_utils::HashMap;
use parking_lot::RwLock; // <~~ requires user crate to depend on `parking_lot`
static S: OnceLock<RwLock<HashMap<TypeId, &'static Schema>>> = OnceLock::new();
// ^^^^^^ error[E0412]: cannot find type `Schema` in this scope
let schema = {
S.get_or_init(Default::default).read().get(&TypeId::of::<Self>()).copied()
};
schema
.unwrap_or_else(|| {
let schema = bones_schema::registry::SCHEMA_REGISTRY
.register(/* ... */);
S.get_or_init(Default::default)
.write()
.insert(TypeId::of::<Self>(), schema);
schema
})
}
}
With errors:
error[E0412]: cannot find type `Schema` in this scope
--> framework_crates/bones_schema/tests/generic_bounds.rs:3:10
|
3 | #[derive(HasSchema, Clone, Default)]
| ^^^^^^^^^ not found in this scope
|
= note: this error originates in the derive macro `HasSchema` (in Nightly builds, run with -Z macro-backtrace for more info)
error[E0229]: associated item constraints are not allowed here
--> framework_crates/bones_schema/tests/generic_bounds.rs:6:29
|
6 | struct HasGenericWithBounds<T: Default>(T);
| ^^^^^^^^^^ associated item constraint not allowed here
Now it generates:
#[repr(C)]
#[allow(dead_code)]
struct HasGenericWithBounds<T: Default>(T);
unsafe impl<T: HasSchema + Clone + Default> bones_schema::HasSchema
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ correct bounds
for HasGenericWithBounds<T> {
// ^ correct parameters
fn schema() -> &'static bones_schema::Schema {
use ::std::any::TypeId;
let schema = {
bones_schema::registry::GENERIC_SCHEMA_CACHE // <~~ use shared `HashMap` in `bones_schema` so user crate doesn't have to depend on `parking_lot`
.read()
.get(&TypeId::of::<Self>())
.copied()
};
schema
.unwrap_or_else(|| {
let schema = bones_schema::registry::SCHEMA_REGISTRY
.register(/* ... */);
bones_schema::registry::GENERIC_SCHEMA_CACHE
.write()
.insert(TypeId::of::<Self>(), schema);
schema
})
}
}
Sorry I don't have time to do a proper review, I just want to throw a warning in here because I've messed this up when dealing with generics in the has schema derive: be very careful to make sure that each different instantiation of the generic type's schema returns it's own unique schema.
There was a really tricky to debug issue once where generic enums like Maybe<T> was accidentally returning the schema of the first registered T for the inner types, even though it needed to make sure each different Maybe<T> used a different schema internally.
Sorry that is really hard to say in a way that makes sense.
This is the PR that fixed the error I'm talking about: https://github.com/fishfolk/bones/pull/456
Anyway, warning aside, big thanks for this!
@zicklag thanks for weighing in. I think I ran into that issue once already 😆
My first push had some failing tests and it looked like it was because of the new shared GENERIC_SCHEMA_CACHE that replaced each variant having its own static HashMap<TypeId, &'static Schema> -- the type ID for Maybe::Set and Maybe::Unset are the same so their schemas were overlapping in the cache. To fix it I added the variant name to the cache key ((TypeId, &'static str)) which I think is ok, since the type ID accounts for generic params and the name differentiates enum variants.
That's the only thing I'm not 100% on, since the rest of the changes are essentially just fixing syntax errors in the derive macro code gen.