From d2b2db66fb0f2409c7302123c03bc692916fa609 Mon Sep 17 00:00:00 2001 From: John-John Tedro Date: Mon, 31 Jul 2023 00:46:10 +0200 Subject: [PATCH 1/3] Improve call index support --- crates/rune/src/runtime/vm.rs | 55 +++++++++++++---------------------- 1 file changed, 21 insertions(+), 34 deletions(-) diff --git a/crates/rune/src/runtime/vm.rs b/crates/rune/src/runtime/vm.rs index 2c75885d8..8968140f0 100644 --- a/crates/rune/src/runtime/vm.rs +++ b/crates/rune/src/runtime/vm.rs @@ -472,14 +472,8 @@ impl Vm { H: ToTypeHash, A: GuardedArgs, { - let count = args.count(); - let full_count = args.count() + 1; + let count = args.count().wrapping_add(1); let type_hash = vm_try!(target.type_hash()); - self.stack.push(target); - - // Safety: We hold onto the guard for the duration of this call. - let _guard = unsafe { vm_try!(args.unsafe_into_stack(&mut self.stack)) }; - let hash = Hash::associated_function(type_hash, hash.to_type_hash()); if let Some(UnitFn::Offset { @@ -488,19 +482,22 @@ impl Vm { args: expected, }) = self.unit.function(hash) { - vm_try!(check_args(full_count, expected)); - vm_try!(self.call_offset_fn(offset, call, full_count)); + self.stack.push(target); + // Safety: We hold onto the guard for the duration of this call. + let _guard = unsafe { vm_try!(args.unsafe_into_stack(&mut self.stack)) }; + vm_try!(check_args(count, expected)); + vm_try!(self.call_offset_fn(offset, call, count)); return VmResult::Ok(CallResult::Ok(())); } if let Some(handler) = self.context.function(hash) { - vm_try!(handler(&mut self.stack, full_count)); + self.stack.push(target); + // Safety: We hold onto the guard for the duration of this call. + let _guard = unsafe { vm_try!(args.unsafe_into_stack(&mut self.stack)) }; + vm_try!(handler(&mut self.stack, count)); return VmResult::Ok(CallResult::Ok(())); } - // Restore the stack! - vm_try!(self.stack.popn(count)); - let target = vm_try!(self.stack.pop()); VmResult::Ok(CallResult::Unsupported(target)) } @@ -515,23 +512,18 @@ impl Vm { ) -> VmResult> where N: IntoHash, - A: Args, + A: GuardedArgs, { - let count = args.count(); - let full_count = count + 1; + let count = args.count().wrapping_add(1); let hash = Hash::field_function(protocol, vm_try!(target.type_hash()), name); - self.stack.push(target); - vm_try!(args.into_stack(&mut self.stack)); - if let Some(handler) = self.context.function(hash) { - vm_try!(handler(&mut self.stack, full_count)); + self.stack.push(target); + let _guard = unsafe { vm_try!(args.unsafe_into_stack(&mut self.stack)) }; + vm_try!(handler(&mut self.stack, count)); return VmResult::Ok(CallResult::Ok(())); } - // Restore the stack! - vm_try!(self.stack.popn(count)); - let target = vm_try!(self.stack.pop()); VmResult::Ok(CallResult::Unsupported(target)) } @@ -545,23 +537,18 @@ impl Vm { args: A, ) -> VmResult> where - A: Args, + A: GuardedArgs, { - let count = args.count(); - let full_count = count + 1; + let count = args.count().wrapping_add(1); let hash = Hash::index_function(protocol, vm_try!(target.type_hash()), Hash::index(index)); - self.stack.push(target); - vm_try!(args.into_stack(&mut self.stack)); - if let Some(handler) = self.context.function(hash) { - vm_try!(handler(&mut self.stack, full_count)); + self.stack.push(target); + let _guard = unsafe { vm_try!(args.unsafe_into_stack(&mut self.stack)) }; + vm_try!(handler(&mut self.stack, count)); return VmResult::Ok(CallResult::Ok(())); } - // Restore the stack! - vm_try!(self.stack.popn(count)); - let target = vm_try!(self.stack.pop()); VmResult::Ok(CallResult::Unsupported(target)) } @@ -1257,7 +1244,7 @@ impl Vm { } TargetFallback::Index(lhs, index, rhs) => { if let CallResult::Unsupported(lhs) = - vm_try!(self.call_index_fn(protocol, lhs.clone(), index, (rhs,))) + vm_try!(self.call_index_fn(protocol, lhs.clone(), index, (&rhs,))) { return err(VmErrorKind::UnsupportedTupleIndexGet { target: vm_try!(lhs.type_info()), From 8366b09a5e04ba41b320c445116912f1cdf3c109 Mon Sep 17 00:00:00 2001 From: John-John Tedro Date: Mon, 31 Jul 2023 00:52:42 +0200 Subject: [PATCH 2/3] Unify handling of CallResult::Unsupported --- crates/rune/src/runtime/vm.rs | 117 +++++++++++++++++----------------- 1 file changed, 59 insertions(+), 58 deletions(-) diff --git a/crates/rune/src/runtime/vm.rs b/crates/rune/src/runtime/vm.rs index 8968140f0..c6746950c 100644 --- a/crates/rune/src/runtime/vm.rs +++ b/crates/rune/src/runtime/vm.rs @@ -568,19 +568,17 @@ impl Vm { (Value::Integer(lhs), Value::Integer(rhs)) => int_op(lhs, rhs), (Value::Float(lhs), Value::Float(rhs)) => float_op(lhs, rhs), (lhs, rhs) => { - let ordering = match vm_try!(self.call_instance_fn(lhs, Protocol::CMP, (&rhs,))) { - CallResult::Ok(()) => { - vm_try!(::from_value(vm_try!(self.stack.pop()))) - } - CallResult::Unsupported(lhs) => { - return err(VmErrorKind::UnsupportedBinaryOperation { - op, - lhs: vm_try!(lhs.type_info()), - rhs: vm_try!(rhs.type_info()), - }); - } - }; + if let CallResult::Unsupported(lhs) = + vm_try!(self.call_instance_fn(lhs, Protocol::CMP, (&rhs,))) + { + return err(VmErrorKind::UnsupportedBinaryOperation { + op, + lhs: vm_try!(lhs.type_info()), + rhs: vm_try!(rhs.type_info()), + }); + } + let ordering = vm_try!(::from_value(vm_try!(self.stack.pop()))); match_ordering(ordering) } }; @@ -983,7 +981,7 @@ impl Vm { match vm_try!(self.call_field_fn(Protocol::SET, target, hash, (value,))) { CallResult::Ok(()) => { - vm_try!(self.stack.pop()); + vm_try!(<()>::from_value(vm_try!(self.stack.pop()))); CallResult::Ok(()) } result => result, @@ -1205,28 +1203,28 @@ impl Vm { TargetValue::Fallback(fallback) => fallback, }; - self.target_fallback(fallback, protocol) + self.target_fallback_assign(fallback, protocol) } /// Execute a fallback operation. - fn target_fallback( + fn target_fallback_assign( &mut self, fallback: TargetFallback<'_>, protocol: Protocol, ) -> VmResult<()> { match fallback { TargetFallback::Value(lhs, rhs) => { - match vm_try!(self.call_instance_fn(lhs, protocol, (&rhs,))) { - CallResult::Ok(()) => vm_try!(<()>::from_value(vm_try!(self.stack.pop()))), - CallResult::Unsupported(lhs) => { - return err(VmErrorKind::UnsupportedBinaryOperation { - op: protocol.name, - lhs: vm_try!(lhs.type_info()), - rhs: vm_try!(rhs.type_info()), - }); - } + if let CallResult::Unsupported(lhs) = + vm_try!(self.call_instance_fn(lhs, protocol, (&rhs,))) + { + return err(VmErrorKind::UnsupportedBinaryOperation { + op: protocol.name, + lhs: vm_try!(lhs.type_info()), + rhs: vm_try!(rhs.type_info()), + }); }; + vm_try!(<()>::from_value(vm_try!(self.stack.pop()))); VmResult::Ok(()) } TargetFallback::Field(lhs, hash, rhs) => { @@ -1287,14 +1285,14 @@ impl Vm { if let CallResult::Unsupported(lhs) = vm_try!(self.call_instance_fn(lhs, protocol, (&rhs,))) { - err(VmErrorKind::UnsupportedBinaryOperation { + return err(VmErrorKind::UnsupportedBinaryOperation { op: protocol.name, lhs: vm_try!(lhs.type_info()), rhs: vm_try!(rhs.type_info()), - }) - } else { - VmResult::Ok(()) + }); } + + VmResult::Ok(()) } /// Internal impl of a numeric operation. @@ -1318,14 +1316,14 @@ impl Vm { if let CallResult::Unsupported(lhs) = vm_try!(self.call_instance_fn(lhs, protocol, (&rhs,))) { - err(VmErrorKind::UnsupportedBinaryOperation { + return err(VmErrorKind::UnsupportedBinaryOperation { op: protocol.name, lhs: vm_try!(lhs.type_info()), rhs: vm_try!(rhs.type_info()), - }) - } else { - VmResult::Ok(()) + }); } + + VmResult::Ok(()) } /// Internal impl of a numeric operation. @@ -1354,14 +1352,14 @@ impl Vm { if let CallResult::Unsupported(lhs) = vm_try!(self.call_instance_fn(lhs, protocol, (&rhs,))) { - err(VmErrorKind::UnsupportedBinaryOperation { + return err(VmErrorKind::UnsupportedBinaryOperation { op: protocol.name, lhs: vm_try!(lhs.type_info()), rhs: vm_try!(rhs.type_info()), - }) - } else { - VmResult::Ok(()) + }); } + + VmResult::Ok(()) } fn internal_infallible_bitwise_assign( @@ -1384,7 +1382,7 @@ impl Vm { TargetValue::Fallback(fallback) => fallback, }; - self.target_fallback(fallback, protocol) + self.target_fallback_assign(fallback, protocol) } fn internal_bitwise( @@ -1409,14 +1407,14 @@ impl Vm { if let CallResult::Unsupported(lhs) = vm_try!(self.call_instance_fn(lhs, protocol, (&rhs,))) { - err(VmErrorKind::UnsupportedBinaryOperation { + return err(VmErrorKind::UnsupportedBinaryOperation { op: protocol.name, lhs: vm_try!(lhs.type_info()), rhs: vm_try!(rhs.type_info()), - }) - } else { - VmResult::Ok(()) + }); } + + VmResult::Ok(()) } fn internal_bitwise_assign( @@ -1441,7 +1439,7 @@ impl Vm { TargetValue::Fallback(fallback) => fallback, }; - self.target_fallback(fallback, protocol) + self.target_fallback_assign(fallback, protocol) } #[cfg_attr(feature = "bench", inline(never))] @@ -2005,17 +2003,17 @@ impl Vm { if let CallResult::Unsupported(target) = vm_try!(self.call_instance_fn(target, Protocol::INDEX_SET, (&index, &value))) { - err(VmErrorKind::UnsupportedIndexSet { + return err(VmErrorKind::UnsupportedIndexSet { target: vm_try!(target.type_info()), index: vm_try!(index.type_info()), value: vm_try!(value.type_info()), - }) - } else { - // Calling index set should not produce a value on the stack, but all - // handler functions to produce a value. So pop it here. - vm_try!(<()>::from_value(vm_try!(self.stack.pop()))); - VmResult::Ok(()) + }); } + + // Calling index set should not produce a value on the stack, but all + // handler functions to produce a value. So pop it here. + vm_try!(<()>::from_value(vm_try!(self.stack.pop()))); + VmResult::Ok(()) } #[inline] @@ -2160,13 +2158,14 @@ impl Vm { if let CallResult::Unsupported(target) = vm_try!(self.call_instance_fn(target, Protocol::INDEX_GET, (&index,))) { - err(VmErrorKind::UnsupportedIndexGet { + return err(VmErrorKind::UnsupportedIndexGet { target: vm_try!(target.type_info()), index: vm_try!(index.type_info()), - }) - } else { - VmResult::Ok(()) + }); } + + // NB: Should leave a value on the stack. + VmResult::Ok(()) } /// Perform an index get operation specialized for tuples. @@ -2188,7 +2187,7 @@ impl Vm { }); } - // NB: should leave a value on the stack. + // NB: Should leave a value on the stack. VmResult::Ok(()) } @@ -2228,6 +2227,7 @@ impl Vm { }); } + // NB: Should leave a value on the stack. VmResult::Ok(()) } @@ -2268,12 +2268,12 @@ impl Vm { if let CallResult::Unsupported(target) = vm_try!(self.try_object_slot_index_set(target, string_slot, value)) { - err(VmErrorKind::UnsupportedObjectSlotIndexSet { + return err(VmErrorKind::UnsupportedObjectSlotIndexSet { target: vm_try!(target.type_info()), - }) - } else { - VmResult::Ok(()) + }); } + + VmResult::Ok(()) } /// Perform a specialized index get operation on an object. @@ -2479,6 +2479,7 @@ impl Vm { actual: vm_try!(target.type_info()), }); } + vm_try!(>::from_value(vm_try!(self .stack .pop()))) From 0808cc2dc8f632a257c2022856c2d3114c8efb0f Mon Sep 17 00:00:00 2001 From: John-John Tedro Date: Mon, 31 Jul 2023 01:14:11 +0200 Subject: [PATCH 3/3] Clean up unnecessary loop --- crates/rune/src/runtime/vm.rs | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/crates/rune/src/runtime/vm.rs b/crates/rune/src/runtime/vm.rs index c6746950c..0b5523fe6 100644 --- a/crates/rune/src/runtime/vm.rs +++ b/crates/rune/src/runtime/vm.rs @@ -1250,8 +1250,7 @@ impl Vm { }); } - let value = vm_try!(self.stack.pop()); - vm_try!(<()>::from_value(value)); + vm_try!(<()>::from_value(vm_try!(self.stack.pop()))); VmResult::Ok(()) } } @@ -1444,8 +1443,7 @@ impl Vm { #[cfg_attr(feature = "bench", inline(never))] fn op_await(&mut self) -> VmResult> { - let value = vm_try!(self.stack.pop()); - value.into_future() + vm_try!(self.stack.pop()).into_future() } #[cfg_attr(feature = "bench", inline(never))] @@ -1945,9 +1943,7 @@ impl Vm { let target = vm_try!(self.stack.pop()); let value = vm_try!(self.stack.pop()); - // This is a useful pattern. - #[allow(clippy::never_loop)] - loop { + 'out: { // NB: local storage for string. let local_field; @@ -1957,7 +1953,7 @@ impl Vm { local_field.as_str() } Value::StaticString(string) => string.as_ref(), - _ => break, + _ => break 'out, }; match &target { @@ -1994,9 +1990,7 @@ impl Vm { target: variant.type_info(), }); } - _ => { - break; - } + _ => {} } } @@ -2010,8 +2004,6 @@ impl Vm { }); } - // Calling index set should not produce a value on the stack, but all - // handler functions to produce a value. So pop it here. vm_try!(<()>::from_value(vm_try!(self.stack.pop()))); VmResult::Ok(()) } @@ -3048,12 +3040,10 @@ impl Vm { } Inst::Await => { let future = vm_try!(self.op_await()); - // NB: the future itself will advance the virtual machine. return VmResult::Ok(VmHalt::Awaited(Awaited::Future(future))); } Inst::Select { len } => { if let Some(select) = vm_try!(self.op_select(len)) { - // NB: the future itself will advance the virtual machine. return VmResult::Ok(VmHalt::Awaited(Awaited::Select(select))); } }