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

global_properties are silently immutable #71

Open
ekr-cfa opened this issue Nov 3, 2024 · 1 comment
Open

global_properties are silently immutable #71

ekr-cfa opened this issue Nov 3, 2024 · 1 comment

Comments

@ekr-cfa
Copy link
Collaborator

ekr-cfa commented Nov 3, 2024

If you try to set the same global property twice, it succeeds but only the first value is used:

    #[test]
    fn set_get_global_property_twice() {
        let params: ParamType = ParamType {
            days: 10,
            diseases: 2,
        };
        let mut context = Context::new();
        context.set_global_property_value(DiseaseParams, params.clone());
        let global_params = context.get_global_property_value(DiseaseParams).clone();
        assert_eq!(global_params.days, params.days);
        assert_eq!(global_params.diseases, params.diseases);
        let params2: ParamType = ParamType {
            days: 11,
            diseases: 3,
        };
        context.set_global_property_value(DiseaseParams, params2.clone());
        assert_eq!(global_params.days, params2.days);
        assert_eq!(global_params.diseases, params2.diseases);
        
    }

Output:

thread 'global_properties::test::set_get_global_property_twice' panicked at src/global_properties.rs:218:9:
assertion `left == right` failed
  left: 10
 right: 11

The source of the problem is:

    fn set_global_property_value<T: GlobalProperty + 'static>(
        &mut self,
        _property: &T,
        value: T::Value,
    ) {
        let _data_container = self
            .global_property_container
            .entry(TypeId::of::<T>())
            .or_insert_with(|| Box::new(value));
    }

This should either be .insert() which would make properties mutable, or .try_insert()? and then return a Result<T>.

But it's bad to have the API appear to succeed but not do anything.

@ekr-cfa
Copy link
Collaborator Author

ekr-cfa commented Nov 3, 2024

ekr-cfa added a commit that referenced this issue Nov 5, 2024
…#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.
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

No branches or pull requests

1 participant