Skip to content
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

[WIP] initial proof of concept for restricted env #21

Closed
wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 7, 2022

@InquisitiveCoder

This is the approach I've had in mind for how to restrict which environment methods a given callback can use. Accomplished by adding a phantom type parameter to RetroEnvironment, and then one empty enum for each callback. Enum is used here to prevent a user from creating instances, an empty struct (struct LoadGame;) could still be created with let x = LoadGame;.

We then provide an impl for each RetroEnvironment<Enum> that just forwards the call along to the corresponding definition. This impl layer will be tedious, so I imagine we'll want a macro to do it for us: retro_env!(Global, get_core_assets_directory, get_content_directory).

To make converting between the different phantom types easily, an associated method is provided called into_state() which exists on all instances of RetroEnvironment<T>.

I see this approach being easy to follow, as you can just read the macro invocation to see which environment calls a particular callback has access to.

Let me know what you think!

@InquisitiveCoder
Copy link
Contributor

InquisitiveCoder commented Dec 8, 2022

I initially tried this approach in my experimental branch but ultimately I ended up handling this with traits. As long as RetroCore methods use RetroEnvironment as an argument type it's not possible to write tests or do much of anything with a RetroCore instance outside of the context of the libretro_core! macro. Users can't create a RetroEnvironment since there's no public constructor, but even if they could, they'd have to provide an unsafe extern "C" fn(u32, *mut c_void) -> bool which isn't very Rusty.

If instead we have a trait for each "scope" (e.g. LoadGameRetroEnvironment) and change the method argument to &mut impl ${Scope}RetroEnvironment users can easily use their own environment implementations if they want, and even use different structs for different scopes if they need to. On our end there's also no need to do any casting since retro_environment_callback can implement all of the traits and thus is always a valid argument for any RetroCore method.

Another issue I ran into is that the libretro-sys C structs usually aren't very nice input or output types; to provide idiomatic Rust methods, there's always some transformations necessary from the Rust input to the C struct and back out to a Rust output type, and for basic types like CStr that means the same boilerplate shows up in multiple methods. I managed to automate this and provide some basic type safety by adding a RetroEnvironmentData marker trait to the C types that are valid arguments to the *_raw methods, and a RetroEnvironmentResult that does the post-processing.

For instance the setup for the CStr-returning methods goes like this:

// Marker for C data
pub trait RetroEnvironmentData {}

// Trait for result types
pub trait RetroEnvironmentResult {
  type Source: RetroEnvironmentData;
  unsafe fn unsafe_from(x: Option<Self::Source>) -> Self;
}

// Equivalent to *const c_char, but implements `Default` and can easily be flattened when nested in another Option
impl RetroEnvironmentData for Option<&c_char>

// An Option<&CStr> result type implies a C representation of `Option<&c_char>` and post-processing with CStr::from_ptr
impl<'a> RetroEnvironmentResult for Option<&'a CStr> {
  type Source = Option<&'a c_char>;

  unsafe fn unsafe_from(option: Option<Self::Source>) -> Self {
    option.flatten().map(|ptr| CStr::from_ptr(ptr))
  }
}

pub trait RetroEnvironment: Sized {
  // impl Into<u32> allows us or the user to provide enums for the keys.
  // data is impl Into<RetroEnvironmentResult::Source> so any types that can be converted into the C type will work
  unsafe fn parameterized_get_raw<T>(&self, cmd: impl Into<u32>, data: impl Into<T::Source>) -> T
  where
    T: RetroEnvironmentResult;  

  // Implements argument-less gets by using the C type's default() value
  unsafe fn get_raw<T, U>(&self, cmd: impl Into<u32>) -> T
  where
    T: RetroEnvironmentResult<Source = U>,
    U: Default + RetroEnvironmentData,
  {
    self.parameterized_get_raw(cmd, U::default())
  }

  // Now these calls to get_raw just work; Rust infers that `get_raw` must return `Option<&CStr>` since
  // that's what `get_libretro_path` returns. `Option<&CStr>` implements `RetroEnvironmentResult` so
  // `get_raw` knows to use `Option<&c_char>` as the C type and how to convert from
  // `Option<Option<&c_char>` to `Option<&CStr>`
  fn get_libretro_path(&self) -> Option<&CStr> {
    unsafe { self.get_raw(RETRO_ENVIRONMENT_GET_LIBRETRO_PATH) }
  }

  fn get_core_assets_directory(&self) -> Option<&CStr> {
    unsafe { self.get_raw(RETRO_ENVIRONMENT_GET_CORE_ASSETS_DIRECTORY) }
  }

  fn get_save_directory(&self) -> Option<&CStr> {
    unsafe { self.get_raw(RETRO_ENVIRONMENT_GET_SAVE_DIRECTORY) }
  }
}

You can take a look at my code if you want to see all the gory details and compare.

One last little API suggestion: the get methods should take an immutable ref to the environment. That way if the user wants to pass the environment to other functions or structs they can restrict access to the mutating methods.

@ghost ghost closed this Jan 9, 2023
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants