feat(tvix/eval): nonrecursive coerce_to_string() r/7176

After this commit, the only non-builtins uses of generators are:

  - coerce_to_string() uses generators::request_enter_lambda()
  - Thunk::force() uses generators::request_enter_lambda()

That's it!  Once those two are taken care of, GenCo can become an
implementation detail of `builtins::BuiltinGen`.  No more crazy
nonlocal flow control within the interpreter: if you've got a GenCo
floating around in your code it's because you're writing a builtin,
which isn't part of the core interpreter.  The interpreter won't
need GenCos to talk to itself anymore.

Technically generators::request_path_import() is also used by
coerce_to_string(), but that's just because the io_handle happens to
be part of the VM.  There's no recursion-depth issue there, so the
call doesn't need to go through the generator mechanism
(request_path_import() doesn't call back to the interpreter!)

Change-Id: I83ce5774d49b88fdafdd61160975b4937a435bb0
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10256
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Autosubmit: Adam Joseph <adam@westernsemico.com>

6 files changed, 138 insertions, 121 deletions

diff --git a/tvix/eval/src/builtins/mod.rs b/tvix/eval/src/builtins/mod.rs
index 5f99704e63..62684b8245 100644
--- a/tvix/eval/src/builtins/mod.rs
+++ b/tvix/eval/src/builtins/mod.rs
@@ -133,7 +133,11 @@ mod pure_builtins {
 
     #[builtin("baseNameOf")]
     async fn builtin_base_name_of(co: GenCo, s: Value) -> Result<Value, ErrorKind> {
-        let s = s.coerce_to_string(co, CoercionKind::Weak).await?.to_str()?;
+        let span = generators::request_span(&co).await;
+        let s = s
+            .coerce_to_string(co, CoercionKind::Weak, span)
+            .await?
+            .to_str()?;
         let result: String = s.rsplit_once('/').map(|(_, x)| x).unwrap_or(&s).into();
         Ok(result.into())
     }
@@ -248,7 +252,11 @@ mod pure_builtins {
     #[builtin("dirOf")]
     async fn builtin_dir_of(co: GenCo, s: Value) -> Result<Value, ErrorKind> {
         let is_path = s.is_path();
-        let str = s.coerce_to_string(co, CoercionKind::Weak).await?.to_str()?;
+        let span = generators::request_span(&co).await;
+        let str = s
+            .coerce_to_string(co, CoercionKind::Weak, span)
+            .await?
+            .to_str()?;
         let result = str
             .rsplit_once('/')
             .map(|(x, _)| match x {
@@ -915,7 +923,8 @@ mod pure_builtins {
     #[builtin("stringLength")]
     async fn builtin_string_length(co: GenCo, #[lazy] s: Value) -> Result<Value, ErrorKind> {
         // also forces the value
-        let s = s.coerce_to_string(co, CoercionKind::Weak).await?;
+        let span = generators::request_span(&co).await;
+        let s = s.coerce_to_string(co, CoercionKind::Weak, span).await?;
         Ok(Value::Integer(s.to_str()?.as_str().len() as i64))
     }
 
@@ -933,7 +942,11 @@ mod pure_builtins {
     ) -> Result<Value, ErrorKind> {
         let beg = start.as_int()?;
         let len = len.as_int()?;
-        let x = s.coerce_to_string(co, CoercionKind::Weak).await?.to_str()?;
+        let span = generators::request_span(&co).await;
+        let x = s
+            .coerce_to_string(co, CoercionKind::Weak, span)
+            .await?
+            .to_str()?;
 
         if beg < 0 {
             return Err(ErrorKind::IndexOutOfBounds { index: beg });
@@ -978,7 +991,8 @@ mod pure_builtins {
     #[builtin("toString")]
     async fn builtin_to_string(co: GenCo, #[lazy] x: Value) -> Result<Value, ErrorKind> {
         // coerce_to_string forces for us
-        x.coerce_to_string(co, CoercionKind::Strong).await
+        let span = generators::request_span(&co).await;
+        x.coerce_to_string(co, CoercionKind::Strong, span).await
     }
 
     #[builtin("toXML")]
@@ -1010,7 +1024,8 @@ mod pure_builtins {
             Err(cek) => Ok(Value::Catchable(cek)),
             Ok(path) => {
                 let path: Value = crate::value::canon_path(path).into();
-                Ok(path.coerce_to_string(co, CoercionKind::Weak).await?)
+                let span = generators::request_span(&co).await;
+                Ok(path.coerce_to_string(co, CoercionKind::Weak, span).await?)
             }
         }
     }
diff --git a/tvix/eval/src/value/attrs.rs b/tvix/eval/src/value/attrs.rs
index 03183810f2..b80d8b2db8 100644
--- a/tvix/eval/src/value/attrs.rs
+++ b/tvix/eval/src/value/attrs.rs
@@ -14,11 +14,9 @@ use serde::Deserialize;
 
 use super::string::NixString;
 use super::thunk::ThunkSet;
-use super::CoercionKind;
 use super::TotalDisplay;
 use super::Value;
 use crate::errors::ErrorKind;
-use crate::generators::{self, GenCo};
 
 lazy_static! {
     static ref NAME_S: NixString = "name".into();
@@ -370,30 +368,6 @@ impl NixAttrs {
     pub(crate) fn from_kv(name: Value, value: Value) -> Self {
         NixAttrs(AttrsRep::KV { name, value })
     }
-
-    /// Attempt to coerce an attribute set with a `__toString`
-    /// attribute to a string.
-    pub(crate) async fn try_to_string(&self, co: &GenCo, kind: CoercionKind) -> Option<NixString> {
-        if let Some(to_string) = self.select("__toString") {
-            let callable = generators::request_force(co, to_string.clone()).await;
-
-            // Leave the attribute set on the stack as an argument
-            // to the function call.
-            generators::request_stack_push(co, Value::Attrs(Box::new(self.clone()))).await;
-
-            // Call the callable ...
-            let result = generators::request_call(co, callable).await;
-
-            // Recurse on the result, as attribute set coercion
-            // actually works recursively, e.g. you can even return
-            // /another/ set with a __toString attr.
-            let s = generators::request_string_coerce(co, result, kind).await;
-
-            return s.ok();
-        }
-
-        None
-    }
 }
 
 impl IntoIterator for NixAttrs {
diff --git a/tvix/eval/src/value/json.rs b/tvix/eval/src/value/json.rs
index bff04487fa..f7ecf121de 100644
--- a/tvix/eval/src/value/json.rs
+++ b/tvix/eval/src/value/json.rs
@@ -44,8 +44,16 @@ impl Value {
                 // Attribute sets with a callable `__toString` attribute
                 // serialise to the string-coerced version of the result of
                 // calling that.
-                if let Some(s) = attrs.try_to_string(co, CoercionKind::Weak).await {
-                    return Ok(Ok(Json::String(s.as_str().to_string())));
+                if attrs.select("__toString").is_some() {
+                    let span = generators::request_span(co).await;
+                    match Value::Attrs(attrs)
+                        .coerce_to_string_(co, CoercionKind::Weak, span)
+                        .await?
+                    {
+                        Value::Catchable(cek) => return Ok(Err(cek)),
+                        Value::String(s) => return Ok(Ok(Json::String(s.as_str().to_owned()))),
+                        _ => panic!("Value::coerce_to_string_() returned a non-string!"),
+                    }
                 }
 
                 // Attribute sets with an `outPath` attribute
diff --git a/tvix/eval/src/value/mod.rs b/tvix/eval/src/value/mod.rs
index 596dddba52..003567bfd4 100644
--- a/tvix/eval/src/value/mod.rs
+++ b/tvix/eval/src/value/mod.rs
@@ -289,102 +289,122 @@ impl Value {
         }
     }
 
-    // TODO(amjoseph): de-asyncify this (when called directly by the VM)
+    pub async fn coerce_to_string(
+        self,
+        co: GenCo,
+        kind: CoercionKind,
+        span: LightSpan,
+    ) -> Result<Value, ErrorKind> {
+        self.coerce_to_string_(&co, kind, span).await
+    }
+
     /// Coerce a `Value` to a string. See `CoercionKind` for a rundown of what
     /// input types are accepted under what circumstances.
-    pub async fn coerce_to_string(self, co: GenCo, kind: CoercionKind) -> Result<Value, ErrorKind> {
-        let value = generators::request_force(&co, self).await;
-
-        match (value, kind) {
-            // coercions that are always done
-            tuple @ (Value::String(_), _) => Ok(tuple.0),
-
-            // TODO(sterni): Think about proper encoding handling here. This needs
-            // general consideration anyways, since one current discrepancy between
-            // C++ Nix and Tvix is that the former's strings are arbitrary byte
-            // sequences without NUL bytes, whereas Tvix only allows valid
-            // Unicode. See also b/189.
-            (Value::Path(p), _) => {
-                // TODO(tazjin): there are cases where coerce_to_string does not import
-                let imported = generators::request_path_import(&co, *p).await;
-                Ok(imported.to_string_lossy().into_owned().into())
-            }
-
-            // Attribute sets can be converted to strings if they either have an
-            // `__toString` attribute which holds a function that receives the
-            // set itself or an `outPath` attribute which should be a string.
-            // `__toString` is preferred.
-            (Value::Attrs(attrs), kind) => {
-                if let Some(s) = attrs.try_to_string(&co, kind).await {
-                    return Ok(Value::String(s));
+    pub async fn coerce_to_string_(
+        self,
+        co: &GenCo,
+        kind: CoercionKind,
+        span: LightSpan,
+    ) -> Result<Value, ErrorKind> {
+        let mut result = String::new();
+        let mut vals = vec![self];
+        loop {
+            let value = if let Some(v) = vals.pop() {
+                v.force(co, span.clone()).await?
+            } else {
+                return Ok(Value::String(result.into()));
+            };
+            let coerced = match (value, kind) {
+                // coercions that are always done
+                (Value::String(s), _) => Ok(s.as_str().to_owned()),
+
+                // TODO(sterni): Think about proper encoding handling here. This needs
+                // general consideration anyways, since one current discrepancy between
+                // C++ Nix and Tvix is that the former's strings are arbitrary byte
+                // sequences without NUL bytes, whereas Tvix only allows valid
+                // Unicode. See also b/189.
+                (Value::Path(p), _) => {
+                    // TODO(tazjin): there are cases where coerce_to_string does not import
+                    let imported = generators::request_path_import(co, *p).await;
+                    Ok(imported.to_string_lossy().into_owned())
                 }
 
-                if let Some(out_path) = attrs.select("outPath") {
-                    return match generators::request_string_coerce(&co, out_path.clone(), kind)
-                        .await
-                    {
-                        Ok(s) => Ok(Value::String(s)),
-                        Err(c) => Ok(Value::Catchable(c)),
-                    };
+                // Attribute sets can be converted to strings if they either have an
+                // `__toString` attribute which holds a function that receives the
+                // set itself or an `outPath` attribute which should be a string.
+                // `__toString` is preferred.
+                (Value::Attrs(attrs), kind) => {
+                    if let Some(to_string) = attrs.select("__toString") {
+                        let callable = to_string.clone().force(co, span.clone()).await?;
+
+                        // Leave the attribute set on the stack as an argument
+                        // to the function call.
+                        generators::request_stack_push(co, Value::Attrs(attrs.clone())).await;
+
+                        // Call the callable ...
+                        let result = generators::request_call(co, callable).await;
+
+                        // Recurse on the result, as attribute set coercion
+                        // actually works recursively, e.g. you can even return
+                        // /another/ set with a __toString attr.
+                        vals.push(result);
+                        continue;
+                    } else if let Some(out_path) = attrs.select("outPath") {
+                        vals.push(out_path.clone());
+                        continue;
+                    } else {
+                        return Err(ErrorKind::NotCoercibleToString { from: "set", kind });
+                    }
                 }
 
-                Err(ErrorKind::NotCoercibleToString { from: "set", kind })
-            }
-
-            // strong coercions
-            (Value::Null, CoercionKind::Strong) | (Value::Bool(false), CoercionKind::Strong) => {
-                Ok("".into())
-            }
-            (Value::Bool(true), CoercionKind::Strong) => Ok("1".into()),
-
-            (Value::Integer(i), CoercionKind::Strong) => Ok(format!("{i}").into()),
-            (Value::Float(f), CoercionKind::Strong) => {
-                // contrary to normal Display, coercing a float to a string will
-                // result in unconditional 6 decimal places
-                Ok(format!("{:.6}", f).into())
-            }
-
-            // Lists are coerced by coercing their elements and interspersing spaces
-            (Value::List(list), CoercionKind::Strong) => {
-                let mut out = String::new();
+                // strong coercions
+                (Value::Null, CoercionKind::Strong)
+                | (Value::Bool(false), CoercionKind::Strong) => Ok("".to_owned()),
+                (Value::Bool(true), CoercionKind::Strong) => Ok("1".to_owned()),
 
-                for (idx, elem) in list.into_iter().enumerate() {
-                    if idx > 0 {
-                        out.push(' ');
-                    }
+                (Value::Integer(i), CoercionKind::Strong) => Ok(format!("{i}")),
+                (Value::Float(f), CoercionKind::Strong) => {
+                    // contrary to normal Display, coercing a float to a string will
+                    // result in unconditional 6 decimal places
+                    Ok(format!("{:.6}", f))
+                }
 
-                    match generators::request_string_coerce(&co, elem, kind).await {
-                        Ok(s) => out.push_str(s.as_str()),
-                        Err(c) => return Ok(Value::Catchable(c)),
+                // Lists are coerced by coercing their elements and interspersing spaces
+                (Value::List(list), CoercionKind::Strong) => {
+                    for elem in list.into_iter().rev() {
+                        vals.push(elem);
                     }
+                    continue;
                 }
 
-                Ok(Value::String(out.into()))
-            }
+                (Value::Thunk(_), _) => panic!("Tvix bug: force returned unforced thunk"),
+
+                val @ (Value::Closure(_), _)
+                | val @ (Value::Builtin(_), _)
+                | val @ (Value::Null, _)
+                | val @ (Value::Bool(_), _)
+                | val @ (Value::Integer(_), _)
+                | val @ (Value::Float(_), _)
+                | val @ (Value::List(_), _) => Err(ErrorKind::NotCoercibleToString {
+                    from: val.0.type_of(),
+                    kind,
+                }),
 
-            (Value::Thunk(_), _) => panic!("Tvix bug: force returned unforced thunk"),
-
-            val @ (Value::Closure(_), _)
-            | val @ (Value::Builtin(_), _)
-            | val @ (Value::Null, _)
-            | val @ (Value::Bool(_), _)
-            | val @ (Value::Integer(_), _)
-            | val @ (Value::Float(_), _)
-            | val @ (Value::List(_), _) => Err(ErrorKind::NotCoercibleToString {
-                from: val.0.type_of(),
-                kind,
-            }),
-
-            (c @ Value::Catchable(_), _) => Ok(c),
-
-            (Value::AttrNotFound, _)
-            | (Value::Blueprint(_), _)
-            | (Value::DeferredUpvalue(_), _)
-            | (Value::UnresolvedPath(_), _)
-            | (Value::Json(_), _)
-            | (Value::FinaliseRequest(_), _) => {
-                panic!("tvix bug: .coerce_to_string() called on internal value")
+                (c @ Value::Catchable(_), _) => return Ok(c),
+
+                (Value::AttrNotFound, _)
+                | (Value::Blueprint(_), _)
+                | (Value::DeferredUpvalue(_), _)
+                | (Value::UnresolvedPath(_), _)
+                | (Value::Json(_), _)
+                | (Value::FinaliseRequest(_), _) => {
+                    panic!("tvix bug: .coerce_to_string() called on internal value")
+                }
+            };
+            if !result.is_empty() {
+                result.push(' ');
             }
+            result.push_str(&coerced?);
         }
     }
 
diff --git a/tvix/eval/src/vm/generators.rs b/tvix/eval/src/vm/generators.rs
index 7b92c1f0af..6563a82790 100644
--- a/tvix/eval/src/vm/generators.rs
+++ b/tvix/eval/src/vm/generators.rs
@@ -344,8 +344,8 @@ impl<'o> VM<'o> {
 
                         VMRequest::StringCoerce(val, kind) => {
                             self.reenqueue_generator(name, span.clone(), generator);
-                            self.enqueue_generator("coerce_to_string", span, |co| {
-                                val.coerce_to_string(co, kind)
+                            self.enqueue_generator("coerce_to_string", span.clone(), |co| {
+                                val.coerce_to_string(co, kind, span)
                             });
                             return Ok(false);
                         }
diff --git a/tvix/eval/src/vm/mod.rs b/tvix/eval/src/vm/mod.rs
index 5ab095b220..6128e3a601 100644
--- a/tvix/eval/src/vm/mod.rs
+++ b/tvix/eval/src/vm/mod.rs
@@ -744,8 +744,8 @@ impl<'o> VM<'o> {
                     let gen_span = frame.current_light_span();
                     self.push_call_frame(span, frame);
 
-                    self.enqueue_generator("coerce_to_string", gen_span, |co| {
-                        value.coerce_to_string(co, CoercionKind::Weak)
+                    self.enqueue_generator("coerce_to_string", gen_span.clone(), |co| {
+                        value.coerce_to_string(co, CoercionKind::Weak, gen_span)
                     });
 
                     return Ok(false);