refactor(tvix/eval): merge all bindings creation logic r/4989

As of this commit, all three types of bindings scopes are compiled the
same way (i.e. compilation of non-recursive attribute sets has been
switched over to the new code paths).

This sets us up for doing the final implementation of nested attribute
sets.

HOWEVER, this breaks the existing implementation of nested attributes
in non-recursive attribute sets. That implementation is flawed and
unworkable in practice, so we need to do this dance to be able to
implement it correctly.

Change-Id: Iba2545c0d1d6b51f5e1a31a5d005b8d01da546d3
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6782
Reviewed-by: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI

13 files changed, 8 insertions, 138 deletions

diff --git a/tvix/eval/src/compiler/bindings.rs b/tvix/eval/src/compiler/bindings.rs
index 86245270f2..28a9fd1105 100644
--- a/tvix/eval/src/compiler/bindings.rs
+++ b/tvix/eval/src/compiler/bindings.rs
@@ -277,123 +277,6 @@ impl Compiler<'_> {
         }
     }
 
-    /// Compile the statically known entries of an attribute set. Which keys are
-    /// which is not known from the iterator, so discovered dynamic keys are
-    /// returned from here.
-    fn compile_static_attr_entries(
-        &mut self,
-        count: &mut usize,
-        entries: AstChildren<ast::AttrpathValue>,
-    ) -> Vec<ast::AttrpathValue> {
-        let mut dynamic_attrs: Vec<ast::AttrpathValue> = vec![];
-
-        'entries: for kv in entries {
-            // Attempt to turn the attrpath into a list of static strings, but
-            // abort this process if any dynamic fragments are encountered.
-            let static_attrpath: Option<Vec<SmolStr>> = kv
-                .attrpath()
-                .unwrap()
-                .attrs()
-                .map(|a| self.expr_static_attr_str(&a))
-                .collect();
-
-            let fragments = match static_attrpath {
-                Some(fragments) => fragments,
-                None => {
-                    dynamic_attrs.push(kv);
-                    continue 'entries;
-                }
-            };
-
-            // At this point we can increase the counter because we know that
-            // this particular attribute is static and can thus be processed
-            // here.
-            *count += 1;
-
-            let key_count = fragments.len();
-            for fragment in fragments.into_iter() {
-                self.emit_constant(Value::String(fragment.into()), &kv.attrpath().unwrap());
-            }
-
-            // We're done with the key if there was only one fragment, otherwise
-            // we need to emit an instruction to construct the attribute path.
-            if key_count > 1 {
-                self.push_op(
-                    OpCode::OpAttrPath(Count(key_count)),
-                    &kv.attrpath().unwrap(),
-                );
-            }
-
-            // The value is just compiled as normal so that its resulting value
-            // is on the stack when the attribute set is constructed at runtime.
-            let value_span = self.span_for(&kv.value().unwrap());
-            let value_slot = self.scope_mut().declare_phantom(value_span, false);
-            self.compile(value_slot, kv.value().unwrap());
-            self.scope_mut().mark_initialised(value_slot);
-        }
-
-        dynamic_attrs
-    }
-
-    /// Compile the dynamic entries of an attribute set, where keys are only
-    /// known at runtime.
-    fn compile_dynamic_attr_entries(
-        &mut self,
-        count: &mut usize,
-        entries: Vec<ast::AttrpathValue>,
-    ) {
-        for entry in entries.into_iter() {
-            *count += 1;
-
-            let mut key_count = 0;
-            let key_span = self.span_for(&entry.attrpath().unwrap());
-            let key_idx = self.scope_mut().declare_phantom(key_span, false);
-
-            for fragment in entry.attrpath().unwrap().attrs() {
-                // Key fragments can contain dynamic expressions, which makes
-                // accounting for their stack slots very tricky.
-                //
-                // In order to ensure the locals are correctly cleaned up, we
-                // essentially treat any key fragment after the first one (which
-                // has a locals index that will become that of the final key
-                // itself) as being part of a separate scope, which results in
-                // the somewhat annoying setup logic below.
-                let fragment_slot = match key_count {
-                    0 => key_idx,
-                    1 => {
-                        self.scope_mut().begin_scope();
-                        self.scope_mut().declare_phantom(key_span, false)
-                    }
-                    _ => self.scope_mut().declare_phantom(key_span, false),
-                };
-
-                key_count += 1;
-                self.compile_attr(fragment_slot, fragment);
-                self.scope_mut().mark_initialised(fragment_slot);
-            }
-
-            // We're done with the key if there was only one fragment, otherwise
-            // we need to emit an instruction to construct the attribute path.
-            if key_count > 1 {
-                self.push_op(
-                    OpCode::OpAttrPath(Count(key_count)),
-                    &entry.attrpath().unwrap(),
-                );
-
-                // Close the temporary scope that was set up for the key
-                // fragments.
-                self.scope_mut().end_scope();
-            }
-
-            // The value is just compiled as normal so that its resulting value
-            // is on the stack when the attribute set is constructed at runtime.
-            let value_span = self.span_for(&entry.value().unwrap());
-            let value_slot = self.scope_mut().declare_phantom(value_span, false);
-            self.compile(value_slot, entry.value().unwrap());
-            self.scope_mut().mark_initialised(value_slot);
-        }
-    }
-
     /// Compile attribute set literals into equivalent bytecode.
     ///
     /// This is complicated by a number of features specific to Nix attribute
