From 922bf7aca9f4d4f4834ba5de7841ff58015c8791 Mon Sep 17 00:00:00 2001
From: Adam Joseph <adam@westernsemico.com>
Date: Mon, 28 Nov 2022 00:18:04 -0800
Subject: feat(tvix/eval): remove `derive(Copy)` from Upvalues

Change-Id: I0fa069fbeff6718a765ece948c2c1bce285496f7
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7449
Reviewed-by: grfn <grfn@gws.fyi>
Tested-by: BuildkiteCI
---
 tvix/eval/src/value/function.rs | 22 ++++++++++++++++++----
 tvix/eval/src/value/thunk.rs    |  8 ++++----
 tvix/eval/src/vm.rs             |  8 ++++----
 3 files changed, 26 insertions(+), 12 deletions(-)

(limited to 'tvix')

diff --git a/tvix/eval/src/value/function.rs b/tvix/eval/src/value/function.rs
index 4c2f3514f0..e31bb92b79 100644
--- a/tvix/eval/src/value/function.rs
+++ b/tvix/eval/src/value/function.rs
@@ -39,7 +39,14 @@ impl Formals {
 /// OpThunkSuspended referencing it.  At runtime `Lambda` is usually wrapped
 /// in `Rc` to avoid copying the `Chunk` it holds (which can be
 /// quite large).
-#[derive(Debug, Default)]
+///
+/// In order to correctly reproduce cppnix's "pointer equality"
+/// semantics it is important that we never clone a Lambda --
+/// use Rc<Lambda>::clone() instead.  This struct deliberately
+/// does not `derive(Clone)` in order to prevent this from being
+/// done accidentally.
+///
+#[derive(/* do not add Clone here */ Debug, Default)]
 pub struct Lambda {
     pub(crate) chunk: Chunk,
 
@@ -62,7 +69,14 @@ impl Lambda {
     }
 }
 
-#[derive(Clone, Debug)]
+///
+/// In order to correctly reproduce cppnix's "pointer equality"
+/// semantics it is important that we never clone a Lambda --
+/// use Rc<Lambda>::clone() instead.  This struct deliberately
+/// does not `derive(Clone)` in order to prevent this from being
+/// done accidentally.
+///
+#[derive(/* do not add Clone here */ Debug)]
 pub struct Closure {
     pub lambda: Rc<Lambda>,
     pub upvalues: Rc<Upvalues>,
@@ -88,7 +102,7 @@ impl Closure {
         self.lambda.clone()
     }
 
-    pub fn upvalues(&self) -> &Upvalues {
-        &self.upvalues
+    pub fn upvalues(&self) -> Rc<Upvalues> {
+        self.upvalues.clone()
     }
 }
diff --git a/tvix/eval/src/value/thunk.rs b/tvix/eval/src/value/thunk.rs
index 54d2b31a2b..9f14159893 100644
--- a/tvix/eval/src/value/thunk.rs
+++ b/tvix/eval/src/value/thunk.rs
@@ -47,7 +47,7 @@ enum ThunkRepr {
     /// execution.
     Suspended {
         lambda: Rc<Lambda>,
-        upvalues: Upvalues,
+        upvalues: Rc<Upvalues>,
         span: Span,
     },
 
@@ -78,7 +78,7 @@ impl Thunk {
 
     pub fn new_suspended(lambda: Rc<Lambda>, span: Span) -> Self {
         Thunk(Rc::new(RefCell::new(ThunkRepr::Suspended {
-            upvalues: Upvalues::with_capacity(lambda.upvalue_count),
+            upvalues: Rc::new(Upvalues::with_capacity(lambda.upvalue_count)),
             lambda: lambda.clone(),
             span,
         })))
@@ -144,7 +144,7 @@ impl Thunk {
 
     pub fn upvalues(&self) -> Ref<'_, Upvalues> {
         Ref::map(self.0.borrow(), |thunk| match thunk {
-            ThunkRepr::Suspended { upvalues, .. } => upvalues,
+            ThunkRepr::Suspended { upvalues, .. } => upvalues.as_ref(),
             ThunkRepr::Evaluated(Value::Closure(c)) => &c.upvalues,
             _ => panic!("upvalues() on non-suspended thunk"),
         })
@@ -152,7 +152,7 @@ impl Thunk {
 
     pub fn upvalues_mut(&self) -> RefMut<'_, Upvalues> {
         RefMut::map(self.0.borrow_mut(), |thunk| match thunk {
-            ThunkRepr::Suspended { upvalues, .. } => upvalues,
+            ThunkRepr::Suspended { upvalues, .. } => Rc::get_mut(upvalues).unwrap(),
             ThunkRepr::Evaluated(Value::Closure(c)) => Rc::get_mut(
                 &mut Rc::get_mut(c).unwrap().upvalues,
             )
diff --git a/tvix/eval/src/vm.rs b/tvix/eval/src/vm.rs
index 4ea155f5a3..f0764c314f 100644
--- a/tvix/eval/src/vm.rs
+++ b/tvix/eval/src/vm.rs
@@ -22,7 +22,7 @@ struct CallFrame {
 
     /// Optional captured upvalues of this frame (if a thunk or
     /// closure if being evaluated).
-    upvalues: Upvalues,
+    upvalues: Rc<Upvalues>,
 
     /// Instruction pointer to the instruction currently being
     /// executed.
@@ -242,7 +242,7 @@ impl<'o> VM<'o> {
     /// been forced.
     pub fn call_value(&mut self, callable: &Value) -> EvalResult<()> {
         match callable {
-            Value::Closure(c) => self.enter_frame(c.lambda(), c.upvalues().clone(), 1),
+            Value::Closure(c) => self.enter_frame(c.lambda(), c.upvalues(), 1),
 
             Value::Builtin(b) => self.call_builtin(b.clone()),
 
@@ -347,7 +347,7 @@ impl<'o> VM<'o> {
     pub fn enter_frame(
         &mut self,
         lambda: Rc<Lambda>,
-        upvalues: Upvalues,
+        upvalues: Rc<Upvalues>,
         arg_count: usize,
     ) -> EvalResult<()> {
         self.observer
@@ -1090,7 +1090,7 @@ pub fn run_lambda(
     // with the span of the entire file for top-level expressions.
     let root_span = lambda.chunk.get_span(CodeIdx(lambda.chunk.code.len() - 1));
 
-    vm.enter_frame(lambda, Upvalues::with_capacity(0), 0)?;
+    vm.enter_frame(lambda, Rc::new(Upvalues::with_capacity(0)), 0)?;
     let value = vm.pop();
 
     value
-- 
cgit 1.4.1