Rework builtins offsets & registry

Introduces a new-type "BuiltinId" to get away from using usize offsets
explicitly throughout the code.

crates/compiler/src/builtins.rs

@@ -12,6 +12,7 @@
 // this program. If not, see <https://www.gnu.org/licenses/>.
 //
 
+use bincode::{Decode, Encode};
 use lazy_static::lazy_static;
 use moor_values::var::Symbol;
 use moor_values::var::VarType;
@@ -22,7 +23,8 @@ use ArgType::{Any, AnyNum, Typed};
 use VarType::{TYPE_FLOAT, TYPE_INT, TYPE_LIST, TYPE_OBJ, TYPE_STR};
 
 lazy_static! {
-    pub static ref BUILTIN_DESCRIPTORS: Vec<Builtin> = mk_builtin_table();
+    static ref BUILTIN_DESCRIPTORS: Vec<Builtin> = mk_builtin_table();
+    pub static ref BUILTINS: Builtins = Builtins::new();
 }
 pub enum ArgCount {
     Q(usize),
@@ -949,27 +951,70 @@ fn mk_builtin_table() -> Vec<Builtin> {
     ]
 }
 
-pub fn make_builtin_offsets() -> HashMap<Symbol, usize> {
+#[derive(Clone, Copy, Debug, PartialOrd, PartialEq, Eq, Hash, Encode, Decode)]
+pub struct BuiltinId(pub u16);
+
+/// The dictionary of all builtins indexed by their name, and by their unique ID.
+pub struct Builtins {
+    pub offsets: HashMap<Symbol, BuiltinId>,
+    pub names: HashMap<BuiltinId, Symbol>,
+}
+
+impl Default for Builtins {
+    fn default() -> Self {
+        Self::new()
+    }
+}
+
+impl Builtins {
+    pub fn new() -> Self {
+        let offsets = make_builtin_offsets();
+        let names = make_offsets_builtins();
+        Self { offsets, names }
+    }
+
+    pub fn find_builtin(&self, bf_name: Symbol) -> Option<BuiltinId> {
+        self.offsets.get(&bf_name).cloned()
+    }
+
+    pub fn name_of(&self, offset: BuiltinId) -> Option<Symbol> {
+        self.names.get(&offset).cloned()
+    }
+
+    pub fn len(&self) -> usize {
+        self.offsets.len()
+    }
+
+    pub fn description_for(&self, offset: BuiltinId) -> Option<&Builtin> {
+        BUILTIN_DESCRIPTORS.get(offset.0 as usize)
+    }
+
+    pub fn descriptions(&self) -> impl Iterator<Item = &Builtin> {
+        BUILTIN_DESCRIPTORS.iter()
+    }
+}
+
+fn make_builtin_offsets() -> HashMap<Symbol, BuiltinId> {
     let mut b = HashMap::new();
     for (offset, builtin) in BUILTIN_DESCRIPTORS.iter().enumerate() {
-        b.insert(builtin.name, offset);
+        b.insert(builtin.name, BuiltinId(offset as u16));
     }
 
     b
 }
-pub fn make_offsets_builtins() -> HashMap<usize, Symbol> {
+pub fn make_offsets_builtins() -> HashMap<BuiltinId, Symbol> {
     let mut b = HashMap::new();
     for (offset, builtin) in BUILTIN_DESCRIPTORS.iter().enumerate() {
-        b.insert(offset, builtin.name);
+        b.insert(BuiltinId(offset as u16), builtin.name);
     }
 
     b
 }
 
 pub fn offset_for_builtin(bf_name: &str) -> usize {
     let bf_name = Symbol::mk(bf_name);
-    BUILTIN_DESCRIPTORS
-        .iter()
-        .position(|b| b.name == bf_name)
-        .unwrap()
+    let builtin = BUILTINS
+        .find_builtin(bf_name)
+        .unwrap_or_else(|| panic!("Unknown builtin: {}", bf_name));
+    builtin.0 as usize
 }

crates/compiler/src/codegen.rs

@@ -16,7 +16,6 @@
 use std::collections::HashMap;
 use std::sync::Arc;
 
-use moor_values::var::Symbol;
 use tracing::error;
 
 use moor_values::var::Var;
@@ -25,7 +24,7 @@ use moor_values::var::Variant;
 use crate::ast::{
     Arg, BinaryOp, CatchCodes, Expr, ScatterItem, ScatterKind, Stmt, StmtNode, UnaryOp,
 };
-use crate::builtins::make_builtin_offsets;
+use crate::builtins::BUILTINS;
 use crate::labels::{JumpLabel, Label, Offset};
 use crate::names::{Name, Names, UnboundName};
 use crate::opcode::Op::Jump;
@@ -53,17 +52,12 @@ pub struct CodegenState {
     pub(crate) saved_stack: Option<Offset>,
     pub(crate) cur_stack: usize,
     pub(crate) max_stack: usize,
-    pub(crate) builtins: HashMap<Symbol, usize>,
     pub(crate) fork_vectors: Vec<Vec<Op>>,
     pub(crate) line_number_spans: Vec<(usize, usize)>,
 }
 
 impl CodegenState {
-    pub fn new(
-        var_names: Names,
-        binding_mappings: HashMap<UnboundName, Name>,
-        builtins: HashMap<Symbol, usize>,
-    ) -> Self {
+    pub fn new(var_names: Names, binding_mappings: HashMap<UnboundName, Name>) -> Self {
         Self {
             ops: vec![],
             jumps: vec![],
@@ -74,7 +68,6 @@ impl CodegenState {
             saved_stack: None,
             cur_stack: 0,
             max_stack: 0,
-            builtins,
             fork_vectors: vec![],
             line_number_spans: vec![],
         }
@@ -422,13 +415,12 @@ impl CodegenState {
             }
             Expr::Call { function, args } => {
                 // Lookup builtin.
-                let Some(builtin) = self.builtins.get(function) else {
+                let Some(id) = BUILTINS.find_builtin(*function) else {
                     error!("Unknown builtin function: {}({:?}", function, args);
                     return Err(CompileError::UnknownBuiltinFunction(function.to_string()));
                 };
-                let builtin = *builtin;
                 self.generate_arg_list(args)?;
-                self.emit(Op::FuncCall { id: builtin as u16 });
+                self.emit(Op::FuncCall { id });
             }
             Expr::Verb {
                 args,
@@ -845,11 +837,10 @@ pub fn compile(program: &str, options: CompileOptions) -> Result<Program, Compil
     let compile_span = tracing::trace_span!("compile");
     let _compile_guard = compile_span.enter();
 
-    let builtins = make_builtin_offsets();
     let parse = parse_program(program, options)?;
 
     // Generate the code into 'cg_state'.
-    let mut cg_state = CodegenState::new(parse.names, parse.names_mapping, builtins);
+    let mut cg_state = CodegenState::new(parse.names, parse.names_mapping);
     for x in parse.stmts {
         cg_state.generate_stmt(&x)?;
     }

crates/compiler/src/codegen_tests.rs

@@ -14,7 +14,7 @@
 
 #[cfg(test)]
 mod tests {
-    use crate::builtins::BUILTIN_DESCRIPTORS;
+    use crate::builtins::BUILTINS;
     use crate::codegen::compile;
     use crate::labels::{Label, Offset};
     use crate::opcode::Op::*;
@@ -514,14 +514,15 @@ mod tests {
          5: 012 000             * CALL_FUNC disassemble
          7: 111                   POP
         */
+        let disassemble = BUILTINS.find_builtin(Symbol::mk("disassemble")).unwrap();
         assert_eq!(
             *binary.main_vector.as_ref(),
             vec![
                 Push(player), // Player
                 MakeSingletonList,
                 Imm(test),
                 ListAddTail,
-                FuncCall { id: 0 },
+                FuncCall { id: disassemble },
                 Pop,
                 Done
             ]
@@ -949,10 +950,7 @@ mod tests {
          15: 014                 * INDEX
          16: 108                   RETURN
         */
-        let raise_num = BUILTIN_DESCRIPTORS
-            .iter()
-            .position(|b| b.name == Symbol::mk("raise"))
-            .unwrap();
+        let raise = BUILTINS.find_builtin(Symbol::mk("raise")).unwrap();
         assert_eq!(
             *binary.main_vector.as_ref(),
             vec![
@@ -963,9 +961,7 @@ mod tests {
                 },
                 ImmErr(E_INVARG),
                 MakeSingletonList,
-                FuncCall {
-                    id: raise_num as u16
-                },
+                FuncCall { id: raise },
                 EndCatch(1.into()),
                 ImmInt(1),
                 Ref,

crates/compiler/src/decompile.rs

@@ -12,7 +12,6 @@
 // this program. If not, see <https://www.gnu.org/licenses/>.
 //
 
-use moor_values::var::Symbol;
 use moor_values::var::{v_err, v_int, v_none, v_objid, Var};
 use moor_values::var::{v_float, Variant};
 use std::collections::{HashMap, VecDeque};
@@ -21,13 +20,14 @@ use crate::ast::{
     Arg, BinaryOp, CatchCodes, CondArm, ElseArm, ExceptArm, Expr, ScatterItem, ScatterKind, Stmt,
     StmtNode, UnaryOp,
 };
-use crate::builtins::make_offsets_builtins;
+use crate::builtins::BuiltinId;
 use crate::decompile::DecompileError::{BuiltinNotFound, MalformedProgram};
 use crate::labels::{JumpLabel, Label};
 use crate::names::{Name, UnboundName, UnboundNames};
 use crate::opcode::{Op, ScatterLabel};
 use crate::parse::Parse;
 use crate::program::Program;
+use crate::BUILTINS;
 
 #[derive(Debug, thiserror::Error)]
 pub enum DecompileError {
@@ -36,7 +36,7 @@ pub enum DecompileError {
     #[error("name not found: {0:?}")]
     NameNotFound(Name),
     #[error("builtin function not found: #{0:?}")]
-    BuiltinNotFound(usize),
+    BuiltinNotFound(BuiltinId),
     #[error("label not found: {0:?}")]
     LabelNotFound(Label),
     #[error("malformed program: {0}")]
@@ -53,7 +53,6 @@ struct Decompile {
     /// The current position in the opcode stream as it is being decompiled.
     position: usize,
     expr_stack: VecDeque<Expr>,
-    builtins: HashMap<usize, Symbol>,
     statements: Vec<Stmt>,
     names_mapping: HashMap<Name, UnboundName>,
 }
@@ -405,7 +404,6 @@ impl Decompile {
                     fork_vector: Some(fv_offset.0 as _),
                     position: 0,
                     expr_stack: self.expr_stack.clone(),
-                    builtins: self.builtins.clone(),
                     statements: vec![],
                     names_mapping: self.names_mapping.clone(),
                 };
@@ -561,8 +559,8 @@ impl Decompile {
             }
             Op::FuncCall { id } => {
                 let args = self.pop_expr()?;
-                let Some(builtin) = self.builtins.get(&(id as usize)) else {
-                    return Err(BuiltinNotFound(id as usize));
+                let Some(function) = BUILTINS.name_of(id) else {
+                    return Err(BuiltinNotFound(id));
                 };
 
                 // Have to reconstruct arg list ...
@@ -571,10 +569,7 @@ impl Decompile {
                         format!("expected list of args, got {:?} instead", args).to_string(),
                     ));
                 };
-                self.push_expr(Expr::Call {
-                    function: *builtin,
-                    args,
-                })
+                self.push_expr(Expr::Call { function, args })
             }
             Op::CallVerb => {
                 let args = self.pop_expr()?;
@@ -958,13 +953,11 @@ pub fn program_to_tree(program: &Program) -> Result<Parse, DecompileError> {
         unbound_to_bound.insert(ub, bound_name);
     }
 
-    let builtins = make_offsets_builtins();
     let mut decompile = Decompile {
         program: program.clone(),
         fork_vector: None,
         position: 0,
         expr_stack: Default::default(),
-        builtins,
         statements: vec![],
         names_mapping: bound_to_unbound,
     };

crates/compiler/src/lib.rs

@@ -29,7 +29,7 @@ mod names;
 mod opcode;
 mod program;
 
-pub use crate::builtins::{offset_for_builtin, ArgCount, ArgType, Builtin, BUILTIN_DESCRIPTORS};
+pub use crate::builtins::{offset_for_builtin, ArgCount, ArgType, Builtin, BuiltinId, BUILTINS};
 pub use crate::codegen::compile;
 pub use crate::decompile::program_to_tree;
 pub use crate::labels::{JumpLabel, Label, Offset};

crates/compiler/src/opcode.rs

@@ -12,13 +12,13 @@
 // this program. If not, see <https://www.gnu.org/licenses/>.
 //
 
+use crate::builtins::BuiltinId;
+use crate::labels::{Label, Offset};
+use crate::names::Name;
 use bincode::{Decode, Encode};
 use moor_values::var::Error;
 use moor_values::var::Objid;
 
-use crate::labels::{Label, Offset};
-use crate::names::Name;
-
 #[derive(Clone, Debug, PartialEq, PartialOrd, Encode, Decode)]
 pub enum Op {
     Add,
@@ -53,7 +53,7 @@ pub enum Op {
         id: Option<Name>,
     },
     FuncCall {
-        id: u16,
+        id: BuiltinId,
     },
     Ge,
     GetProp,

crates/kernel/src/builtins/bf_list_sets.rs

@@ -14,13 +14,12 @@
 
 use std::ops::BitOr;
 
-use onig::{Region, SearchOptions, SyntaxOperator};
-
 use moor_compiler::offset_for_builtin;
 use moor_values::var::Error::{E_ARGS, E_INVARG, E_TYPE};
 use moor_values::var::Variant;
 use moor_values::var::{v_empty_list, v_int, v_list, v_string};
 use moor_values::var::{v_listv, Error};
+use onig::{Region, SearchOptions, SyntaxOperator};
 
 use crate::bf_declare;
 use crate::builtins::BfRet::Ret;