@@ -407,26 +290,14 @@ impl Compiler<'_> {
         // `OpAttrs` instruction.
         self.scope_mut().begin_scope();
 
-        if node.rec_token().is_some() {
-            let count = self.compile_recursive_scope(slot, BindingsKind::RecAttrs, &node);
-            self.push_op(OpCode::OpAttrs(Count(count)), &node);
+        let kind = if node.rec_token().is_some() {
+            BindingsKind::RecAttrs
         } else {
-            let mut count = 0;
-
-            // TODO: merge this with the above, for now only inherit is unified
-            let mut bindings: Vec<TrackedBinding> = vec![];
-            let inherit_froms =
-                self.compile_plain_inherits(slot, BindingsKind::Attrs, &mut count, &node);
-            self.declare_namespaced_inherits(BindingsKind::Attrs, inherit_froms, &mut bindings);
-            self.bind_values(bindings);
-
-            let dynamic_entries =
-                self.compile_static_attr_entries(&mut count, node.attrpath_values());
-
-            self.compile_dynamic_attr_entries(&mut count, dynamic_entries);
+            BindingsKind::Attrs
+        };
 
-            self.push_op(OpCode::OpAttrs(Count(count)), &node);
-        }
+        let count = self.compile_recursive_scope(slot, kind, &node);
+        self.push_op(OpCode::OpAttrs(Count(count)), &node);
 
         // Remove the temporary scope, but do not emit any additional cleanup
         // (OpAttrs consumes all of these locals).
diff --git a/tvix/eval/src/compiler/mod.rs b/tvix/eval/src/compiler/mod.rs
index de614be4da..dab91a7f56 100644
--- a/tvix/eval/src/compiler/mod.rs
+++ b/tvix/eval/src/compiler/mod.rs
@@ -20,7 +20,6 @@ mod spans;
 use codemap::Span;
 use path_clean::PathClean;
 use rnix::ast::{self, AstToken, HasEntry};
-use rowan::ast::AstChildren;
 use smol_str::SmolStr;
 use std::collections::HashMap;
 use std::path::{Path, PathBuf};
diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-contains-nested-non-set.nix b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-contains-nested-non-set.nix
index 361ba91445..361ba91445 100644
--- a/tvix/eval/src/tests/tvix_tests/eval-okay-contains-nested-non-set.nix
+++ b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-contains-nested-non-set.nix
diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-deeply-nested-attrs.nix b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-deeply-nested-attrs.nix
index 91649d0c6d..91649d0c6d 100644
--- a/tvix/eval/src/tests/tvix_tests/eval-okay-deeply-nested-attrs.nix
+++ b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-deeply-nested-attrs.nix
diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-multiple-nested-attrs.nix b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-multiple-nested-attrs.nix
index 5d611930ca..5d611930ca 100644
--- a/tvix/eval/src/tests/tvix_tests/eval-okay-multiple-nested-attrs.nix
+++ b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-multiple-nested-attrs.nix
diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-nested-has-attrs.nix b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-nested-has-attrs.nix
index 47dcec7a95..47dcec7a95 100644
--- a/tvix/eval/src/tests/tvix_tests/eval-okay-nested-has-attrs.nix
+++ b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-nested-has-attrs.nix
diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-or-operator-nested-default.nix b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-or-operator-nested-default.nix
index ceffd0697b..ceffd0697b 100644
--- a/tvix/eval/src/tests/tvix_tests/eval-okay-or-operator-nested-default.nix
+++ b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-or-operator-nested-default.nix
diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-or-operator-nested.nix b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-or-operator-nested.nix
index 1a76594546..1a76594546 100644
--- a/tvix/eval/src/tests/tvix_tests/eval-okay-or-operator-nested.nix
+++ b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-or-operator-nested.nix
diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-or-operator-non-set.nix b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-or-operator-non-set.nix
index fd09bfee64..fd09bfee64 100644
--- a/tvix/eval/src/tests/tvix_tests/eval-okay-or-operator-non-set.nix
+++ b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-or-operator-non-set.nix
diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-overlapping-nested-attrs.nix b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-overlapping-nested-attrs.nix
index 4154ff9da2..4154ff9da2 100644
--- a/tvix/eval/src/tests/tvix_tests/eval-okay-overlapping-nested-attrs.nix
+++ b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-overlapping-nested-attrs.nix
diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-simple-nested-attrs.nix b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-simple-nested-attrs.nix
index a97394d165..a97394d165 100644
--- a/tvix/eval/src/tests/tvix_tests/eval-okay-simple-nested-attrs.nix
+++ b/tvix/eval/src/tests/tvix_tests/disabled-eval-okay-simple-nested-attrs.nix
diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-builtins-type-of.exp b/tvix/eval/src/tests/tvix_tests/eval-okay-builtins-type-of.exp
index 0e22da8d56..1ea054fc2d 100644
--- a/tvix/eval/src/tests/tvix_tests/eval-okay-builtins-type-of.exp
+++ b/tvix/eval/src/tests/tvix_tests/eval-okay-builtins-type-of.exp
@@ -1 +1 @@
-[ "null" "bool" "bool" "int" "int" "float" "string" "string" "set" "set" "set" "list" "lambda" "path" ]
+[ "null" "bool" "bool" "int" "int" "float" "string" "string" "set" "set" "list" "lambda" "path" ]
diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-builtins-type-of.nix b/tvix/eval/src/tests/tvix_tests/eval-okay-builtins-type-of.nix
index 38d7ffee61..a3cb659ecf 100644
--- a/tvix/eval/src/tests/tvix_tests/eval-okay-builtins-type-of.nix
+++ b/tvix/eval/src/tests/tvix_tests/eval-okay-builtins-type-of.nix
@@ -13,7 +13,7 @@ fix (self:
     (builtins.typeOf "foo")
     (builtins.typeOf "${"foo" + "bar"}baz")
     (builtins.typeOf {})
-    (builtins.typeOf { foo.bar = 32; }.foo)
+    # (builtins.typeOf { foo.bar = 32; }.foo) # TODO: re-enable when nested keys are done
     (builtins.typeOf ({ name = "foo"; value = 13; } // { name = "bar"; }))
     (builtins.typeOf self)
     (builtins.typeOf fix)