Merge #171

171: Use arbitrary self types so that we can use `self: Gc<Self>`. r=jacob-hughes a=ltratt

VM objects are all allocated on the heap. The Val::recover function turns an object back into a `Val`, but is deeply unsafe: if you didn't allocate the object via `Val::from_obj` all sorts of bad, weird things will happen. Previously we entirely relied on the programmer doing The Right Thing in order that the bad case didn't occur.

This commit makes it *much* clearer that objects are `Gc`d things by making their `self` types `self: Gc<Self>`. For example this means that String::concatenate function previously looked like this:

```rust
pub fn concatenate(&self, ...) -> Result<Val, Box<VMError>> {
    ...
    Val::recover(self)
    ...
}
```

But you could call that with a non-GCd `&self` and then things would explode. This function now looks like:

```rust
pub fn concatenate(self: Gc<Self>, ...) -> Result<Val, Box<VMError>> {
    ...
    Val::recover(self)
    ...
}
```

and `Val::recover` now looks like:

```rust
pub fn recover<T: Obj + 'static>(obj: Gc<T>) -> Self {
```

so we can statically guarantee that you can't `recover` something that wasn't Gc'd.rs

There is more to do to squeeze more static guarantees out of the codebase, but this is the minimal change which starts that process off.

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

.buildbot.sh

@@ -9,20 +9,13 @@ export CARGO_HOME="`pwd`/.cargo"
 export RUSTUP_HOME="`pwd`/.rustup"
 
 curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs > rustup.sh
-sh rustup.sh --default-host x86_64-unknown-linux-gnu --default-toolchain nightly -y --no-modify-path
+# Install no toolchain initially to ensure toolchain rollback.
+sh rustup.sh --default-host x86_64-unknown-linux-gnu --default-toolchain none -y --no-modify-path
 
 export PATH=`pwd`/.cargo/bin/:$PATH
+rustup install nightly
 
-# Sometimes rustfmt is so broken that there is no way to install it at all.
-# Rather than refusing to merge, we just can't rust rustfmt at such a point.
-rustup component add --toolchain nightly rustfmt-preview \
-    || cargo +nightly install --force rustfmt-nightly \
-    || true
-rustfmt=0
-cargo fmt 2>&1 | grep "not installed for the toolchain" > /dev/null || rustfmt=1
-if [ $rustfmt -eq 1 ]; then
-    cargo +nightly fmt --all -- --check
-fi
+cargo +nightly fmt --all -- --check
 
 cargo test
 cargo test --release

src/lib/mod.rs

@@ -12,8 +12,10 @@
 
 #![feature(alloc_layout_extra)]
 #![feature(allocator_api)]
+#![feature(arbitrary_self_types)]
 #![feature(box_patterns)]
 #![feature(coerce_unsized)]
+#![feature(dispatch_from_dyn)]
 #![feature(unsize)]
 #![allow(clippy::cognitive_complexity)]
 #![allow(clippy::float_cmp)]

src/lib/vm/core.rs

