From bd5eafb5e8dd6970b294f5782d4b20ca7461ddb3 Mon Sep 17 00:00:00 2001 From: Oliver Hamlet Date: Tue, 22 Apr 2025 23:46:47 +0100 Subject: [PATCH] Deny various rustc and clippy lints in CXX wrapper --- cxx/src/database.rs | 34 +++++----- cxx/src/error.rs | 30 +++++---- cxx/src/game.rs | 10 ++- cxx/src/lib.rs | 147 +++++++++++++++++++++++++++++++++++++++----- cxx/src/metadata.rs | 4 +- cxx/src/plugin.rs | 4 +- 6 files changed, 179 insertions(+), 50 deletions(-) diff --git a/cxx/src/database.rs b/cxx/src/database.rs index 3b5e5eb9..3b17f2b7 100644 --- a/cxx/src/database.rs +++ b/cxx/src/database.rs @@ -25,7 +25,7 @@ impl Database { pub fn load_masterlist(&self, path: &str) -> Result<(), VerboseError> { self.0 .write() - .map_err(|_| DatabaseLockPoisonError)? + .map_err(DatabaseLockPoisonError::from)? .load_masterlist(Path::new(path)) .map_err(Into::into) } @@ -37,7 +37,7 @@ impl Database { ) -> Result<(), VerboseError> { self.0 .write() - .map_err(|_| DatabaseLockPoisonError)? + .map_err(DatabaseLockPoisonError::from)? .load_masterlist_with_prelude(Path::new(masterlist_path), Path::new(prelude_path)) .map_err(Into::into) } @@ -45,7 +45,7 @@ impl Database { pub fn load_userlist(&self, path: &str) -> Result<(), VerboseError> { self.0 .write() - .map_err(|_| DatabaseLockPoisonError)? + .map_err(DatabaseLockPoisonError::from)? .load_userlist(Path::new(path)) .map_err(Into::into) } @@ -63,7 +63,7 @@ impl Database { self.0 .read() - .map_err(|_| DatabaseLockPoisonError)? + .map_err(DatabaseLockPoisonError::from)? .write_user_metadata(Path::new(output_path), write_mode) .map_err(Into::into) } @@ -81,7 +81,7 @@ impl Database { self.0 .read() - .map_err(|_| DatabaseLockPoisonError)? + .map_err(DatabaseLockPoisonError::from)? .write_minimal_list(Path::new(output_path), write_mode) .map_err(Into::into) } @@ -89,7 +89,7 @@ impl Database { pub fn evaluate(&self, condition: &str) -> Result { self.0 .read() - .map_err(|_| DatabaseLockPoisonError)? + .map_err(DatabaseLockPoisonError::from)? .evaluate(condition) .map_err(Into::into) } @@ -98,7 +98,7 @@ impl Database { Ok(self .0 .read() - .map_err(|_| DatabaseLockPoisonError)? + .map_err(DatabaseLockPoisonError::from)? .known_bash_tags()) } @@ -108,7 +108,7 @@ impl Database { ) -> Result, VerboseError> { self.0 .write() - .map_err(|_| DatabaseLockPoisonError)? + .map_err(DatabaseLockPoisonError::from)? .general_messages(evaluate_conditions) .map(|v| v.into_iter().map(Into::into).collect()) .map_err(Into::into) @@ -118,7 +118,7 @@ impl Database { Ok(self .0 .read() - .map_err(|_| DatabaseLockPoisonError)? + .map_err(DatabaseLockPoisonError::from)? .groups(include_user_metadata) .into_iter() .map(Into::into) @@ -130,7 +130,7 @@ impl Database { Ok(self .0 .read() - .map_err(|_| DatabaseLockPoisonError)? + .map_err(DatabaseLockPoisonError::from)? .user_groups() .iter() .cloned() @@ -143,7 +143,7 @@ impl Database { let groups = to_vec_of_unwrapped(groups); self.0 .write() - .map_err(|_| DatabaseLockPoisonError)? + .map_err(DatabaseLockPoisonError::from)? .set_user_groups(groups); Ok(()) } @@ -155,7 +155,7 @@ impl Database { ) -> Result, VerboseError> { self.0 .read() - .map_err(|_| DatabaseLockPoisonError)? + .map_err(DatabaseLockPoisonError::from)? .groups_path(from_group_name, to_group_name) .map(|v| v.into_iter().map(Into::into).collect()) .map_err(Into::into) @@ -169,7 +169,7 @@ impl Database { ) -> Result, VerboseError> { self.0 .read() - .map_err(|_| DatabaseLockPoisonError)? + .map_err(DatabaseLockPoisonError::from)? .plugin_metadata(plugin_name, include_user_metadata, evaluate_conditions) .map(|p| Box::new(p.map(Into::into).into())) .map_err(Into::into) @@ -182,7 +182,7 @@ impl Database { ) -> Result, VerboseError> { self.0 .read() - .map_err(|_| DatabaseLockPoisonError)? + .map_err(DatabaseLockPoisonError::from)? .plugin_user_metadata(plugin_name, evaluate_conditions) .map(|p| Box::new(p.map(Into::into).into())) .map_err(Into::into) @@ -194,7 +194,7 @@ impl Database { ) -> Result<(), VerboseError> { self.0 .write() - .map_err(|_| DatabaseLockPoisonError)? + .map_err(DatabaseLockPoisonError::from)? .set_plugin_user_metadata(plugin_metadata.into()); Ok(()) } @@ -202,7 +202,7 @@ impl Database { pub fn discard_plugin_user_metadata(&self, plugin: &str) -> Result<(), VerboseError> { self.0 .write() - .map_err(|_| DatabaseLockPoisonError)? + .map_err(DatabaseLockPoisonError::from)? .discard_plugin_user_metadata(plugin); Ok(()) } @@ -210,7 +210,7 @@ impl Database { pub fn discard_all_user_metadata(&self) -> Result<(), VerboseError> { self.0 .write() - .map_err(|_| DatabaseLockPoisonError)? + .map_err(DatabaseLockPoisonError::from)? .discard_all_user_metadata(); Ok(()) } diff --git a/cxx/src/error.rs b/cxx/src/error.rs index c9ab5196..c2fba11f 100644 --- a/cxx/src/error.rs +++ b/cxx/src/error.rs @@ -31,16 +31,16 @@ impl std::fmt::Display for VerboseError { Self::CyclicInteractionError(cycle) => { write!(f, "CyclicInteractionError: ")?; for vertex in cycle { - let name = vertex.name().replace("\\", "\\\\").replace("-", "\\-"); + let name = vertex.name().replace('\\', "\\\\").replace('-', "\\-"); match vertex.out_edge_type() { - Some(e) => write!(f, "{}--{}--", name, e)?, - None => write!(f, "{}", name)?, + Some(e) => write!(f, "{name}--{e}--")?, + None => write!(f, "{name}")?, } } Ok(()) } Self::UndefinedGroupError(group) => { - write!(f, "UndefinedGroupError: {}", group) + write!(f, "UndefinedGroupError: {group}",) } Self::SystemError(e) => { let prefix = match e.category() { @@ -51,8 +51,8 @@ impl std::fmt::Display for VerboseError { }; write!(f, "{}: {}: {}", prefix, e.code(), e.message()) } - Self::FileAccessError(s) => write!(f, "FileAccessError: {}", s), - Self::InvalidArgument(s) => write!(f, "InvalidArgument: {}", s), + Self::FileAccessError(s) => write!(f, "FileAccessError: {s}"), + Self::InvalidArgument(s) => write!(f, "InvalidArgument: {s}"), Self::Other(e) => fmt_error_chain(e.as_ref(), f), } } @@ -79,7 +79,9 @@ impl From for VerboseError { match value { LoadPluginsError::PluginDataError(e) => e.into(), LoadPluginsError::PluginValidationError(_) => Self::InvalidArgument(value.to_string()), - _ => Self::Other(Box::new(value)), + LoadPluginsError::DatabaseLockPoisoned | LoadPluginsError::IoError(_) | _ => { + Self::Other(Box::new(value)) + } } } } @@ -91,7 +93,11 @@ impl From for VerboseError { SortPluginsError::UndefinedGroup(g) => Self::UndefinedGroupError(g), SortPluginsError::CycleFound(cycle) => Self::CyclicInteractionError(cycle), SortPluginsError::PluginDataError(e) => e.into(), - _ => Self::Other(Box::new(value)), + SortPluginsError::DatabaseLockPoisoned + | SortPluginsError::PluginNotLoaded(_) + | SortPluginsError::CycleFoundInvolving(_) + | SortPluginsError::PathfindingError(_) + | _ => Self::Other(Box::new(value)), } } } @@ -100,7 +106,7 @@ impl From for VerboseError { fn from(value: LoadOrderStateError) -> Self { match value { LoadOrderStateError::LoadOrderError(e) => e.into(), - _ => Self::Other(Box::new(value)), + LoadOrderStateError::DatabaseLockPoisoned | _ => Self::Other(Box::new(value)), } } } @@ -134,7 +140,7 @@ impl From for VerboseError { match value { GroupsPathError::UndefinedGroup(g) => Self::UndefinedGroupError(g), GroupsPathError::CycleFound(cycle) => Self::CyclicInteractionError(cycle), - _ => Self::Other(Box::new(value)), + GroupsPathError::PathfindingError(_) => Self::Other(Box::new(value)), } } } @@ -143,7 +149,7 @@ impl From for VerboseError { fn from(value: MetadataRetrievalError) -> Self { match value { MetadataRetrievalError::ConditionEvaluationError(e) => e.into(), - _ => Self::Other(Box::new(value)), + MetadataRetrievalError::RegexError(_) => Self::Other(Box::new(value)), } } } @@ -154,7 +160,7 @@ impl From for VerboseError { } } -#[derive(Debug)] +#[derive(Clone, Copy, Debug)] pub struct EmptyOptionalError; impl std::fmt::Display for EmptyOptionalError { diff --git a/cxx/src/game.rs b/cxx/src/game.rs index 95c507c3..bf216edf 100644 --- a/cxx/src/game.rs +++ b/cxx/src/game.rs @@ -59,7 +59,7 @@ impl From for Game { } } -#[derive(Debug)] +#[derive(Clone, Copy, Debug)] pub struct NotValidUtf8; impl std::fmt::Display for NotValidUtf8 { @@ -97,7 +97,7 @@ pub fn new_game_with_local_path( fn path_to_string(path: &Path) -> Result { path.to_str() - .map(|s| s.to_owned()) + .map(str::to_owned) .ok_or(NotValidUtf8) .map_err(Into::into) } @@ -177,7 +177,11 @@ impl Game { } pub fn load_order(&self) -> Vec { - self.0.load_order().iter().map(|s| s.to_string()).collect() + self.0 + .load_order() + .iter() + .map(ToString::to_string) + .collect() } pub fn set_load_order(&mut self, load_order: &[&str]) -> Result<(), VerboseError> { diff --git a/cxx/src/lib.rs b/cxx/src/lib.rs index eb87bb3d..81a790fa 100644 --- a/cxx/src/lib.rs +++ b/cxx/src/lib.rs @@ -1,3 +1,103 @@ +// Deny some rustc lints that are allow-by-default. +#![deny( + ambiguous_negative_literals, + impl_trait_overcaptures, + let_underscore_drop, + missing_copy_implementations, + missing_debug_implementations, + non_ascii_idents, + redundant_imports, + redundant_lifetimes, + trivial_casts, + trivial_numeric_casts, + unit_bindings +)] +#![deny(clippy::pedantic)] +// Allow a few clippy pedantic lints. +#![allow(clippy::must_use_candidate)] +#![allow(clippy::missing_errors_doc)] +#![allow( + clippy::unnecessary_box_returns, + reason = "CXX requires many returns to be boxed" +)] +// Selectively deny clippy restriction lints. +#![deny( + clippy::as_conversions, + clippy::as_underscore, + clippy::assertions_on_result_states, + clippy::big_endian_bytes, + clippy::cfg_not_test, + clippy::clone_on_ref_ptr, + clippy::create_dir, + clippy::dbg_macro, + clippy::decimal_literal_representation, + clippy::default_numeric_fallback, + clippy::doc_include_without_cfg, + clippy::empty_drop, + clippy::error_impl_error, + clippy::exit, + clippy::exhaustive_enums, + clippy::expect_used, + clippy::filetype_is_file, + clippy::float_cmp_const, + clippy::fn_to_numeric_cast_any, + clippy::get_unwrap, + clippy::host_endian_bytes, + clippy::if_then_some_else_none, + clippy::indexing_slicing, + clippy::infinite_loop, + clippy::integer_division, + clippy::integer_division_remainder_used, + clippy::iter_over_hash_type, + clippy::let_underscore_must_use, + clippy::lossy_float_literal, + clippy::map_err_ignore, + clippy::map_with_unused_argument_over_ranges, + clippy::mem_forget, + clippy::missing_assert_message, + clippy::missing_asserts_for_indexing, + clippy::missing_asserts_for_indexing, + clippy::mixed_read_write_in_expression, + clippy::multiple_inherent_impl, + clippy::multiple_unsafe_ops_per_block, + clippy::mutex_atomic, + clippy::mutex_integer, + clippy::needless_raw_strings, + clippy::non_ascii_literal, + clippy::non_zero_suggestions, + clippy::panic, + clippy::panic_in_result_fn, + clippy::partial_pub_fields, + clippy::pathbuf_init_then_push, + clippy::precedence_bits, + clippy::print_stderr, + clippy::print_stdout, + clippy::rc_buffer, + clippy::rc_mutex, + clippy::redundant_type_annotations, + clippy::ref_patterns, + clippy::rest_pat_in_fully_bound_structs, + clippy::str_to_string, + clippy::string_lit_chars_any, + clippy::string_slice, + clippy::string_to_string, + clippy::suspicious_xor_used_as_pow, + clippy::tests_outside_test_module, + clippy::todo, + clippy::try_err, + clippy::undocumented_unsafe_blocks, + clippy::unimplemented, + clippy::unnecessary_safety_comment, + clippy::unneeded_field_pattern, + clippy::unreachable, + clippy::unused_result_ok, + clippy::unwrap_in_result, + clippy::unwrap_used, + clippy::use_debug, + clippy::verbose_file_reads, + clippy::wildcard_enum_match_arm +)] + mod database; mod error; mod game; @@ -35,11 +135,12 @@ impl OptionalRef { /// # Safety /// - /// This is safe as long as the pointer in the OptionalRef is still valid. + /// This is safe as long as the pointer in the `OptionalRef` is still valid. pub unsafe fn as_ref(&self) -> Result<&T, EmptyOptionalError> { if self.0.is_null() { Err(EmptyOptionalError) } else { + // SAFETY: This is safe as long as self.0 is still valid. unsafe { Ok(&*self.0) } } } @@ -99,7 +200,13 @@ impl TryFrom for libloot::LogLevel { } } -#[allow(clippy::needless_lifetimes)] +#[allow( + let_underscore_drop, + missing_debug_implementations, + clippy::multiple_unsafe_ops_per_block, + clippy::needless_lifetimes, + reason = "Required by CXX" +)] #[cxx::bridge(namespace = "loot::rust")] mod ffi { @@ -581,22 +688,33 @@ pub static LIBLOOT_VERSION_MINOR: c_uint = libloot::LIBLOOT_VERSION_MINOR; pub static LIBLOOT_VERSION_PATCH: c_uint = libloot::LIBLOOT_VERSION_PATCH; #[unsafe(no_mangle)] -pub static LIBLOOT_LOG_LEVEL_TRACE: c_uchar = libloot::LogLevel::Trace as u8; +pub static LIBLOOT_LOG_LEVEL_TRACE: c_uchar = 0; #[unsafe(no_mangle)] -pub static LIBLOOT_LOG_LEVEL_DEBUG: c_uchar = libloot::LogLevel::Debug as u8; +pub static LIBLOOT_LOG_LEVEL_DEBUG: c_uchar = 1; #[unsafe(no_mangle)] -pub static LIBLOOT_LOG_LEVEL_INFO: c_uchar = libloot::LogLevel::Info as u8; +pub static LIBLOOT_LOG_LEVEL_INFO: c_uchar = 2; #[unsafe(no_mangle)] -pub static LIBLOOT_LOG_LEVEL_WARNING: c_uchar = libloot::LogLevel::Warning as u8; +pub static LIBLOOT_LOG_LEVEL_WARNING: c_uchar = 3; #[unsafe(no_mangle)] -pub static LIBLOOT_LOG_LEVEL_ERROR: c_uchar = libloot::LogLevel::Error as u8; +pub static LIBLOOT_LOG_LEVEL_ERROR: c_uchar = 4; #[unsafe(no_mangle)] -pub static LIBLOOT_LOG_LEVEL_FATAL: c_uchar = libloot::LogLevel::Fatal as u8; +pub static LIBLOOT_LOG_LEVEL_FATAL: c_uchar = 5; + +fn to_u8(value: libloot::LogLevel) -> u8 { + match value { + libloot::LogLevel::Trace => LIBLOOT_LOG_LEVEL_TRACE, + libloot::LogLevel::Debug => LIBLOOT_LOG_LEVEL_DEBUG, + libloot::LogLevel::Info => LIBLOOT_LOG_LEVEL_INFO, + libloot::LogLevel::Warning => LIBLOOT_LOG_LEVEL_WARNING, + libloot::LogLevel::Error => LIBLOOT_LOG_LEVEL_ERROR, + libloot::LogLevel::Fatal => LIBLOOT_LOG_LEVEL_FATAL, + } +} #[unsafe(no_mangle)] unsafe extern "C" fn libloot_set_logging_callback( @@ -606,9 +724,8 @@ unsafe extern "C" fn libloot_set_logging_callback( let mutex = Mutex::new(AtomicPtr::new(context)); set_logging_callback(move |level, message| { - let (level, c_string) = match CString::new(message) { - Ok(c) => (level, c), - Err(_) => { + let (level, c_string) = CString::new(message).map_or_else( + |_| { let c_string = CString::new(format!( "Attempted to log a message containing a null byte: {}", message.replace('\0', "\\0") @@ -617,8 +734,9 @@ unsafe extern "C" fn libloot_set_logging_callback( CString::from(c"Attempted to log a message containing a null byte") }); (libloot::LogLevel::Error, c_string) - } - }; + }, + |c| (level, c), + ); let mut context = match mutex.lock() { Ok(c) => c, @@ -629,8 +747,9 @@ unsafe extern "C" fn libloot_set_logging_callback( } }; + // SAFETY: This is safe so long as callback remains a valid function pointer. unsafe { - callback(level as u8, c_string.as_ptr(), *context.get_mut()); + callback(to_u8(level), c_string.as_ptr(), *context.get_mut()); } }); } diff --git a/cxx/src/metadata.rs b/cxx/src/metadata.rs index 7eff0ebb..4501d3ac 100644 --- a/cxx/src/metadata.rs +++ b/cxx/src/metadata.rs @@ -16,10 +16,10 @@ unsafe trait TransparentWrapper { where Self: Sized, { - let v = value as *const Self::Wrapped; + let v: *const Self::Wrapped = value; // SAFETY: Reinterpreting the pointer of a transparent wrapper to the type it wraps is safe. unsafe { - let v = v as *const Self; + let v: *const Self = v.cast(); &*v } } diff --git a/cxx/src/plugin.rs b/cxx/src/plugin.rs index 54365f84..a0ddd296 100644 --- a/cxx/src/plugin.rs +++ b/cxx/src/plugin.rs @@ -26,7 +26,7 @@ impl Plugin { } pub fn crc(&self) -> i64 { - self.0.crc().map(Into::into).unwrap_or(-1) + self.0.crc().map_or(-1, Into::into) } pub fn is_valid_as_light_plugin(&self) -> Result { @@ -46,7 +46,7 @@ impl Plugin { } pub fn boxed_clone(&self) -> Box { - Box::new(Self(self.0.clone())) + Box::new(Self(Arc::clone(&self.0))) } delegate! {