Merge #170

170: Clarify the layers of instance variables. r=vext01 a=ltratt

Because of SOM's meta-class hierarchy, it's quite easy to get a bit lost as to where you are in it. I compounded that by not understanding it at first and hacking it in a bit later. That meant that we had a variable "num_inst_vars" in classes which described how many instance variables *instances* of the class would possess, but not how many the class itself has. Since I just managed to thoroughly confuse myself over this, this commit moves a few things around so that we no longer have that attribute, which hopefully makes things a little clearer for the next person who stumbles into this part of the code.

Co-authored-by: Laurence Tratt <[email protected]>

src/lib/compiler/ast_to_instrs.rs

@@ -64,15 +64,15 @@ impl<'a, 'input> Compiler<'a, 'input> {
             (vm.nil, vm.nil)
         };
 
-        // Create the "main" class.
         let mut errs = vec![];
-        let cls = match compiler.c_class(
+        // Create the metaclass (i.e. for a class C, this creates a class called "C class").
+        let metacls = match compiler.c_class(
             vm,
             lexer,
-            name.clone(),
-            supercls_val,
-            &astcls.inst_vars,
-            &astcls.methods,
+            format!("{} class", name.as_str()).into(),
+            supercls_meta_val,
+            &astcls.class_inst_vars,
+            &astcls.class_methods,
         ) {
             Ok(c) => Some(c),
             Err(e) => {
@@ -81,14 +81,14 @@ impl<'a, 'input> Compiler<'a, 'input> {
             }
         };
 
-        // Create the metaclass (i.e. for a class C, this creates a class called "C class").
-        let metacls = match compiler.c_class(
+        // Create the "main" class.
+        let cls = match compiler.c_class(
             vm,
             lexer,
-            format!("{} class", name.as_str()).into(),
-            supercls_meta_val,
-            &astcls.class_inst_vars,
-            &astcls.class_methods,
+            name.clone(),
+            supercls_val,
+            &astcls.inst_vars,
+            &astcls.methods,
         ) {
             Ok(c) => Some(c),
             Err(e) => {
@@ -147,7 +147,7 @@ impl<'a, 'input> Compiler<'a, 'input> {
         let mut inst_vars = if supercls_val != vm.nil {
             let supercls = supercls_val.downcast::<Class>(vm).unwrap();
             let mut inst_vars =
-                HashMap::with_capacity(supercls.num_inst_vars + ast_inst_vars.len());
+                HashMap::with_capacity(supercls.inst_vars_map.len() + ast_inst_vars.len());
             inst_vars.extend(
                 supercls
                     .inst_vars_map

src/lib/vm/core.rs

@@ -275,7 +275,7 @@ impl VM {
     fn compile(&mut self, path: &Path, inst_vars_allowed: bool) -> Val {
         let (name, cls_val) = compile(self, path);
         let cls: Gc<Class> = cls_val.downcast(self).unwrap();
-        if !inst_vars_allowed && cls.num_inst_vars > 0 {
+        if !inst_vars_allowed && cls.inst_vars_map.len() > 0 {
             panic!("No instance vars allowed in {}", path.to_str().unwrap());
         }
         self.set_global(&name, cls_val);

src/lib/vm/objects/class.rs

@@ -26,7 +26,6 @@ pub struct Class {
     /// Offset to this class's instructions in VM::instrs.
     pub instrs_off: usize,
     supercls: Cell<Val>,
-    pub num_inst_vars: usize,
     pub inst_vars_map: HashMap<SmartString, usize>,
     /// A SOM Array of methods (though note that it is *not* guaranteed that these definitely point
     /// to SOM `Method` instances -- anything can be stored in this array!).
@@ -48,12 +47,19 @@ impl Obj for Class {
         self.metacls.get()
     }
 
+    fn num_inst_vars(&self) -> usize {
+        let inst_vars = unsafe { &*self.inst_vars.get() };
+        inst_vars.len()
+    }
+
     unsafe fn unchecked_inst_var_get(&self, n: usize) -> Val {
+        debug_assert!(n < self.num_inst_vars());
         let inst_vars = &mut *self.inst_vars.get();
         inst_vars[n]
     }
 
     unsafe fn unchecked_inst_var_set(&self, n: usize, v: Val) {
+        debug_assert!(n < self.num_inst_vars());
         let inst_vars = &mut *self.inst_vars.get();
         inst_vars[n] = v;
     }
@@ -101,7 +107,6 @@ impl Class {
             path,
             instrs_off,
             supercls: Cell::new(supercls),
-            num_inst_vars: inst_vars_map.len(),
             inst_vars_map,
             methods,
             methods_map,
@@ -138,8 +143,8 @@ impl Class {
         // references.
         if cls_val.valkind() != ValKind::ILLEGAL {
             let cls: Gc<Class> = cls_val.downcast(vm).unwrap();
-            let mut inst_vars = Vec::with_capacity(cls.num_inst_vars);
-            inst_vars.resize(cls.num_inst_vars, Val::illegal());
+            let mut inst_vars = Vec::with_capacity(cls.inst_vars_map.len());
+            inst_vars.resize(cls.inst_vars_map.len(), vm.nil);
             self.metacls.set(cls_val);
             *unsafe { &mut *self.inst_vars.get() } = inst_vars;
         }

src/lib/vm/objects/instance.rs

@@ -7,7 +7,7 @@ use rboehm::Gc;
 use crate::vm::{
     core::VM,
     objects::{Class, Obj, ObjType, StaticObjType},
-    val::{NotUnboxable, Val},
+    val::{NotUnboxable, Val, ValKind},
 };
 
 /// An instance of a user class.
@@ -32,11 +32,14 @@ impl Obj for Inst {
 
     unsafe fn unchecked_inst_var_get(&self, n: usize) -> Val {
         let inst_vars = &mut *self.inst_vars.get();
+        debug_assert!(n < inst_vars.len());
+        debug_assert!(inst_vars[n].valkind() != ValKind::ILLEGAL);
         inst_vars[n]
     }
 
     unsafe fn unchecked_inst_var_set(&self, n: usize, v: Val) {
         let inst_vars = &mut *self.inst_vars.get();
+        debug_assert!(n < inst_vars.len());
         inst_vars[n] = v;
     }
 
@@ -58,8 +61,8 @@ impl StaticObjType for Inst {
 impl Inst {
     pub fn new(vm: &mut VM, class: Val) -> Val {
         let cls: Gc<Class> = class.downcast(vm).unwrap();
-        let mut inst_vars = Vec::with_capacity(cls.num_inst_vars);
-        inst_vars.resize(cls.num_inst_vars, vm.nil);
+        let mut inst_vars = Vec::with_capacity(cls.inst_vars_map.len());
+        inst_vars.resize(cls.inst_vars_map.len(), vm.nil);
         let inst = Inst {
             class,
             inst_vars: UnsafeCell::new(inst_vars),