WIP: move mutability to be internal to the object instead of the pointer

I'm not super happy with this. But, the RwLock has been moved to the
`BaseObjInst::attrs` member. Although this is not exactly how it appears
in code, it basically does this:

    type Ptr<T> = Arc<RwLock<T>>;

    struct BaseObjInst {
        attr: HashMap<String, Ptr<dyn Obj>>,
        // etc
    }

becomes

    type Ptr<T> = Arc<T>;

    struct BaseObjInst {
        attr: RwLock<HashMap<String, ObjP>>,
        // etc
    }

This makes things a lot more ergonomic (don't have to use try_read() and
try_write() everywhere), but it also eliminates compile-time errors that
would catch mutability errors. This is currently rearing its ugly head
when initializing the typesystem, since `Type` needs to hold a circular
reference itself (which it already shouldn't be doing since it's a
reference-counted pointer!). Currently, all tests are failing because of
this limitation.

There are a couple of ways around this limitation.

The first solution would be just copying  all of the object
instantiation code into the `init_types` function and avoid calling
`some_base_type.instantiate()`. This would probably be literal
copy-pasting, or maybe an (ugly) macro, and probably a nightmare to
maintain long-term. I don't like this option, but it would make
everything "just work" with reference-counted pointers.

The second solution would be to write our own garbage collector, which
would allow for circular references and (hypothetically) mutably
updating these references. This is something that I am looking into,
because I really want a RefCell that you can pass around in a more
ergonomic way.

I think the fundamental error that I'm running into is trying to borrow
the same value multiple times mutably, which you *really* shouldn't be
doing. I believe I need to write better code and does the same thing.

The only unsolved problem is circular references. This is not a problem
right now because I'm not writing code that has circular references
besides the base typesystem (which is not a problem because they need to
live the entire lifetime of the program), but it will be a latent
problem until it gets fixed.

Signed-off-by: Alek Ratzloff <alekratz@gmail.com>

src/builtins.rs

@@ -3,11 +3,11 @@ use crate::obj::{NilInst, ObjP};
 use crate::vm::Vm;
 
 pub(crate) fn println(_vm: &mut Vm, args: Vec<ObjP>) -> ObjP {
-    println!("{}", args[0].try_read().unwrap());
+    println!("{}", args[0]);
     NilInst::create()
 }
 
 pub(crate) fn print(_vm: &mut Vm, args: Vec<ObjP>) -> ObjP {
-    print!("{}", args[0].try_read().unwrap());
+    print!("{}", args[0]);
     NilInst::create()
 }

src/compiler.rs

