fix(tvix/eval): coerce string interpolation parts to string

With this puzzle piece of string compilation in place, `compile_str`
becomes less redundant, as every part now needs to be compiled the same.
The thunking logic becomes a bit trickier, since we need to thunk even
in the case of `count == 1` if the single part is interpolating.
Splitting the inner (shared) code in a separate function turned out to
be easier for making rustc content.

Change-Id: I6a554ca599926ae5907d7acffce349c9616f568f
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6582
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI

tvix/eval/src/compiler/mod.rs

@@ -257,12 +257,17 @@ impl Compiler<'_, '_> {
         self.emit_constant(value, &node);
     }
 
-    fn compile_str(&mut self, slot: LocalIdx, node: ast::Str) {
+    /// Helper that compiles the given string parts strictly. The caller
-        let parts = node.normalized_parts();
+    /// (`compile_str`) needs to figure out if the result of compiling this
+    /// needs to be thunked or not.
+    fn compile_str_parts(
+        &mut self,
+        slot: LocalIdx,
+        parent_node: &ast::Str,
+        parts: Vec<ast::InterpolPart<String>>,
+    ) {
         let count = parts.len();
 
-        if count != 1 {
-            self.thunk(slot, &node, |c, n, s| {
         // The string parts are produced in literal order, however
         // they need to be reversed on the stack in order to
         // efficiently create the real string in case of
@@ -271,34 +276,39 @@ impl Compiler<'_, '_> {
             match part {
                 // Interpolated expressions are compiled as normal and
                 // dealt with by the VM before being assembled into
-                        // the final string. We need to force them here,
+                // the final string. We need to coerce them here,
                 // so OpInterpolate definitely has a string to consume.
-                        // TODO(sterni): coerce to string
                 ast::InterpolPart::Interpolation(ipol) => {
-                            c.compile(s, ipol.expr().unwrap());
+                    self.compile(slot, ipol.expr().unwrap());
-                            c.emit_force(&ipol);
+                    // implicitly forces as well
+                    self.push_op(OpCode::OpCoerceToString, &ipol);
                 }
 
                 ast::InterpolPart::Literal(lit) => {
-                            c.emit_constant(Value::String(lit.into()), n);
+                    self.emit_constant(Value::String(lit.into()), parent_node);
                 }
             }
         }
 
-                c.push_op(OpCode::OpInterpolate(Count(count)), n);
+        if count != 1 {
+            self.push_op(OpCode::OpInterpolate(Count(count)), parent_node);
+        }
+    }
+
+    fn compile_str(&mut self, slot: LocalIdx, node: ast::Str) {
+        let parts = node.normalized_parts();
+
+        // We need to thunk string expressions if they are the result of
+        // interpolation. A string that only consists of a single part (`"${foo}"`)
+        // can't desugar to the enclosed expression (`foo`) because we need to
+        // coerce the result to a string value. This would require forcing the
+        // value of the inner expression, so we need to wrap it in another thunk.
+        if parts.len() != 1 || matches!(&parts[0], ast::InterpolPart::Interpolation(_)) {
+            self.thunk(slot, &node, move |c, n, s| {
+                c.compile_str_parts(s, n, parts);
             });
         } else {
-            match &parts[0] {
+            self.compile_str_parts(slot, &node, parts);
-                // Since we only have a single part, it is okay if this yields a thunk
-                // TODO(sterni): coerce to string
-                ast::InterpolPart::Interpolation(node) => {
-                    self.compile(slot, node.expr().unwrap());
-                }
-
-                ast::InterpolPart::Literal(lit) => {
-                    self.emit_constant(Value::String(lit.as_str().into()), &node);
-                }
-            }
         }
     }
 

tvix/eval/src/opcode.rs

@@ -105,6 +105,9 @@ pub enum OpCode {
 
     // Strings
     OpInterpolate(Count),
+    /// Force the Value on the stack and coerce it to a string, always using
+    /// `CoercionKind::Weak`.
+    OpCoerceToString,
 
     // Type assertion operators
     OpAssertBool,

tvix/eval/src/vm.rs

@@ -9,7 +9,7 @@ use crate::{
     observer::Observer,
     opcode::{CodeIdx, Count, JumpOffset, OpCode, StackIdx, UpvalueIdx},
     upvalues::{UpvalueCarrier, Upvalues},
-    value::{Builtin, Closure, Lambda, NixAttrs, NixList, Thunk, Value},
+    value::{Builtin, Closure, CoercionKind, Lambda, NixAttrs, NixList, Thunk, Value},
 };
 
 struct CallFrame {
@@ -344,6 +344,16 @@ impl<'o> VM<'o> {
 
                 OpCode::OpInterpolate(Count(count)) => self.run_interpolate(count)?,
 
+                OpCode::OpCoerceToString => {
+                    // TODO: handle string context, copying to store
+                    let string = fallible!(
+                        self,
+                        // note that coerce_to_string also forces
+                        self.pop().coerce_to_string(CoercionKind::Weak, self)
+                    );
+                    self.push(Value::String(string));
+                }
+
                 OpCode::OpJump(JumpOffset(offset)) => {
                     debug_assert!(offset != 0);
                     self.frame_mut().ip += offset;