@@ -494,12 +494,13 @@ impl VM {
                 }
                 Instr::InstVarLookup(n) => {
                     let inst = stry!(rcv.tobj(self));
-                    self.stack.push(unsafe { inst.unchecked_inst_var_get(n) });
+                    self.stack
+                        .push(unsafe { inst.as_gc().unchecked_inst_var_get(n) });
                     pc += 1;
                 }
                 Instr::InstVarSet(n) => {
                     let inst = stry!(rcv.tobj(self));
-                    unsafe { inst.unchecked_inst_var_set(n, self.stack.peek()) };
+                    unsafe { inst.as_gc().unchecked_inst_var_set(n, self.stack.peek()) };
                     pc += 1;
                 }
                 Instr::Int(i) => {
@@ -674,15 +675,15 @@ impl VM {
             }
             Primitive::At => {
                 let rcv_tobj = stry!(rcv.tobj(self));
-                let arr = stry!(rcv_tobj.to_array());
+                let arr = stry!(rcv_tobj.as_gc().to_array());
                 let idx = stry!(self.stack.pop().as_usize(self));
                 let v = stry!(arr.at(self, idx));
                 self.stack.push(v);
                 SendReturn::Val
             }
             Primitive::AtPut => {
                 let rcv_tobj = stry!(rcv.tobj(self));
-                let arr = stry!(rcv_tobj.to_array());
+                let arr = stry!(rcv_tobj.as_gc().to_array());
                 let v = self.stack.pop();
                 let idx = stry!(self.stack.pop().as_usize(self));
                 stry!(arr.at_put(self, idx, v));
@@ -824,15 +825,15 @@ impl VM {
             Primitive::InstVarAt => {
                 let n = stry!(self.stack.pop().as_usize(self));
                 let inst = stry!(rcv.tobj(self));
-                let v = stry!(inst.inst_var_at(self, n));
+                let v = stry!(inst.as_gc().inst_var_at(self, n));
                 self.stack.push(v);
                 SendReturn::Val
             }
             Primitive::InstVarAtPut => {
                 let v = self.stack.pop();
                 let n = stry!(self.stack.pop().as_usize(self));
                 let inst = stry!(rcv.tobj(self));
-                stry!(inst.inst_var_at_put(self, n, v));
+                stry!(inst.as_gc().inst_var_at_put(self, n, v));
                 self.stack.push(rcv);
                 SendReturn::Val
             }
@@ -854,7 +855,7 @@ impl VM {
                 // Only Arrays and Strings can have this primitive installed.
                 debug_assert!(rcv.valkind() == ValKind::GCBOX);
                 let tobj = rcv.tobj(self).unwrap();
-                let v = Val::from_usize(self, tobj.length());
+                let v = Val::from_usize(self, tobj.as_gc().length());
                 self.stack.push(v);
                 SendReturn::Val
             }
@@ -1088,17 +1089,17 @@ impl VM {
         }
 
         let args_tobj = stry!(args_val.tobj(self));
-        let args = stry!(args_tobj.to_array());
+        let args = stry!(args_tobj.as_gc().to_array());
         let sig = stry!(String_::symbol_to_string_(self, sig_val));
         let cls = stry!(cls_val.downcast::<Class>(self));
         match cls.get_method(self, sig.as_str()) {
             Ok(m) => {
-                if args_tobj.length() != m.num_params() {
+                if args_tobj.as_gc().length() != m.num_params() {
                     return SendReturn::Err(VMError::new(
                         self,
                         VMErrorKind::WrongNumberOfArgs {
                             wanted: m.num_params(),
-                            got: args_tobj.length(),
+                            got: args_tobj.as_gc().length(),
                         },
                     ));
                 }

src/lib/vm/objects/array.rs

@@ -2,6 +2,8 @@
 
 use std::{cell::UnsafeCell, collections::hash_map::DefaultHasher, hash::Hasher};
 
+use rboehm::Gc;
+
 use crate::vm::{
     core::VM,
     error::{VMError, VMErrorKind},
@@ -12,18 +14,18 @@ use crate::vm::{
 pub trait Array {
     /// Return the item at index `idx` (using SOM indexing starting at 1) or an error if the index
     /// is invalid.
-    fn at(&self, vm: &VM, idx: usize) -> Result<Val, Box<VMError>>;
+    fn at(self: Gc<Self>, vm: &VM, idx: usize) -> Result<Val, Box<VMError>>;
 
     /// Return the item at index `idx` (using SOM indexing starting at 1). This will lead to
     /// undefined behaviour if the index is invalid.
-    unsafe fn unchecked_at(&self, idx: usize) -> Val;
+    unsafe fn unchecked_at(self: Gc<Self>, idx: usize) -> Val;
 
     /// Set the item at index `idx` (using SOM indexing starting at 1) to `val` or return an error
     /// if the index is invalid.
-    fn at_put(&self, vm: &mut VM, idx: usize, val: Val) -> Result<(), Box<VMError>>;
+    fn at_put(self: Gc<Self>, vm: &mut VM, idx: usize, val: Val) -> Result<(), Box<VMError>>;
 
     /// Iterate over this array's values.
-    fn iter(&self) -> ArrayIterator<'_>;
+    fn iter(self: Gc<Self>) -> ArrayIterator;
 }
 
 #[derive(Debug)]
@@ -32,25 +34,25 @@ pub struct NormalArray {
 }
 
 impl Obj for NormalArray {
-    fn dyn_objtype(&self) -> ObjType {
+    fn dyn_objtype(self: Gc<Self>) -> ObjType {
         ObjType::Array
     }
 
-    fn get_class(&self, vm: &mut VM) -> Val {
+    fn get_class(self: Gc<Self>, vm: &mut VM) -> Val {
         vm.array_cls
     }
 
-    fn to_array(&self) -> Result<&dyn Array, Box<VMError>> {
+    fn to_array(self: Gc<Self>) -> Result<Gc<dyn Array>, Box<VMError>> {
         Ok(self)
     }
 
-    fn hashcode(&self) -> u64 {
+    fn hashcode(self: Gc<Self>) -> u64 {
         let mut hasher = DefaultHasher::new();
-        hasher.write_usize(self as *const _ as usize);
+        hasher.write_usize(Gc::into_raw(self) as *const _ as usize);
         hasher.finish()
     }
 
-    fn length(&self) -> usize {
+    fn length(self: Gc<Self>) -> usize {
         let store = unsafe { &*self.store.get() };
         store.len()
     }
@@ -65,7 +67,7 @@ impl StaticObjType for NormalArray {
 }
 
 impl Array for NormalArray {
-    fn at(&self, vm: &VM, mut idx: usize) -> Result<Val, Box<VMError>> {
+    fn at(self: Gc<Self>, vm: &VM, mut idx: usize) -> Result<Val, Box<VMError>> {
         let store = unsafe { &*self.store.get() };
         if idx > 0 && idx <= store.len() {
             idx -= 1;
@@ -81,15 +83,15 @@ impl Array for NormalArray {
         }
     }
 
-    unsafe fn unchecked_at(&self, mut idx: usize) -> Val {
+    unsafe fn unchecked_at(self: Gc<Self>, mut idx: usize) -> Val {
         debug_assert!(idx > 0);
         let store = &*self.store.get();
         debug_assert!(idx <= store.len());
         idx -= 1;
         *store.get_unchecked(idx)
     }
 
-    fn at_put(&self, vm: &mut VM, mut idx: usize, val: Val) -> Result<(), Box<VMError>> {
+    fn at_put(self: Gc<Self>, vm: &mut VM, mut idx: usize, val: Val) -> Result<(), Box<VMError>> {
         let store = unsafe { &mut *self.store.get() };
         if idx > 0 && idx <= store.len() {
             idx -= 1;
@@ -106,7 +108,7 @@ impl Array for NormalArray {
         }
     }
 
-    fn iter(&self) -> ArrayIterator<'_> {
+    fn iter(self: Gc<Self>) -> ArrayIterator {
         ArrayIterator {
             arr: self,
             len: self.length(),
@@ -143,25 +145,25 @@ pub struct MethodsArray {
 }
 
 impl Obj for MethodsArray {
-    fn dyn_objtype(&self) -> ObjType {
+    fn dyn_objtype(self: Gc<Self>) -> ObjType {
         ObjType::Array
     }
 
-    fn get_class(&self, vm: &mut VM) -> Val {
+    fn get_class(self: Gc<Self>, vm: &mut VM) -> Val {
         vm.array_cls
     }
 
-    fn to_array(&self) -> Result<&dyn Array, Box<VMError>> {
+    fn to_array(self: Gc<Self>) -> Result<Gc<dyn Array>, Box<VMError>> {
         Ok(self)
     }
 
-    fn hashcode(&self) -> u64 {
+    fn hashcode(self: Gc<Self>) -> u64 {
         let mut hasher = DefaultHasher::new();
-        hasher.write_usize(self as *const _ as usize);
+        hasher.write_usize(Gc::into_raw(self) as *const _ as usize);
         hasher.finish()
     }
 
-    fn length(&self) -> usize {
+    fn length(self: Gc<Self>) -> usize {
         let store = unsafe { &*self.store.get() };
         store.len()
     }
@@ -176,7 +178,7 @@ impl StaticObjType for MethodsArray {
 }
 
 impl Array for MethodsArray {
-    fn at(&self, vm: &VM, mut idx: usize) -> Result<Val, Box<VMError>> {
+    fn at(self: Gc<Self>, vm: &VM, mut idx: usize) -> Result<Val, Box<VMError>> {
         let store = unsafe { &*self.store.get() };
         if idx > 0 && idx <= store.len() {
             idx -= 1;
@@ -192,15 +194,15 @@ impl Array for MethodsArray {
         }
     }
 
-    unsafe fn unchecked_at(&self, mut idx: usize) -> Val {
+    unsafe fn unchecked_at(self: Gc<Self>, mut idx: usize) -> Val {
         debug_assert!(idx > 0);
         let store = &*self.store.get();
         debug_assert!(idx <= store.len());
         idx -= 1;
         *store.get_unchecked(idx)
     }
 
-    fn at_put(&self, vm: &mut VM, mut idx: usize, val: Val) -> Result<(), Box<VMError>> {
+    fn at_put(self: Gc<Self>, vm: &mut VM, mut idx: usize, val: Val) -> Result<(), Box<VMError>> {
         let store = unsafe { &mut *self.store.get() };
         if idx > 0 && idx <= store.len() {
             idx -= 1;
@@ -218,7 +220,7 @@ impl Array for MethodsArray {
         }
     }
 
-    fn iter(&self) -> ArrayIterator<'_> {
+    fn iter(self: Gc<Self>) -> ArrayIterator {
         ArrayIterator {
             arr: self,
             len: self.length(),
@@ -238,13 +240,13 @@ impl MethodsArray {
     }
 }
 
-pub struct ArrayIterator<'a> {
-    arr: &'a dyn Array,
+pub struct ArrayIterator {
+    arr: Gc<dyn Array>,
     len: usize,
     i: usize,
 }
 
-impl<'a> Iterator for ArrayIterator<'a> {
+impl Iterator for ArrayIterator {
     type Item = Val;
 
     fn next(&mut self) -> Option<Val> {

src/lib/vm/objects/block.rs

@@ -33,17 +33,17 @@ pub struct Block {
 }
 
 impl Obj for Block {
-    fn dyn_objtype(&self) -> ObjType {
+    fn dyn_objtype(self: Gc<Self>) -> ObjType {
         ObjType::Block
     }
 
-    fn get_class(&self, _: &mut VM) -> Val {
+    fn get_class(self: Gc<Self>, _: &mut VM) -> Val {
         self.blockn_cls
     }
 
-    fn hashcode(&self) -> u64 {
+    fn hashcode(self: Gc<Self>) -> u64 {
         let mut hasher = DefaultHasher::new();
-        hasher.write_usize(self as *const _ as usize);
+        hasher.write_usize(Gc::into_raw(self) as *const _ as usize);
         hasher.finish()
     }
 }

src/lib/vm/objects/class.rs

@@ -38,35 +38,35 @@ pub struct Class {
 }
 
 impl Obj for Class {
-    fn dyn_objtype(&self) -> ObjType {
+    fn dyn_objtype(self: Gc<Self>) -> ObjType {
         ObjType::Class
     }
 
-    fn get_class(&self, _: &mut VM) -> Val {
+    fn get_class(self: Gc<Self>, _: &mut VM) -> Val {
         debug_assert!(self.metacls.get().valkind() != ValKind::ILLEGAL);
         self.metacls.get()
     }
 
-    fn num_inst_vars(&self) -> usize {
+    fn num_inst_vars(self: Gc<Self>) -> usize {
         let inst_vars = unsafe { &*self.inst_vars.get() };
         inst_vars.len()
     }
 
-    unsafe fn unchecked_inst_var_get(&self, n: usize) -> Val {
+    unsafe fn unchecked_inst_var_get(self: Gc<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) {
+    unsafe fn unchecked_inst_var_set(self: Gc<Self>, n: usize, v: Val) {
         debug_assert!(n < self.num_inst_vars());
         let inst_vars = &mut *self.inst_vars.get();
         inst_vars[n] = v;
     }
 
-    fn hashcode(&self) -> u64 {
+    fn hashcode(self: Gc<Self>) -> u64 {
         let mut hasher = DefaultHasher::new();
-        hasher.write_usize(self as *const _ as usize);
+        hasher.write_usize(Gc::into_raw(self) as *const _ as usize);
         hasher.finish()
     }
 }