From c46025d52011632b9e53bc96cc115c1d74bfbf7a Mon Sep 17 00:00:00 2001 From: Vincent Ambo Date: Fri, 23 Sep 2022 00:31:40 +0300 Subject: fix(tvix/eval): support string identifiers in inherits This updates rnix-parser to a version where inherits provide an iterator over `ast::Attr` instead of `ast::Ident`, which mirrors the behaviour of Nix (inherits can have (statically known) strings as their identifiers). This actually required some fairly significant code reshuffling in the compiler, as there was an implicit assumption in many places that we would have an `ast::Ident` node available when dealing with variable access (which is then explicitly only not true in this case). Change-Id: I12f1e786c0030c85107b1aa409bd49adb5465546 Reviewed-on: https://cl.tvl.fyi/c/depot/+/6747 Tested-by: BuildkiteCI Reviewed-by: sterni --- corp/tvixbolt/Cargo.lock | 2 +- corp/tvixbolt/Cargo.toml | 2 +- corp/tvixbolt/default.nix | 2 +- tvix/eval/Cargo.lock | 2 +- tvix/eval/Cargo.toml | 4 +- tvix/eval/src/compiler/attrs.rs | 47 +++++-- tvix/eval/src/compiler/mod.rs | 150 ++++++++++++--------- tvix/eval/src/compiler/scope.rs | 2 +- .../tvix_tests/eval-okay-inherit-string-ident.exp | 1 + .../tvix_tests/eval-okay-inherit-string-ident.nix | 7 + 10 files changed, 138 insertions(+), 81 deletions(-) create mode 100644 tvix/eval/src/tests/tvix_tests/eval-okay-inherit-string-ident.exp create mode 100644 tvix/eval/src/tests/tvix_tests/eval-okay-inherit-string-ident.nix diff --git a/corp/tvixbolt/Cargo.lock b/corp/tvixbolt/Cargo.lock index e6572fc03d09..05d9a8fb6111 100644 --- a/corp/tvixbolt/Cargo.lock +++ b/corp/tvixbolt/Cargo.lock @@ -402,7 +402,7 @@ dependencies = [ [[package]] name = "rnix" version = "0.11.0-dev" -source = "git+https://github.com/nix-community/rnix-parser.git?rev=7d0d929c22ad27bdcc0779afe445b541d3ce9631#7d0d929c22ad27bdcc0779afe445b541d3ce9631" +source = "git+https://github.com/nix-community/rnix-parser.git?rev=85a045afd33e073a3eab4c0ea2f515b6bed557ab#85a045afd33e073a3eab4c0ea2f515b6bed557ab" dependencies = [ "rowan", ] diff --git a/corp/tvixbolt/Cargo.toml b/corp/tvixbolt/Cargo.toml index fc7cedfd5846..fa05350db644 100644 --- a/corp/tvixbolt/Cargo.toml +++ b/corp/tvixbolt/Cargo.toml @@ -17,7 +17,7 @@ wasm-bindgen = "= 0.2.82" [dependencies.rnix] git = "https://github.com/nix-community/rnix-parser.git" -rev = "7d0d929c22ad27bdcc0779afe445b541d3ce9631" +rev = "85a045afd33e073a3eab4c0ea2f515b6bed557ab" [dependencies.tvix-eval] path = "../../tvix/eval" diff --git a/corp/tvixbolt/default.nix b/corp/tvixbolt/default.nix index f210a520402a..963be6623826 100644 --- a/corp/tvixbolt/default.nix +++ b/corp/tvixbolt/default.nix @@ -58,7 +58,7 @@ pkgs.rustPlatform.buildRustPackage rec { cargoLock.lockFile = ./Cargo.lock; cargoLock.outputHashes = { - "rnix-0.11.0-dev" = "sha256:1vvpnv0jbyr96z8cb1r6k613zqsryan9awi53f901q3qjc856iz0"; + "rnix-0.11.0-dev" = "sha256:01c3fdsfyp8iwr36nv2mvr2xw33ci3vcx6pw8a9qrc8pjr6q22f8"; }; patches = [ diff --git a/tvix/eval/Cargo.lock b/tvix/eval/Cargo.lock index 71c6525b0a41..ec91d1c0520e 100644 --- a/tvix/eval/Cargo.lock +++ b/tvix/eval/Cargo.lock @@ -886,7 +886,7 @@ dependencies = [ [[package]] name = "rnix" version = "0.11.0-dev" -source = "git+https://github.com/nix-community/rnix-parser.git?rev=7d0d929c22ad27bdcc0779afe445b541d3ce9631#7d0d929c22ad27bdcc0779afe445b541d3ce9631" +source = "git+https://github.com/nix-community/rnix-parser.git?rev=85a045afd33e073a3eab4c0ea2f515b6bed557ab#85a045afd33e073a3eab4c0ea2f515b6bed557ab" dependencies = [ "rowan", ] diff --git a/tvix/eval/Cargo.toml b/tvix/eval/Cargo.toml index b23fc057ad5a..2edeccb9d1ef 100644 --- a/tvix/eval/Cargo.toml +++ b/tvix/eval/Cargo.toml @@ -25,11 +25,11 @@ proptest = { version = "1.0.0", default_features = false, features = ["std", "al test-strategy = { version = "0.2.1", optional = true } clap = { version = "3.2.22", optional = true, features = ["derive", "env"] } -# rnix has not been released in a while (as of 2022-09-18), we will +# rnix has not been released in a while (as of 2022-09-23), we will # use it from git. [dependencies.rnix] git = "https://github.com/nix-community/rnix-parser.git" -rev = "7d0d929c22ad27bdcc0779afe445b541d3ce9631" +rev = "85a045afd33e073a3eab4c0ea2f515b6bed557ab" [dev-dependencies] criterion = "0.3.6" diff --git a/tvix/eval/src/compiler/attrs.rs b/tvix/eval/src/compiler/attrs.rs index 5dee3a905d01..b4d183c51df8 100644 --- a/tvix/eval/src/compiler/attrs.rs +++ b/tvix/eval/src/compiler/attrs.rs @@ -36,14 +36,25 @@ impl Compiler<'_> { for inherit in inherits { match inherit.from() { Some(from) => { - for ident in inherit.idents() { + for attr in inherit.attrs() { count += 1; + let name = match self.expr_static_attr_str(&attr) { + Some(name) => name, + None => { + // TODO(tazjin): error variant for dynamic + // key in *inherit* (or generalise it) + self.emit_error(&attr, ErrorKind::DynamicKeyInLet); + continue; + } + }; + + let name_span = self.span_for(&attr); + // First emit the identifier itself (this // becomes the new key). - self.emit_literal_ident(&ident); - let ident_span = self.span_for(&ident); - self.scope_mut().declare_phantom(ident_span, true); + self.emit_constant(Value::String(SmolStr::new(&name).into()), &attr); + self.scope_mut().declare_phantom(name_span, true); // Then emit the node that we're inheriting // from. @@ -53,27 +64,37 @@ impl Compiler<'_> { // instruction followed by a merge, rather // than pushing/popping the same attrs // potentially a lot of times. - let val_idx = self.scope_mut().declare_phantom(ident_span, false); + let val_idx = self.scope_mut().declare_phantom(name_span, false); self.compile(val_idx, from.expr().unwrap()); self.emit_force(&from.expr().unwrap()); - self.emit_literal_ident(&ident); - self.push_op(OpCode::OpAttrsSelect, &ident); + self.emit_constant(Value::String(name.into()), &attr); + self.push_op(OpCode::OpAttrsSelect, &attr); self.scope_mut().mark_initialised(val_idx); } } None => { - for ident in inherit.idents() { - let ident_span = self.span_for(&ident); + for attr in inherit.attrs() { count += 1; // Emit the key to use for OpAttrs - self.emit_literal_ident(&ident); - self.scope_mut().declare_phantom(ident_span, true); + let name = match self.expr_static_attr_str(&attr) { + Some(name) => name, + None => { + // TODO(tazjin): error variant for dynamic + // key in *inherit* (or generalise it) + self.emit_error(&attr, ErrorKind::DynamicKeyInLet); + continue; + } + }; + + let name_span = self.span_for(&attr); + self.emit_constant(Value::String(SmolStr::new(&name).into()), &attr); + self.scope_mut().declare_phantom(name_span, true); // Emit the value. - self.compile_ident(slot, ident); - self.scope_mut().declare_phantom(ident_span, true); + self.compile_identifier_access(slot, &name, &attr); + self.scope_mut().declare_phantom(name_span, true); } } } diff --git a/tvix/eval/src/compiler/mod.rs b/tvix/eval/src/compiler/mod.rs index fb04581bfcc2..55b8d960a9a6 100644 --- a/tvix/eval/src/compiler/mod.rs +++ b/tvix/eval/src/compiler/mod.rs @@ -17,6 +17,7 @@ mod attrs; mod scope; mod spans; +use codemap::Span; use path_clean::PathClean; use rnix::ast::{self, AstToken, HasEntry}; use rowan::ast::AstChildren; @@ -72,7 +73,7 @@ impl LambdaCtx { /// Alias for the map of globally available functions that should /// implicitly be resolvable in the global scope. -type GlobalsMap = HashMap<&'static str, Rc>; +type GlobalsMap = HashMap<&'static str, Rc>; struct Compiler<'observer> { contexts: Vec, @@ -564,7 +565,7 @@ impl Compiler<'_> { // While we are looping through the inherits, already note all inherit // (from) expressions, that may very well resolve recursively and need // to be compiled like normal let in bindings. - let mut inherit_froms: Vec<(ast::Expr, ast::Ident)> = vec![]; + let mut inherit_froms: Vec<(ast::Expr, String, Span)> = vec![]; for inherit in node.inherits() { match inherit.from() { // Within a `let` binding, inheriting from the outer @@ -576,36 +577,55 @@ impl Compiler<'_> { } None => { - for ident in inherit.idents() { + for attr in inherit.attrs() { + let name = match self.expr_static_attr_str(&attr) { + Some(name) => name, + None => { + // TODO(tazjin): error variant for dynamic + // key in *inherit* (or generalise it) + self.emit_error(&attr, ErrorKind::DynamicKeyInLet); + continue; + } + }; + // If the identifier resolves statically in a // `let`, it has precedence over dynamic // bindings, and the inherit is useless. if !rec_attrs && matches!( - self.scope_mut() - .resolve_local(ident.ident_token().unwrap().text()), + self.scope_mut().resolve_local(&name), LocalPosition::Known(_) ) { - self.emit_warning(&ident, WarningKind::UselessInherit); + self.emit_warning(&attr, WarningKind::UselessInherit); continue; } if rec_attrs { - self.emit_literal_ident(&ident); - let span = self.span_for(&ident); + self.emit_constant(Value::String(SmolStr::new(&name).into()), &attr); + let span = self.span_for(&attr); self.scope_mut().declare_phantom(span, true); } - self.compile_ident(slot, ident.clone()); - let idx = self.declare_local(&ident, ident.ident_token().unwrap().text()); + self.compile_identifier_access(slot, &name, &attr); + let idx = self.declare_local(&attr, &name); self.scope_mut().mark_initialised(idx); } } Some(from) => { - for ident in inherit.idents() { - inherit_froms.push((from.expr().unwrap(), ident)); + for attr in inherit.attrs() { + let name = match self.expr_static_attr_str(&attr) { + Some(name) => name, + None => { + // TODO(tazjin): error variant for dynamic + // key in *inherit* (or generalise it) + self.emit_error(&attr, ErrorKind::DynamicKeyInLet); + continue; + } + }; + + inherit_froms.push((from.expr().unwrap(), name, self.span_for(&attr))); } } } @@ -617,7 +637,8 @@ impl Compiler<'_> { enum BindingKind { InheritFrom { namespace: ast::Expr, - ident: ast::Ident, + name: SmolStr, + span: Span, }, Plain { @@ -643,25 +664,25 @@ impl Compiler<'_> { // (that may resolve recursively) are known. // Begin with the inherit (from)s since they all become a thunk anyway - for (from, ident) in inherit_froms { + for (from, name, span) in inherit_froms { let key_slot = if rec_attrs { - let span = self.span_for(&ident); Some(KeySlot { slot: self.scope_mut().declare_phantom(span, false), - name: SmolStr::new(ident.ident_token().unwrap().text()), + name: SmolStr::new(&name), }) } else { None }; - let value_slot = self.declare_local(&ident, ident.ident_token().unwrap().text()); + let value_slot = self.declare_local(&span, &name); bindings.push(TrackedBinding { key_slot, value_slot, kind: BindingKind::InheritFrom { - ident, namespace: from, + name: SmolStr::new(&name), + span, }, }); } @@ -711,28 +732,27 @@ impl Compiler<'_> { value_indices.push(binding.value_slot); if let Some(key_slot) = binding.key_slot { - // TODO: emit_constant should be able to take a span directly let span = self.scope()[key_slot.slot].span; - let idx = self - .chunk() - .push_constant(Value::String(key_slot.name.into())); - - self.chunk().push_op(OpCode::OpConstant(idx), span); + self.emit_constant(Value::String(key_slot.name.into()), &span); self.scope_mut().mark_initialised(key_slot.slot); } match binding.kind { // This entry is an inherit (from) expr. The value is // placed on the stack by selecting an attribute. - BindingKind::InheritFrom { namespace, ident } => { + BindingKind::InheritFrom { + namespace, + name, + span, + } => { // Create a thunk wrapping value (which may be one as well) to // avoid forcing the from expr too early. self.thunk(binding.value_slot, &namespace, move |c, n, s| { c.compile(s, n.clone()); c.emit_force(n); - c.emit_literal_ident(&ident); - c.push_op(OpCode::OpAttrsSelect, &ident); + c.emit_constant(Value::String(name.into()), &span); + c.push_op(OpCode::OpAttrsSelect, &span); }) } @@ -768,25 +788,27 @@ impl Compiler<'_> { self.cleanup_scope(&node); } - fn compile_ident(&mut self, slot: LocalIdx, node: ast::Ident) { - let ident = node.ident_token().unwrap(); - + /// Resolve and compile access to an identifier in the scope. + fn compile_identifier_access( + &mut self, + slot: LocalIdx, + ident: &str, + node: &N, + ) { // If the identifier is a global, and it is not poisoned, emit // the global directly. - if let Some(global) = self.globals.get(ident.text()) { - if !self.scope().is_poisoned(ident.text()) { - global.clone()(self, node.clone()); + if let Some(global) = self.globals.get(ident) { + if !self.scope().is_poisoned(ident) { + global.clone()(self, self.span_for(node)); return; } } - match self.scope_mut().resolve_local(ident.text()) { + match self.scope_mut().resolve_local(ident) { LocalPosition::Unknown => { // Are we possibly dealing with an upvalue? - if let Some(idx) = - self.resolve_upvalue(self.contexts.len() - 1, ident.text(), &node) - { - self.push_op(OpCode::OpGetUpvalue(idx), &node); + if let Some(idx) = self.resolve_upvalue(self.contexts.len() - 1, ident, node) { + self.push_op(OpCode::OpGetUpvalue(idx), node); return; } @@ -794,24 +816,24 @@ impl Compiler<'_> { // context with one), emit a runtime dynamic // resolution instruction. if self.has_dynamic_ancestor() { - self.emit_literal_ident(&node); - self.push_op(OpCode::OpResolveWith, &node); + self.emit_constant(Value::String(SmolStr::new(ident).into()), node); + self.push_op(OpCode::OpResolveWith, node); return; } // Otherwise, this variable is missing. - self.emit_error(&node, ErrorKind::UnknownStaticVariable); + self.emit_error(node, ErrorKind::UnknownStaticVariable); } LocalPosition::Known(idx) => { let stack_idx = self.scope().stack_index(idx); - self.push_op(OpCode::OpGetLocal(stack_idx), &node); + self.push_op(OpCode::OpGetLocal(stack_idx), node); } // This identifier is referring to a value from the same // scope which is not yet defined. This identifier access // must be thunked. - LocalPosition::Recursive(idx) => self.thunk(slot, &node, move |compiler, node, _| { + LocalPosition::Recursive(idx) => self.thunk(slot, node, move |compiler, node, _| { let upvalue_idx = compiler.add_upvalue( compiler.contexts.len() - 1, node, @@ -822,6 +844,11 @@ impl Compiler<'_> { }; } + fn compile_ident(&mut self, slot: LocalIdx, node: ast::Ident) { + let ident = node.ident_token().unwrap(); + self.compile_identifier_access(slot, ident.text(), &node); + } + /// Compile `with` expressions by emitting instructions that /// pop/remove the indices of attribute sets that are implicitly /// in scope through `with` on the "with-stack". @@ -1104,15 +1131,15 @@ impl Compiler<'_> { // resolution must be deferred until the scope is // fully initialised and can be finalised. if this_depth == target_depth && this_stack_slot < stack_idx { - self.push_op(OpCode::DataDeferredLocal(stack_idx), &upvalue.node); + self.push_op(OpCode::DataDeferredLocal(stack_idx), &upvalue.span); self.scope_mut().mark_needs_finaliser(slot); } else { - self.push_op(OpCode::DataLocalIdx(stack_idx), &upvalue.node); + self.push_op(OpCode::DataLocalIdx(stack_idx), &upvalue.span); } } UpvalueKind::Upvalue(idx) => { - self.push_op(OpCode::DataUpvalueIdx(idx), &upvalue.node); + self.push_op(OpCode::DataUpvalueIdx(idx), &upvalue.span); } }; } @@ -1211,11 +1238,11 @@ impl Compiler<'_> { self.scope_mut().declare_local(name, span) } - fn resolve_upvalue( + fn resolve_upvalue( &mut self, ctx_idx: usize, name: &str, - node: &rnix::ast::Ident, + node: &N, ) -> Option { if ctx_idx == 0 { // There can not be any upvalue at the outermost context. @@ -1266,10 +1293,10 @@ impl Compiler<'_> { ancestor_has_with } - fn add_upvalue( + fn add_upvalue( &mut self, ctx_idx: usize, - node: &rnix::ast::Ident, + node: &N, kind: UpvalueKind, ) -> UpvalueIdx { // If there is already an upvalue closing over the specified @@ -1280,10 +1307,11 @@ impl Compiler<'_> { } } - self.contexts[ctx_idx].scope.upvalues.push(Upvalue { - kind, - node: node.clone(), - }); + let span = self.span_for(node); + self.contexts[ctx_idx] + .scope + .upvalues + .push(Upvalue { kind, span }); let idx = UpvalueIdx(self.contexts[ctx_idx].lambda.upvalue_count); self.contexts[ctx_idx].lambda.upvalue_count += 1; @@ -1394,29 +1422,29 @@ fn prepare_globals(additional: HashMap<&'static str, Value>) -> GlobalsMap { globals.insert( "true", - Rc::new(|compiler, node| { - compiler.push_op(OpCode::OpTrue, &node); + Rc::new(|compiler, span| { + compiler.push_op(OpCode::OpTrue, &span); }), ); globals.insert( "false", - Rc::new(|compiler, node| { - compiler.push_op(OpCode::OpFalse, &node); + Rc::new(|compiler, span| { + compiler.push_op(OpCode::OpFalse, &span); }), ); globals.insert( "null", - Rc::new(|compiler, node| { - compiler.push_op(OpCode::OpNull, &node); + Rc::new(|compiler, span| { + compiler.push_op(OpCode::OpNull, &span); }), ); for (ident, value) in additional.into_iter() { globals.insert( ident, - Rc::new(move |compiler, node| compiler.emit_constant(value.clone(), &node)), + Rc::new(move |compiler, span| compiler.emit_constant(value.clone(), &span)), ); } diff --git a/tvix/eval/src/compiler/scope.rs b/tvix/eval/src/compiler/scope.rs index 8a90a3d9dc2a..88df218ebdf7 100644 --- a/tvix/eval/src/compiler/scope.rs +++ b/tvix/eval/src/compiler/scope.rs @@ -105,7 +105,7 @@ pub enum UpvalueKind { #[derive(Clone, Debug)] pub struct Upvalue { pub kind: UpvalueKind, - pub node: rnix::ast::Ident, + pub span: codemap::Span, } /// Represents the index of a local in the scope's local array, which diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-inherit-string-ident.exp b/tvix/eval/src/tests/tvix_tests/eval-okay-inherit-string-ident.exp new file mode 100644 index 000000000000..d81cc0710eb6 --- /dev/null +++ b/tvix/eval/src/tests/tvix_tests/eval-okay-inherit-string-ident.exp @@ -0,0 +1 @@ +42 diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-inherit-string-ident.nix b/tvix/eval/src/tests/tvix_tests/eval-okay-inherit-string-ident.nix new file mode 100644 index 000000000000..dde81e5a7c30 --- /dev/null +++ b/tvix/eval/src/tests/tvix_tests/eval-okay-inherit-string-ident.nix @@ -0,0 +1,7 @@ +# identifiers in inherits can be string-like expressions + +let + set = { + inherit ({ value = 42; }) "value"; + }; +in set.value -- cgit 1.4.1