irc/evaluator: fix variable lookup, recursive let, and value handling

Bunch of things:

- Decode depth and offset from encoded variable indices
- Pre-allocate Values for recursive let bindings before eval
- Use mk* methods for value copying instead of direct assignment
- Evaluate attrset values immediately to avoid dangling thunks

Signed-off-by: NotAShelf <raf@notashelf.dev>
Change-Id: I4dd40c93d74df5973a642fb9f123e70e6a6a6964

src/irc/evaluator.cpp

@@ -21,15 +21,20 @@ struct IREnvironment {
 
   void bind(Value* val) { bindings.push_back(val); }
 
-  Value* lookup(uint32_t index) {
+  Value* lookup(uint32_t encoded_index) {
+    // Decode the index: high 16 bits = depth, low 16 bits = offset
+    uint32_t depth = encoded_index >> 16;
+    uint32_t offset = encoded_index & 0xFFFF;
+
     IREnvironment* env = this;
-    while (env) {
+    // Skip 'depth' levels to get to the right scope
-      if (index < env->bindings.size()) {
+    for (uint32_t i = 0; i < depth && env; i++) {
-        return env->bindings[index];
-      }
-      index -= env->bindings.size();
       env = env->parent;
     }
+
+    if (env && offset < env->bindings.size()) {
+      return env->bindings[offset];
+    }
     return nullptr;
   }
 
@@ -147,7 +152,35 @@ struct Evaluator::Impl {
         state.error<EvalError>("variable not found").debugThrow();
       }
       force(bound);
-      v = *bound;
+      // Copy the forced value's data into v
+      // For simple types, use mk* methods to ensure proper initialization
+      // For complex types (attrs, lists, functions), direct assignment is safe
+      state.forceValue(*bound, noPos);
+      switch (bound->type()) {
+      case nInt:
+        v.mkInt(bound->integer());
+        break;
+      case nBool:
+        v.mkBool(bound->boolean());
+        break;
+      case nString:
+        v.mkString(bound->c_str());
+        break;
+      case nPath:
+        v.mkPath(bound->path());
+        break;
+      case nNull:
+        v.mkNull();
+        break;
+      case nFloat:
+        v.mkFloat(bound->fpoint());
+        break;
+      default:
+        // For attrs, lists, functions, etc., direct assignment is safe
+        // as they use reference counting internally
+        v = *bound;
+        break;
+      }
     } else if (auto* n = node->get_if<LambdaNode>()) {
       auto lambda_env = env;
       auto body = n->body;
@@ -422,20 +455,33 @@ struct Evaluator::Impl {
       }
     } else if (auto* n = node->get_if<LetNode>()) {
       auto let_env = make_env(env);
+      // Nix's let is recursive: bind all names first, then evaluate
+      // We allocate Values immediately and evaluate into them
+      std::vector<Value*> values;
       for (const auto& [name, expr] : n->bindings) {
-        // Create thunks in let_env so bindings can reference each other
+        Value* val = state.allocValue();
-        Value* val = make_thunk(expr, let_env);
+        values.push_back(val);
         let_env->bind(val);
       }
+      // Now evaluate each binding expression into its pre-allocated Value
+      size_t idx = 0;
+      for (const auto& [name, expr] : n->bindings) {
+        eval_node(expr, *values[idx++], let_env);
+      }
       eval_node(n->body, v, let_env);
     } else if (auto* n = node->get_if<LetRecNode>()) {
       auto letrec_env = make_env(env);
-
+      // Same as LetNode - both are recursive in Nix
+      std::vector<Value*> values;
       for (const auto& [name, expr] : n->bindings) {
-        Value* val = make_thunk(expr, letrec_env);
+        Value* val = state.allocValue();
+        values.push_back(val);
         letrec_env->bind(val);
       }
-
+      size_t idx = 0;
+      for (const auto& [name, expr] : n->bindings) {
+        eval_node(expr, *values[idx++], letrec_env);
+      }
       eval_node(n->body, v, letrec_env);
     } else if (auto* n = node->get_if<AttrsetNode>()) {
       auto bindings = state.buildBindings(n->attrs.size());
@@ -453,9 +499,12 @@ struct Evaluator::Impl {
         }
       }
 
-      // Attributes should be lazy, so store as thunks and not evaluated values
+      // Evaluate attribute values immediately to avoid dangling thunks
+      // Our thunk system is tied to the Evaluator lifetime, so we can't
+      // return lazy thunks that outlive the evaluator
       for (const auto& binding : n->attrs) {
-        Value* attr_val = make_thunk(binding.value, attr_env);
+        Value* attr_val = state.allocValue();
+        eval_node(binding.value, *attr_val, attr_env);
 
         if (binding.is_dynamic()) {
           // Evaluate key expression to get attribute name
@@ -498,7 +547,35 @@ struct Evaluator::Impl {
       if (attr) {
         Value* val = attr->value;
         force(val);
-        v = *val;
+        // Copy the forced value's data into v
+        // For simple types, use mk* methods to ensure proper initialization
+        // For complex types (attrs, lists, functions), direct assignment is safe
+        state.forceValue(*val, noPos);
+        switch (val->type()) {
+        case nInt:
+          v.mkInt(val->integer());
+          break;
+        case nBool:
+          v.mkBool(val->boolean());
+          break;
+        case nString:
+          v.mkString(val->c_str());
+          break;
+        case nPath:
+          v.mkPath(val->path());
+          break;
+        case nNull:
+          v.mkNull();
+          break;
+        case nFloat:
+          v.mkFloat(val->fpoint());
+          break;
+        default:
+          // For attrs, lists, functions, etc., direct assignment is safe
+          // as they use reference counting internally
+          v = *val;
+          break;
+        }
       } else if (n->default_expr) {
         eval_node(*n->default_expr, v, env);
       } else {