Return plugins as Arc<Plugin> instead of &Plugin

This is the only way I can see for the PyO3 wrapper to work safely, and it's probably a good idea anyway (it simplifies the CXX wrapper too).

cxx/src/api/game.cpp

@@ -189,7 +189,7 @@ const PluginInterface* Game::GetPlugin(const std::string& pluginName) const {
   }
 
   try {
-    auto plugin = std::make_shared<Plugin>(std::move(pluginOpt->as_ref()));
+    auto plugin = std::make_shared<Plugin>(std::move(pluginOpt->as_ref().boxed_clone()));
     const auto result = plugins_.emplace(key, plugin);
 
     return result.first->second.get();

cxx/src/api/plugin.cpp

@@ -7,7 +7,7 @@
 #include "api/exception.h"
 
 namespace loot {
-Plugin::Plugin(::rust::Box<loot::rust::PluginRef> plugin) :
+Plugin::Plugin(::rust::Box<loot::rust::Plugin> plugin) :
     plugin_(std::move(plugin)) {}
 
 std::string Plugin::GetName() const { return std::string(plugin_->name()); }

cxx/src/api/plugin.h

@@ -16,7 +16,7 @@ namespace loot {
 
 class Plugin final : public PluginInterface {
 public:
-  explicit Plugin(::rust::Box<loot::rust::PluginRef> plugin);
+  explicit Plugin(::rust::Box<loot::rust::Plugin> plugin);
 
   std::string GetName() const override;
   std::optional<float> GetHeaderVersion() const override;
@@ -39,7 +39,7 @@ public:
   bool DoRecordsOverlap(const PluginInterface& plugin) const override;
 
 private:
-  ::rust::Box<loot::rust::PluginRef> plugin_;
+  ::rust::Box<loot::rust::Plugin> plugin_;
 };
 }
 

cxx/src/game.rs

@@ -3,8 +3,8 @@ use std::path::Path;
 use delegate::delegate;
 
 use crate::{
-    PluginRef, UnsupportedEnumValueError, VerboseError, database::Database, ffi::GameType,
-    plugin::OptionalPluginRef,
+    Plugin, UnsupportedEnumValueError, VerboseError, database::Database, ffi::GameType,
+    plugin::OptionalPlugin,
 };
 
 impl TryFrom<libloot::GameType> for GameType {
@@ -150,11 +150,11 @@ impl Game {
             .map_err(Into::into)
     }
 
-    pub fn plugin(&self, plugin_name: &str) -> Box<OptionalPluginRef> {
+    pub fn plugin(&self, plugin_name: &str) -> Box<OptionalPlugin> {
         Box::new(self.0.plugin(plugin_name).into())
     }
 
-    pub fn loaded_plugins(&self) -> Vec<PluginRef<'_>> {
+    pub fn loaded_plugins(&self) -> Vec<Plugin> {
         self.0
             .loaded_plugins()
             .into_iter()

cxx/src/lib.rs

@@ -13,7 +13,7 @@ use metadata::{
     new_location, new_message, new_message_content, new_plugin_cleaning_data, new_plugin_metadata,
     new_tag, select_message_content,
 };
-use plugin::{OptionalPluginRef, PluginRef};
+use plugin::{OptionalPlugin, Plugin};
 use std::{
     error::Error,
     ffi::{CString, c_char, c_uchar, c_uint, c_void},
@@ -407,10 +407,10 @@ mod ffi {
         pub fn clear_loaded_plugins(&mut self);
 
         // The plugin's lifetime is actually less than &Game's, it's only valid so long as the loaded plugin is not overwritten or cleared.
-        pub fn plugin(&self, plugin_name: &str) -> Box<OptionalPluginRef>;
+        pub fn plugin(&self, plugin_name: &str) -> Box<OptionalPlugin>;
 
         // The plugin's lifetime is actually less than &Game's, it's only valid so long as the loaded plugin is not overwritten or cleared.
-        pub unsafe fn loaded_plugins<'a>(&'a self) -> Vec<PluginRef<'a>>;
+        pub fn loaded_plugins(&self) -> Vec<Plugin>;
 
         pub fn sort_plugins(&self, plugin_names: &[&str]) -> Result<Vec<String>>;
 
@@ -541,19 +541,19 @@ mod ffi {
     }
 
     extern "Rust" {
-        type PluginRef<'a>;
+        type Plugin;
 
-        pub unsafe fn name<'a>(&'a self) -> &'a str;
+        pub fn name(&self) -> &str;
 
         // The None case is signalled by NaN.
         pub fn header_version(&self) -> f32;
 
         // The None case is signalled by an empty string (which is not a valid version).
-        pub unsafe fn version<'a>(&'a self) -> &'a str;
+        pub fn version(&self) -> &str;
 
         pub fn masters(&self) -> Result<Vec<String>>;
 
-        pub unsafe fn bash_tags<'a>(&'a self) -> &'a [String];
+        pub fn bash_tags(&self) -> &[String];
 
         // The None case is signalled by -1, all other values fit in u32.
         pub fn crc(&self) -> i64;
@@ -578,18 +578,18 @@ mod ffi {
 
         pub fn loads_archive(&self) -> bool;
 
-        pub unsafe fn do_records_overlap<'a>(&self, plugin: &PluginRef<'a>) -> Result<bool>;
+        pub fn do_records_overlap(&self, plugin: &Plugin) -> Result<bool>;
 
-        pub unsafe fn boxed_clone<'a>(&'a self) -> Box<PluginRef<'a>>;
+        pub fn boxed_clone(&self) -> Box<Plugin>;
     }
 
     extern "Rust" {
-        type OptionalPluginRef;
+        type OptionalPlugin;
 
         pub fn is_some(&self) -> bool;
 
         // Again, these lifetimes are wrong.
-        pub unsafe fn as_ref<'a>(&'a self) -> Result<Box<PluginRef<'a>>>;
+        pub unsafe fn as_ref<'a>(&'a self) -> Result<&'a Plugin>;
     }
 
     extern "Rust" {

cxx/src/plugin.rs

@@ -1,13 +1,15 @@
+use std::sync::Arc;
+
 use delegate::delegate;
 
-use crate::{EmptyOptionalError, OptionalRef, VerboseError};
+use crate::{Optional, VerboseError};
 
 #[derive(Debug)]
 #[repr(transparent)]
-pub struct PluginRef<'a>(&'a libloot::Plugin);
+pub struct Plugin(Arc<libloot::Plugin>);
 
