From d648011e51298dac7da820fddfc46173dff4c155 Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Tue, 22 Apr 2025 18:39:13 +0100 Subject: [PATCH] Refactor FFI error handling To reduce verbosity and duplication. --- cxx/src/database.rs | 3 +- cxx/src/error.rs | 90 +++++++--------------------- cxx/src/game.rs | 6 +- cxx/src/lib.rs | 3 +- ffi-errors/src/lib.rs | 60 ++++++++++++++++++- pyo3/src/database.rs | 15 ++--- pyo3/src/error.rs | 132 +++++++----------------------------------- pyo3/src/game.rs | 7 +-- pyo3/src/metadata.rs | 3 +- 9 files changed, 121 insertions(+), 198 deletions(-) diff --git a/cxx/src/database.rs b/cxx/src/database.rs index 660c75ec..3b5e5eb9 100644 --- a/cxx/src/database.rs +++ b/cxx/src/database.rs @@ -5,9 +5,10 @@ use std::{ use delegate::delegate; use libloot::{WriteMode, error::DatabaseLockPoisonError}; +use libloot_ffi_errors::UnsupportedEnumValueError; use crate::{ - UnsupportedEnumValueError, VerboseError, + VerboseError, ffi::EdgeType, metadata::{Group, Message, OptionalPluginMetadata, PluginMetadata, to_vec_of_unwrapped}, }; diff --git a/cxx/src/error.rs b/cxx/src/error.rs index d9bd7f1a..c735125c 100644 --- a/cxx/src/error.rs +++ b/cxx/src/error.rs @@ -1,5 +1,8 @@ use crate::game::NotValidUtf8; -use libloot_ffi_errors::SystemError; +use libloot_ffi_errors::{ + SystemError, SystemErrorCategory, UnsupportedEnumValueError, fmt_error_chain, + variant_box_from_error, +}; use libloot::{ error::{ @@ -16,9 +19,7 @@ use libloot::{ pub enum VerboseError { CyclicInteractionError(Vec), UndefinedGroupError(String), - EspluginError(i32, String), - LibloadorderError(i32, String), - LciError(i32, String), + SystemError(SystemError), FileAccessError(String), InvalidArgument(String), Other(Box), @@ -41,30 +42,27 @@ impl std::fmt::Display for VerboseError { Self::UndefinedGroupError(group) => { write!(f, "UndefinedGroupError: {}", group) } - Self::EspluginError(c, s) => { - write!(f, "EspluginError: {}: {}", c, s) - } - Self::LibloadorderError(c, s) => { - write!(f, "LibloadorderError: {}: {}", c, s) - } - Self::LciError(c, s) => { - write!(f, "LciError: {}: {}", c, s) + Self::SystemError(e) => { + let prefix = match e.category() { + SystemErrorCategory::Esplugin => "EspluginError", + SystemErrorCategory::Libloadorder => "LibloadorderError", + SystemErrorCategory::LootConditionInterpreter => "LciError", + }; + write!(f, "{}: {}: {}", prefix, e.code(), e.message()) } Self::FileAccessError(s) => write!(f, "FileAccessError: {}", s), Self::InvalidArgument(s) => write!(f, "InvalidArgument: {}", s), - Self::Other(e) => { - write!(f, "{}", e)?; - let mut error = e.as_ref(); - while let Some(source) = error.source() { - write!(f, ": {}", source)?; - error = source; - } - Ok(()) - } + Self::Other(e) => fmt_error_chain(e.as_ref(), f), } } } +variant_box_from_error!(UnsupportedEnumValueError, VerboseError::Other); +variant_box_from_error!(NotValidUtf8, VerboseError::Other); +variant_box_from_error!(DatabaseLockPoisonError, VerboseError::Other); +variant_box_from_error!(MultilingualMessageContentsError, VerboseError::Other); +variant_box_from_error!(RegexError, VerboseError::Other); + impl From for VerboseError { fn from(value: GameHandleCreationError) -> Self { match value { @@ -75,24 +73,6 @@ impl From for VerboseError { } } -impl From for VerboseError { - fn from(value: UnsupportedEnumValueError) -> Self { - Self::Other(Box::new(value)) - } -} - -impl From for VerboseError { - fn from(value: NotValidUtf8) -> Self { - Self::Other(Box::new(value)) - } -} - -impl From for VerboseError { - fn from(value: DatabaseLockPoisonError) -> Self { - Self::Other(Box::new(value)) - } -} - impl From for VerboseError { fn from(value: LoadPluginsError) -> Self { match value { @@ -126,8 +106,7 @@ impl From for VerboseError { impl From for VerboseError { fn from(value: LoadOrderError) -> Self { - let error = SystemError::from(value); - Self::LibloadorderError(error.code(), error.message().to_string()) + Self::SystemError(SystemError::from(value)) } } @@ -145,8 +124,7 @@ impl From for VerboseError { impl From for VerboseError { fn from(value: ConditionEvaluationError) -> Self { - let error = SystemError::from(value); - Self::LciError(error.code(), error.message().to_string()) + Self::SystemError(SystemError::from(value)) } } @@ -171,20 +149,7 @@ impl From for VerboseError { impl From for VerboseError { fn from(value: PluginDataError) -> Self { - let error = SystemError::from(value); - Self::EspluginError(error.code(), error.message().to_string()) - } -} - -impl From for VerboseError { - fn from(value: MultilingualMessageContentsError) -> Self { - Self::Other(Box::new(value)) - } -} - -impl From for VerboseError { - fn from(value: RegexError) -> Self { - Self::Other(Box::new(value)) + Self::SystemError(SystemError::from(value)) } } @@ -198,14 +163,3 @@ impl std::fmt::Display for EmptyOptionalError { } impl std::error::Error for EmptyOptionalError {} - -#[derive(Debug)] -pub struct UnsupportedEnumValueError; - -impl std::fmt::Display for UnsupportedEnumValueError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "Enum value is unsupported") - } -} - -impl std::error::Error for UnsupportedEnumValueError {} diff --git a/cxx/src/game.rs b/cxx/src/game.rs index 6236edaa..95c507c3 100644 --- a/cxx/src/game.rs +++ b/cxx/src/game.rs @@ -1,11 +1,9 @@ use std::path::Path; use delegate::delegate; +use libloot_ffi_errors::UnsupportedEnumValueError; -use crate::{ - Plugin, UnsupportedEnumValueError, VerboseError, database::Database, ffi::GameType, - plugin::OptionalPlugin, -}; +use crate::{Plugin, VerboseError, database::Database, ffi::GameType, plugin::OptionalPlugin}; impl TryFrom for GameType { type Error = UnsupportedEnumValueError; diff --git a/cxx/src/lib.rs b/cxx/src/lib.rs index 66c965e5..eb87bb3d 100644 --- a/cxx/src/lib.rs +++ b/cxx/src/lib.rs @@ -5,8 +5,9 @@ mod metadata; mod plugin; use database::{Database, Vertex, new_vertex}; -use error::{EmptyOptionalError, UnsupportedEnumValueError, VerboseError}; +use error::{EmptyOptionalError, VerboseError}; use game::{Game, new_game, new_game_with_local_path}; +use libloot_ffi_errors::UnsupportedEnumValueError; use metadata::{ File, Filename, Group, Location, Message, MessageContent, OptionalMessageContentRef, OptionalPluginMetadata, PluginCleaningData, PluginMetadata, Tag, group_default_name, diff --git a/ffi-errors/src/lib.rs b/ffi-errors/src/lib.rs index 34b09daa..d2c2d750 100644 --- a/ffi-errors/src/lib.rs +++ b/ffi-errors/src/lib.rs @@ -29,7 +29,19 @@ pub enum SystemErrorCategory { LootConditionInterpreter = LIBLOOT_SYSTEM_ERROR_CATEGORY_LCI, } -#[derive(Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)] +impl std::fmt::Display for SystemErrorCategory { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + SystemErrorCategory::Esplugin => write!(f, "esplugin"), + SystemErrorCategory::Libloadorder => write!(f, "libloadorder"), + SystemErrorCategory::LootConditionInterpreter => { + write!(f, "loot-condition-interpreter") + } + } + } +} + +#[derive(Debug)] pub struct SystemError { code: c_int, category: SystemErrorCategory, @@ -50,6 +62,18 @@ impl SystemError { } } +impl std::fmt::Display for SystemError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!( + f, + "{} error, code {}: {}", + self.category, self.code, self.message + ) + } +} + +impl std::error::Error for SystemError {} + impl From for SystemError { fn from(value: PluginDataError) -> Self { let error = value @@ -97,3 +121,37 @@ impl From for SystemError { } } } + +#[derive(Clone, Copy, Debug, Default, Eq, PartialEq, Ord, PartialOrd, Hash)] +pub struct UnsupportedEnumValueError; + +impl std::fmt::Display for UnsupportedEnumValueError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "Enum value is unsupported") + } +} + +impl std::error::Error for UnsupportedEnumValueError {} + +pub fn fmt_error_chain( + mut error: &dyn std::error::Error, + f: &mut std::fmt::Formatter<'_>, +) -> std::fmt::Result { + write!(f, "{}", error)?; + while let Some(source) = error.source() { + write!(f, ": {}", source)?; + error = source; + } + Ok(()) +} + +#[macro_export] +macro_rules! variant_box_from_error { + ( $from_type:ident, $to_type:ident::$to_variant:ident ) => { + impl From<$from_type> for $to_type { + fn from(value: $from_type) -> Self { + Self::$to_variant(Box::new(value)) + } + } + }; +} diff --git a/pyo3/src/database.rs b/pyo3/src/database.rs index e94bc6b9..99dc5ee4 100644 --- a/pyo3/src/database.rs +++ b/pyo3/src/database.rs @@ -5,13 +5,14 @@ use std::{ }; use libloot::{WriteMode, error::DatabaseLockPoisonError}; +use libloot_ffi_errors::UnsupportedEnumValueError; use pyo3::{ Bound, PyResult, pyclass, pymethods, types::{PyAnyMethods, PyTypeMethods}, }; use crate::{ - error::{UnsupportedEnumValueError, VerboseError}, + error::VerboseError, metadata::{Group, Message, NONE_REPR, PluginMetadata}, }; @@ -237,15 +238,15 @@ impl Vertex { } #[getter] - fn out_edge_type(&self) -> Result, UnsupportedEnumValueError> { - self.0.out_edge_type().map(|e| e.try_into()).transpose() + fn out_edge_type(&self) -> Result, VerboseError> { + self.0 + .out_edge_type() + .map(|e| e.try_into().map_err(Into::into)) + .transpose() } #[setter] - fn set_out_edge_type( - &mut self, - out_edge_type: EdgeType, - ) -> Result<(), UnsupportedEnumValueError> { + fn set_out_edge_type(&mut self, out_edge_type: EdgeType) -> Result<(), VerboseError> { let out_edge_type = out_edge_type.try_into()?; self.0.set_out_edge_type(out_edge_type); Ok(()) diff --git a/pyo3/src/error.rs b/pyo3/src/error.rs index 119a4afa..ff48b6b9 100644 --- a/pyo3/src/error.rs +++ b/pyo3/src/error.rs @@ -8,60 +8,42 @@ use libloot::{ LoadMetadataError, MultilingualMessageContentsError, RegexError, WriteMetadataError, }, }; -use libloot_ffi_errors::SystemError; +use libloot_ffi_errors::{ + SystemError, UnsupportedEnumValueError, fmt_error_chain, variant_box_from_error, +}; use pyo3::{PyErr, exceptions::PyValueError}; use crate::{CyclicInteractionError, EspluginError, UndefinedGroupError, database::Vertex}; -#[derive(Clone, Copy, Debug, Default, Eq, PartialEq, Ord, PartialOrd, Hash)] -pub struct UnsupportedEnumValueError; - -impl std::fmt::Display for UnsupportedEnumValueError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "Enum value is unsupported") - } -} - -impl std::error::Error for UnsupportedEnumValueError {} - -impl From for PyErr { - fn from(value: UnsupportedEnumValueError) -> Self { - PyValueError::new_err(value.to_string()) - } -} - #[derive(Debug)] pub enum VerboseError { CyclicInteractionError(Vec), UndefinedGroupError(String), - EspluginError(i32, String), + EspluginError(SystemError), Other(Box), } impl std::fmt::Display for VerboseError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - Self::CyclicInteractionError(c) => { - let cycle = display_cycle(c); - write!(f, "cyclic interaction detected: {}", cycle) - } - Self::UndefinedGroupError(g) => { - write!(f, "the group \"{}\" does not exist", g) - } - Self::EspluginError(_, s) => s.fmt(f), - Self::Other(e) => { - write!(f, "{}", e)?; - let mut error = e.as_ref(); - while let Some(source) = error.source() { - write!(f, ": {}", source)?; - error = source; - } - Ok(()) - } + Self::CyclicInteractionError(c) => SortPluginsError::CycleFound(c.clone()).fmt(f), + Self::UndefinedGroupError(g) => SortPluginsError::UndefinedGroup(g.clone()).fmt(f), + Self::EspluginError(e) => e.message().fmt(f), + Self::Other(e) => fmt_error_chain(e.as_ref(), f), } } } +variant_box_from_error!(UnsupportedEnumValueError, VerboseError::Other); +variant_box_from_error!(DatabaseLockPoisonError, VerboseError::Other); +variant_box_from_error!(LoadPluginsError, VerboseError::Other); +variant_box_from_error!(LoadOrderError, VerboseError::Other); +variant_box_from_error!(LoadMetadataError, VerboseError::Other); +variant_box_from_error!(WriteMetadataError, VerboseError::Other); +variant_box_from_error!(ConditionEvaluationError, VerboseError::Other); +variant_box_from_error!(MultilingualMessageContentsError, VerboseError::Other); +variant_box_from_error!(RegexError, VerboseError::Other); + impl From for VerboseError { fn from(value: GameHandleCreationError) -> Self { match value { @@ -71,27 +53,6 @@ impl From for VerboseError { } } -impl From for VerboseError { - fn from(value: UnsupportedEnumValueError) -> Self { - Self::Other(Box::new(value)) - } -} - -impl From for VerboseError { - fn from(value: DatabaseLockPoisonError) -> Self { - Self::Other(Box::new(value)) - } -} - -impl From for VerboseError { - fn from(value: LoadPluginsError) -> Self { - match value { - LoadPluginsError::PluginDataError(e) => e.into(), - _ => Self::Other(Box::new(value)), - } - } -} - impl From for VerboseError { fn from(value: SortPluginsError) -> Self { match value { @@ -113,30 +74,6 @@ impl From for VerboseError { } } -impl From for VerboseError { - fn from(value: LoadOrderError) -> Self { - Self::Other(Box::new(value)) - } -} - -impl From for VerboseError { - fn from(value: LoadMetadataError) -> Self { - Self::Other(Box::new(value)) - } -} - -impl From for VerboseError { - fn from(value: WriteMetadataError) -> Self { - Self::Other(Box::new(value)) - } -} - -impl From for VerboseError { - fn from(value: ConditionEvaluationError) -> Self { - Self::Other(Box::new(value)) - } -} - impl From for VerboseError { fn from(value: GroupsPathError) -> Self { match value { @@ -158,20 +95,7 @@ impl From for VerboseError { impl From for VerboseError { fn from(value: PluginDataError) -> Self { - let error = SystemError::from(value); - Self::EspluginError(error.code(), error.message().to_string()) - } -} - -impl From for VerboseError { - fn from(value: MultilingualMessageContentsError) -> Self { - Self::Other(Box::new(value)) - } -} - -impl From for VerboseError { - fn from(value: RegexError) -> Self { - Self::Other(Box::new(value)) + Self::EspluginError(SystemError::from(value)) } } @@ -187,22 +111,10 @@ impl From for PyErr { VerboseError::UndefinedGroupError(g) => { PyErr::new::((g, message)) } - VerboseError::EspluginError(i, s) => PyErr::new::((i, s)), + VerboseError::EspluginError(e) => { + PyErr::new::((e.code(), e.message().to_owned())) + } VerboseError::Other(_) => PyValueError::new_err(message), } } } - -fn display_cycle(cycle: &[libloot::Vertex]) -> String { - cycle - .iter() - .map(|v| { - if let Some(edge_type) = v.out_edge_type() { - format!("{} --[{}]-> ", v.name(), edge_type) - } else { - v.name().to_string() - } - }) - .chain(cycle.first().iter().map(|v| v.name().to_string())) - .collect() -} diff --git a/pyo3/src/game.rs b/pyo3/src/game.rs index 0bca69c2..484ca47d 100644 --- a/pyo3/src/game.rs +++ b/pyo3/src/game.rs @@ -1,12 +1,9 @@ use std::path::{Path, PathBuf}; +use libloot_ffi_errors::UnsupportedEnumValueError; use pyo3::{pyclass, pymethods}; -use crate::{ - database::Database, - error::{UnsupportedEnumValueError, VerboseError}, - plugin::Plugin, -}; +use crate::{database::Database, error::VerboseError, plugin::Plugin}; #[allow(non_camel_case_types)] #[pyclass(eq, frozen, hash, ord)] diff --git a/pyo3/src/metadata.rs b/pyo3/src/metadata.rs index 826d0227..5a55b9c2 100644 --- a/pyo3/src/metadata.rs +++ b/pyo3/src/metadata.rs @@ -1,11 +1,12 @@ use std::hash::{DefaultHasher, Hash, Hasher}; +use libloot_ffi_errors::UnsupportedEnumValueError; use pyo3::{ Bound, FromPyObject, PyResult, pyclass, pyfunction, pymethods, types::{PyAnyMethods, PyTypeMethods}, }; -use crate::error::{UnsupportedEnumValueError, VerboseError}; +use crate::error::VerboseError; pub const NONE_REPR: &str = "None";