fix(tvix/eval): use realpaths for import cache r/6341

I've noticed this behavior when writing the admittedly cursed test case
included in this CL. Alternatively we could use some sort of machinery
using `builtins.trace`, but I don't think we capture stderr anywhere.

I've elected to put this into the eval cache itself while C++ Nix does
it in builtins.import already, namely via `realisePath`. We don't have
an equivalent for this yet, since we don't support any kind of IfD, but
we could revise that later. In any case, it seems good to encapsulate
`ImportCache` in this way, as it'll also allow using file hashes as
identifiers, for example.

C++ Nix also does our equivalent of canon_path in `builtins.import`
which we still don't, but I suspect it hardly makes a difference.

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

8 files changed, 51 insertions, 3 deletions

diff --git a/tvix/eval/src/compiler/import.rs b/tvix/eval/src/compiler/import.rs
index 7e1aabbf1f8b..59e3d92e8127 100644
--- a/tvix/eval/src/compiler/import.rs
+++ b/tvix/eval/src/compiler/import.rs
@@ -24,6 +24,7 @@ async fn import_impl(
     source: SourceCode,
     mut args: Vec<Value>,
 ) -> Result<Value, ErrorKind> {
+    // TODO(sterni): canon_path()?
     let mut path = coerce_value_to_path(&co, args.pop().unwrap()).await?;
 
     if path.is_dir() {
diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-observable-eval-cache.exp b/tvix/eval/src/tests/tvix_tests/eval-okay-observable-eval-cache.exp
new file mode 100644
index 000000000000..aaa53b602586
--- /dev/null
+++ b/tvix/eval/src/tests/tvix_tests/eval-okay-observable-eval-cache.exp
@@ -0,0 +1 @@
+[ true true false false true ]
diff --git a/tvix/eval/src/tests/tvix_tests/eval-okay-observable-eval-cache.nix b/tvix/eval/src/tests/tvix_tests/eval-okay-observable-eval-cache.nix
new file mode 100644
index 000000000000..24003d0637ae
--- /dev/null
+++ b/tvix/eval/src/tests/tvix_tests/eval-okay-observable-eval-cache.nix
@@ -0,0 +1,7 @@
+[
+  (import ./observable-eval-cache1.nix == import ./observable-eval-cache1.nix)
+  (import ./observable-eval-cache1.nix == import ./observable-eval-cache2.nix)
+  (import ./observable-eval-cache1.nix == import ./observable-eval-cache3.nix)
+  (import ./observable-eval-cache2.nix == import ./observable-eval-cache3.nix)
+  (import ./observable-eval-cache3.nix == import ./observable-eval-cache3.nix)
+]
diff --git a/tvix/eval/src/tests/tvix_tests/observable-eval-cache1.nix b/tvix/eval/src/tests/tvix_tests/observable-eval-cache1.nix
new file mode 100644
index 000000000000..b5f3f59a792d
--- /dev/null
+++ b/tvix/eval/src/tests/tvix_tests/observable-eval-cache1.nix
@@ -0,0 +1 @@
+let id = x: x; in { inherit id; }
diff --git a/tvix/eval/src/tests/tvix_tests/observable-eval-cache2.nix b/tvix/eval/src/tests/tvix_tests/observable-eval-cache2.nix
new file mode 120000
index 000000000000..7f69c0eb47eb
--- /dev/null
+++ b/tvix/eval/src/tests/tvix_tests/observable-eval-cache2.nix
@@ -0,0 +1 @@
+observable-eval-cache1.nix
\ No newline at end of file
diff --git a/tvix/eval/src/tests/tvix_tests/observable-eval-cache3.nix b/tvix/eval/src/tests/tvix_tests/observable-eval-cache3.nix
new file mode 100644
index 000000000000..b5f3f59a792d
--- /dev/null
+++ b/tvix/eval/src/tests/tvix_tests/observable-eval-cache3.nix
@@ -0,0 +1 @@
+let id = x: x; in { inherit id; }
diff --git a/tvix/eval/src/vm/generators.rs b/tvix/eval/src/vm/generators.rs
index 138600be194a..fb60a45f20cc 100644
--- a/tvix/eval/src/vm/generators.rs
+++ b/tvix/eval/src/vm/generators.rs
@@ -393,7 +393,7 @@ impl<'o> VM<'o> {
                         }
 
                         VMRequest::ImportCacheLookup(path) => {
-                            if let Some(cached) = self.import_cache.get(&path) {
+                            if let Some(cached) = self.import_cache.get(path) {
                                 message = VMResponse::Value(cached.clone());
                             } else {
                                 message = VMResponse::Empty;
diff --git a/tvix/eval/src/vm/mod.rs b/tvix/eval/src/vm/mod.rs
index d004af3c6f45..cc66ee4b6f36 100644
--- a/tvix/eval/src/vm/mod.rs
+++ b/tvix/eval/src/vm/mod.rs
@@ -14,7 +14,7 @@ mod macros;
 
 use codemap::Span;
 use serde_json::json;
-use std::{cmp::Ordering, collections::HashMap, ops::DerefMut, path::PathBuf, rc::Rc};
+use std::{cmp::Ordering, collections::HashMap, ops::DerefMut, path::Path, path::PathBuf, rc::Rc};
 
 use crate::{
     arithmetic_op,
@@ -218,6 +218,42 @@ impl Frame {
     }
 }
 
+#[derive(Default)]
+struct ImportCache(Box<HashMap<PathBuf, Value>>);
+
+/// The `ImportCache` holds the `Value` resulting from `import`ing a certain
+/// file, so that the same file doesn't need to be re-evaluated multiple times.
+/// Currently the real path of the imported file (determined using
+/// [`std::fs::canonicalize()`], not to be confused with our
+/// [`value::canon_path()`]) is used to identify the file, just like C++ Nix
+/// does.
+///
+/// Errors while determining the real path are currently just ignored, since we
+/// pass around some fake paths like `/__corepkgs__/fetchurl.nix`.
+///
+/// In the future, we could use something more sophisticated, like file hashes.
+/// However, a consideration is that the eval cache is observable via impurities
+/// like pointer equality and `builtins.trace`.
+impl ImportCache {
+    fn get(&self, path: PathBuf) -> Option<&Value> {
+        let path = match std::fs::canonicalize(path.as_path()).map_err(ErrorKind::from) {
+            Ok(path) => path,
+            Err(_) => path,
+        };
+        self.0.get(&path)
+    }
+
+    fn insert(&mut self, path: PathBuf, value: Value) -> Option<Value> {
+        self.0.insert(
+            match std::fs::canonicalize(path.as_path()).map_err(ErrorKind::from) {
+                Ok(path) => path,
+                Err(_) => path,
+            },
+            value,
+        )
+    }
+}
+
 struct VM<'o> {
     /// VM's frame stack, representing the execution contexts the VM is working
     /// through. Elements are usually pushed when functions are called, or
@@ -241,7 +277,7 @@ struct VM<'o> {
     /// Import cache, mapping absolute file paths to the value that
     /// they compile to. Note that this reuses thunks, too!
     // TODO: should probably be based on a file hash
-    pub import_cache: Box<HashMap<PathBuf, Value>>,
+    pub import_cache: ImportCache,
 
     /// Parsed Nix search path, which is used to resolve `<...>`
     /// references.