-impl<'a> PluginRef<'a> {
-    pub fn new(plugin: &'a libloot::Plugin) -> Self {
+impl Plugin {
+    pub fn new(plugin: Arc<libloot::Plugin>) -> Self {
         Self(plugin)
     }
 
@@ -40,11 +42,11 @@ impl<'a> PluginRef<'a> {
     }
 
     pub fn do_records_overlap(&self, plugin: &Self) -> Result<bool, VerboseError> {
-        self.0.do_records_overlap(plugin.0).map_err(Into::into)
+        self.0.do_records_overlap(&plugin.0).map_err(Into::into)
     }
 
     pub fn boxed_clone(&self) -> Box<Self> {
-        Box::new(Self(self.0))
+        Box::new(Self(self.0.clone()))
     }
 
     delegate! {
@@ -70,23 +72,16 @@ impl<'a> PluginRef<'a> {
     }
 }
 
-impl<'a> From<&'a libloot::Plugin> for PluginRef<'a> {
-    fn from(value: &'a libloot::Plugin) -> Self {
-        PluginRef(value)
+impl From<Arc<libloot::Plugin>> for Plugin {
+    fn from(value: Arc<libloot::Plugin>) -> Self {
+        Plugin(value)
     }
 }
 
-pub type OptionalPluginRef = OptionalRef<libloot::Plugin>;
+pub type OptionalPlugin = Optional<Plugin>;
 
-impl OptionalRef<libloot::Plugin> {
-    /// # Safety
-    ///
-    /// This is safe as long as the pointer in the OptionalRef is still valid.
-    pub unsafe fn as_ref(&self) -> Result<Box<PluginRef<'_>>, EmptyOptionalError> {
-        if self.0.is_null() {
-            Err(EmptyOptionalError)
-        } else {
-            unsafe { Ok(Box::new(PluginRef::new(&*self.0))) }
-        }
+impl From<Option<Arc<libloot::Plugin>>> for Optional<Plugin> {
+    fn from(value: Option<Arc<libloot::Plugin>>) -> Self {
+        Self(value.map(Into::into))
     }
 }

pyo3/README.md

@@ -16,7 +16,7 @@ An **incomplete** and **experimental** Python wrapper around the libloot Rust im
 - [x] `GameType`
 - [ ] `LogLevel`
 - [x] `Database`
-- [ ] `Game`
+- [x] `Game`
 - [x] `Plugin`
 - [x] `Vertex`
 - [x] `File`

pyo3/src/game.rs

@@ -120,15 +120,17 @@ impl Game {
         self.0.clear_loaded_plugins();
     }
 
-    // TODO: Game::plugin().
-    // fn plugin(&self, plugin_name: &str) -> Option<&Plugin> {
-    //     self.0.plugin(plugin_name).map(Plugin::wrap)
-    // }
+    fn plugin(&self, plugin_name: &str) -> Option<Plugin> {
+        self.0.plugin(plugin_name).map(Into::into)
+    }
 
-    // TODO: Game::loaded_plugins().
-    // fn loaded_plugins(&self) -> Vec<&Plugin> {
-    //     wrap_plugins(self.0.loaded_plugins())
-    // }
+    fn loaded_plugins(&self) -> Vec<Plugin> {
+        self.0
+            .loaded_plugins()
+            .into_iter()
+            .map(Into::into)
+            .collect()
+    }
 
     fn sort_plugins(&self, plugin_names: Vec<String>) -> Result<Vec<String>, VerboseError> {
         Ok(self.0.sort_plugins(&as_strs(&plugin_names))?)

pyo3/src/plugin.rs

@@ -1,3 +1,5 @@
+use std::sync::Arc;
+
 use pyo3::{pyclass, pymethods};
 
 use crate::VerboseError;
@@ -5,7 +7,7 @@ use crate::VerboseError;
 #[pyclass(eq, frozen)]
 #[derive(Clone, Debug, Eq, PartialEq)]
 #[repr(transparent)]
-pub struct Plugin(libloot::Plugin);
+pub struct Plugin(Arc<libloot::Plugin>);
 
 #[pymethods]
 impl Plugin {
@@ -78,18 +80,8 @@ impl Plugin {
     }
 }
 
-impl Plugin {
-    pub fn wrap(plugin: &libloot::Plugin) -> &Plugin {
-        let p = plugin as *const libloot::Plugin;
-        // SAFETY: Plugin is a transparent wrapper, so reinterpreting the pointer is safe.
-        unsafe {
-            let p = p as *const Plugin;
-            &*p
-        }
+impl From<Arc<libloot::Plugin>> for Plugin {
+    fn from(value: Arc<libloot::Plugin>) -> Self {
+        Self(value)
     }
 }
-
-pub fn wrap_plugins(mut vec: Vec<&libloot::Plugin>) -> Vec<&Plugin> {
-    // SAFETY: This is safe because Plugin is a transparent wrapper around the libloot type.
-    unsafe { Vec::from_raw_parts(vec.as_mut_ptr().cast(), vec.len(), vec.capacity()) }
-}

src/game.rs

@@ -335,7 +335,7 @@ impl Game {
                 .cache
                 .plugins()
                 .iter()
-                .map(|(k, v)| (k.clone(), v))
+                .map(|(k, v)| (k.clone(), v.as_ref()))
                 .collect();
 
             for plugin in &plugins {
@@ -420,13 +420,13 @@ impl Game {
     }
 
     /// Get data for a loaded plugin.
-    pub fn plugin(&self, plugin_name: &str) -> Option<&Plugin> {
-        self.cache.plugin(plugin_name)
+    pub fn plugin(&self, plugin_name: &str) -> Option<Arc<Plugin>> {
+        self.cache.plugin(plugin_name).cloned()
     }
 
     /// Get data for all loaded plugins.
-    pub fn loaded_plugins(&self) -> Vec<&Plugin> {
-        self.cache.plugins_iter().collect()
+    pub fn loaded_plugins(&self) -> Vec<Arc<Plugin>> {
+        self.cache.plugins_iter().cloned().collect()
     }
 
     /// Calculates a new load order for the game's installed plugins (including
@@ -444,7 +444,8 @@ impl Game {
         let plugins = plugin_names
             .iter()
             .map(|n| {
-                self.plugin(n)
+                self.cache
+                    .plugin(n)
                     .ok_or_else(|| SortPluginsError::PluginNotLoaded(n.to_string()))
             })
             .collect::<Result<Vec<_>, _>>()?;
@@ -458,7 +459,7 @@ impl Game {
                 let masterlist_metadata = database.plugin_metadata(p.name(), false, true)?;
                 let user_metadata = database.plugin_user_metadata(p.name(), true)?;
                 let plugin = PluginSortingData::new(
-                    p,
+                    p.as_ref(),
                     masterlist_metadata.as_ref(),
                     user_metadata.as_ref(),
                     i,
@@ -711,7 +712,7 @@ fn resolve_plugin_path(game_type: GameType, data_path: &Path, plugin_path: &Path
 
 fn update_loaded_plugin_state<'a>(
     state: &mut loot_condition_interpreter::State,
-    plugins: impl Iterator<Item = &'a Plugin>,
+    plugins: impl Iterator<Item = &'a Arc<Plugin>>,
 ) {
     let mut plugin_versions = Vec::new();
     let mut plugin_crcs = Vec::new();
@@ -748,7 +749,7 @@ fn update_loaded_plugin_state<'a>(
 
 #[derive(Clone, Debug, Default, Eq, PartialEq)]
 pub(crate) struct GameCache {
-    plugins: HashMap<Filename, Plugin>,
+    plugins: HashMap<Filename, Arc<Plugin>>,
     archive_paths: HashSet<PathBuf>,
 }
 
@@ -761,7 +762,7 @@ impl GameCache {
     fn insert_plugins(&mut self, plugins: Vec<Plugin>) {
         for plugin in plugins {
             self.plugins
-                .insert(Filename::new(plugin.name().to_string()), plugin);
+                .insert(Filename::new(plugin.name().to_string()), Arc::new(plugin));
         }
     }
 
@@ -769,15 +770,15 @@ impl GameCache {
         self.plugins.clear();
     }
 
-    fn plugins(&self) -> &HashMap<Filename, Plugin> {
+    fn plugins(&self) -> &HashMap<Filename, Arc<Plugin>> {
         &self.plugins
     }
 
-    fn plugins_iter(&self) -> impl Iterator<Item = &Plugin> {
+    fn plugins_iter(&self) -> impl Iterator<Item = &Arc<Plugin>> {
         self.plugins.values()
     }
 
-    fn plugin(&self, plugin_name: &str) -> Option<&Plugin> {
+    fn plugin(&self, plugin_name: &str) -> Option<&Arc<Plugin>> {
         self.plugins.get(&Filename::new(plugin_name.to_string()))
     }