From c333e043e9e231d240659f547122738fff048c5b Mon Sep 17 00:00:00 2001 From: Noah <33094578+coolreader18@users.noreply.github.com> Date: Mon, 10 Aug 2020 18:25:05 -0500 Subject: [PATCH 1/3] Add vm.downcast_exact and use it for tuple.__new__ --- vm/src/obj/objint.rs | 2 +- vm/src/obj/objtuple.rs | 14 +++++++++----- vm/src/pyobject.rs | 24 +++++++++++++++++++----- 3 files changed, 29 insertions(+), 11 deletions(-) diff --git a/vm/src/obj/objint.rs b/vm/src/obj/objint.rs index 316b276ad..afcfd7477 100644 --- a/vm/src/obj/objint.rs +++ b/vm/src/obj/objint.rs @@ -240,7 +240,7 @@ impl PyInt { T: Into + ToPrimitive, { if cls.is(&vm.ctx.int_type()) { - Ok(PyRef::from_obj_unchecked(vm.ctx.new_int(value))) + Ok(vm.ctx.new_int(value).downcast().unwrap()) } else { PyInt::from(value).into_ref_with_type(vm, cls) } diff --git a/vm/src/obj/objtuple.rs b/vm/src/obj/objtuple.rs index 63ae51f49..99f1ead2f 100644 --- a/vm/src/obj/objtuple.rs +++ b/vm/src/obj/objtuple.rs @@ -8,7 +8,7 @@ use crate::function::OptionalArg; use crate::pyobject::{ self, BorrowValue, IdProtocol, IntoPyObject, PyArithmaticValue::{self, *}, - PyClassImpl, PyComparisonValue, PyContext, PyObjectRef, PyRef, PyResult, PyValue, TypeProtocol, + PyClassImpl, PyComparisonValue, PyContext, PyObjectRef, PyRef, PyResult, PyValue, }; use crate::sequence::{self, SimpleSeq}; use crate::vm::{ReprGuard, VirtualMachine}; @@ -238,11 +238,15 @@ impl PyTuple { vm: &VirtualMachine, ) -> PyResult { let elements = if let OptionalArg::Present(iterable) = iterable { - if iterable.lease_class().is(&cls) { - return Ok(PyRef::from_obj_unchecked(iterable)); + let exact_res = if cls.is(&vm.ctx.types.tuple_type) { + iterable.downcast_exact::(vm) + } else { + Err(iterable) + }; + match exact_res { + Ok(tup) => return Ok(tup), + Err(iterable) => vm.extract_elements(&iterable)?, } - - vm.extract_elements(&iterable)? } else { vec![] }; diff --git a/vm/src/pyobject.rs b/vm/src/pyobject.rs index 9f2fe327f..ce8a748b6 100644 --- a/vm/src/pyobject.rs +++ b/vm/src/pyobject.rs @@ -656,12 +656,26 @@ impl PyObject { /// /// If the downcast fails, the original ref is returned in as `Err` so /// another downcast can be attempted without unnecessary cloning. - pub fn downcast(self: PyRc) -> Result, PyObjectRef> { + pub fn downcast( + self: PyRc, + ) -> Result, PyObjectRef> { if self.payload_is::() { - Ok(PyRef { - obj: self, - _payload: PhantomData, - }) + Ok(PyRef::from_obj_unchecked(self)) + } else { + Err(self) + } + } + + /// Attempt to downcast this reference to the specific class that is associated `T`. + /// + /// If the downcast fails, the original ref is returned in as `Err` so + /// another downcast can be attempted without unnecessary cloning. + pub fn downcast_exact( + self: PyRc, + vm: &VirtualMachine, + ) -> Result, PyObjectRef> { + if self.lease_class().is(&T::class(vm)) { + Ok(PyRef::from_obj_unchecked(self)) } else { Err(self) } From 1d26cced8d97ca13dba425805b551100d7ab1cb3 Mon Sep 17 00:00:00 2001 From: Noah <33094578+coolreader18@users.noreply.github.com> Date: Mon, 10 Aug 2020 18:45:29 -0500 Subject: [PATCH 2/3] Make it a safety invariant that PyRef.obj is of payload T --- vm/src/pyobject.rs | 41 +++++++++++++++++++++++++++++------------ vm/src/vm.rs | 8 +++++--- 2 files changed, 34 insertions(+), 15 deletions(-) diff --git a/vm/src/pyobject.rs b/vm/src/pyobject.rs index ce8a748b6..b311cef04 100644 --- a/vm/src/pyobject.rs +++ b/vm/src/pyobject.rs @@ -138,7 +138,7 @@ impl PyContext { let exceptions = exceptions::ExceptionZoo::new(&types.type_type, &types.object_type); fn create_object(payload: T, cls: &PyClassRef) -> PyRef { - PyRef::from_obj_unchecked(PyObject::new(payload, cls.clone(), None)) + PyRef::new_ref(payload, cls.clone(), None) } let none_type = create_type("NoneType", &types.type_type, &types.object_type); @@ -660,7 +660,7 @@ impl PyObject { self: PyRc, ) -> Result, PyObjectRef> { if self.payload_is::() { - Ok(PyRef::from_obj_unchecked(self)) + Ok(unsafe { PyRef::from_obj_unchecked(self) }) } else { Err(self) } @@ -675,7 +675,12 @@ impl PyObject { vm: &VirtualMachine, ) -> Result, PyObjectRef> { if self.lease_class().is(&T::class(vm)) { - Ok(PyRef::from_obj_unchecked(self)) + // TODO: is this always true? + assert!( + self.payload_is::(), + "obj.__class__ is T::class() but payload is not T" + ); + Ok(unsafe { PyRef::from_obj_unchecked(self) }) } else { Err(self) } @@ -726,12 +731,15 @@ impl Clone for PyRef { impl PyRef { #[allow(clippy::new_ret_no_self)] pub fn new_ref(payload: T, typ: PyClassRef, dict: Option) -> Self { - Self::from_obj_unchecked(PyObject::new(payload, typ, dict)) + let obj = PyObject::new(payload, typ, dict); + // SAFETY: we just created the object from a payload of type T + unsafe { Self::from_obj_unchecked(obj) } } fn from_obj(obj: PyObjectRef, vm: &VirtualMachine) -> PyResult { if obj.payload_is::() { - Ok(Self::from_obj_unchecked(obj)) + // SAFETY: we just checked the payload + Ok(unsafe { Self::from_obj_unchecked(obj) }) } else { Err(vm.new_runtime_error(format!( "Unexpected payload for type {:?}", @@ -740,7 +748,8 @@ impl PyRef { } } - pub(crate) fn from_obj_unchecked(obj: PyObjectRef) -> Self { + /// Safety: payload type of `obj` must be `T` + pub(crate) unsafe fn from_obj_unchecked(obj: PyObjectRef) -> Self { PyRef { obj, _payload: PhantomData, @@ -767,7 +776,13 @@ where type Target = T; fn deref(&self) -> &T { - self.obj.payload().expect("unexpected payload for type") + self.obj.payload().unwrap_or_else(|| { + if cfg!(debug_assertions) { + panic!("PyRef.obj did not have a payload of T") + } + // SAFETY: self.obj has invariant that payload is `T` + unsafe { std::hint::unreachable_unchecked() } + }) } } @@ -900,7 +915,7 @@ impl<'a, T: PyObjectPayload + PyValue> PyLease<'a, T> { // Associated function on purpose, because of deref #[allow(clippy::wrong_self_convention)] pub fn into_pyref(zelf: Self) -> PyRef { - PyRef::from_obj_unchecked(zelf.inner.clone() as PyObjectRef) + zelf.inner.clone().into_pyref() } } @@ -1171,7 +1186,7 @@ where P: PyValue + IntoPyObject + From, { fn into_pyref(self, vm: &VirtualMachine) -> PyRef

{ - PyRef::from_obj_unchecked(P::from(self).into_pyobject(vm)) + P::from(self).into_ref(vm) } } @@ -1258,7 +1273,9 @@ where where T: PyValue, { - PyRef::from_obj_unchecked(self as PyObjectRef) + // SAFETY: we know just casted from PyRc> to PyObjectRef, so we know the + // payload is `T` + unsafe { PyRef::from_obj_unchecked(self as PyObjectRef) } } } @@ -1322,7 +1339,7 @@ pub trait PyValue: fmt::Debug + PyThreadingConstraint + Sized + 'static { fn class(vm: &VirtualMachine) -> PyClassRef; fn into_simple_object(self, vm: &VirtualMachine) -> PyObjectRef { - self.into_object(vm, Self::class(vm), None) + self.into_ref(vm).into_object() } fn into_object( @@ -1335,7 +1352,7 @@ pub trait PyValue: fmt::Debug + PyThreadingConstraint + Sized + 'static { } fn into_ref(self, vm: &VirtualMachine) -> PyRef { - PyRef::from_obj_unchecked(self.into_simple_object(vm)) + PyRef::new_ref(self, Self::class(vm), None) } fn into_ref_with_type(self, vm: &VirtualMachine, cls: PyClassRef) -> PyResult> { diff --git a/vm/src/vm.rs b/vm/src/vm.rs index afb9f1897..cda62aaad 100644 --- a/vm/src/vm.rs +++ b/vm/src/vm.rs @@ -402,9 +402,11 @@ impl VirtualMachine { // TODO: add repr of args into logging? vm_trace!("New exception created: {}", exc_type.name); - let pyobj = - PyBaseException::new(args, self).into_object(self, exc_type, Some(self.ctx.new_dict())); - PyRef::from_obj_unchecked(pyobj) + PyRef::new_ref( + PyBaseException::new(args, self), + exc_type, + Some(self.ctx.new_dict()), + ) } /// Instantiate an exception with no arguments. From 30831eb3bbe2d6659c910beb356244880c5a0210 Mon Sep 17 00:00:00 2001 From: Noah <33094578+coolreader18@users.noreply.github.com> Date: Tue, 11 Aug 2020 16:24:25 -0500 Subject: [PATCH 3/3] Update vm/src/obj/objtuple.rs Co-authored-by: Jeong YunWon --- vm/src/obj/objtuple.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/vm/src/obj/objtuple.rs b/vm/src/obj/objtuple.rs index 99f1ead2f..11a257d1c 100644 --- a/vm/src/obj/objtuple.rs +++ b/vm/src/obj/objtuple.rs @@ -238,15 +238,15 @@ impl PyTuple { vm: &VirtualMachine, ) -> PyResult { let elements = if let OptionalArg::Present(iterable) = iterable { - let exact_res = if cls.is(&vm.ctx.types.tuple_type) { - iterable.downcast_exact::(vm) + let iterable = if cls.is(&vm.ctx.types.tuple_type) { + match iterable.downcast_exact::(vm) { + Ok(tuple) => return Ok(tuple), + Err(iterable) => iterable, + } } else { - Err(iterable) + iterable }; - match exact_res { - Ok(tup) => return Ok(tup), - Err(iterable) => vm.extract_elements(&iterable)?, - } + vm.extract_elements(&iterable)? } else { vec![] };