rune: Get rid of more formatting-related unsafety

crates/rune/src/modules/option.rs

@@ -1,7 +1,5 @@
 //! The [`Option`] type.
 
-use core::ptr::NonNull;
-
 use crate as rune;
 use crate::alloc::String;
 use crate::runtime::{
@@ -47,21 +45,20 @@ pub fn module() -> Result<Module, ContextError> {
         )),
     })?;
 
-    // Sorted for ease of finding
     m.function_meta(expect)?;
-    m.function_meta(unwrap)?;
-    m.function_meta(unwrap_or)?;
-    m.function_meta(unwrap_or_else)?;
     m.function_meta(is_some)?;
     m.function_meta(is_none)?;
     m.function_meta(iter)?;
+    m.function_meta(into_iter)?;
     m.function_meta(and_then)?;
     m.function_meta(map)?;
     m.function_meta(take)?;
     m.function_meta(transpose)?;
+    m.function_meta(unwrap)?;
+    m.function_meta(unwrap_or)?;
+    m.function_meta(unwrap_or_else)?;
     m.function_meta(ok_or)?;
     m.function_meta(ok_or_else)?;
-    m.function_meta(into_iter)?;
     m.function_meta(option_try__meta)?;
 
     m.ty::<Iter>()?;
@@ -113,14 +110,12 @@ pub fn module() -> Result<Module, ContextError> {
 /// Styles"](../../std/error/index.html#common-message-styles) in the
 /// [`std::error`](../../std/error/index.html) module docs.
 #[rune::function(instance)]
-fn expect(option: Option<Value>, message: Value) -> VmResult<Value> {
+fn expect(option: &Option<Value>, message: Value) -> VmResult<Value> {
     match option {
-        Some(some) => VmResult::Ok(some),
+        Some(some) => VmResult::Ok(some.clone()),
         None => {
             let mut s = String::new();
-            // SAFETY: Formatter does not outlive the string it references.
-            let mut f = unsafe { Formatter::new(NonNull::from(&mut s)) };
-            vm_try!(message.display_fmt(&mut f));
+            vm_try!(Formatter::format_with(&mut s, |f| message.display_fmt(f)));
             VmResult::err(Panic::custom(s))
         }
     }
@@ -175,8 +170,8 @@ fn is_none(this: &Option<Value>) -> bool {
 /// assert_eq!(None, it.next());
 /// ```
 #[rune::function(instance)]
-fn iter(value: Option<Value>) -> Iter {
-    Iter { value }
+fn iter(value: &Option<Value>) -> Iter {
+    Iter::new(value.clone())
 }
 
 /// Construct an iterator over an optional value.
@@ -195,8 +190,8 @@ fn iter(value: Option<Value>) -> Iter {
 /// assert_eq!(out, [1]);
 /// ```
 #[rune::function(instance, protocol = INTO_ITER)]
-fn into_iter(this: Option<Value>) -> Iter {
-    Iter::new(this)
+fn into_iter(this: &Option<Value>) -> Iter {
+    Iter::new(this.clone())
 }
 
 /// Returns [`None`] if the option is [`None`], otherwise calls `f` with the
@@ -228,10 +223,10 @@ fn into_iter(this: Option<Value>) -> Iter {
 /// assert_eq!(item_2_0, None);
 /// ```
 #[rune::function(instance)]
-fn and_then(option: Option<Value>, then: Function) -> VmResult<Option<Value>> {
+fn and_then(option: &Option<Value>, then: Function) -> VmResult<Option<Value>> {
     match option {
         // no need to clone v, passing the same reference forward
-        Some(v) => VmResult::Ok(vm_try!(then.call((v,)))),
+        Some(v) => VmResult::Ok(vm_try!(then.call((v.clone(),)))),
         None => VmResult::Ok(None),
     }
 }
@@ -256,10 +251,10 @@ fn and_then(option: Option<Value>, then: Function) -> VmResult<Option<Value>> {
 /// assert_eq!(x.map(|s| s.len()), None);
 /// ```
 #[rune::function(instance)]
-fn map(option: Option<Value>, then: Function) -> VmResult<Option<Value>> {
+fn map(option: &Option<Value>, then: Function) -> VmResult<Option<Value>> {
     match option {
         // no need to clone v, passing the same reference forward
-        Some(v) => VmResult::Ok(Some(vm_try!(then.call((v,))))),
+        Some(v) => VmResult::Ok(Some(vm_try!(then.call((v.clone(),))))),
         None => VmResult::Ok(None),
     }
 }
@@ -298,7 +293,7 @@ fn take(option: &mut Option<Value>) -> Option<Value> {
 /// assert_eq!(x, y.transpose());
 /// ```
 #[rune::function(instance)]
-fn transpose(this: Option<Value>) -> VmResult<Value> {
+fn transpose(this: &Option<Value>) -> VmResult<Value> {
     let value = match this {
         Some(value) => value,
         None => {
@@ -308,7 +303,7 @@ fn transpose(this: Option<Value>) -> VmResult<Value> {
         }
     };
 
-    match &*vm_try!(value.into_result_ref()) {
+    match &*vm_try!(value.borrow_result_ref()) {
         Ok(ok) => {
             let some = vm_try!(Value::try_from(Some(ok.clone())));
             let result = vm_try!(Value::try_from(Ok(some)));
@@ -347,9 +342,9 @@ fn transpose(this: Option<Value>) -> VmResult<Value> {
 /// assert_eq!(x.unwrap(), "air"); // fails
 /// ```
 #[rune::function(instance)]
-fn unwrap(option: Option<Value>) -> VmResult<Value> {
+fn unwrap(option: &Option<Value>) -> VmResult<Value> {
     match option {
-        Some(some) => VmResult::Ok(some),
+        Some(some) => VmResult::Ok(some.clone()),
         None => VmResult::err(Panic::custom("Called `Option::unwrap()` on a `None` value")),
     }
 }
@@ -369,8 +364,11 @@ fn unwrap(option: Option<Value>) -> VmResult<Value> {
 /// assert_eq!(None.unwrap_or("bike"), "bike");
 /// ```
 #[rune::function(instance)]
-fn unwrap_or(this: Option<Value>, default: Value) -> Value {
-    this.unwrap_or(default)
+fn unwrap_or(this: &Option<Value>, default: Value) -> Value {
+    match this {
+        Some(value) => value.clone(),
+        None => default,
+    }
 }
 
 /// Returns the contained [`Some`] value or computes it from a closure.
@@ -383,9 +381,9 @@ fn unwrap_or(this: Option<Value>, default: Value) -> Value {
 /// assert_eq!(None.unwrap_or_else(|| 2 * k), 20);
 /// ```
 #[rune::function(instance)]
-fn unwrap_or_else(this: Option<Value>, default: Function) -> VmResult<Value> {
+fn unwrap_or_else(this: &Option<Value>, default: Function) -> VmResult<Value> {
     match this {
-        Some(value) => VmResult::Ok(value),
+        Some(value) => VmResult::Ok(value.clone()),
         None => default.call(()),
     }
 }
@@ -412,8 +410,11 @@ fn unwrap_or_else(this: Option<Value>, default: Function) -> VmResult<Value> {
 /// assert_eq!(x.ok_or(0), Err(0));
 /// ```
 #[rune::function(instance)]
-fn ok_or(this: Option<Value>, err: Value) -> Result<Value, Value> {
-    this.ok_or(err)
+fn ok_or(this: &Option<Value>, err: Value) -> Result<Value, Value> {
+    match this {
+        Some(value) => Ok(value.clone()),
+        None => Err(err),
+    }
 }
 
 /// Transforms the `Option<T>` into a [`Result<T, E>`], mapping [`Some(v)`] to
@@ -433,9 +434,9 @@ fn ok_or(this: Option<Value>, err: Value) -> Result<Value, Value> {
 /// assert_eq!(x.ok_or_else(|| 0), Err(0));
 /// ```
 #[rune::function(instance)]
-fn ok_or_else(this: Option<Value>, err: Function) -> VmResult<Result<Value, Value>> {
+fn ok_or_else(this: &Option<Value>, err: Function) -> VmResult<Result<Value, Value>> {
     match this {
-        Some(value) => VmResult::Ok(Ok(value)),
+        Some(value) => VmResult::Ok(Ok(value.clone())),
         None => VmResult::Ok(Err(vm_try!(err.call(())))),
     }
 }

crates/rune/src/modules/result.rs

@@ -1,7 +1,5 @@
 //! The [`Result`] type.
 
-use core::ptr::NonNull;
-
 use crate as rune;
 use crate::alloc::fmt::TryWrite;
 use crate::alloc::prelude::*;
@@ -130,16 +128,17 @@ fn is_err(result: &Result<Value, Value>) -> bool {
 /// x.unwrap(); // panics with `emergency failure`
 /// ```
 #[rune::function(instance)]
-fn unwrap(result: Result<Value, Value>) -> VmResult<Value> {
+fn unwrap(result: &Result<Value, Value>) -> VmResult<Value> {
     match result {
-        Ok(value) => VmResult::Ok(value),
+        Ok(value) => VmResult::Ok(value.clone()),
         Err(err) => {
-            let message = vm_try!(format_args!(
-                "Called `Result::unwrap()` on an `Err` value: {:?}",
-                err
-            )
-            .try_to_string());
-            VmResult::err(Panic::custom(message))
+            let mut m = String::new();
+            vm_try!(vm_write!(
+                m,
+                "Called `Result::unwrap()` on an `Err` value: "
+            ));
+            vm_try!(Formatter::format_with(&mut m, |f| err.debug_fmt(f)));
+            VmResult::err(Panic::custom(m))
         }
     }
 }
@@ -163,8 +162,11 @@ fn unwrap(result: Result<Value, Value>) -> VmResult<Value> {
 /// assert_eq!(x.unwrap_or(default_value), default_value);
 /// ```
 #[rune::function(instance)]
-fn unwrap_or(this: Result<Value, Value>, default: Value) -> Value {
-    this.unwrap_or(default)
+fn unwrap_or(this: &Result<Value, Value>, default: Value) -> Value {
+    match this {
+        Ok(value) => value.clone(),
+        Err(_) => default.clone(),
+    }
 }
 
 /// Returns the contained [`Ok`] value or computes it from a closure.
@@ -181,9 +183,9 @@ fn unwrap_or(this: Result<Value, Value>, default: Value) -> Value {
 /// assert_eq!(Err("foo").unwrap_or_else(count), 3);
 /// ```
 #[rune::function(instance)]
-fn unwrap_or_else(this: Result<Value, Value>, default: Function) -> VmResult<Value> {
+fn unwrap_or_else(this: &Result<Value, Value>, default: Function) -> VmResult<Value> {
     match this {
-        Ok(value) => VmResult::Ok(value),
+        Ok(value) => VmResult::Ok(value.clone()),
         Err(error) => default.call((error,)),
     }
 }
@@ -218,16 +220,17 @@ fn unwrap_or_else(this: Result<Value, Value>, default: Function) -> VmResult<Val
 /// as in "env variable should be set by blah" or "the given binary should be
 /// available and executable by the current user".
 #[rune::function(instance)]
-fn expect(result: Result<Value, Value>, message: Value) -> VmResult<Value> {
+fn expect(result: &Result<Value, Value>, message: Value) -> VmResult<Value> {
     match result {
-        Ok(value) => VmResult::Ok(value),
+        Ok(value) => VmResult::Ok(value.clone()),
         Err(err) => {
             let mut s = String::new();
-            // SAFETY: Formatter does not outlive the string it references.
-            let mut f = unsafe { Formatter::new(NonNull::from(&mut s)) };
-            vm_try!(message.display_fmt(&mut f));
-            vm_try!(f.try_write_str(": "));
-            vm_try!(err.debug_fmt(&mut f));
+            vm_try!(Formatter::format_with(&mut s, |f| {
+                vm_try!(message.debug_fmt(f));
+                vm_try!(f.try_write_str(": "));
+                vm_try!(err.debug_fmt(f));
+                VmResult::Ok(())
+            }));
             VmResult::err(Panic::custom(s))
         }
     }

crates/rune/src/runtime/fmt.rs

@@ -2,6 +2,7 @@ use core::ptr::NonNull;
 
 use crate::alloc::fmt::TryWrite;
 use crate::alloc::{self, String};
+use crate::runtime::VmResult;
 use crate::Any;
 
 /// A formatter for the rune virtual machine.
@@ -19,13 +20,18 @@ pub struct Formatter {
 }
 
 impl Formatter {
-    /// Construct a new formatter wrapping a [`TryWrite`].
-    #[inline]
-    pub(crate) unsafe fn new(out: NonNull<dyn TryWrite>) -> Self {
-        Self {
-            out,
+    /// Format onto the given trywrite.
+    pub(crate) fn format_with(
+        out: &mut String,
+        f: impl FnOnce(&mut Self) -> VmResult<()>,
+    ) -> VmResult<()> {
+        // SAFETY: Call to this function ensures that the formatter does not
+        // outlive the passed in reference.
+        let mut fmt = Formatter {
+            out: NonNull::from(out),
             buf: String::new(),
-        }
+        };
+        f(&mut fmt)
     }
 
     #[inline]

crates/rune/src/runtime/value.rs

@@ -1746,10 +1746,9 @@ impl fmt::Debug for Value {
         }
 
         let mut s = String::new();
-        // SAFETY: Formatter does not outlive the string it references.
-        let mut o = unsafe { Formatter::new(NonNull::from(&mut s)) };
+        let result = Formatter::format_with(&mut s, |f| self.debug_fmt(f));
 
-        if let Err(e) = self.debug_fmt(&mut o).into_result() {
+        if let Err(e) = result.into_result() {
             match &self.repr {
                 Repr::Empty => {
                     write!(f, "<empty: {e}>")?;

crates/rune/src/runtime/vm.rs

@@ -3073,13 +3073,16 @@ impl Vm {
         let values = vm_try!(values.iter().cloned().try_collect::<alloc::Vec<_>>());
 
         let mut s = vm_try!(String::try_with_capacity(size_hint));
-        // SAFETY: Formatter does not outlive the string it references.
-        let mut f = unsafe { Formatter::new(NonNull::from(&mut s)) };
 
-        for value in values {
-            vm_try!(value.display_fmt_with(&mut f, &mut *self));
-        }
+        let result = Formatter::format_with(&mut s, |f| {
+            for value in values {
+                vm_try!(value.display_fmt_with(f, &mut *self));
+            }
+
+            VmResult::Ok(())
+        });
 
+        vm_try!(result);
         vm_try!(out.store(&mut self.stack, s));
         VmResult::Ok(())
     }