builtins.hashString: Devirtualize lazy paths, and re-enable lazy trees tests

[edolstra]

Motivation

Fixes DeterminateSystems/determinate#160.

Context

Summary by CodeRabbit

• Refactor
  • Improved devirtualization of string and path handling, affecting hash calculations and app resolution for more consistent results.
• Tests
  • Added a functional test that verifies hashing behavior with lazy/deferred paths.
• Chores
  • CI test configuration updated to include a lazyTrees test variant.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

---

📝 Walkthrough

Walkthrough

Devirtualization is applied to materialized strings/paths before they are used: builtins.hashString now devirtualizes the result of forceString, and app resolution devirtualizes opaque path/program values. A functional test and a CI test attribute for lazy trees were added.

Changes

Cohort / File(s)	Summary
hashString builtin src/libexpr/primops.cc	Call state.devirtualize on the result of state.forceString(...) when computing builtins.hashString, so the hashed value reflects devirtualized content.
app resolution src/nix/app.cc	Devirtualize opaque/contextual path and program values via state.devirtualize(...) when constructing unresolved apps.
Functional tests tests/functional/fetchGit.sh	Add test verifying builtins.hashString determinism when hashing lazily-fetched Git paths (uses fetchGit + hashString).
CI test config ci/gha/tests/default.nix	Add top-level lazyTrees attribute which overrides nix-functional-tests with lazyTrees = true (new test component variant).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

• builtins.hashString non-deterministic when paired with builtins.fetchGit in pure evaluation mode. determinate#160 — Addresses the reported non-deterministic builtins.hashString with fetchGit by ensuring devirtualized string content is hashed.

Possibly related PRs

• builtins.getFlake fixes #337 — Modifies devirtualization and hashing-related logic (narHash primop and lazy NAR-hash fallback), closely related to the devirtualization changes in hashing here.

Suggested reviewers

• grahamc
• cole-h

Poem

🐰
I nudged a phantom string to ground,
So hashes stay the same all round,
From lazy fetch to final byte,
No more jitter through the night,
Consistent hops—determinism found!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title accurately describes the main changes: devirtualizing lazy paths in builtins.hashString and re-enabling lazy trees tests, which directly addresses the objective to fix the non-determinism issue.
Linked Issues check	✅ Passed	The pull request implements devirtualization in builtins.hashString and app resolution to fix the non-determinism issue (#160), validated by a new functional test that verifies hashString determinism with fetchGit.
Out of Scope Changes check	✅ Passed	All changes are scoped to addressing the devirtualization objective: modifications to hashString and app resolution logic, test additions, and lazy trees test re-enablement are all directly related to the linked issue.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch hashString-devirtualize

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

src/libexpr/primops.cc (1)

4731-4736: Stale comment: context is no longer discarded.

The comment // discarded on line 4731 is now misleading. The context is populated by forceString and subsequently used by devirtualize to resolve virtual paths. Consider updating or removing this comment to reflect the current behavior.

📝 Suggested comment update

-    NixStringContext context; // discarded
+    NixStringContext context;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/libexpr/primops.cc` around lines 4731 - 4736, The inline comment "//
discarded" next to the NixStringContext is stale because NixStringContext
`context` is populated by `state.forceString` and used by `state.devirtualize`;
remove or update that comment to accurately reflect that `context` is captured
and passed into `state.devirtualize` (or replace with a brief note like "context
is filled by forceString and used for devirtualize") — change the comment
adjacent to `NixStringContext context;` near the
`state.forceString`/`state.devirtualize` call so it no longer falsely claims the
context is discarded.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/libexpr/primops.cc`:
- Around line 4731-4736: The inline comment "// discarded" next to the
NixStringContext is stale because NixStringContext `context` is populated by
`state.forceString` and used by `state.devirtualize`; remove or update that
comment to accurately reflect that `context` is captured and passed into
`state.devirtualize` (or replace with a brief note like "context is filled by
forceString and used for devirtualize") — change the comment adjacent to
`NixStringContext context;` near the `state.forceString`/`state.devirtualize`
call so it no longer falsely claims the context is discarded.

[github-actions]

🚀 Deployed on https://6998c5e4cb71424f71d99bbe--determinate-nix.netlify.app