@@ -478,11 +478,7 @@ impl Compiler {
         // simple interning - try to find a constant that is exactly equal to this one and just
         // return its value instead
         for (index, interned) in self.constants.iter().enumerate() {
-            if constant
+            if constant.equals(interned.as_ref()) {
-                .try_read()
-                .unwrap()
-                .equals(&*interned.try_read().unwrap())
-            {
                 return Ok(index as ConstantId);
             }
         }
@@ -665,7 +661,7 @@ impl StmtVisitor for Compiler {
         // TODO - maybe this would be smarter to set up in the AST. I'm 99% sure that the last
         // object created, if it were a function object, will be what we're assigning it to, but I
         // want to be 100% sure instead of 99%.
-        let mut obj = self.constants.last_mut().unwrap().try_write().unwrap();
+        let obj = Arc::get_mut(self.constants.last_mut().unwrap()).unwrap();
         if let Some(fun) = obj.as_any_mut().downcast_mut::<UserFunctionInst>() {
             fun.set_name(Arc::new(name.to_string()));
         }

src/disassemble.rs

@@ -33,7 +33,7 @@ fn disassemble_chunk(chunk: &Chunk, constants: &Vec<ObjP>, globals: &Vec<String>
             }
             Op::PushConstant(constant_id) => {
                 op_str = "PUSH_CONSTANT";
-                arg = format!("{}", &constants[*constant_id as usize].try_read().unwrap());
+                arg = format!("{}", &constants[*constant_id as usize]);
                 info = format!("(constant ID {constant_id})");
             }
             Op::GetLocal(local_id) => {
@@ -60,12 +60,12 @@ fn disassemble_chunk(chunk: &Chunk, constants: &Vec<ObjP>, globals: &Vec<String>
             }
             Op::GetAttr(constant_id) => {
                 op_str = "GET_ATTR";
-                arg = format!("{}", &constants[*constant_id as usize].try_read().unwrap());
+                arg = format!("{}", &constants[*constant_id as usize]);
                 info = format!("(constant ID {constant_id})");
             }
             Op::SetAttr(constant_id) => {
                 op_str = "SET_ATTR";
-                arg = format!("{}", &constants[*constant_id as usize].try_read().unwrap());
+                arg = format!("{}", &constants[*constant_id as usize]);
                 info = format!("(constant ID {constant_id})");
             }
             Op::Jump(jump_offset) => {
@@ -145,8 +145,7 @@ pub fn disassemble(chunk: &Chunk, constants: &Vec<ObjP>, globals: &Vec<String>)
     disassemble_chunk(chunk, constants, globals);
 
     for constant in constants {
-        let borrowed = constant.try_read().unwrap();
+        if let Some(fun) = constant.as_any().downcast_ref::<UserFunctionInst>() {
-        if let Some(fun) = borrowed.as_any().downcast_ref::<UserFunctionInst>() {
             println!();
             println!(
                 "== {} starting on line {}",

src/obj.rs

@@ -11,22 +11,21 @@ use common_macros::hash_map;
 
 use crate::vm::{Argc, Chunk, Frame, Vm};
 
-pub type Ptr<T> = Arc<RwLock<T>>;
+pub type Ptr<T> = Arc<T>;
 pub type ObjP = Ptr<dyn Obj + 'static>;
-pub type Attrs = HashMap<String, ObjP>;
+pub type Attrs = RwLock<HashMap<String, ObjP>>;
 
 /// Downcast an object pointer to a concrete type, and do something with that object.
 pub fn with_obj_downcast<T, Out>(ptr: ObjP, closure: impl FnOnce(&T) -> Out) -> Out
 where
     T: Obj + 'static,
 {
-    let borrowed = ptr.try_read().expect("could not lock object for reading");
+    if let Some(obj) = ptr.as_any().downcast_ref::<T>() {
-    if let Some(obj) = borrowed.as_any().downcast_ref::<T>() {
         closure(obj)
     } else {
         panic!(
             "could not downcast '{:?}' to {}",
-            borrowed,
+            ptr,
             std::any::type_name::<T>()
         )
     }
@@ -36,8 +35,7 @@ pub fn obj_is_inst<T>(ptr: &ObjP) -> bool
 where
     T: Obj + 'static,
 {
-    let borrowed = ptr.try_read().expect("could not lock object for reading");
+    ptr.as_any().downcast_ref::<T>().is_some()
-    borrowed.as_any().downcast_ref::<T>().is_some()
 }
 
 /// Builtin types macro
@@ -64,8 +62,9 @@ macro_rules! builtin_types {
             // instantiate
             $(
                 if stringify!($type_name) != "Type" {
-                    let ty = Ptr::clone(&TYPES.try_read().unwrap()[stringify!($type_name)]);
+                    let mut types_ptr = TYPES.try_write().unwrap();
-                    ty.try_write().unwrap().instantiate();
+                    let mut ty = types_ptr.get_mut(stringify!($type_name)).unwrap();
+                    Arc::get_mut(&mut ty).unwrap().instantiate();
                 }
             )+
 
@@ -74,19 +73,16 @@ macro_rules! builtin_types {
                 {
                     let name = StrInst::create(stringify!($type_name));
                     let ty = Ptr::clone(&TYPES.try_read().unwrap()[stringify!($type_name)]);
-                    ty.try_write()
+                    ty.set_attr("__name__", name);
-                        .unwrap()
-                        .set_attr("__name__", name);
                 }
             )+
 
             // vtable
             $(
                 {
-                    let ptr = Ptr::clone(&TYPES.try_read().unwrap()[stringify!($type_name)]);
+                    let ptr = Ptr::clone(&TYPES.try_write().unwrap()[stringify!($type_name)]);
-                    let ty = ptr.try_write().unwrap();
                     $(
-                        ty.vtable.insert($vtable_name.into(), $vtable_value);
+                        ptr.vtable.insert($vtable_name.into(), $vtable_value);
                     )*
                 }
             )+
@@ -110,13 +106,16 @@ pub(crate) fn init_types() {
 
     // Init type_type here
     {
-        let type_type_ptr = Ptr::clone(&TYPES.try_read().unwrap()["Type"]);
+        let types_ptr = TYPES.try_read().unwrap();
-        let mut type_type = type_type_ptr.try_write().unwrap();
+        let type_ptr = types_ptr.get("Type").unwrap();
-        type_type.set_attr(
+        type_ptr.set_attr(
             "__type__",
             Ptr::clone(&TYPES.try_read().unwrap()["Type"]) as ObjP,
         );
-        type_type.base.is_instantiated = true;
+        drop(types_ptr);
+        //let mut types_ptr = TYPES.try_write().unwrap();
+        //let mut type_ptr = types_ptr.get_mut("Type").unwrap();
+        //Arc::get_mut(&mut type_ptr).unwrap().base.is_instantiated = true;
     }
 
     // Init the rest of the types
@@ -130,7 +129,7 @@ fn placeholder(_: &mut Vm, _: Vec<ObjP>) -> ObjP {
 }
 
 fn to_string(_: &mut Vm, args: Vec<ObjP>) -> ObjP {
-    let str_value = format!("{}", args[0].try_read().unwrap());
+    let str_value = format!("{}", args[0]);
     StrInst::create(str_value)
 }
 
@@ -153,7 +152,7 @@ builtin_types! {
 /// I would implement this as a `From<T>` but it doesn't seem to work for a foreign type, and I'm
 /// not sure why.
 pub fn make_ptr<T: Obj>(obj: T) -> Ptr<T> {
-    Arc::new(RwLock::new(obj))
+    Arc::new(obj)
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -165,14 +164,15 @@ pub trait Obj: Debug + Display + Any + Send + Sync {
 
     fn is_instantiated(&self) -> bool;
     fn attrs(&self) -> &Attrs;
-    fn attrs_mut(&mut self) -> &mut Attrs;
 
-    fn set_attr(&mut self, name: &str, value: ObjP) {
+    fn set_attr(&self, name: &str, value: ObjP) {
-        self.attrs_mut().insert(name.to_string(), value);
+        let mut borrowed = self.attrs().try_write().unwrap();
+        borrowed.insert(name.to_string(), value);
     }
 
     fn get_attr(&self, name: &str) -> Option<ObjP> {
-        self.attrs().get(name).map(Arc::clone)
+        let borrowed = self.attrs().try_read().unwrap();
+        borrowed.get(name).map(Arc::clone)
     }
 
     fn type_inst(&self) -> ObjP {
@@ -210,12 +210,21 @@ pub trait Obj: Debug + Display + Any + Send + Sync {
 // BaseObjInst
 ////////////////////////////////////////////////////////////////////////////////
 
-#[derive(Debug, Default, Clone)]
+#[derive(Debug, Default)]
 struct BaseObjInst {
-    attrs: HashMap<String, ObjP>,
+    attrs: RwLock<HashMap<String, ObjP>>,
     is_instantiated: bool,
 }
 
+impl Clone for BaseObjInst {
+    fn clone(&self) -> Self {
+        Self {
+            attrs: RwLock::new(self.attrs.try_read().unwrap().clone()),
+            is_instantiated: self.is_instantiated,
+        }
+    }
+}
+
 impl Display for BaseObjInst {
     fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
         write!(fmt, "<BaseObjInst at {:x}>", (self as *const _ as usize))
@@ -266,18 +275,15 @@ impl Obj for BaseObjInst {
         &self.attrs
     }
 
-    fn attrs_mut(&mut self) -> &mut Attrs {
-        &mut self.attrs
-    }
-
     fn equals(&self, other: &dyn Obj) -> bool {
         if let Some(other) = other.as_any().downcast_ref::<BaseObjInst>() {
             // compare all attrs
-            self.attrs.iter().all(|(k1, v1)| {
+            let borrowed = self.attrs.try_read().unwrap();
-                other
+            borrowed.iter().all(|(k1, v1)| {
-                    .attrs
+                let borrowed = other.attrs.try_read().unwrap();
+                borrowed
                     .get(k1)
-                    .map(|v2| v2.try_read().unwrap().equals(&*v1.try_read().unwrap()))
+                    .map(|v2| v2.equals(v1.as_ref()))
                     .unwrap_or(false)
             }) && self.is_instantiated == other.is_instantiated
         } else {
@@ -304,10 +310,6 @@ macro_rules! impl_base_obj {
             self.$base_name.attrs()
         }
 
-        fn attrs_mut(&mut self) -> &mut Attrs {
-            self.$base_name.attrs_mut()
-        }
-
         fn as_any(&self) -> &dyn Any {
             self
         }
@@ -950,7 +952,7 @@ impl MethodInst {
 
 impl Display for MethodInst {
     fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
-        write!(fmt, "{}", self.function.try_read().unwrap())
+        write!(fmt, "{}", self.function)
     }
 }
 
@@ -964,11 +966,11 @@ impl Obj for MethodInst {
     }
 
     fn arity(&self) -> Option<Argc> {
-        self.function.try_read().unwrap().arity()
+        self.function.arity()
     }
 
     fn call(&self, vm: &mut Vm, argc: Argc) {
-        self.function.try_read().unwrap().call(vm, argc)
+        self.function.call(vm, argc)
     }
 
     fn equals(&self, other: &dyn Obj) -> bool {
@@ -992,19 +994,19 @@ fn test_new_objects() {
     init_types();
 
     let type_value = TypeInst::create("Type");
-    assert_eq!(&*type_value.try_read().unwrap().type_name(), "Type");
+    assert_eq!(&*type_value.type_name(), "Type");
 
     let str_value = StrInst::create("asdfasdfasdfasdfasdf");
-    assert_eq!(&*str_value.try_read().unwrap().type_name(), "Str");
+    assert_eq!(&*str_value.type_name(), "Str");
 
     let int_value = IntInst::create(1234);
-    assert_eq!(&*int_value.try_read().unwrap().type_name(), "Int");
+    assert_eq!(&*int_value.type_name(), "Int");
 
     let float_value = FloatInst::create(1234.5678);
-    assert_eq!(&*float_value.try_read().unwrap().type_name(), "Float");
+    assert_eq!(&*float_value.type_name(), "Float");
 
     let nil_value = NilInst::create();
-    assert_eq!(&*nil_value.try_read().unwrap().type_name(), "Nil");
+    assert_eq!(&*nil_value.type_name(), "Nil");
 }
 
 #[test]
@@ -1014,46 +1016,33 @@ fn test_obj_equals() {
     let int1 = IntInst::create(1234);
     let int2 = IntInst::create(1234);
 
-    assert!(int1.try_read().unwrap().equals(&*int2.try_read().unwrap()));
+    assert!(int1.equals(int2.as_ref()));
-    assert!(int2.try_read().unwrap().equals(&*int1.try_read().unwrap()));
+    assert!(int2.equals(int1.as_ref()));
 
     let float1 = FloatInst::create(1234.0);
-    assert!(int1
+    assert!(int1.equals(float1.as_ref()));
-        .try_read()
+    assert!(float1.equals(int2.as_ref()));
-        .unwrap()
-        .equals(&*float1.try_read().unwrap()));
-    assert!(float1
-        .try_read()
-        .unwrap()
-        .equals(&*int2.try_read().unwrap()));
 
     // self-equality
     let str1 = StrInst::create("1234");
-    assert!(str1.try_read().unwrap().equals(&*str1.try_read().unwrap()));
+    assert!(str1.equals(str1.as_ref()));
 
     let str2 = StrInst::create("1234");
-    assert!(str1.try_read().unwrap().equals(&*str2.try_read().unwrap()));
+    assert!(str1.equals(str2.as_ref()));
-    assert!(str2.try_read().unwrap().equals(&*str1.try_read().unwrap()));
+    assert!(str2.equals(str1.as_ref()));
 
-    assert!(!str1
+    assert!(!str1.equals(float1.as_ref()));
-        .try_read()
+    assert!(!str1.equals(int1.as_ref()));
-        .unwrap()
-        .equals(&*float1.try_read().unwrap()));
-    assert!(!str1.try_read().unwrap().equals(&*int1.try_read().unwrap()));
 
     let obj1 = ObjInst::create();
     let obj2 = ObjInst::create();
-    assert!(obj1.try_read().unwrap().equals(&*obj2.try_read().unwrap()));
+    assert!(obj1.equals(obj2.as_ref()));
 
     // these objects aren't equal anymore
-    obj1.try_write()
+    obj1.set_attr("my_attr", Ptr::clone(&str2) as ObjP);
-        .unwrap()
+    assert!(!obj1.equals(obj2.as_ref()));
-        .set_attr("my_attr", Ptr::clone(&str2) as ObjP);
-    assert!(!obj1.try_read().unwrap().equals(&*obj2.try_read().unwrap()));
 
     // but now they are!
-    obj2.try_write()
+    obj2.set_attr("my_attr", Ptr::clone(&str2) as ObjP);
-        .unwrap()
+    assert!(obj2.equals(obj1.as_ref()));
-        .set_attr("my_attr", Ptr::clone(&str2) as ObjP);
-    assert!(obj2.try_read().unwrap().equals(&*obj1.try_read().unwrap()));
 }

src/vm.rs

@@ -233,7 +233,7 @@ impl Vm {
                     let name =
                         with_obj_downcast(name_obj, |name: &StrInst| Arc::clone(&name.str_value()));
                     let owner = self.pop();
-                    let value = owner.try_read().unwrap().get_attr(&name);
+                    let value = owner.get_attr(&name);
                     if let Some(value) = value {
                         self.push(value);
                     } else {
@@ -243,7 +243,7 @@ impl Vm {
                         todo!(
                             "throw an error because we couldn't read attr '{}' on '{}'",
                             name,
-                            owner.try_read().unwrap(),
+                            owner,
                         );
                     }
                 }
@@ -254,8 +254,7 @@ impl Vm {
                     let value = self.pop();
                     let target = self.pop();
 
-                    let mut target_ptr = target.try_write().unwrap();
+                    target.set_attr(&name, value);
-                    target_ptr.set_attr(&name, value);
                 }
                 Op::Jump(offset) => {
                     let base = (self.ip() - 1) as JumpOpArg;
@@ -265,14 +264,14 @@ impl Vm {
                 Op::JumpFalse(offset) => {
                     let base = (self.ip() - 1) as JumpOpArg;
                     let value = self.peek();
-                    if !value.try_read().unwrap().is_truthy() {
+                    if !value.is_truthy() {
                         self.set_ip((base + offset) as usize);
                     }
                 }
                 Op::JumpTrue(offset) => {
                     let base = (self.ip() - 1) as JumpOpArg;
                     let value = self.peek();
-                    if value.try_read().unwrap().is_truthy() {
+                    if value.is_truthy() {
                         self.set_ip((base + offset) as usize);
                     }
                 }
@@ -280,7 +279,6 @@ impl Vm {
                     let argc = argc as usize;
                     let index = self.stack.len() - argc - 1;
                     let fun_ptr = Ptr::clone(&self.stack[index]);
-                    let fun_ptr = fun_ptr.try_read().unwrap();
 
                     let arity = if let Some(arity) = fun_ptr.arity() {
                         arity as usize