clean up Frame

youknowone · youknowone · commit 2cc83ed1fef0 · 2023-03-19T04:57:07.000+09:00
diff --git a/stdlib/src/faulthandler.rs b/stdlib/src/faulthandler.rs
@@ -2,11 +2,9 @@ pub(crate) use decl::make_module;
 
 #[pymodule(name = "faulthandler")]
 mod decl {
-    use crate::vm::{
-        frame::FrameRef, function::OptionalArg, stdlib::sys::PyStderr, VirtualMachine,
-    };
+    use crate::vm::{frame::Frame, function::OptionalArg, stdlib::sys::PyStderr, VirtualMachine};
 
-    fn dump_frame(frame: &FrameRef, vm: &VirtualMachine) {
+    fn dump_frame(frame: &Frame, vm: &VirtualMachine) {
         let stderr = PyStderr(vm);
         writeln!(
             stderr,
diff --git a/vm/src/builtins/frame.rs b/vm/src/builtins/frame.rs
@@ -16,9 +16,6 @@ pub fn init(context: &Context) {
     Frame::extend_class(context, context.types.frame_type);
 }
 
-#[pyclass(with(Constructor, PyRef, Representable))]
-impl Frame {}
-
 impl Unconstructible for Frame {}
 
 impl Representable for Frame {
@@ -34,61 +31,46 @@ impl Representable for Frame {
     }
 }
 
-#[pyclass]
-impl PyRef<Frame> {
+#[pyclass(with(Constructor, Py))]
+impl Frame {
     #[pymethod]
-    fn clear(self) {
+    fn clear(&self) {
         // TODO
     }
 
     #[pygetset]
-    fn f_globals(self) -> PyDictRef {
+    fn f_globals(&self) -> PyDictRef {
         self.globals.clone()
     }
 
     #[pygetset]
-    fn f_locals(self, vm: &VirtualMachine) -> PyResult {
+    fn f_locals(&self, vm: &VirtualMachine) -> PyResult {
         self.locals(vm).map(Into::into)
     }
 
     #[pygetset]
-    pub fn f_code(self) -> PyRef<PyCode> {
+    pub fn f_code(&self) -> PyRef<PyCode> {
         self.code.clone()
     }
 
     #[pygetset]
-    pub fn f_back(self, vm: &VirtualMachine) -> Option<Self> {
-        // TODO: actually store f_back inside Frame struct
-
-        // get the frame in the frame stack that appears before this one.
-        // won't work if  this frame isn't in the frame stack, hence the todo above
-        vm.frames
-            .borrow()
-            .iter()
-            .rev()
-            .skip_while(|p| !p.is(&self))
-            .nth(1)
-            .cloned()
-    }
-
-    #[pygetset]
-    fn f_lasti(self) -> u32 {
+    fn f_lasti(&self) -> u32 {
         self.lasti()
     }
 
     #[pygetset]
-    pub fn f_lineno(self) -> usize {
+    pub fn f_lineno(&self) -> usize {
         self.current_location().row()
     }
 
     #[pygetset]
-    fn f_trace(self) -> PyObjectRef {
+    fn f_trace(&self) -> PyObjectRef {
         let boxed = self.trace.lock();
         boxed.clone()
     }
 
     #[pygetset(setter)]
-    fn set_f_trace(self, value: PySetterValue, vm: &VirtualMachine) {
+    fn set_f_trace(&self, value: PySetterValue, vm: &VirtualMachine) {
         let mut storage = self.trace.lock();
         *storage = value.unwrap_or_none(vm);
     }
@@ -126,3 +108,21 @@ impl PyRef<Frame> {
         }
     }
 }
+
+#[pyclass]
+impl Py<Frame> {
+    #[pygetset]
+    pub fn f_back(&self, vm: &VirtualMachine) -> Option<PyRef<Frame>> {
+        // TODO: actually store f_back inside Frame struct
+
+        // get the frame in the frame stack that appears before this one.
+        // won't work if  this frame isn't in the frame stack, hence the todo above
+        vm.frames
+            .borrow()
+            .iter()
+            .rev()
+            .skip_while(|p| !p.is(self.as_object()))
+            .nth(1)
+            .cloned()
+    }
+}
diff --git a/vm/src/frame.rs b/vm/src/frame.rs
@@ -164,24 +164,20 @@ impl Frame {
             temporary_refs: PyMutex::new(vec![]),
         }
     }
-}
 
-impl FrameRef {
-    #[inline(always)]
-    fn with_exec<R>(&self, f: impl FnOnce(ExecutingFrame) -> R) -> R {
-        let mut state = self.state.lock();
-        let exec = ExecutingFrame {
-            code: &self.code,
-            fastlocals: &self.fastlocals,
-            cells_frees: &self.cells_frees,
-            locals: &self.locals,
-            globals: &self.globals,
-            builtins: &self.builtins,
-            lasti: &self.lasti,
-            object: self,
-            state: &mut state,
-        };
-        f(exec)
+    pub fn current_location(&self) -> bytecode::Location {
+        self.code.locations[self.lasti() as usize - 1]
+    }
+
+    pub fn lasti(&self) -> u32 {
+        #[cfg(feature = "threading")]
+        {
+            self.lasti.load(atomic::Ordering::Relaxed)
+        }
+        #[cfg(not(feature = "threading"))]
+        {
+            self.lasti.get()
+        }
     }
 
     pub fn locals(&self, vm: &VirtualMachine) -> PyResult<ArgMapping> {
@@ -221,6 +217,25 @@ impl FrameRef {
         }
         Ok(locals.clone())
     }
+}
+
+impl Py<Frame> {
+    #[inline(always)]
+    fn with_exec<R>(&self, f: impl FnOnce(ExecutingFrame) -> R) -> R {
+        let mut state = self.state.lock();
+        let exec = ExecutingFrame {
+            code: &self.code,
+            fastlocals: &self.fastlocals,
+            cells_frees: &self.cells_frees,
+            locals: &self.locals,
+            globals: &self.globals,
+            builtins: &self.builtins,
+            lasti: &self.lasti,
+            object: self,
+            state: &mut state,
+        };
+        f(exec)
+    }
 
     // #[cfg_attr(feature = "flame-it", flame("Frame"))]
     pub fn run(&self, vm: &VirtualMachine) -> PyResult<ExecutionResult> {
@@ -250,34 +265,19 @@ impl FrameRef {
         self.with_exec(|mut exec| exec.gen_throw(vm, exc_type, exc_val, exc_tb))
     }
 
-    pub fn current_location(&self) -> bytecode::Location {
-        self.code.locations[self.lasti() as usize - 1]
-    }
-
     pub fn yield_from_target(&self) -> Option<PyObjectRef> {
         self.with_exec(|exec| exec.yield_from_target().map(PyObject::to_owned))
     }
 
-    pub fn lasti(&self) -> u32 {
-        #[cfg(feature = "threading")]
-        {
-            self.lasti.load(atomic::Ordering::Relaxed)
-        }
-        #[cfg(not(feature = "threading"))]
-        {
-            self.lasti.get()
-        }
-    }
-
     pub fn is_internal_frame(&self) -> bool {
-        let code = self.clone().f_code();
+        let code = self.f_code();
         let filename = code.co_filename();
 
         filename.as_str().contains("importlib") && filename.as_str().contains("_bootstrap")
     }
 
     pub fn next_external_frame(&self, vm: &VirtualMachine) -> Option<FrameRef> {
-        self.clone().f_back(vm).map(|mut back| loop {
+        self.f_back(vm).map(|mut back| loop {
             back = if let Some(back) = back.to_owned().f_back(vm) {
                 back
             } else {
@@ -300,7 +300,7 @@ struct ExecutingFrame<'a> {
     locals: &'a ArgMapping,
     globals: &'a PyDictRef,
     builtins: &'a PyDictRef,
-    object: &'a FrameRef,
+    object: &'a Py<Frame>,
     lasti: &'a Lasti,
     state: &'a mut FrameState,
 }
@@ -376,8 +376,12 @@ impl ExecutingFrame<'_> {
 
                         let loc = frame.code.locations[idx];
                         let next = exception.traceback();
-                        let new_traceback =
-                            PyTraceback::new(next, frame.object.clone(), frame.lasti(), loc.row());
+                        let new_traceback = PyTraceback::new(
+                            next,
+                            frame.object.to_owned(),
+                            frame.lasti(),
+                            loc.row(),
+                        );
                         vm_trace!("Adding to traceback: {:?} {:?}", new_traceback, loc.row());
                         exception.set_traceback(Some(new_traceback.into_ref(vm)));
 
