fix(tvix/eval): remove incorrect imports when coercing

The default behavior of string coercion in C++ Nix is to weakly coerce
and import to store if necessary. There is a flag to make it strongly
coerce (coerceMore) and a flag that controls whether path values have
the corresponding file/directory imported into the store before
returning the (store) path as a string (copyToStore). We need to
implement our equivalent to the copyToStore (import_paths) flag for the
benefit of weak coercions that don't import into the store (dirOf,
baseNameOf, readFile, ...) and strong coercions that don't import into
the store (toString).

This makes coerce_to_string as well as CoercionKind weirder and more
versatile, but prevents us from reimplementing parts of the coercion
logic constantly as can be seen in the case of baseNameOf.

Note that it is not possible to test this properly in //tvix/eval tests
due to the lack of an appropriate EvalIO implementation being available.
Tests should be added to //tvix/glue down the line.

Change-Id: I8fb8ab99c7fe08e311d2ba1c36960746bf22f566
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10361
Autosubmit: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
Reviewed-by: Adam Joseph <adam@westernsemico.com>
This commit is contained in:
sterni 2023-12-13 14:52:07 +01:00 committed by clbot
parent a30dd0905a
commit 7165ebc43b
8 changed files with 207 additions and 54 deletions

View file

@ -41,6 +41,8 @@ pub const CURRENT_PLATFORM: &str = env!("TVIX_CURRENT_SYSTEM");
/// builtin. This coercion can _never_ be performed in a Nix program /// builtin. This coercion can _never_ be performed in a Nix program
/// without using builtins (i.e. the trick `path: /. + path` to /// without using builtins (i.e. the trick `path: /. + path` to
/// convert from a string to a path wouldn't hit this code). /// convert from a string to a path wouldn't hit this code).
///
/// This operation doesn't import a Nix path value into the store.
pub async fn coerce_value_to_path( pub async fn coerce_value_to_path(
co: &GenCo, co: &GenCo,
v: Value, v: Value,
@ -50,7 +52,16 @@ pub async fn coerce_value_to_path(
return Ok(Ok(*p)); return Ok(Ok(*p));
} }
match generators::request_string_coerce(co, value, CoercionKind::Weak).await { match generators::request_string_coerce(
co,
value,
CoercionKind {
strong: false,
import_paths: false,
},
)
.await
{
Ok(vs) => { Ok(vs) => {
let path = PathBuf::from(vs.as_str()); let path = PathBuf::from(vs.as_str());
if path.is_absolute() { if path.is_absolute() {
@ -71,6 +82,7 @@ mod pure_builtins {
#[builtin("abort")] #[builtin("abort")]
async fn builtin_abort(co: GenCo, message: Value) -> Result<Value, ErrorKind> { async fn builtin_abort(co: GenCo, message: Value) -> Result<Value, ErrorKind> {
// TODO(sterni): coerces to string
Err(ErrorKind::Abort(message.to_str()?.to_string())) Err(ErrorKind::Abort(message.to_str()?.to_string()))
} }
@ -136,14 +148,15 @@ mod pure_builtins {
let span = generators::request_span(&co).await; let span = generators::request_span(&co).await;
let s = match s { let s = match s {
val @ Value::Catchable(_) => return Ok(val), val @ Value::Catchable(_) => return Ok(val),
// it is important that builtins.baseNameOf does not
// coerce paths into strings, since this will turn them
// into store paths, and `builtins.baseNameOf
// ./config.nix` will return a hash-prefixed value like
// "hpmyf3vlqg5aadri97xglcvvjbk8xw3g-config.nix"
Value::Path(p) => NixString::from(p.to_string_lossy().to_string()),
_ => s _ => s
.coerce_to_string(co, CoercionKind::Weak, span) .coerce_to_string(
co,
CoercionKind {
strong: false,
import_paths: false,
},
span,
)
.await? .await?
.to_str()?, .to_str()?,
}; };
@ -239,7 +252,16 @@ mod pure_builtins {
if i != 0 { if i != 0 {
res.push_str(&separator); res.push_str(&separator);
} }
match generators::request_string_coerce(&co, val, CoercionKind::Weak).await { match generators::request_string_coerce(
&co,
val,
CoercionKind {
strong: false,
import_paths: true,
},
)
.await
{
Ok(s) => res.push_str(s.as_str()), Ok(s) => res.push_str(s.as_str()),
Err(c) => return Ok(Value::Catchable(c)), Err(c) => return Ok(Value::Catchable(c)),
} }
@ -263,7 +285,14 @@ mod pure_builtins {
let is_path = s.is_path(); let is_path = s.is_path();
let span = generators::request_span(&co).await; let span = generators::request_span(&co).await;
let str = s let str = s
.coerce_to_string(co, CoercionKind::Weak, span) .coerce_to_string(
co,
CoercionKind {
strong: false,
import_paths: false,
},
span,
)
.await? .await?
.to_str()?; .to_str()?;
let result = str let result = str
@ -983,7 +1012,16 @@ mod pure_builtins {
async fn builtin_string_length(co: GenCo, #[lazy] s: Value) -> Result<Value, ErrorKind> { async fn builtin_string_length(co: GenCo, #[lazy] s: Value) -> Result<Value, ErrorKind> {
// also forces the value // also forces the value
let span = generators::request_span(&co).await; let span = generators::request_span(&co).await;
let s = s.coerce_to_string(co, CoercionKind::Weak, span).await?; let s = s
.coerce_to_string(
co,
CoercionKind {
strong: false,
import_paths: true,
},
span,
)
.await?;
Ok(Value::Integer(s.to_str()?.as_str().len() as i64)) Ok(Value::Integer(s.to_str()?.as_str().len() as i64))
} }
@ -1002,7 +1040,16 @@ mod pure_builtins {
let beg = start.as_int()?; let beg = start.as_int()?;
let len = len.as_int()?; let len = len.as_int()?;
let span = generators::request_span(&co).await; let span = generators::request_span(&co).await;
let x = s.coerce_to_string(co, CoercionKind::Weak, span).await?; let x = s
.coerce_to_string(
co,
CoercionKind {
strong: false,
import_paths: true,
},
span,
)
.await?;
if x.is_catchable() { if x.is_catchable() {
return Ok(x); return Ok(x);
} }
@ -1043,6 +1090,7 @@ mod pure_builtins {
#[builtin("throw")] #[builtin("throw")]
async fn builtin_throw(co: GenCo, message: Value) -> Result<Value, ErrorKind> { async fn builtin_throw(co: GenCo, message: Value) -> Result<Value, ErrorKind> {
// TODO(sterni): coerces to string
Ok(Value::Catchable(CatchableErrorKind::Throw( Ok(Value::Catchable(CatchableErrorKind::Throw(
message.to_str()?.to_string(), message.to_str()?.to_string(),
))) )))
@ -1052,7 +1100,15 @@ mod pure_builtins {
async fn builtin_to_string(co: GenCo, #[lazy] x: Value) -> Result<Value, ErrorKind> { async fn builtin_to_string(co: GenCo, #[lazy] x: Value) -> Result<Value, ErrorKind> {
// coerce_to_string forces for us // coerce_to_string forces for us
let span = generators::request_span(&co).await; let span = generators::request_span(&co).await;
x.coerce_to_string(co, CoercionKind::Strong, span).await x.coerce_to_string(
co,
CoercionKind {
strong: true,
import_paths: false,
},
span,
)
.await
} }
#[builtin("toXML")] #[builtin("toXML")]
@ -1085,7 +1141,16 @@ mod pure_builtins {
Ok(path) => { Ok(path) => {
let path: Value = crate::value::canon_path(path).into(); let path: Value = crate::value::canon_path(path).into();
let span = generators::request_span(&co).await; let span = generators::request_span(&co).await;
Ok(path.coerce_to_string(co, CoercionKind::Weak, span).await?) Ok(path
.coerce_to_string(
co,
CoercionKind {
strong: false,
import_paths: false,
},
span,
)
.await?)
} }
} }
} }

View file

@ -378,10 +378,7 @@ to a missing value in the attribute set(s) included via `with`."#,
ErrorKind::BytecodeError(_) => write!(f, "while evaluating this Nix code"), ErrorKind::BytecodeError(_) => write!(f, "while evaluating this Nix code"),
ErrorKind::NotCoercibleToString { kind, from } => { ErrorKind::NotCoercibleToString { kind, from } => {
let kindly = match kind { let kindly = if kind.strong { "strongly" } else { "weakly" };
CoercionKind::Strong => "strongly",
CoercionKind::Weak => "weakly",
};
let hint = if *from == "set" { let hint = if *from == "set" {
", missing a `__toString` or `outPath` attribute" ", missing a `__toString` or `outPath` attribute"

View file

@ -202,7 +202,8 @@ pub enum OpCode {
OpInterpolate(Count), OpInterpolate(Count),
/// Force the Value on the stack and coerce it to a string, always using /// Force the Value on the stack and coerce it to a string, always using
/// `CoercionKind::Weak`. /// `CoercionKind::Weak { import_paths: true }`. This is the behavior
/// necessary for path interpolation.
OpCoerceToString, OpCoerceToString,
// Paths // Paths

View file

@ -47,7 +47,14 @@ impl Value {
if attrs.select("__toString").is_some() { if attrs.select("__toString").is_some() {
let span = generators::request_span(co).await; let span = generators::request_span(co).await;
match Value::Attrs(attrs) match Value::Attrs(attrs)
.coerce_to_string_(co, CoercionKind::Weak, span) .coerce_to_string_(
co,
CoercionKind {
strong: false,
import_paths: false,
},
span,
)
.await? .await?
{ {
Value::Catchable(cek) => return Ok(Err(cek)), Value::Catchable(cek) => return Ok(Err(cek)),

View file

@ -162,15 +162,22 @@ macro_rules! gen_is {
/// Describes what input types are allowed when coercing a `Value` to a string /// Describes what input types are allowed when coercing a `Value` to a string
#[derive(Clone, Copy, PartialEq, Debug)] #[derive(Clone, Copy, PartialEq, Debug)]
pub enum CoercionKind { pub struct CoercionKind {
/// Only coerce already "stringly" types like strings and paths, but also /// If false only coerce already "stringly" types like strings and paths, but
/// coerce sets that have a `__toString` attribute. Equivalent to /// also coerce sets that have a `__toString` attribute. In Tvix, this is
/// `!coerceMore` in C++ Nix. /// usually called a weak coercion. Equivalent to passing `false` as the
Weak, /// `coerceMore` argument of `EvalState::coerceToString` in C++ Nix.
/// Coerce all value types included by `Weak`, but also coerce `null`, ///
/// booleans, integers, floats and lists of coercible types. Equivalent to /// If true coerce all value types included by a weak coercion, but also
/// `coerceMore` in C++ Nix. /// coerce `null`, booleans, integers, floats and lists of coercible types.
Strong, /// Consequently, we call this a strong coercion. Equivalent to passing
/// `true` as `coerceMore` in C++ Nix.
pub strong: bool,
/// If `import_paths` is `true`, paths are imported into the store and their
/// store path is the result of the coercion (equivalent to the
/// `copyToStore` argument of `EvalState::coerceToString` in C++ Nix).
pub import_paths: bool,
} }
impl<T> From<T> for Value impl<T> From<T> for Value
@ -323,11 +330,22 @@ impl Value {
// C++ Nix and Tvix is that the former's strings are arbitrary byte // C++ Nix and Tvix is that the former's strings are arbitrary byte
// sequences without NUL bytes, whereas Tvix only allows valid // sequences without NUL bytes, whereas Tvix only allows valid
// Unicode. See also b/189. // Unicode. See also b/189.
(Value::Path(p), _) => { (
// TODO(tazjin): there are cases where coerce_to_string does not import Value::Path(p),
CoercionKind {
import_paths: true, ..
},
) => {
let imported = generators::request_path_import(co, *p).await; let imported = generators::request_path_import(co, *p).await;
Ok(imported.to_string_lossy().into_owned()) Ok(imported.to_string_lossy().into_owned())
} }
(
Value::Path(p),
CoercionKind {
import_paths: false,
..
},
) => Ok(p.to_string_lossy().into_owned()),
// Attribute sets can be converted to strings if they either have an // Attribute sets can be converted to strings if they either have an
// `__toString` attribute which holds a function that receives the // `__toString` attribute which holds a function that receives the
@ -358,19 +376,19 @@ impl Value {
} }
// strong coercions // strong coercions
(Value::Null, CoercionKind::Strong) (Value::Null, CoercionKind { strong: true, .. })
| (Value::Bool(false), CoercionKind::Strong) => Ok("".to_owned()), | (Value::Bool(false), CoercionKind { strong: true, .. }) => Ok("".to_owned()),
(Value::Bool(true), CoercionKind::Strong) => Ok("1".to_owned()), (Value::Bool(true), CoercionKind { strong: true, .. }) => Ok("1".to_owned()),
(Value::Integer(i), CoercionKind::Strong) => Ok(format!("{i}")), (Value::Integer(i), CoercionKind { strong: true, .. }) => Ok(format!("{i}")),
(Value::Float(f), CoercionKind::Strong) => { (Value::Float(f), CoercionKind { strong: true, .. }) => {
// contrary to normal Display, coercing a float to a string will // contrary to normal Display, coercing a float to a string will
// result in unconditional 6 decimal places // result in unconditional 6 decimal places
Ok(format!("{:.6}", f)) Ok(format!("{:.6}", f))
} }
// Lists are coerced by coercing their elements and interspersing spaces // Lists are coerced by coercing their elements and interspersing spaces
(Value::List(list), CoercionKind::Strong) => { (Value::List(list), CoercionKind { strong: true, .. }) => {
for elem in list.into_iter().rev() { for elem in list.into_iter().rev() {
vals.push(elem); vals.push(elem);
} }

View file

@ -144,10 +144,19 @@ impl Display for VMRequest {
} }
VMRequest::StackPush(v) => write!(f, "stack_push({})", v.type_of()), VMRequest::StackPush(v) => write!(f, "stack_push({})", v.type_of()),
VMRequest::StackPop => write!(f, "stack_pop"), VMRequest::StackPop => write!(f, "stack_pop"),
VMRequest::StringCoerce(v, kind) => match kind { VMRequest::StringCoerce(
CoercionKind::Weak => write!(f, "weak_string_coerce({})", v.type_of()), v,
CoercionKind::Strong => write!(f, "strong_string_coerce({})", v.type_of()), CoercionKind {
}, strong,
import_paths,
},
) => write!(
f,
"{}_{}importing_string_coerce({})",
if *strong { "strong" } else { "weak" },
if *import_paths { "" } else { "non_" },
v.type_of()
),
VMRequest::Call(v) => write!(f, "call({})", v), VMRequest::Call(v) => write!(f, "call({})", v),
VMRequest::EnterLambda { lambda, .. } => { VMRequest::EnterLambda { lambda, .. } => {
write!(f, "enter_lambda({:p})", *lambda) write!(f, "enter_lambda({:p})", *lambda)

View file

@ -780,7 +780,14 @@ impl<'o> VM<'o> {
self.push_call_frame(span, frame); self.push_call_frame(span, frame);
self.enqueue_generator("coerce_to_string", gen_span.clone(), |co| { self.enqueue_generator("coerce_to_string", gen_span.clone(), |co| {
value.coerce_to_string(co, CoercionKind::Weak, gen_span) value.coerce_to_string(
co,
CoercionKind {
strong: false,
import_paths: true,
},
gen_span,
)
}); });
return Ok(false); return Ok(false);
@ -1206,7 +1213,23 @@ async fn add_values(co: GenCo, a: Value, b: Value) -> Result<Value, ErrorKind> {
let result = match (a, b) { let result = match (a, b) {
(Value::Path(p), v) => { (Value::Path(p), v) => {
let mut path = p.to_string_lossy().into_owned(); let mut path = p.to_string_lossy().into_owned();
match generators::request_string_coerce(&co, v, CoercionKind::Weak).await { match generators::request_string_coerce(
&co,
v,
CoercionKind {
strong: false,
// Concatenating a Path with something else results in a
// Path, so we don't need to import any paths (paths
// imported by Nix always exist as a string, unless
// converted by the user). In C++ Nix they even may not
// contain any string context, the resulting error of such a
// case can not be replicated by us.
import_paths: false,
},
)
.await
{
Ok(vs) => { Ok(vs) => {
path.push_str(vs.as_str()); path.push_str(vs.as_str());
crate::value::canon_path(PathBuf::from(path)).into() crate::value::canon_path(PathBuf::from(path)).into()
@ -1215,14 +1238,38 @@ async fn add_values(co: GenCo, a: Value, b: Value) -> Result<Value, ErrorKind> {
} }
} }
(Value::String(s1), Value::String(s2)) => Value::String(s1.concat(&s2)), (Value::String(s1), Value::String(s2)) => Value::String(s1.concat(&s2)),
(Value::String(s1), v) => generators::request_string_coerce(&co, v, CoercionKind::Weak) (Value::String(s1), v) => generators::request_string_coerce(
.await &co,
.map(|s2| Value::String(s1.concat(&s2))) v,
.into(), CoercionKind {
strong: false,
// Behaves the same as string interpolation
import_paths: true,
},
)
.await
.map(|s2| Value::String(s1.concat(&s2)))
.into(),
(a @ Value::Integer(_), b) | (a @ Value::Float(_), b) => arithmetic_op!(&a, &b, +)?, (a @ Value::Integer(_), b) | (a @ Value::Float(_), b) => arithmetic_op!(&a, &b, +)?,
(a, b) => { (a, b) => {
let r1 = generators::request_string_coerce(&co, a, CoercionKind::Weak).await; let r1 = generators::request_string_coerce(
let r2 = generators::request_string_coerce(&co, b, CoercionKind::Weak).await; &co,
a,
CoercionKind {
strong: false,
import_paths: false,
},
)
.await;
let r2 = generators::request_string_coerce(
&co,
b,
CoercionKind {
strong: false,
import_paths: false,
},
)
.await;
match (r1, r2) { match (r1, r2) {
(Ok(s1), Ok(s2)) => Value::String(s1.concat(&s2)), (Ok(s1), Ok(s2)) => Value::String(s1.concat(&s2)),
(Err(c), _) => return Ok(Value::Catchable(c)), (Err(c), _) => return Ok(Value::Catchable(c)),

View file

@ -161,7 +161,7 @@ async fn handle_derivation_parameters(
"args" => { "args" => {
let args = value.to_list()?; let args = value.to_list()?;
for arg in args { for arg in args {
match strong_coerce_to_string(co, arg).await? { match strong_importing_coerce_to_string(co, arg).await? {
Err(cek) => return Ok(Err(cek)), Err(cek) => return Ok(Err(cek)),
Ok(s) => drv.arguments.push(s), Ok(s) => drv.arguments.push(s),
} }
@ -194,12 +194,21 @@ async fn handle_derivation_parameters(
Ok(Ok(true)) Ok(Ok(true))
} }
async fn strong_coerce_to_string( async fn strong_importing_coerce_to_string(
co: &GenCo, co: &GenCo,
val: Value, val: Value,
) -> Result<Result<String, CatchableErrorKind>, ErrorKind> { ) -> Result<Result<String, CatchableErrorKind>, ErrorKind> {
let val = generators::request_force(co, val).await; let val = generators::request_force(co, val).await;
match generators::request_string_coerce(co, val, CoercionKind::Strong).await { match generators::request_string_coerce(
co,
val,
CoercionKind {
strong: true,
import_paths: true,
},
)
.await
{
Err(cek) => Ok(Err(cek)), Err(cek) => Ok(Err(cek)),
Ok(val_str) => Ok(Ok(val_str.as_str().to_string())), Ok(val_str) => Ok(Ok(val_str.as_str().to_string())),
} }
@ -268,7 +277,7 @@ pub(crate) mod derivation_builtins {
key: &str, key: &str,
) -> Result<Result<Option<String>, CatchableErrorKind>, ErrorKind> { ) -> Result<Result<Option<String>, CatchableErrorKind>, ErrorKind> {
if let Some(attr) = attrs.select(key) { if let Some(attr) = attrs.select(key) {
match strong_coerce_to_string(co, attr.clone()).await? { match strong_importing_coerce_to_string(co, attr.clone()).await? {
Err(cek) => return Ok(Err(cek)), Err(cek) => return Ok(Err(cek)),
Ok(str) => return Ok(Ok(Some(str))), Ok(str) => return Ok(Ok(Some(str))),
} }
@ -283,7 +292,7 @@ pub(crate) mod derivation_builtins {
continue; continue;
} }
match strong_coerce_to_string(&co, value.clone()).await? { match strong_importing_coerce_to_string(&co, value.clone()).await? {
Err(cek) => return Ok(Value::Catchable(cek)), Err(cek) => return Ok(Value::Catchable(cek)),
Ok(val_str) => { Ok(val_str) => {
// handle_derivation_parameters tells us whether the // handle_derivation_parameters tells us whether the