From 8724d2fff871827dc66503f9b3dfa1d29149ddc7 Mon Sep 17 00:00:00 2001
From: Vincent Ambo <mail@tazj.in>
Date: Fri, 21 Oct 2022 16:45:14 +0300
Subject: [PATCH] fix(tvix/eval): use top-level span for `force_with_output`

When forcing thunks in `force_with_output`, the call stack of the VM
is actually empty (as the calls are synthetic and no longer part of
the evaluation of the top-level expression).

This means that Tvix crashed when constructing error spans for the
`fallible` macro, as the assumption of there being an enclosing span
was violated.

To work around this, we instead pass the span for the whole top-level
expression to force_for_output and set this as the span for the
enclosing error chain. Existing output logic will already avoid
printing the entire expression as an error span.

This fixes b/213.

Change-Id: I93978e0deaf5bcb0f47a6fa95b3f5bebef5bad4c
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7052
Autosubmit: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Reviewed-by: grfn <grfn@gws.fyi>
---
 .../eval-fail-builtins-thunk-error.nix        |  1 +
 .../eval-fail-deep-forced-thunk-error.nix     |  1 +
 tvix/eval/src/vm.rs                           | 33 +++++++++++++++----
 3 files changed, 29 insertions(+), 6 deletions(-)
 create mode 100644 tvix/eval/src/tests/tvix_tests/eval-fail-builtins-thunk-error.nix
 create mode 100644 tvix/eval/src/tests/tvix_tests/eval-fail-deep-forced-thunk-error.nix

diff --git a/tvix/eval/src/tests/tvix_tests/eval-fail-builtins-thunk-error.nix b/tvix/eval/src/tests/tvix_tests/eval-fail-builtins-thunk-error.nix
new file mode 100644
index 000000000..bb0d5920d
--- /dev/null
+++ b/tvix/eval/src/tests/tvix_tests/eval-fail-builtins-thunk-error.nix
@@ -0,0 +1 @@
+builtins.genList (_: {}.foo) 1
diff --git a/tvix/eval/src/tests/tvix_tests/eval-fail-deep-forced-thunk-error.nix b/tvix/eval/src/tests/tvix_tests/eval-fail-deep-forced-thunk-error.nix
new file mode 100644
index 000000000..b7a758302
--- /dev/null
+++ b/tvix/eval/src/tests/tvix_tests/eval-fail-deep-forced-thunk-error.nix
@@ -0,0 +1 @@
+[ (throw "error!") ]
diff --git a/tvix/eval/src/vm.rs b/tvix/eval/src/vm.rs
index 1128031b4..9e25da7d2 100644
--- a/tvix/eval/src/vm.rs
+++ b/tvix/eval/src/vm.rs
@@ -3,6 +3,8 @@
 
 use std::{ops::DerefMut, path::PathBuf, rc::Rc};
 
+use codemap::Span;
+
 use crate::{
     chunk::Chunk,
     errors::{Error, ErrorKind, EvalResult},
@@ -860,21 +862,31 @@ impl<'o> VM<'o> {
     /// will ensure that lists and attribute sets do not contain
     /// chunks which, for users, are displayed in a strange and often
     /// unexpected way.
-    fn force_for_output(&mut self, value: &Value) -> EvalResult<()> {
+    fn force_for_output(&mut self, value: &Value, root_span: Span) -> EvalResult<()> {
         match value {
             Value::Attrs(attrs) => {
                 for (_, value) in attrs.iter() {
-                    self.force_for_output(value)?;
+                    self.force_for_output(value, root_span)?;
                 }
                 Ok(())
             }
 
-            Value::List(list) => list.iter().try_for_each(|elem| self.force_for_output(elem)),
+            Value::List(list) => list
+                .iter()
+                .try_for_each(|elem| self.force_for_output(elem, root_span)),
 
             Value::Thunk(thunk) => {
-                fallible!(self, thunk.force(self));
+                // This force is "synthetic", in that there is no
+                // outer expression from which a top-level span for
+                // the call can be derived, so we need to set this
+                // span manually.
+                thunk.force(self).map_err(|kind| Error {
+                    kind,
+                    span: root_span,
+                })?;
+
                 let value = thunk.value().clone();
-                self.force_for_output(&value)
+                self.force_for_output(&value, root_span)
             }
 
             // If any of these internal values are encountered here a
@@ -925,9 +937,18 @@ pub fn run_lambda(
     lambda: Rc<Lambda>,
 ) -> EvalResult<RuntimeResult> {
     let mut vm = VM::new(nix_search_path, observer);
+
+    // Retain the top-level span of the expression in this lambda, as
+    // synthetic "calls" in force_for_output will otherwise not have a
+    // span to fall back to.
+    //
+    // We exploit the fact that the compiler emits a final instruction
+    // with the span of the entire file for top-level expressions.
+    let root_span = lambda.chunk.get_span(CodeIdx(lambda.chunk.code.len() - 1));
+
     vm.enter_frame(lambda, Upvalues::with_capacity(0), 0)?;
     let value = vm.pop();
-    vm.force_for_output(&value)?;
+    vm.force_for_output(&value, root_span)?;
 
     Ok(RuntimeResult {
         value,