ensure first-writer-wins consistently

Author: Ariel Ben-Yehuda

library/core/src/error/provide.rs

@@ -1225,7 +1225,17 @@ impl<'a> Tagged<dyn Erased<'a> + 'a> {
         if self.is_virtual() {
             // consume returns None if the space is not satisfied
             // SAFETY: `&raw const self.value` is valid
-            unsafe { (&raw const self.value).consume(TypeId::of::<I>()).is_some() }
+            // SAFETY: consume is defined to return either None or Some(I::Reified)
+            unsafe {
+                if let Some(res) = (&raw const self.value).consume(TypeId::of::<I>()) {
+                    let ptr: NonNull<Option<I::Reified>> = res.cast();
+                    // cast is fine since consume returns a pointer to an Option<I::Reified>
+                    // value can be satisfied if the cell is empty
+                    ptr.as_ref().is_none()
+                } else {
+                    false
+                }
+            }
         } else {
             matches!(self.downcast::<I>(), Some(TaggedOption(None)))
         }
@@ -1242,8 +1252,10 @@ impl<'a> Tagged<dyn Erased<'a> + 'a> {
                 if let Some(res) = (&raw const self.value).consume(TypeId::of::<I>()) {
                     let mut ptr: NonNull<Option<I::Reified>> = res.cast();
                     // cast is fine since consume_mut returns a pointer to an Option<I::Reified>
-                    // could use `ptr::write` here, but this is not expected to be important enough
-                    *ptr.as_mut() = Some(value);
+                    let opt: &mut Option<I::Reified> = ptr.as_mut();
+                    if opt.is_none() {
+                        *opt = Some(value);
+                    }
                 }
             }
         } else {
@@ -1259,13 +1271,15 @@ impl<'a> Tagged<dyn Erased<'a> + 'a> {
         I: tags::Type<'a>,
     {
         if self.is_virtual() {
-            // SAFETY: consume_mut is defined to return either None or Some(I::Reified)
+            // SAFETY: consume is defined to return either None or Some(I::Reified)
             unsafe {
                 if let Some(res) = (&raw const self.value).consume(TypeId::of::<I>()) {
                     let mut ptr: NonNull<Option<I::Reified>> = res.cast();
-                    // cast is fine since consume_mut returns a pointer to an Option<I::Reified>
-                    // could use `ptr::write` here, but this is not expected to be important enough
-                    *ptr.as_mut() = Some(fulfil());
+                    // cast is fine since consume returns a pointer to an Option<I::Reified>
+                    let opt: &mut Option<I::Reified> = ptr.as_mut();
+                    if opt.is_none() {
+                        *opt = Some(fulfil());
+                    }
                 }
             }
         } else {

library/coretests/tests/error.rs

@@ -1,6 +1,10 @@
 use core::error::provide::{MultiRequestBuilder, MultiResponse, Request};
 use core::error::{Error, request_ref, request_value};
 
+struct Invalid;
+#[derive(Debug, PartialEq, Eq)]
+struct Valid;
+
 // Test the `Request` API.
 #[derive(Debug)]
 struct SomeConcreteType {
@@ -13,10 +17,6 @@ impl std::fmt::Display for SomeConcreteType {
     }
 }
 
-struct Invalid;
-#[derive(Debug, PartialEq, Eq)]
-struct Valid;
-
 impl std::error::Error for SomeConcreteType {
     fn provide<'a>(&'a self, request: &mut Request<'a>) {
         request
@@ -128,6 +128,54 @@ fn test_error_combined_access_dyn() {
     assert_eq!(request.retrieve_value::<*const ()>(), None);
 }
 
+#[derive(Debug)]
+struct ProvideMultipleTimes;
+
+impl std::fmt::Display for ProvideMultipleTimes {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "A")
+    }
+}
+
+impl std::error::Error for ProvideMultipleTimes {
+    fn provide<'a>(&'a self, request: &mut Request<'a>) {
+        let previous_satisfied_by_ref_of_string = request.would_be_satisfied_by_ref_of::<String>();
+        request.provide_value::<String>("good".to_string());
+        assert!(
+            !request.would_be_satisfied_by_value_of::<String>(),
+            "already provided String by value"
+        );
+        // "overwriting" the String should just be ignored
+        request.provide_value::<String>("bad".to_string());
+        // but providing a ref works
+        if previous_satisfied_by_ref_of_string {
+            assert!(request.would_be_satisfied_by_ref_of::<String>());
+            request.provide_ref::<String>(const { &String::new() });
+        }
+    }
+}
+
+#[test]
+fn test_provide_multiple_times_single() {
+    let obj = ProvideMultipleTimes;
+    let obj: &dyn Error = &obj;
+
+    assert_eq!(*request_ref::<String>(&*obj).unwrap(), String::new());
+    assert_eq!(request_value::<String>(&*obj).unwrap(), "good");
+}
+
+#[test]
+fn test_provide_multiple_times_multi() {
+    let obj = ProvideMultipleTimes;
+    let obj: &dyn Error = &obj;
+
+    let mut request =
+        MultiRequestBuilder::new().with_value::<String>().with_ref::<String>().request(obj);
+
+    assert_eq!(*request.retrieve_ref::<String>().unwrap(), String::new());
+    assert_eq!(request.retrieve_value::<String>().unwrap(), "good");
+}
+
 fn assert_copy<T: Copy>(_t: T) {}
 
 #[test]