Add more tests for weakref and fix objects not holding there weak

references alive. Also run rustfmt.
RustPython · BenLewis-Seequent · Feb 13, 2019 · Feb 16, 2019 · Feb 17, 2019 · Feb 18, 2019
commit 195a56a66b67f117471057589872ab407f72703f
diff --git a/tests/snippets/weakrefs.py b/tests/snippets/weakrefs.py
@@ -1,13 +1,60 @@
+import sys
 from _weakref import ref
 
 
+data_holder = {}
+
+
 class X:
-    pass
+    def __init__(self, param=0):
+        self.param = param
+
+    def __str__(self):
+        return f"param: {self.param}"
 
 
 a = X()
 b = ref(a)
 
+
+def callback(weak_ref):
+    assert weak_ref is c
+    assert b() is None, 'reference to same object is dead'
+    assert c() is None, 'reference is dead'
+    data_holder['first'] = True
+
+
+c = ref(a, callback)
+
+
+def never_callback(_weak_ref):
+    data_holder['never'] = True
+
+
+# weakref should be cleaned up before object, so callback is never called
+ref(a, never_callback)
+
 assert callable(b)
 assert b() is a
 
+assert 'first' not in data_holder
+del a
+assert b() is None
+assert 'first' in data_holder
+assert 'never' not in data_holder
+
+# TODO proper detection of RustPython if sys.implementation.name == 'RustPython':
+if not hasattr(sys, 'implementation'):
+    # implementation detail that the object isn't dropped straight away
+    # this tests that when an object is resurrected it still acts as normal
+    delayed_drop = X(5)
+    delayed_drop_ref = ref(delayed_drop)
+
+    delayed_drop = None
+
+    assert delayed_drop_ref() is not None
+    value = delayed_drop_ref()
+    del delayed_drop  # triggers process_deletes
+
+    assert str(value) == "param: 5"
+
diff --git a/vm/src/builtins.rs b/vm/src/builtins.rs
@@ -16,8 +16,7 @@ use super::obj::objstr;
 use super::obj::objtype;
 
 use super::pyobject::{
-    AttributeProtocol, IdProtocol, PyContext, PyFuncArgs, PyObjectRef,
-    PyResult, TypeProtocol,
+    AttributeProtocol, IdProtocol, PyContext, PyFuncArgs, PyObjectRef, PyResult, TypeProtocol,
 };
 use super::stdlib::io::io_open;
 

diff --git a/vm/src/frame.rs b/vm/src/frame.rs
@@ -844,6 +844,10 @@ impl Frame {
         // Assume here that locals is a dict
         let name = vm.ctx.new_str(name.to_string());
         vm.call_method(&locals, "__delitem__", vec![name])?;
+
+        // process possible delete
+        PyObjectRef::process_deletes(vm);
+
         Ok(None)
     }
 

diff --git a/vm/src/obj/objweakref.rs b/vm/src/obj/objweakref.rs
@@ -1,16 +1,24 @@
 use super::super::pyobject::{
-    PyContext, PyFuncArgs, PyObject, PyObjectRef, PyObjectWeakRef, PyObjectPayload, PyResult, TypeProtocol,
+    PyContext, PyFuncArgs, PyObject, PyObjectPayload, PyObjectRef, PyObjectWeakRef, PyResult,
+    TypeProtocol,
 };
 use super::super::vm::VirtualMachine;
 use super::objtype; // Required for arg_check! to use isinstance
 
 fn ref_new(vm: &mut VirtualMachine, args: PyFuncArgs) -> PyResult {
     // TODO: check first argument for subclass of `ref`.
-    arg_check!(vm, args, required = [(cls, Some(vm.ctx.type_type())), (referent, None)],
-        optional = [(callback, None)]);
+    arg_check!(
+        vm,
+        args,
+        required = [(cls, Some(vm.ctx.type_type())), (referent, None)],
+        optional = [(callback, None)]
+    );
     let weak_referent = PyObjectRef::downgrade(referent);
     let weakref = PyObject::new(
-        PyObjectPayload::WeakRef { referent: weak_referent, callback: callback.cloned() },
+        PyObjectPayload::WeakRef {
+            referent: weak_referent,
+            callback: callback.cloned(),
+        },
         cls.clone(),
     );
     referent.borrow_mut().add_weakref(&weakref);
@@ -37,23 +45,30 @@ fn get_value(obj: &PyObjectRef) -> PyObjectWeakRef {
     }
 }
 
