Merge pull request #2104 from RustPython/coolreader18/downcast-exact

Make it a safety invariant that PyRef<T>.obj has a payload of T

vm/src/obj/objint.rs

@@ -240,7 +240,7 @@ impl PyInt {
         T: Into<BigInt> + 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)
         }

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,10 +238,14 @@ impl PyTuple {
         vm: &VirtualMachine,
     ) -> PyResult<PyTupleRef> {
         let elements = if let OptionalArg::Present(iterable) = iterable {
-            if iterable.lease_class().is(&cls) {
-                return Ok(PyRef::from_obj_unchecked(iterable));
-            }
-
+            let iterable = if cls.is(&vm.ctx.types.tuple_type) {
+                match iterable.downcast_exact::<PyTuple>(vm) {
+                    Ok(tuple) => return Ok(tuple),
+                    Err(iterable) => iterable,
+                }
+            } else {
+                iterable
+            };
             vm.extract_elements(&iterable)?
         } else {
             vec![]

vm/src/pyobject.rs

@@ -138,7 +138,7 @@ impl PyContext {
         let exceptions = exceptions::ExceptionZoo::new(&types.type_type, &types.object_type);
 
         fn create_object<T: PyObjectPayload + PyValue>(payload: T, cls: &PyClassRef) -> PyRef<T> {
-            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);
@@ -656,12 +656,31 @@ impl PyObject<dyn PyObjectPayload> {
     ///
     /// If the downcast fails, the original ref is returned in as `Err` so
     /// another downcast can be attempted without unnecessary cloning.
-    pub fn downcast<T: PyObjectPayload>(self: PyRc<Self>) -> Result<PyRef<T>, PyObjectRef> {
+    pub fn downcast<T: PyObjectPayload + PyValue>(
+        self: PyRc<Self>,
+    ) -> Result<PyRef<T>, PyObjectRef> {
         if self.payload_is::<T>() {
-            Ok(PyRef {
-                obj: self,
-                _payload: PhantomData,
-            })
+            Ok(unsafe { 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<T: PyObjectPayload + PyValue>(
+        self: PyRc<Self>,
+        vm: &VirtualMachine,
+    ) -> Result<PyRef<T>, PyObjectRef> {
+        if self.lease_class().is(&T::class(vm)) {
+            // TODO: is this always true?
+            assert!(
+                self.payload_is::<T>(),
+                "obj.__class__ is T::class() but payload is not T"
+            );
+            Ok(unsafe { PyRef::from_obj_unchecked(self) })
         } else {
             Err(self)
         }
@@ -712,12 +731,15 @@ impl<T> Clone for PyRef<T> {
 impl<T: PyValue> PyRef<T> {
     #[allow(clippy::new_ret_no_self)]
     pub fn new_ref(payload: T, typ: PyClassRef, dict: Option<PyDictRef>) -> 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<Self> {
         if obj.payload_is::<T>() {
-            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 {:?}",
@@ -726,7 +748,8 @@ impl<T: PyValue> PyRef<T> {
         }
     }
 
-    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,
@@ -753,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() }
+        })
     }
 }
 
@@ -886,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<T> {
-        PyRef::from_obj_unchecked(zelf.inner.clone() as PyObjectRef)
+        zelf.inner.clone().into_pyref()
     }
 }
 
@@ -1157,7 +1186,7 @@ where
     P: PyValue + IntoPyObject + From<T>,
 {
     fn into_pyref(self, vm: &VirtualMachine) -> PyRef<P> {
-        PyRef::from_obj_unchecked(P::from(self).into_pyobject(vm))
+        P::from(self).into_ref(vm)
     }
 }
 
@@ -1244,7 +1273,9 @@ where
     where
         T: PyValue,
     {
-        PyRef::from_obj_unchecked(self as PyObjectRef)
+        // SAFETY: we know just casted from PyRc<PyObject<T>> to PyObjectRef, so we know the
+        // payload is `T`
+        unsafe { PyRef::from_obj_unchecked(self as PyObjectRef) }
     }
 }
 
@@ -1308,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(
@@ -1321,7 +1352,7 @@ pub trait PyValue: fmt::Debug + PyThreadingConstraint + Sized + 'static {
     }
 
     fn into_ref(self, vm: &VirtualMachine) -> PyRef<Self> {
-        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<PyRef<Self>> {

vm/src/vm.rs

@@ -403,9 +403,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.