maurice/snix
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

refactor(tvix/eval): add macros for generating Value casters

Browse source 
The casting methods of `Value` are pretty verbose, and actually
incorrect before this commit as they did not account for inner thunk
values.

To address this, we first attempt to make them correct by introducing
a standard macro to generate them and traverse the inner thunk(s) if
necessary.

This is likely to be a performance hit as it will now involve more
cloning of values. We can do multiple things to alleviate this, but
should do some measurements first.

Change-Id: If315d6e2afe7b69db727df535bc6cbfb89a691aa
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6412
Reviewed-by: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
This commit is contained in:
Vincent Ambo 2022-09-02 05:40:50 +03:00 • committed by tazjin
parent 0d7ad5e6d1
commit 6b3c3c9826
3 changed files with 56 additions and 76 deletions
Download patch file Download diff file Expand all files Collapse all files

8
tvix/eval/src/builtins/mod.rs
View file

@ -24,12 +24,12 @@ fn pure_builtins() -> Vec<Builtin> {
}),
Builtin::new("abort", 1, |mut args, _| {
return Err(ErrorKind::Abort(
args.pop().unwrap().to_string()?.as_str().to_owned(),
args.pop().unwrap().to_str()?.as_str().to_owned(),
));
}),
Builtin::new("catAttrs", 2, |mut args, _| {
let list = args.pop().unwrap().to_list()?;
let key = args.pop().unwrap().to_string()?;
let key = args.pop().unwrap().to_str()?;
let mut output = vec![];
for set in list.into_iter() {
@ -46,7 +46,7 @@ fn pure_builtins() -> Vec<Builtin> {
arithmetic_op!(a, b, /)
}),
Builtin::new("length", 1, |args, _| {
Ok(Value::Integer(args[0].as_list()?.len() as i64))
Ok(Value::Integer(args[0].to_list()?.len() as i64))
}),
Builtin::new("isAttrs", 1, |args, _| {
Ok(Value::Bool(matches!(args[0], Value::Attrs(_))))
@ -90,7 +90,7 @@ fn pure_builtins() -> Vec<Builtin> {
}),
Builtin::new("throw", 1, |mut args, _| {
return Err(ErrorKind::Throw(
args.pop().unwrap().to_string()?.as_str().to_owned(),
args.pop().unwrap().to_str()?.as_str().to_owned(),
));
}),
Builtin::new("toString", 1, |args, _| {

104
tvix/eval/src/value/mod.rs
View file

@ -43,11 +43,42 @@ pub enum Value {
DeferredUpvalue(StackIdx),
}
impl Value {
pub fn is_number(&self) -> bool {
matches!(self, Value::Integer(_) | Value::Float(_))
}
// Helper macros to generate the to_*/as_* macros while accounting for
// thunks.
/// Generate an `as_*` method returning a reference to the expected
/// type, or a type error. This only works for types that implement
/// `Copy`, as returning a reference to an inner thunk value is not
/// possible.
/// Generate an `as_*/to_*` accessor method that returns either the
/// expected type, or a type error.
macro_rules! gen_cast {
( $name:ident, $type:ty, $expected:expr, $variant:pat, $result:expr ) => {
pub fn $name(&self) -> Result<$type, ErrorKind> {
match self {
$variant => Ok($result),
Value::Thunk(thunk) => Self::$name(&thunk.value()),
other => Err(type_error($expected, &other)),
}
}
};
}
/// Generate an `is_*` type-checking method.
macro_rules! gen_is {
( $name:ident, $variant:pat ) => {
pub fn $name(&self) -> bool {
match self {
$variant => true,
Value::Thunk(thunk) => Self::$name(&thunk.value()),
_ => false,
}
}
};
}
impl Value {
pub fn type_of(&self) -> &'static str {
match self {
Value::Null => "null",
@ -70,65 +101,14 @@ impl Value {
}
}
pub fn as_bool(&self) -> Result<bool, ErrorKind> {
match self {
Value::Bool(b) => Ok(*b),
other => Err(type_error("bool", &other)),
}
}
gen_cast!(as_bool, bool, "bool", Value::Bool(b), *b);
gen_cast!(to_str, NixString, "string", Value::String(s), s.clone());
gen_cast!(to_attrs, Rc<NixAttrs>, "set", Value::Attrs(a), a.clone());
gen_cast!(to_list, NixList, "list", Value::List(l), l.clone());
gen_cast!(to_closure, Closure, "lambda", Value::Closure(c), c.clone());
pub fn as_attrs(&self) -> Result<&NixAttrs, ErrorKind> {
match self {
Value::Attrs(attrs) => Ok(attrs),
other => Err(type_error("set", &other)),
}
}
pub fn as_str(&self) -> Result<&str, ErrorKind> {
match self {
Value::String(s) => Ok(s.as_str()),
other => Err(type_error("string", &other)),
}
}
pub fn as_list(&self) -> Result<&NixList, ErrorKind> {
match self {
Value::List(xs) => Ok(xs),
other => Err(type_error("list", &other)),
}
}
pub fn to_string(self) -> Result<NixString, ErrorKind> {
match self {
Value::String(s) => Ok(s),
other => Err(type_error("string", &other)),
}
}
pub fn to_attrs(self) -> Result<Rc<NixAttrs>, ErrorKind> {
match self {
Value::Attrs(s) => Ok(s),
other => Err(type_error("set", &other)),
}
}
pub fn to_list(self) -> Result<NixList, ErrorKind> {
match self {
Value::List(l) => Ok(l),
other => Err(type_error("list", &other)),
}
}
pub fn to_closure(self) -> Result<Closure, ErrorKind> {
match self {
Value::Closure(c) => Ok(c),
other => Err(type_error("lambda", &other)),
}
}
pub fn is_bool(&self) -> bool {
matches!(self, Value::Bool(_))
}
gen_is!(is_number, Value::Integer(_) | Value::Float(_));
gen_is!(is_bool, Value::Bool(_));
}
impl Display for Value {

20
tvix/eval/src/vm.rs
View file

@ -259,7 +259,7 @@ impl VM {
}
OpCode::OpAttrsSelect => {
let key = fallible!(self, self.pop().to_string());
let key = fallible!(self, self.pop().to_str());
let attrs = fallible!(self, self.pop().to_attrs());
match attrs.select(key.as_str()) {
@ -274,7 +274,7 @@ impl VM {
}
OpCode::OpAttrsTrySelect => {
let key = fallible!(self, self.pop().to_string());
let key = fallible!(self, self.pop().to_str());
let value = match self.pop() {
Value::Attrs(attrs) => match attrs.select(key.as_str()) {
Some(value) => value.clone(),
@ -288,7 +288,7 @@ impl VM {
}
OpCode::OpAttrsIsSet => {
let key = fallible!(self, self.pop().to_string());
let key = fallible!(self, self.pop().to_str());
let result = match self.pop() {
Value::Attrs(attrs) => attrs.contains(key.as_str()),
@ -379,13 +379,13 @@ impl VM {
}
OpCode::OpResolveWith => {
let ident = fallible!(self, self.pop().to_string());
let ident = fallible!(self, self.pop().to_str());
let value = self.resolve_with(ident.as_str())?;
self.push(value)
}
OpCode::OpResolveWithOrUpvalue(idx) => {
let ident = fallible!(self, self.pop().to_string());
let ident = fallible!(self, self.pop().to_str());
match self.resolve_with(ident.as_str()) {
// Variable found in local `with`-stack.
Ok(value) => self.push(value),
@ -529,7 +529,7 @@ impl VM {
let mut path = Vec::with_capacity(count);
for _ in 0..count {
path.push(fallible!(self, self.pop().to_string()));
path.push(fallible!(self, self.pop().to_str()));
}
self.push(Value::AttrPath(path));
@ -553,7 +553,7 @@ impl VM {
let mut out = String::new();
for _ in 0..count {
out.push_str(fallible!(self, self.pop().to_string()).as_str());
out.push_str(fallible!(self, self.pop().to_str()).as_str());
}
self.push(Value::String(out.into()));
@ -562,7 +562,7 @@ impl VM {
fn resolve_dynamic_upvalue(&mut self, ident_idx: ConstantIdx) -> EvalResult<Value> {
let chunk = self.chunk();
let ident = fallible!(self, chunk.constant(ident_idx).as_str()).to_string();
let ident = fallible!(self, chunk.constant(ident_idx).to_str());
// Peek at the current instruction (note: IP has already
// advanced!) to see if it is actually data indicating a
@ -577,7 +577,7 @@ impl VM {
_ => None,
};
match self.resolve_with(&ident) {
match self.resolve_with(ident.as_str()) {
Ok(v) => Ok(v),
Err(Error {
@ -595,7 +595,7 @@ impl VM {
/// Resolve a dynamic identifier through the with-stack at runtime.
fn resolve_with(&self, ident: &str) -> EvalResult<Value> {
for idx in self.with_stack.iter().rev() {
let with = fallible!(self, self.stack[*idx].as_attrs());
let with = fallible!(self, self.stack[*idx].to_attrs());
match with.select(ident) {
None => continue,
Some(val) => return Ok(val.clone()),