fix(tvix/eval): only finalise formal arguments if defaulting r/6336

When dealing with a formal argument in a function argument pattern that
has a default expression, there are two different things that can happen
at runtime: Either we select its value from the passed attribute
successfully or we need to use the default expression. Both of these may
be thunks and both of these may need finalisers. However, in the former
case this is taken care of elsewhere, the value will always be finalised
already if necessary. In the latter case we may need to finalise the
thunk resulting from the default expression. However, the thunk
corresponding to the expression may never end up in the local's stack
slot. Since finalisation goes by stack slot (and not constants), we need
to prevent a case where we don't fall back to the default expression,
but finalise anyways.

Previously, we worked around this by making `OpFinalise` ignore
non-thunks. Since finalisation of already evaluated thunks still
crashed, the faulty compilation of function pattern arguments could
still cause a crash.

As a new approach, we reinstate the old behavior of `OpFinalise` to
crash whenever encountering something that is either not a thunk or
doesn't need finalisation. This can also help catching (similar)
miscompilations in the future. To then prevent the crash, we need to
track whether we have fallen back or not at runtime. This is done using
an additional phantom on the stack that holds a new `FinaliseRequest`
value. When it comes to finalisation we check this value and
conditionally execute `OpFinalise` based on its value.

Resolves b/261 and b/265 (partially).

Change-Id: Ic04fb80ec671a2ba11fa645090769c335fb7f58b
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8705
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Autosubmit: sterni <sternenseemann@systemli.org>

2 files changed, 15 insertions, 4 deletions

diff --git a/tvix/eval/src/value/json.rs b/tvix/eval/src/value/json.rs
index 33e16ebffcd0..1e73278fa013 100644
--- a/tvix/eval/src/value/json.rs
+++ b/tvix/eval/src/value/json.rs
@@ -70,7 +70,10 @@ impl Value {
             | val @ Value::Blueprint(_)
             | val @ Value::DeferredUpvalue(_)
             | val @ Value::UnresolvedPath(_)
-            | val @ Value::Json(_) => return Err(ErrorKind::NotSerialisableToJson(val.type_of())),
+            | val @ Value::Json(_)
+            | val @ Value::FinaliseRequest(_) => {
+                return Err(ErrorKind::NotSerialisableToJson(val.type_of()))
+            }
         };
 
         Ok(value)
diff --git a/tvix/eval/src/value/mod.rs b/tvix/eval/src/value/mod.rs
index 7e701d52ba25..29b73f26f061 100644
--- a/tvix/eval/src/value/mod.rs
+++ b/tvix/eval/src/value/mod.rs
@@ -78,6 +78,9 @@ pub enum Value {
     UnresolvedPath(Box<PathBuf>),
     #[serde(skip)]
     Json(serde_json::Value),
+
+    #[serde(skip)]
+    FinaliseRequest(bool),
 }
 
 lazy_static! {
@@ -235,7 +238,8 @@ impl Value {
             | Value::Blueprint(_)
             | Value::DeferredUpvalue(_)
             | Value::UnresolvedPath(_)
-            | Value::Json(_) => panic!(
+            | Value::Json(_)
+            | Value::FinaliseRequest(_) => panic!(
                 "Tvix bug: internal value left on stack: {}",
                 value.type_of()
             ),
@@ -328,7 +332,8 @@ impl Value {
             | (Value::Blueprint(_), _)
             | (Value::DeferredUpvalue(_), _)
             | (Value::UnresolvedPath(_), _)
-            | (Value::Json(_), _) => {
+            | (Value::Json(_), _)
+            | (Value::FinaliseRequest(_), _) => {
                 panic!("tvix bug: .coerce_to_string() called on internal value")
             }
         }
@@ -520,6 +525,7 @@ impl Value {
             Value::DeferredUpvalue(_) => "internal[deferred_upvalue]",
             Value::UnresolvedPath(_) => "internal[unresolved_path]",
             Value::Json(_) => "internal[json]",
+            Value::FinaliseRequest(_) => "internal[finaliser_sentinel]",
         }
     }
 
@@ -658,7 +664,8 @@ impl Value {
             | Value::Blueprint(_)
             | Value::DeferredUpvalue(_)
             | Value::UnresolvedPath(_)
-            | Value::Json(_) => "an internal Tvix evaluator value".into(),
+            | Value::Json(_)
+            | Value::FinaliseRequest(_) => "an internal Tvix evaluator value".into(),
         }
     }
 }
@@ -773,6 +780,7 @@ impl TotalDisplay for Value {
             Value::DeferredUpvalue(_) => f.write_str("internal[deferred_upvalue]"),
             Value::UnresolvedPath(_) => f.write_str("internal[unresolved_path]"),
             Value::Json(_) => f.write_str("internal[json]"),
+            Value::FinaliseRequest(_) => f.write_str("internal[finaliser_sentinel]"),
 
             // Delegate thunk display to the type, as it must handle
             // the case of already evaluated or cyclic thunks.