From 97fef9c1ef22f8a62612e44ab720ea3a7cc1c8a0 Mon Sep 17 00:00:00 2001 From: John-John Tedro Date: Sat, 2 Nov 2024 18:41:48 +0100 Subject: [PATCH] rune: Get rid of more formatting-related unsafety --- crates/rune/src/modules/option.rs | 65 ++++++++++++++++--------------- crates/rune/src/modules/result.rs | 45 +++++++++++---------- crates/rune/src/runtime/fmt.rs | 18 ++++++--- crates/rune/src/runtime/value.rs | 5 +-- crates/rune/src/runtime/vm.rs | 13 ++++--- crates/rune/src/tests/option.rs | 14 +++---- 6 files changed, 85 insertions(+), 75 deletions(-) diff --git a/crates/rune/src/modules/option.rs b/crates/rune/src/modules/option.rs index 59aab7fab..d6cd13542 100644 --- a/crates/rune/src/modules/option.rs +++ b/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 { )), })?; - // 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::()?; @@ -113,14 +110,12 @@ pub fn module() -> Result { /// 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, message: Value) -> VmResult { +fn expect(option: &Option, message: Value) -> VmResult { 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) -> bool { /// assert_eq!(None, it.next()); /// ``` #[rune::function(instance)] -fn iter(value: Option) -> Iter { - Iter { value } +fn iter(value: &Option) -> Iter { + Iter::new(value.clone()) } /// Construct an iterator over an optional value. @@ -195,8 +190,8 @@ fn iter(value: Option) -> Iter { /// assert_eq!(out, [1]); /// ``` #[rune::function(instance, protocol = INTO_ITER)] -fn into_iter(this: Option) -> Iter { - Iter::new(this) +fn into_iter(this: &Option) -> 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) -> Iter { /// assert_eq!(item_2_0, None); /// ``` #[rune::function(instance)] -fn and_then(option: Option, then: Function) -> VmResult> { +fn and_then(option: &Option, then: Function) -> VmResult> { 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, then: Function) -> VmResult> { /// assert_eq!(x.map(|s| s.len()), None); /// ``` #[rune::function(instance)] -fn map(option: Option, then: Function) -> VmResult> { +fn map(option: &Option, then: Function) -> VmResult> { 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) -> Option { /// assert_eq!(x, y.transpose()); /// ``` #[rune::function(instance)] -fn transpose(this: Option) -> VmResult { +fn transpose(this: &Option) -> VmResult { let value = match this { Some(value) => value, None => { @@ -308,7 +303,7 @@ fn transpose(this: Option) -> VmResult { } }; - 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) -> VmResult { /// assert_eq!(x.unwrap(), "air"); // fails /// ``` #[rune::function(instance)] -fn unwrap(option: Option) -> VmResult { +fn unwrap(option: &Option) -> VmResult { 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) -> VmResult { /// assert_eq!(None.unwrap_or("bike"), "bike"); /// ``` #[rune::function(instance)] -fn unwrap_or(this: Option, default: Value) -> Value { - this.unwrap_or(default) +fn unwrap_or(this: &Option, 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, default: Value) -> Value { /// assert_eq!(None.unwrap_or_else(|| 2 * k), 20); /// ``` #[rune::function(instance)] -fn unwrap_or_else(this: Option, default: Function) -> VmResult { +fn unwrap_or_else(this: &Option, default: Function) -> VmResult { 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, default: Function) -> VmResult { /// assert_eq!(x.ok_or(0), Err(0)); /// ``` #[rune::function(instance)] -fn ok_or(this: Option, err: Value) -> Result { - this.ok_or(err) +fn ok_or(this: &Option, err: Value) -> Result { + match this { + Some(value) => Ok(value.clone()), + None => Err(err), + } } /// Transforms the `Option` into a [`Result`], mapping [`Some(v)`] to @@ -433,9 +434,9 @@ fn ok_or(this: Option, err: Value) -> Result { /// assert_eq!(x.ok_or_else(|| 0), Err(0)); /// ``` #[rune::function(instance)] -fn ok_or_else(this: Option, err: Function) -> VmResult> { +fn ok_or_else(this: &Option, err: Function) -> VmResult> { match this { - Some(value) => VmResult::Ok(Ok(value)), + Some(value) => VmResult::Ok(Ok(value.clone())), None => VmResult::Ok(Err(vm_try!(err.call(())))), } } diff --git a/crates/rune/src/modules/result.rs b/crates/rune/src/modules/result.rs index a3256a2ff..4b2a5db0a 100644 --- a/crates/rune/src/modules/result.rs +++ b/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) -> bool { /// x.unwrap(); // panics with `emergency failure` /// ``` #[rune::function(instance)] -fn unwrap(result: Result) -> VmResult { +fn unwrap(result: &Result) -> VmResult { 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) -> VmResult { /// assert_eq!(x.unwrap_or(default_value), default_value); /// ``` #[rune::function(instance)] -fn unwrap_or(this: Result, default: Value) -> Value { - this.unwrap_or(default) +fn unwrap_or(this: &Result, 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, default: Value) -> Value { /// assert_eq!(Err("foo").unwrap_or_else(count), 3); /// ``` #[rune::function(instance)] -fn unwrap_or_else(this: Result, default: Function) -> VmResult { +fn unwrap_or_else(this: &Result, default: Function) -> VmResult { 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, default: Function) -> VmResult, message: Value) -> VmResult { +fn expect(result: &Result, message: Value) -> VmResult { 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.display_fmt(f)); + vm_try!(f.try_write_str(": ")); + vm_try!(err.debug_fmt(f)); + VmResult::Ok(()) + })); VmResult::err(Panic::custom(s)) } } diff --git a/crates/rune/src/runtime/fmt.rs b/crates/rune/src/runtime/fmt.rs index 8b86f4c04..8a8236968 100644 --- a/crates/rune/src/runtime/fmt.rs +++ b/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) -> 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] diff --git a/crates/rune/src/runtime/value.rs b/crates/rune/src/runtime/value.rs index 56030acc1..88ba2d463 100644 --- a/crates/rune/src/runtime/value.rs +++ b/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, "")?; diff --git a/crates/rune/src/runtime/vm.rs b/crates/rune/src/runtime/vm.rs index 2b64b29c4..2c55c74be 100644 --- a/crates/rune/src/runtime/vm.rs +++ b/crates/rune/src/runtime/vm.rs @@ -3073,13 +3073,16 @@ impl Vm { let values = vm_try!(values.iter().cloned().try_collect::>()); 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(()) } diff --git a/crates/rune/src/tests/option.rs b/crates/rune/src/tests/option.rs index 59fbc68d5..1a808ab47 100644 --- a/crates/rune/src/tests/option.rs +++ b/crates/rune/src/tests/option.rs @@ -38,11 +38,10 @@ fn test_expect() { r#" pub fn main() { None.expect("None") - } - "#, + } + "#, Panic { reason} => { - assert_eq!(reason.to_string(), - "None") + assert_eq!(reason.to_string(), "None") } ); } @@ -63,11 +62,10 @@ fn test_unwrap() { r#" pub fn main() { None.unwrap() - } - "#, + } + "#, Panic { reason} => { - assert_eq!(reason.to_string(), - "Called `Option::unwrap()` on a `None` value") + assert_eq!(reason.to_string(), "Called `Option::unwrap()` on a `None` value") } ); }