-
Notifications
You must be signed in to change notification settings - Fork 14
Improve stepper structs a bit #224
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
cc2f382
b7dbb22
d2a27aa
cd34b20
b15388e
fdc405d
24e5a6d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,65 +1,54 @@ | ||
| use std::ptr::NonNull; | ||
|
|
||
| use shared::StepperStates; | ||
| use windows::core::PCWSTR; | ||
|
|
||
| use crate::{Tree, dlkr::DLAllocatorBase, dltx::DLString}; | ||
|
|
||
| use super::FD4TaskBase; | ||
| use crate::{Tree, dlkr::DLAllocatorRef, dltx::DLString, fd4::FD4Time}; | ||
|
|
||
| /// Source of name: RTTI | ||
| #[repr(C)] | ||
| pub struct FD4StepTemplateBase<const N: usize, T> { | ||
| pub task: FD4TaskBase, | ||
| pub stepper_fns: NonNull<[StepperFn<T>; N]>, | ||
| unk18: FD4StepTemplateBase0x18, | ||
| /// Index into the stepper_fns array. | ||
| pub current_state: u32, | ||
| /// Target step for next cycle. | ||
| pub request_state: u32, | ||
| unk50: bool, | ||
| unk51: [u8; 7], | ||
| allocator: NonNull<DLAllocatorBase>, | ||
| unk60: usize, | ||
| unk68: i8, | ||
| unk69: bool, | ||
| _pad6a: [u8; 6], | ||
| unk70: DLString, | ||
| state_description: PCWSTR, | ||
| unka8: bool, | ||
| unka9: [u8; 3], | ||
| } | ||
|
|
||
| impl<const N: usize, T> AsRef<FD4TaskBase> for FD4StepTemplateBase<N, T> { | ||
| fn as_ref(&self) -> &FD4TaskBase { | ||
| &self.task | ||
| } | ||
| pub struct FD4StepTemplateBase<States: StepperStates, Subject> { | ||
| vftable: *const (), | ||
| pub stepper_fns: NonNull<States::StepperFnArray<StepperFn<Subject>>>, | ||
| pub attach: FD4ComponentAttachSystem_Step, | ||
| /// Current state executing this frame. | ||
| pub current_state: States, | ||
| /// Target step for next frames execution. | ||
| pub requested_state: States, | ||
| unk48: u8, | ||
|
|
||
| // Seemingly all debug stuff after this point. | ||
| pub allocator: DLAllocatorRef, | ||
| unk58: usize, | ||
| unk60: i8, | ||
| unk61: bool, | ||
| unk68: DLString, | ||
| /// State label seemingly used for debug tooling. | ||
| /// Examples: "NotExecuting", "State Finished.(No StepMethod is Executing.)" | ||
| pub debug_state_label: PCWSTR, | ||
| unka0: bool, | ||
| unka4: i32, | ||
| } | ||
|
|
||
| /// Single state for the stepper to be executing from. | ||
| #[repr(C)] | ||
| pub struct StepperFn<T> { | ||
| pub executor: fn(&mut T, usize), | ||
| name: PCWSTR, | ||
| } | ||
|
|
||
| impl<T> StepperFn<T> { | ||
| pub fn name(&self) -> String { | ||
| unsafe { self.name.to_string().unwrap() } | ||
| } | ||
| pub executor: extern "C" fn(&mut T, &FD4Time), | ||
| pub name: PCWSTR, | ||
| } | ||
|
|
||
| /// Source of name: RTTI | ||
| #[repr(C)] | ||
| pub struct FD4StepTemplateBase0x18 { | ||
| unk0: NonNull<()>, | ||
| pub struct FD4ComponentAttachSystem { | ||
| vftable: *const (), | ||
| unk8: Tree<()>, | ||
| unk20: NonNull<DLAllocatorBase>, | ||
| unk28: NonNull<DLAllocatorBase>, | ||
| pub allocator: DLAllocatorRef, | ||
| } | ||
|
|
||
| /// Source of name: RTTI | ||
| #[allow(non_camel_case_types)] | ||
| #[repr(C)] | ||
| pub struct FD4StepBaseInterface<const N: usize, T> { | ||
| vftable: usize, | ||
| pub stepper_fns: NonNull<[StepperFn<T>; N]>, | ||
| unk10: NonNull<()>, | ||
| pub struct FD4ComponentAttachSystem_Step { | ||
| pub base: FD4ComponentAttachSystem, | ||
| pub allocator: DLAllocatorRef, | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ use syn::*; | |
| mod multi_param; | ||
|
|
||
| mod for_all_subclasses; | ||
| mod stepper; | ||
| mod subclass; | ||
| mod superclass; | ||
| mod utils; | ||
|
|
@@ -303,3 +304,50 @@ pub fn for_all_subclasses(_args: TokenStream, input: TokenStream) -> TokenStream | |
| Err(err) => err.into_compile_error().into(), | ||
| } | ||
| } | ||
|
|
||
| /// A derive macro that implements the StepperStates trait on a given enum. | ||
| /// | ||
| /// - The enum must be exhaustive (represent all states and no more). | ||
| /// - The enum must have a -1 state for inactive steppers. | ||
| /// - The enum must have no gaps in the discriminants. | ||
| /// | ||
| /// # Safety | ||
| /// | ||
| /// The implementer must ensure that the enum is exhaustive as unknown discriminants can be used to | ||
| /// trigger undefined behavior. | ||
| /// The implementer must ensure that the enum does not have more states than the game defines. | ||
| /// Failing to do so will allow for out-of-bound access to the stepper array. | ||
| /// The implementer must ensure that the enum discriminants have no gaps. Failing to do so will | ||
| /// allow out of bounds access to the stepper array as well as cause unknown discriminants. | ||
| #[proc_macro_derive(StepperStates)] | ||
| pub fn derive_stepper_states(input: TokenStream) -> TokenStream { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a cool idea and I like the direction, but I think some improvements can be made in terms of safety:
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't the expectation that implementing an unsafe trait even through a derive macro is sound as long as the safety requirements are followed? Not to say I'm not looking forward to
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Dasaav-dsv I think this is a soundness issue the same way implementing FromStatic using a macro would be. I'd rather just add the discriminator gap validation and laugh at people fucking up their enum in deranged ways |
||
| fn error(ident: &Ident, message: &str) -> TokenStream { | ||
| syn::Error::new_spanned(ident, message) | ||
| .to_compile_error() | ||
| .into() | ||
| } | ||
|
|
||
| let input = parse_macro_input!(input as DeriveInput); | ||
| let input_struct_ident = &input.ident; | ||
|
|
||
| let Data::Enum(e) = &input.data else { | ||
| return error(&input.ident, "StepperStates can only be derived on enums"); | ||
| }; | ||
|
|
||
| if let Err(e) = stepper::validate_stepper_enum_storage(&input) { | ||
| return e.to_compile_error().into(); | ||
| }; | ||
|
|
||
| if let Err(e) = stepper::validate_stepper_enum_variants(e) { | ||
| return e.to_compile_error().into(); | ||
| }; | ||
|
|
||
| let count = e.variants.len(); | ||
| let expanded = quote! { | ||
| unsafe impl ::fromsoftware_shared::StepperStates for #input_struct_ident { | ||
| type StepperFnArray<TStepperFn> = [TStepperFn; #count]; | ||
| } | ||
| }; | ||
|
|
||
| TokenStream::from(expanded) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,133 @@ | ||
| use std::collections::BTreeSet; | ||
|
|
||
| use syn::{DataEnum, DeriveInput, Expr, ExprLit, ExprUnary, Fields, Lit, Meta, UnOp}; | ||
|
|
||
| pub fn validate_stepper_enum_storage(i: &DeriveInput) -> syn::Result<()> { | ||
| let Some(repr_attr) = i.attrs.iter().find(|a| a.path().is_ident("repr")) else { | ||
| return Err(syn::Error::new_spanned( | ||
| &i.ident, | ||
| "Enum must apply a #[repr(i32)], there is currently no repr specified at all", | ||
| )); | ||
| }; | ||
|
|
||
| let Meta::List(repr_args) = &repr_attr.meta else { | ||
| return Err(syn::Error::new_spanned( | ||
| &i.ident, | ||
| "Enum must apply a #[repr(i32)], the repr attribute currently has no arguments", | ||
| )); | ||
| }; | ||
|
|
||
| if !repr_args | ||
| .tokens | ||
| .to_string() | ||
| .split(',') | ||
| .map(|s| s.trim()) | ||
| .any(|s| s == "i32") | ||
| { | ||
| return Err(syn::Error::new_spanned( | ||
| &i.ident, | ||
| "Enum must apply a #[repr(i32)]", | ||
| )); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| pub fn validate_stepper_enum_variants(e: &DataEnum) -> syn::Result<()> { | ||
| let mut values = BTreeSet::<i32>::new(); | ||
|
|
||
| for v in &e.variants { | ||
| if !matches!(v.fields, Fields::Unit) { | ||
| return Err(syn::Error::new_spanned( | ||
| &v.ident, | ||
| "All variants must be unit", | ||
| )); | ||
| } | ||
|
|
||
| let Some((_, expr)) = &v.discriminant else { | ||
| return Err(syn::Error::new_spanned( | ||
| &v.ident, | ||
| "All variants must have explicit discriminants (e.g. `GuestInviteWait = 3`)", | ||
| )); | ||
| }; | ||
|
|
||
| let val = read_i32_lit(expr)?; | ||
| if val < 0 && val != -1 { | ||
| return Err(syn::Error::new_spanned( | ||
| &v.ident, | ||
| "Disciminant cannot be a negative unless it's the Inactive state", | ||
| )); | ||
| } | ||
|
|
||
| if !values.insert(val) { | ||
| return Err(syn::Error::new_spanned( | ||
| &v.ident, | ||
| format!("Duplicate discriminant value {val}"), | ||
| )); | ||
| } | ||
| } | ||
|
|
||
| if !values.contains(&-1) { | ||
| return Err(syn::Error::new_spanned( | ||
| &e.variants[0].ident, | ||
| "Missing Inactive variant with discriminant -1", | ||
| )); | ||
| } | ||
|
|
||
| let non_sentinel: Vec<i32> = values.iter().copied().filter(|&x| x != -1).collect(); | ||
| if non_sentinel.is_empty() { | ||
| return Err(syn::Error::new_spanned( | ||
| &e.variants[0].ident, | ||
| "Stepper states must have more states than just the Inactive variant (-1)", | ||
| )); | ||
| } | ||
|
|
||
| let min = *non_sentinel.first().unwrap(); | ||
| let max = *non_sentinel.last().unwrap(); | ||
|
|
||
| let expected_len = (max - min + 1) as usize; | ||
| if expected_len != non_sentinel.len() { | ||
| let set: BTreeSet<i32> = non_sentinel.iter().copied().collect(); | ||
| let missing: Vec<i32> = (min..=max).filter(|x| !set.contains(x)).collect(); | ||
|
|
||
| return Err(syn::Error::new_spanned( | ||
| &e.variants[0].ident, | ||
| format!("Discriminants contain gaps; missing values: {missing:?}"), | ||
| )); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| fn read_i32_lit(expr: &Expr) -> syn::Result<i32> { | ||
| match expr { | ||
| Expr::Lit(ExprLit { | ||
| lit: Lit::Int(i), .. | ||
| }) => i | ||
| .base10_parse::<i32>() | ||
| .map_err(|_| syn::Error::new_spanned(expr, "Discriminant out of i32 range")), | ||
| Expr::Unary(ExprUnary { | ||
| op: UnOp::Neg(_), | ||
| expr: inner, | ||
| .. | ||
| }) => match inner.as_ref() { | ||
| Expr::Lit(ExprLit { | ||
| lit: Lit::Int(i), .. | ||
| }) => { | ||
| let v = i | ||
| .base10_parse::<i32>() | ||
| .map_err(|_| syn::Error::new_spanned(inner, "Discriminant out of i32 range"))?; | ||
| v.checked_neg() | ||
| .ok_or_else(|| syn::Error::new_spanned(expr, "Discriminant out of i32 range")) | ||
| } | ||
| _ => Err(syn::Error::new_spanned( | ||
| expr, | ||
| "Use an integer literal like -1 or 3", | ||
| )), | ||
| }, | ||
| _ => Err(syn::Error::new_spanned( | ||
| expr, | ||
| "Use an integer literal like -1 or 3", | ||
| )), | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.