mdebray/tvl-depot
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(tvix/eval): support string identifiers in inherits

Browse source 
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 <sternenseemann@systemli.org>
This commit is contained in:
Vincent Ambo 2022-09-23 00:31:40 +03:00 committed by tazjin
parent ee0b89c402
commit c46025d520
10 changed files with 138 additions and 81 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
corp/tvixbolt/Cargo.lock generated
View file

@ -402,7 +402,7 @@ dependencies = [
[[package]] [[package]]
name = "rnix" name = "rnix"
version = "0.11.0-dev" 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 = [ dependencies = [
"rowan", "rowan",
] ]

2
corp/tvixbolt/Cargo.toml
View file

@ -17,7 +17,7 @@ wasm-bindgen = "= 0.2.82"
[dependencies.rnix] [dependencies.rnix]
git = "https://github.com/nix-community/rnix-parser.git" git = "https://github.com/nix-community/rnix-parser.git"
rev = "7d0d929c22ad27bdcc0779afe445b541d3ce9631" rev = "85a045afd33e073a3eab4c0ea2f515b6bed557ab"
[dependencies.tvix-eval] [dependencies.tvix-eval]
path = "../../tvix/eval" path = "../../tvix/eval"

2
corp/tvixbolt/default.nix
View file

@ -58,7 +58,7 @@ pkgs.rustPlatform.buildRustPackage rec {
cargoLock.lockFile = ./Cargo.lock; cargoLock.lockFile = ./Cargo.lock;
cargoLock.outputHashes = { cargoLock.outputHashes = {
"rnix-0.11.0-dev" = "sha256:1vvpnv0jbyr96z8cb1r6k613zqsryan9awi53f901q3qjc856iz0"; "rnix-0.11.0-dev" = "sha256:01c3fdsfyp8iwr36nv2mvr2xw33ci3vcx6pw8a9qrc8pjr6q22f8";
}; };
patches = [ patches = [

2
tvix/eval/Cargo.lock generated
View file

@ -886,7 +886,7 @@ dependencies = [
[[package]] [[package]]
name = "rnix" name = "rnix"
version = "0.11.0-dev" 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 = [ dependencies = [
"rowan", "rowan",
] ]

4
tvix/eval/Cargo.toml
View file

@ -25,11 +25,11 @@ proptest = { version = "1.0.0", default_features = false, features = ["std", "al
test-strategy = { version = "0.2.1", optional = true } test-strategy = { version = "0.2.1", optional = true }
clap = { version = "3.2.22", optional = true, features = ["derive", "env"] } 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. # use it from git.
[dependencies.rnix] [dependencies.rnix]
git = "https://github.com/nix-community/rnix-parser.git" git = "https://github.com/nix-community/rnix-parser.git"
rev = "7d0d929c22ad27bdcc0779afe445b541d3ce9631" rev = "85a045afd33e073a3eab4c0ea2f515b6bed557ab"
[dev-dependencies] [dev-dependencies]
criterion = "0.3.6" criterion = "0.3.6"

47
tvix/eval/src/compiler/attrs.rs
View file

@ -36,14 +36,25 @@ impl Compiler<'_> {
for inherit in inherits { for inherit in inherits {
match inherit.from() { match inherit.from() {
Some(from) => { Some(from) => {
for ident in inherit.idents() { for attr in inherit.attrs() {
count += 1; 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 // First emit the identifier itself (this
// becomes the new key). // becomes the new key).
self.emit_literal_ident(&ident); self.emit_constant(Value::String(SmolStr::new(&name).into()), &attr);
let ident_span = self.span_for(&ident); self.scope_mut().declare_phantom(name_span, true);
self.scope_mut().declare_phantom(ident_span, true);
// Then emit the node that we're inheriting // Then emit the node that we're inheriting
// from. // from.
@ -53,27 +64,37 @@ impl Compiler<'_> {
// instruction followed by a merge, rather // instruction followed by a merge, rather
// than pushing/popping the same attrs // than pushing/popping the same attrs
// potentially a lot of times. // 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.compile(val_idx, from.expr().unwrap());
self.emit_force(&from.expr().unwrap()); self.emit_force(&from.expr().unwrap());
self.emit_literal_ident(&ident); self.emit_constant(Value::String(name.into()), &attr);
self.push_op(OpCode::OpAttrsSelect, &ident); self.push_op(OpCode::OpAttrsSelect, &attr);
self.scope_mut().mark_initialised(val_idx); self.scope_mut().mark_initialised(val_idx);
} }
} }
None => { None => {
for ident in inherit.idents() { for attr in inherit.attrs() {
let ident_span = self.span_for(&ident);
count += 1; count += 1;
// Emit the key to use for OpAttrs // Emit the key to use for OpAttrs
self.emit_literal_ident(&ident); let name = match self.expr_static_attr_str(&attr) {
self.scope_mut().declare_phantom(ident_span, true); 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. // Emit the value.
self.compile_ident(slot, ident); self.compile_identifier_access(slot, &name, &attr);
self.scope_mut().declare_phantom(ident_span, true); self.scope_mut().declare_phantom(name_span, true);
} }
} }
} }

150
tvix/eval/src/compiler/mod.rs
View file

@ -17,6 +17,7 @@ mod attrs;
mod scope; mod scope;
mod spans; mod spans;
use codemap::Span;
use path_clean::PathClean; use path_clean::PathClean;
use rnix::ast::{self, AstToken, HasEntry}; use rnix::ast::{self, AstToken, HasEntry};
use rowan::ast::AstChildren; use rowan::ast::AstChildren;
@ -72,7 +73,7 @@ impl LambdaCtx {
/// Alias for the map of globally available functions that should /// Alias for the map of globally available functions that should
/// implicitly be resolvable in the global scope. /// implicitly be resolvable in the global scope.
type GlobalsMap = HashMap<&'static str, Rc<dyn Fn(&mut Compiler, rnix::ast::Ident)>>; type GlobalsMap = HashMap<&'static str, Rc<dyn Fn(&mut Compiler, Span)>>;
struct Compiler<'observer> { struct Compiler<'observer> {
contexts: Vec<LambdaCtx>, contexts: Vec<LambdaCtx>,
@ -564,7 +565,7 @@ impl Compiler<'_> {
// While we are looping through the inherits, already note all inherit // While we are looping through the inherits, already note all inherit
// (from) expressions, that may very well resolve recursively and need // (from) expressions, that may very well resolve recursively and need
// to be compiled like normal let in bindings. // 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() { for inherit in node.inherits() {
match inherit.from() { match inherit.from() {
// Within a `let` binding, inheriting from the outer // Within a `let` binding, inheriting from the outer
@ -576,36 +577,55 @@ impl Compiler<'_> {
} }
None => { 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 // If the identifier resolves statically in a
// `let`, it has precedence over dynamic // `let`, it has precedence over dynamic
// bindings, and the inherit is useless. // bindings, and the inherit is useless.
if !rec_attrs if !rec_attrs
&& matches!( && matches!(
self.scope_mut() self.scope_mut().resolve_local(&name),
.resolve_local(ident.ident_token().unwrap().text()),
LocalPosition::Known(_) LocalPosition::Known(_)
) )
{ {
self.emit_warning(&ident, WarningKind::UselessInherit); self.emit_warning(&attr, WarningKind::UselessInherit);
continue; continue;
} }
if rec_attrs { if rec_attrs {
self.emit_literal_ident(&ident); self.emit_constant(Value::String(SmolStr::new(&name).into()), &attr);
let span = self.span_for(&ident); let span = self.span_for(&attr);
self.scope_mut().declare_phantom(span, true); self.scope_mut().declare_phantom(span, true);
} }
self.compile_ident(slot, ident.clone()); self.compile_identifier_access(slot, &name, &attr);
let idx = self.declare_local(&ident, ident.ident_token().unwrap().text()); let idx = self.declare_local(&attr, &name);
self.scope_mut().mark_initialised(idx); self.scope_mut().mark_initialised(idx);
} }
} }
Some(from) => { Some(from) => {
for ident in inherit.idents() { for attr in inherit.attrs() {
inherit_froms.push((from.expr().unwrap(), ident)); 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 { enum BindingKind {
InheritFrom { InheritFrom {
namespace: ast::Expr, namespace: ast::Expr,
ident: ast::Ident, name: SmolStr,
span: Span,
}, },
Plain { Plain {
@ -643,25 +664,25 @@ impl Compiler<'_> {
// (that may resolve recursively) are known. // (that may resolve recursively) are known.
// Begin with the inherit (from)s since they all become a thunk anyway // 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 key_slot = if rec_attrs {
let span = self.span_for(&ident);
Some(KeySlot { Some(KeySlot {
slot: self.scope_mut().declare_phantom(span, false), slot: self.scope_mut().declare_phantom(span, false),
name: SmolStr::new(ident.ident_token().unwrap().text()), name: SmolStr::new(&name),
}) })
} else { } else {
None None
}; };
let value_slot = self.declare_local(&ident, ident.ident_token().unwrap().text()); let value_slot = self.declare_local(&span, &name);
bindings.push(TrackedBinding { bindings.push(TrackedBinding {
key_slot, key_slot,
value_slot, value_slot,
kind: BindingKind::InheritFrom { kind: BindingKind::InheritFrom {
ident,
namespace: from, namespace: from,
name: SmolStr::new(&name),
span,
}, },
}); });
} }
@ -711,28 +732,27 @@ impl Compiler<'_> {
value_indices.push(binding.value_slot); value_indices.push(binding.value_slot);
if let Some(key_slot) = binding.key_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 span = self.scope()[key_slot.slot].span;
let idx = self self.emit_constant(Value::String(key_slot.name.into()), &span);
.chunk()
.push_constant(Value::String(key_slot.name.into()));
self.chunk().push_op(OpCode::OpConstant(idx), span);
self.scope_mut().mark_initialised(key_slot.slot); self.scope_mut().mark_initialised(key_slot.slot);
} }
match binding.kind { match binding.kind {
// This entry is an inherit (from) expr. The value is // This entry is an inherit (from) expr. The value is
// placed on the stack by selecting an attribute. // 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 // Create a thunk wrapping value (which may be one as well) to
// avoid forcing the from expr too early. // avoid forcing the from expr too early.
self.thunk(binding.value_slot, &namespace, move |c, n, s| { self.thunk(binding.value_slot, &namespace, move |c, n, s| {
c.compile(s, n.clone()); c.compile(s, n.clone());
c.emit_force(n); c.emit_force(n);
c.emit_literal_ident(&ident); c.emit_constant(Value::String(name.into()), &span);
c.push_op(OpCode::OpAttrsSelect, &ident); c.push_op(OpCode::OpAttrsSelect, &span);
}) })
} }
@ -768,25 +788,27 @@ impl Compiler<'_> {
self.cleanup_scope(&node); self.cleanup_scope(&node);
} }
fn compile_ident(&mut self, slot: LocalIdx, node: ast::Ident) { /// Resolve and compile access to an identifier in the scope.
let ident = node.ident_token().unwrap(); fn compile_identifier_access<N: ToSpan + Clone>(
&mut self,
slot: LocalIdx,
ident: &str,
node: &N,
) {
// If the identifier is a global, and it is not poisoned, emit // If the identifier is a global, and it is not poisoned, emit
// the global directly. // the global directly.
if let Some(global) = self.globals.get(ident.text()) { if let Some(global) = self.globals.get(ident) {
if !self.scope().is_poisoned(ident.text()) { if !self.scope().is_poisoned(ident) {
global.clone()(self, node.clone()); global.clone()(self, self.span_for(node));
return; return;
} }
} }
match self.scope_mut().resolve_local(ident.text()) { match self.scope_mut().resolve_local(ident) {
LocalPosition::Unknown => { LocalPosition::Unknown => {
// Are we possibly dealing with an upvalue? // Are we possibly dealing with an upvalue?
if let Some(idx) = if let Some(idx) = self.resolve_upvalue(self.contexts.len() - 1, ident, node) {
self.resolve_upvalue(self.contexts.len() - 1, ident.text(), &node) self.push_op(OpCode::OpGetUpvalue(idx), node);
{
self.push_op(OpCode::OpGetUpvalue(idx), &node);
return; return;
} }
@ -794,24 +816,24 @@ impl Compiler<'_> {
// context with one), emit a runtime dynamic // context with one), emit a runtime dynamic
// resolution instruction. // resolution instruction.
if self.has_dynamic_ancestor() { if self.has_dynamic_ancestor() {
self.emit_literal_ident(&node); self.emit_constant(Value::String(SmolStr::new(ident).into()), node);
self.push_op(OpCode::OpResolveWith, &node); self.push_op(OpCode::OpResolveWith, node);
return; return;
} }
// Otherwise, this variable is missing. // Otherwise, this variable is missing.
self.emit_error(&node, ErrorKind::UnknownStaticVariable); self.emit_error(node, ErrorKind::UnknownStaticVariable);
} }
LocalPosition::Known(idx) => { LocalPosition::Known(idx) => {
let stack_idx = self.scope().stack_index(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 // This identifier is referring to a value from the same
// scope which is not yet defined. This identifier access // scope which is not yet defined. This identifier access
// must be thunked. // 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( let upvalue_idx = compiler.add_upvalue(
compiler.contexts.len() - 1, compiler.contexts.len() - 1,
node, 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 /// Compile `with` expressions by emitting instructions that
/// pop/remove the indices of attribute sets that are implicitly /// pop/remove the indices of attribute sets that are implicitly
/// in scope through `with` on the "with-stack". /// in scope through `with` on the "with-stack".
@ -1104,15 +1131,15 @@ impl Compiler<'_> {
// resolution must be deferred until the scope is // resolution must be deferred until the scope is
// fully initialised and can be finalised. // fully initialised and can be finalised.
if this_depth == target_depth && this_stack_slot < stack_idx { 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); self.scope_mut().mark_needs_finaliser(slot);
} else { } else {
self.push_op(OpCode::DataLocalIdx(stack_idx), &upvalue.node); self.push_op(OpCode::DataLocalIdx(stack_idx), &upvalue.span);
} }
} }
UpvalueKind::Upvalue(idx) => { 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) self.scope_mut().declare_local(name, span)
} }
fn resolve_upvalue( fn resolve_upvalue<N: ToSpan>(
&mut self, &mut self,
ctx_idx: usize, ctx_idx: usize,
name: &str, name: &str,
node: &rnix::ast::Ident, node: &N,
) -> Option<UpvalueIdx> { ) -> Option<UpvalueIdx> {
if ctx_idx == 0 { if ctx_idx == 0 {
// There can not be any upvalue at the outermost context. // There can not be any upvalue at the outermost context.
@ -1266,10 +1293,10 @@ impl Compiler<'_> {
ancestor_has_with ancestor_has_with
} }
fn add_upvalue( fn add_upvalue<N: ToSpan>(
&mut self, &mut self,
ctx_idx: usize, ctx_idx: usize,
node: &rnix::ast::Ident, node: &N,
kind: UpvalueKind, kind: UpvalueKind,
) -> UpvalueIdx { ) -> UpvalueIdx {
// If there is already an upvalue closing over the specified // If there is already an upvalue closing over the specified
@ -1280,10 +1307,11 @@ impl Compiler<'_> {
} }
} }
self.contexts[ctx_idx].scope.upvalues.push(Upvalue { let span = self.span_for(node);
kind, self.contexts[ctx_idx]
node: node.clone(), .scope
}); .upvalues
.push(Upvalue { kind, span });
let idx = UpvalueIdx(self.contexts[ctx_idx].lambda.upvalue_count); let idx = UpvalueIdx(self.contexts[ctx_idx].lambda.upvalue_count);
self.contexts[ctx_idx].lambda.upvalue_count += 1; self.contexts[ctx_idx].lambda.upvalue_count += 1;
@ -1394,29 +1422,29 @@ fn prepare_globals(additional: HashMap<&'static str, Value>) -> GlobalsMap {
globals.insert( globals.insert(
"true", "true",
Rc::new(|compiler, node| { Rc::new(|compiler, span| {
compiler.push_op(OpCode::OpTrue, &node); compiler.push_op(OpCode::OpTrue, &span);
}), }),
); );
globals.insert( globals.insert(
"false", "false",
Rc::new(|compiler, node| { Rc::new(|compiler, span| {
compiler.push_op(OpCode::OpFalse, &node); compiler.push_op(OpCode::OpFalse, &span);
}), }),
); );
globals.insert( globals.insert(
"null", "null",
Rc::new(|compiler, node| { Rc::new(|compiler, span| {
compiler.push_op(OpCode::OpNull, &node); compiler.push_op(OpCode::OpNull, &span);
}), }),
); );
for (ident, value) in additional.into_iter() { for (ident, value) in additional.into_iter() {
globals.insert( globals.insert(
ident, ident,
Rc::new(move |compiler, node| compiler.emit_constant(value.clone(), &node)), Rc::new(move |compiler, span| compiler.emit_constant(value.clone(), &span)),
); );
} }

2
tvix/eval/src/compiler/scope.rs
View file

@ -105,7 +105,7 @@ pub enum UpvalueKind {
#[derive(Clone, Debug)] #[derive(Clone, Debug)]
pub struct Upvalue { pub struct Upvalue {
pub kind: UpvalueKind, 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 /// Represents the index of a local in the scope's local array, which

1
tvix/eval/src/tests/tvix_tests/eval-okay-inherit-string-ident.exp Normal file
View file

@ -0,0 +1 @@
42

7
tvix/eval/src/tests/tvix_tests/eval-okay-inherit-string-ident.nix Normal file
View file

@ -0,0 +1,7 @@
# identifiers in inherits can be string-like expressions
let
set = {
inherit ({ value = 42; }) "value";
};
in set.value