From d0478c05ec3b103e9cbaa791f4c3d4e5aca8dacb Mon Sep 17 00:00:00 2001 From: Eric Rescorla <141454109+ekr-cfa@users.noreply.github.com> Date: Sun, 1 Dec 2024 16:51:21 -0800 Subject: [PATCH] Return error when you try to change a global property. Fixes #71. (#74) * Change |set_global_property_value()| to return Result. Fixes #71. This keeps the semantics that global property values are immutable but returns an error when you try to change one. We might still want to change this semantic, but at least this prevents silent failure. --- examples/births-deaths/infection_manager.rs | 4 +- examples/births-deaths/parameters_loader.rs | 8 ++- examples/births-deaths/population_manager.rs | 8 ++- .../parameter-loading/infection_manager.rs | 4 +- .../parameter-loading/parameters_loader.rs | 2 +- .../parameter-loading/transmission_manager.rs | 4 +- .../exposure_manager.rs | 8 ++- .../infection_manager.rs | 12 +++- .../parameters_loader.rs | 2 +- src/error.rs | 4 +- src/global_properties.rs | 57 ++++++++++++++----- 11 files changed, 83 insertions(+), 30 deletions(-) diff --git a/examples/births-deaths/infection_manager.rs b/examples/births-deaths/infection_manager.rs index c0a49d8a..4657fe82 100644 --- a/examples/births-deaths/infection_manager.rs +++ b/examples/births-deaths/infection_manager.rs @@ -127,7 +127,9 @@ mod test { let mut context = Context::new(); - context.set_global_property_value(Parameters, p_values.clone()); + context + .set_global_property_value(Parameters, p_values.clone()) + .unwrap(); context.init_random(p_values.seed); init(&mut context); diff --git a/examples/births-deaths/parameters_loader.rs b/examples/births-deaths/parameters_loader.rs index 2dbee8f2..322fb5f9 100644 --- a/examples/births-deaths/parameters_loader.rs +++ b/examples/births-deaths/parameters_loader.rs @@ -32,7 +32,9 @@ define_global_property!(Foi, HashMap); pub fn init_parameters(context: &mut Context, file_path: &Path) -> Result<(), IxaError> { let parameters_json = context.load_parameters_from_json::(file_path)?; - context.set_global_property_value(Parameters, parameters_json.clone()); + context + .set_global_property_value(Parameters, parameters_json.clone()) + .unwrap(); let foi_map = parameters_json .foi_groups @@ -41,7 +43,9 @@ pub fn init_parameters(context: &mut Context, file_path: &Path) -> Result<(), Ix .map(|x| (x.group_name, x.foi)) .collect::>(); - context.set_global_property_value(Foi, foi_map.clone()); + context + .set_global_property_value(Foi, foi_map.clone()) + .unwrap(); Ok(()) } diff --git a/examples/births-deaths/population_manager.rs b/examples/births-deaths/population_manager.rs index 16a562aa..6ffbb832 100644 --- a/examples/births-deaths/population_manager.rs +++ b/examples/births-deaths/population_manager.rs @@ -278,7 +278,9 @@ mod test { }; let mut context = Context::new(); - context.set_global_property_value(Parameters, p_values.clone()); + context + .set_global_property_value(Parameters, p_values.clone()) + .unwrap(); context.init_random(p_values.seed); schedule_birth(&mut context); @@ -300,7 +302,9 @@ mod test { }; let mut context = Context::new(); - context.set_global_property_value(Parameters, p_values.clone()); + context + .set_global_property_value(Parameters, p_values.clone()) + .unwrap(); context.init_random(p_values.seed); let _person = context.create_new_person(0); schedule_death(&mut context); diff --git a/examples/parameter-loading/infection_manager.rs b/examples/parameter-loading/infection_manager.rs index 1cc2f7ed..0a70600f 100644 --- a/examples/parameter-loading/infection_manager.rs +++ b/examples/parameter-loading/infection_manager.rs @@ -66,7 +66,9 @@ mod test { }; let mut context = Context::new(); - context.set_global_property_value(Parameters, p_values.clone()); + context + .set_global_property_value(Parameters, p_values.clone()) + .unwrap(); context.init_random(42); init(&mut context); diff --git a/examples/parameter-loading/parameters_loader.rs b/examples/parameter-loading/parameters_loader.rs index aeee1127..551d2778 100644 --- a/examples/parameter-loading/parameters_loader.rs +++ b/examples/parameter-loading/parameters_loader.rs @@ -21,6 +21,6 @@ define_global_property!(Parameters, ParametersValues); pub fn init_parameters(context: &mut Context, file_path: &Path) -> Result<(), IxaError> { let parameters_json = context.load_parameters_from_json::(file_path)?; - context.set_global_property_value(Parameters, parameters_json); + context.set_global_property_value(Parameters, parameters_json)?; Ok(()) } diff --git a/examples/parameter-loading/transmission_manager.rs b/examples/parameter-loading/transmission_manager.rs index c5ff3c9e..fc994ca7 100644 --- a/examples/parameter-loading/transmission_manager.rs +++ b/examples/parameter-loading/transmission_manager.rs @@ -70,7 +70,9 @@ mod test { }; let mut context = Context::new(); - context.set_global_property_value(Parameters, p_values); + context + .set_global_property_value(Parameters, p_values) + .unwrap(); context.init_random(123); let pid = context.add_person(()).unwrap(); attempt_infection(&mut context); diff --git a/examples/time-varying-infection/exposure_manager.rs b/examples/time-varying-infection/exposure_manager.rs index 6ac786b5..478bdb5e 100644 --- a/examples/time-varying-infection/exposure_manager.rs +++ b/examples/time-varying-infection/exposure_manager.rs @@ -94,7 +94,9 @@ mod test { output_file: ".".to_string(), }; let mut context = Context::new(); - context.set_global_property_value(Parameters, p_values); + context + .set_global_property_value(Parameters, p_values) + .unwrap(); let parameters = context .get_global_property_value(Parameters) .unwrap() @@ -132,7 +134,9 @@ mod test { output_file: ".".to_string(), }; let mut context = Context::new(); - context.set_global_property_value(Parameters, p_values); + context + .set_global_property_value(Parameters, p_values) + .unwrap(); let parameters = context .get_global_property_value(Parameters) .unwrap() diff --git a/examples/time-varying-infection/infection_manager.rs b/examples/time-varying-infection/infection_manager.rs index e87fcd2b..74b37ee9 100644 --- a/examples/time-varying-infection/infection_manager.rs +++ b/examples/time-varying-infection/infection_manager.rs @@ -120,7 +120,9 @@ mod test { }; let mut context = Context::new(); - context.set_global_property_value(Parameters, p_values); + context + .set_global_property_value(Parameters, p_values) + .unwrap(); let parameters = context .get_global_property_value(Parameters) .unwrap() @@ -161,7 +163,9 @@ mod test { output_dir: ".".to_string(), output_file: ".".to_string(), }; - context.set_global_property_value(Parameters, parameters.clone()); + context + .set_global_property_value(Parameters, parameters.clone()) + .unwrap(); context.init_random(parameters.seed); for _ in 0..parameters.population { let person_id = context.add_person(()).unwrap(); @@ -211,7 +215,9 @@ mod test { let mut sum = 0.0; for seed in 0..n_iter { let mut context = Context::new(); - context.set_global_property_value(Parameters, parameters.clone()); + context + .set_global_property_value(Parameters, parameters.clone()) + .unwrap(); context.init_random(seed); init(&mut context); let person_id = context diff --git a/examples/time-varying-infection/parameters_loader.rs b/examples/time-varying-infection/parameters_loader.rs index d6871df1..b12f58dd 100644 --- a/examples/time-varying-infection/parameters_loader.rs +++ b/examples/time-varying-infection/parameters_loader.rs @@ -23,6 +23,6 @@ define_global_property!(Parameters, ParametersValues); pub fn init_parameters(context: &mut Context, file_path: &Path) -> Result<(), IxaError> { let parameters_json = context.load_parameters_from_json::(file_path)?; - context.set_global_property_value(Parameters, parameters_json); + context.set_global_property_value(Parameters, parameters_json)?; Ok(()) } diff --git a/src/error.rs b/src/error.rs index e300721a..06be26e2 100644 --- a/src/error.rs +++ b/src/error.rs @@ -4,8 +4,8 @@ use std::io; #[derive(Debug)] #[allow(clippy::module_name_repetitions)] -/// A wrapper around other error types, as well -/// as our specific error type. +/// Provides `IxaError` and maps to other errors to +/// convert to an `IxaError` pub enum IxaError { IoError(io::Error), JsonError(serde_json::Error), diff --git a/src/global_properties.rs b/src/global_properties.rs index 6148d02a..68958072 100644 --- a/src/global_properties.rs +++ b/src/global_properties.rs @@ -20,7 +20,7 @@ use crate::error::IxaError; use serde::de::DeserializeOwned; use std::any::{Any, TypeId}; use std::cell::RefCell; -use std::collections::HashMap; +use std::collections::{hash_map::Entry, HashMap}; use std::fmt::Debug; use std::fs; use std::io::BufReader; @@ -58,7 +58,7 @@ where if context.get_global_property_value(T::new()).is_some() { return Err(IxaError::IxaError(format!("Duplicate property {name}"))); } - context.set_global_property_value(T::new(), val); + let _ = context.set_global_property_value(T::new(), val); Ok(()) }, ), @@ -142,11 +142,14 @@ crate::context::define_data_plugin!( pub trait ContextGlobalPropertiesExt { /// Set the value of a global property of type T + /// + /// # Errors + /// Will return an error if an attempt is made to change a value. fn set_global_property_value( &mut self, property: T, value: T::Value, - ); + ) -> Result<(), IxaError>; /// Return value of global property T fn get_global_property_value( @@ -195,11 +198,14 @@ impl GlobalPropertiesDataContainer { &mut self, _property: &T, value: T::Value, - ) { - let _data_container = self - .global_property_container - .entry(TypeId::of::()) - .or_insert_with(|| Box::new(value)); + ) -> Result<(), IxaError> { + match self.global_property_container.entry(TypeId::of::()) { + Entry::Vacant(entry) => { + entry.insert(Box::new(value)); + Ok(()) + } + Entry::Occupied(_) => Err(IxaError::from("Entry already exists")), + } } #[must_use] @@ -218,9 +224,9 @@ impl ContextGlobalPropertiesExt for Context { &mut self, property: T, value: T::Value, - ) { + ) -> Result<(), IxaError> { let data_container = self.get_data_container_mut(GlobalPropertiesPlugin); - data_container.set_global_property_value(&property, value); + data_container.set_global_property_value(&property, value) } #[allow(unused_variables)] @@ -276,16 +282,37 @@ mod test { } define_global_property!(DiseaseParams, ParamType); - //Since global properties aren't mutable right now, only - // check that they are properly set + #[test] fn set_get_global_property() { let params: ParamType = ParamType { days: 10, diseases: 2, }; + let params2: ParamType = ParamType { + days: 11, + diseases: 3, + }; + let mut context = Context::new(); - context.set_global_property_value(DiseaseParams, params.clone()); + + // Set and check the stored value. + context + .set_global_property_value(DiseaseParams, params.clone()) + .unwrap(); + let global_params = context + .get_global_property_value(DiseaseParams) + .unwrap() + .clone(); + assert_eq!(global_params.days, params.days); + assert_eq!(global_params.diseases, params.diseases); + + // Setting again should fail because global properties are immutable. + assert!(context + .set_global_property_value(DiseaseParams, params2.clone()) + .is_err()); + + // Check that the value is unchanged. let global_params = context .get_global_property_value(DiseaseParams) .unwrap() @@ -322,7 +349,9 @@ mod test { .load_parameters_from_json::(&file_path) .unwrap(); - context.set_global_property_value(Parameters, params_json); + context + .set_global_property_value(Parameters, params_json) + .unwrap(); let params_read = context .get_global_property_value(Parameters)