+fn get_callback(obj: &PyObjectRef) -> Option<PyObjectRef> {
+    if let PyObjectPayload::WeakRef { callback, .. } = &obj.borrow().payload {
+        callback.as_ref().cloned()
+    } else {
+        panic!("Inner error getting weak ref callback {:?}", obj);
+    }
+}
+
 pub fn clear_weak_ref(obj: &PyObjectRef) {
-    if let PyObjectPayload::WeakRef { ref mut referent, .. } = &mut obj.borrow_mut().payload {
+    if let PyObjectPayload::WeakRef {
+        ref mut referent, ..
+    } = &mut obj.borrow_mut().payload
+    {
         referent.clear();
     } else {
         panic!("Inner error getting weak ref {:?}", obj);
     }
 }
 
-pub fn notify_weak_ref(vm: &mut VirtualMachine, obj: &PyObjectRef) -> PyResult {
-    if let PyObjectPayload::WeakRef { callback, .. } = &obj.borrow().payload {
-        if let Some(callback) = callback {
-            vm.invoke(callback.clone(), PyFuncArgs::empty())
-        } else {
-            Ok(vm.get_none())
-        }
+pub fn notify_weak_ref(vm: &mut VirtualMachine, obj: PyObjectRef) -> PyResult {
+    if let Some(callback) = get_callback(&obj) {
+        vm.invoke(callback.clone(), PyFuncArgs::new(vec![obj], vec![]))
     } else {
-        panic!("Inner error getting weak ref callback {:?}", obj);
+        Ok(vm.get_none())
     }
 }
 

diff --git a/vm/src/pyobject.rs b/vm/src/pyobject.rs
@@ -38,10 +38,10 @@ use num_traits::{One, Zero};
 use std::cell::RefCell;
 use std::collections::{HashMap, VecDeque};
 use std::fmt;
-use std::rc::{Rc, Weak};
+use std::mem;
 use std::ops::Deref;
 use std::ptr;
-use std::mem;
+use std::rc::{Rc, Weak};
 
 /* Python objects and references.
 
@@ -69,17 +69,19 @@ https://doc.rust-lang.org/std/cell/index.html#introducing-mutability-inside-of-s
 /// to the python object by 1.
 #[derive(Debug, Clone)]
 pub struct PyObjectRef {
-    rc: Rc<RefCell<PyObject>>
+    rc: Rc<RefCell<PyObject>>,
 }
 
 // TODO support multiple virtual machines, how?
-thread_local!  {
+thread_local! {
     static DELETE_QUEUE: RefCell<VecDeque<PyObjectRef>> = RefCell::new(VecDeque::new());
 }
 
 impl PyObjectRef {
     pub fn downgrade(this: &Self) -> PyObjectWeakRef {
-        PyObjectWeakRef { weak: Rc::downgrade(&this.rc) }
+        PyObjectWeakRef {
+            weak: Rc::downgrade(&this.rc),
+        }
     }
 
     pub fn strong_count(this: &Self) -> usize {
@@ -90,11 +92,18 @@ impl PyObjectRef {
         while let Some(obj) = DELETE_QUEUE.with(|q| q.borrow_mut().pop_front()) {
             // check is still needs to be deleted
             if PyObjectRef::strong_count(&obj) == 1 {
-                for weak_ref in obj.borrow().weak_refs.iter() {
+                let weak_refs: Vec<PyObjectRef> = obj
+                    .borrow()
+                    .weak_refs
+                    .iter()
+                    .flat_map(|x| x.upgrade())
+                    .collect();
+
+                for weak_ref in weak_refs.iter() {
                     objweakref::clear_weak_ref(weak_ref);
                 }
 
-                for weak_ref in obj.borrow().weak_refs.iter().rev() {
+                for weak_ref in weak_refs.into_iter().rev() {
                     if let Err(err) = objweakref::notify_weak_ref(vm, weak_ref) {
                         exceptions::print_exception(vm, &err);
                     }
@@ -108,9 +117,11 @@ impl PyObjectRef {
 
                 // dispose of value without calling normal drop
                 unsafe {
-                    let rc_ptr = &mut manually_drop.rc as * mut _;
+                    let rc_ptr = &mut manually_drop.rc as *mut _;
                     ptr::drop_in_place(rc_ptr);
                 }
+            } else {
+                info!("Object {:?} resurrected", obj);
             }
         }
     }
@@ -139,12 +150,12 @@ impl Drop for PyObjectRef {
 
 #[derive(Debug, Clone)]
 pub struct PyObjectWeakRef {
-    weak: Weak<RefCell<PyObject>>
+    weak: Weak<RefCell<PyObject>>,
 }
 
 impl PyObjectWeakRef {
     pub fn upgrade(&self) -> Option<PyObjectRef> {
-        self.weak.upgrade().map(|x| PyObjectRef { rc:x })
+        self.weak.upgrade().map(|x| PyObjectRef { rc: x })
     }
 
     pub fn clear(&mut self) {
@@ -530,7 +541,9 @@ impl PyContext {
     pub fn member_descriptor_type(&self) -> PyObjectRef {
         self.member_descriptor_type.clone()
     }
-    pub fn weakref_type(&self) -> PyObjectRef { self.weakref_type.clone() }
+    pub fn weakref_type(&self) -> PyObjectRef {
+        self.weakref_type.clone()
+    }
     pub fn type_type(&self) -> PyObjectRef {
         self.type_type.clone()
     }
@@ -622,7 +635,11 @@ impl PyContext {
         self.new_scope_with_locals(parent, locals)
     }
 
-    pub fn new_scope_with_locals(&self, parent: Option<PyObjectRef>, locals: PyObjectRef) -> PyObjectRef {
+    pub fn new_scope_with_locals(
+        &self,
+        parent: Option<PyObjectRef>,
+        locals: PyObjectRef,
+    ) -> PyObjectRef {
         let scope = Scope { locals, parent };
         PyObject::new_no_type(PyObjectPayload::Scope { scope })
     }
@@ -781,8 +798,7 @@ impl PyContext {
 pub struct PyObject {
     pub payload: PyObjectPayload,
     pub typ: Option<PyObjectRef>,
-    weak_refs: Vec<PyObjectRef>
-    // pub dict: HashMap<String, PyObjectRef>, // __dict__ member
+    weak_refs: Vec<PyObjectWeakRef>, // pub dict: HashMap<String, PyObjectRef>, // __dict__ member
 }
 
 pub trait IdProtocol {
@@ -975,10 +991,6 @@ impl PyFuncArgs {
         PyFuncArgs { args, kwargs }
     }
 
-    pub fn empty() -> PyFuncArgs {
-        PyFuncArgs { args: vec![], kwargs: vec![] }
-    }
-
     pub fn insert(&self, item: PyObjectRef) -> PyFuncArgs {
         let mut args = PyFuncArgs {
             args: self.args.clone(),
@@ -1162,7 +1174,7 @@ impl PyObject {
             payload,
             typ: Some(typ),
             // dict: HashMap::new(),  // dict,
-            weak_refs: Vec::new()
+            weak_refs: Vec::new(),
         }
         .into_ref()
     }
@@ -1172,18 +1184,20 @@ impl PyObject {
             payload,
             typ: None,
             // dict: HashMap::new(),  // dict,
-            weak_refs: Vec::new()
+            weak_refs: Vec::new(),
         }
         .into_ref()
     }
 
     // Move this object into a reference object, transferring ownership.
     fn into_ref(self) -> PyObjectRef {
-        PyObjectRef { rc: Rc::new(RefCell::new(self)) }
+        PyObjectRef {
+            rc: Rc::new(RefCell::new(self)),
+        }
     }
 
     pub fn add_weakref(&mut self, weakref: &PyObjectRef) {
-        self.weak_refs.push(weakref.clone())
+        self.weak_refs.push(PyObjectRef::downgrade(weakref))
     }
 }
 

diff --git a/vm/src/stdlib/weakref.rs b/vm/src/stdlib/weakref.rs
@@ -5,14 +5,11 @@
 //! - [rust weak struct](https://doc.rust-lang.org/std/rc/struct.Weak.html)
 //!
 
-use super::super::pyobject::{
-    PyContext, PyObjectRef
-};
+use super::super::pyobject::{PyContext, PyObjectRef};
 
 pub fn mk_module(ctx: &PyContext) -> PyObjectRef {
     let py_mod = ctx.new_module("_weakref", ctx.new_scope(None));
 
     ctx.set_attr(&py_mod, "ref", ctx.weakref_type());
     py_mod
 }
-