From c450e07e082d9d2ce130ff4d71717f4e9f22acc6 Mon Sep 17 00:00:00 2001
From: Alek Ratzloff <alekratz@gmail.com>
Date: Fri, 7 Apr 2023 01:17:22 -0700
Subject: [PATCH] Update GetAttr: use Arc<String> instead of String

This lets us share strings more easily. It needs to be Arc (as opposed
to Rc) because these are stored inside of UserFun objects, which need to
be Send (for some reason).

Signed-off-by: Alek Ratzloff <alekratz@gmail.com>
---
 src/compile/compiler.rs | 17 +++++++++++++++--
 src/vm/inst.rs          |  6 ++++--
 src/vm/mod.rs           | 27 +++++++++------------------
 3 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/src/compile/compiler.rs b/src/compile/compiler.rs
index af05aa4..646eca7 100644
--- a/src/compile/compiler.rs
+++ b/src/compile/compiler.rs
@@ -1,4 +1,5 @@
-use std::collections::VecDeque;
+use std::collections::{HashMap, VecDeque};
+use std::sync::Arc;
 
 use crate::compile::{scope::GlobalScope, thunk::Thunk};
 use crate::obj::prelude::*;
@@ -8,12 +9,24 @@ use crate::vm::inst::*;
 
 pub struct Compiler {
     scope: GlobalScope,
+    attr_names: HashMap<String, Arc<String>>,
 }
 
 impl Compiler {
     pub fn new() -> Self {
         Self {
             scope: Default::default(),
+            attr_names: Default::default(),
+        }
+    }
+
+    fn intern_attr(&mut self, name: &str) -> Arc<String> {
+        if let Some(name) = self.attr_names.get(name) {
+            Arc::clone(name)
+        } else {
+            let name = Arc::new(name.to_string());
+            self.attr_names.insert(name.to_string(), Arc::clone(&name));
+            name
         }
     }
 
@@ -151,7 +164,7 @@ impl Compiler {
             }
             Expr::Get(expr, name) => {
                 let mut thunk = self.emit_expr(expr);
-                thunk.push_back(Inst::GetAttr(name.clone()));
+                thunk.push_back(Inst::GetAttr(self.intern_attr(name)));
                 thunk
             }
             Expr::Atom(atom) => self.emit_atom(atom),
diff --git a/src/vm/inst.rs b/src/vm/inst.rs
index b1075d5..904f3d4 100644
--- a/src/vm/inst.rs
+++ b/src/vm/inst.rs
@@ -1,6 +1,8 @@
 use crate::obj::ObjPtr;
 use crate::vm::name::Name;
 
+use std::sync::Arc;
+
 #[derive(Debug, Clone)]
 pub enum JumpCondition {
     False,
@@ -20,7 +22,7 @@ pub enum Inst {
     Store(Name),
 
     /// Get an attribute from the top item of the stack.
-    GetAttr(String),
+    GetAttr(Arc<String>),
 
     /// Pops the top item off of the stack, discarding it.
     Pop,
@@ -58,7 +60,7 @@ impl Inst {
             Inst::Push(ptr) => format!("push <{:#x}>", (ptr as *const _ as usize)),
             Inst::Load(name) => format!("load {name:?}"),
             Inst::Store(name) => format!("store {name:?}"),
-            Inst::GetAttr(attr) => format!("getattr {attr}"),
+            Inst::GetAttr(name) => format!("getattr {name}"),
             Inst::Pop => format!("pop"),
             Inst::Call(argc) => format!("call {argc}"),
             Inst::Return => format!("return"),
diff --git a/src/vm/mod.rs b/src/vm/mod.rs
index a37daeb..e35060e 100644
--- a/src/vm/mod.rs
+++ b/src/vm/mod.rs
@@ -163,29 +163,20 @@ impl Vm {
                 let value = self.pop_stack().expect("Stack underflow");
                 self.store(name, Some(value));
             }
-            Inst::GetAttr(attr) => {
-                // XXX
-                // there's an annoying thing with this. We can't use
-                // self.pop_stack() and then use "attr" afterwards because they
-                // belong to the same owner (self).
-                // ALSO:
-                // We can't just do peek_stack() either, because since `attr`
-                // above was borrowed while `self` is mutable. If you want to
-                // borrow `self` immutably, you have to drop the reference above
-                // as well.
-                // So either we have to do some unsafe stuff (not a bad option
-                // here, since nothing is going to move) or clone(). Maybe a
-                // `get_attr()` function is in order, I don't know.
-                // For now we're cloning.
-                let attr = attr.clone();
+            Inst::GetAttr(name) => {
+                let name = std::sync::Arc::clone(name);
                 let obj = self
                     .pop_stack()
-                    .expect("Inst::GetAttr executed, but there was no value on top of the stack.");
-                let value = obj.get(&attr);
+                    .expect("Inst::GetAttr, but there was no value on top of the stack.");
+                let value = obj.get(&name);
                 if let Some(value) = value {
                     self.push_stack(value);
                 } else {
-                    todo!("Throw exception: Could not get attr {attr:?} on object {obj:?}");
+                    todo!(
+                        "Throw exception: GetAttr object {:?} did not have attribute {}",
+                        obj,
+                        name,
+                    );
                 }
             }
             Inst::Pop => {