I'm still trying to work out the exact stack invariants for tvix.
We really should add assertions for them; getting the stack messed
up is no fun. This commit adds one simple assertion. It also adds
a missing stack-push (my mistake) in one place, which was uncovered
by the assertion.
Change-Id: I9d8b4bd1702d954e325832c5935b0d7e3eb68422
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10369
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
r/7176 introduced an incorrect assumption was the benefit of the
nonrecursive coercion algorithm, namely that a coercion operation always
returns a non empty string. This allows to detect whether we are
coercing a list or not by checking if the intermediate result is empty
or not. Unfortunately, coercing null and false yields an empty string,
so we need to explicitly track whether we are coercing a list.
Updated the test case to hopefully catch similar bugs in the future. I'm
not a hundred percent certain I have not introduced a new edge case with
this, so it may be interesting to add a prop test case for this to
nix_oracle down the line. At least lists are the only nested data
structures that can be serialized as nested data structures, so the
problem is kind of limited.
Change-Id: Ia41e904356f1c41a9d35e4e65ec02f2fe5a4100e
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10418
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Autosubmit: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
The default behavior of string coercion in C++ Nix is to weakly coerce
and import to store if necessary. There is a flag to make it strongly
coerce (coerceMore) and a flag that controls whether path values have
the corresponding file/directory imported into the store before
returning the (store) path as a string (copyToStore). We need to
implement our equivalent to the copyToStore (import_paths) flag for the
benefit of weak coercions that don't import into the store (dirOf,
baseNameOf, readFile, ...) and strong coercions that don't import into
the store (toString).
This makes coerce_to_string as well as CoercionKind weirder and more
versatile, but prevents us from reimplementing parts of the coercion
logic constantly as can be seen in the case of baseNameOf.
Note that it is not possible to test this properly in //tvix/eval tests
due to the lack of an appropriate EvalIO implementation being available.
Tests should be added to //tvix/glue down the line.
Change-Id: I8fb8ab99c7fe08e311d2ba1c36960746bf22f566
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10361
Autosubmit: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
Reviewed-by: Adam Joseph <adam@westernsemico.com>
This matches the behavior of C++ Nix more closely where the decision is
made based on the first type based to ExprConcatStrings:
1f93fa2ed2/src/libexpr/eval.cc (L1967-L2025)
Note that this doesn't make a difference in any successful
evaluation (at least to my knowledge), but ensures that our error
messages will match C++ Nix more closely, e.g. in the case of
`1 + "string"`.
Change-Id: I8059930788f9c8d98baf98e3d93d8a060ef961f2
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10360
Tested-by: BuildkiteCI
Autosubmit: sterni <sternenseemann@systemli.org>
Reviewed-by: Adam Joseph <adam@westernsemico.com>
This commit fixes out builtins.length so it propagates catchables
like cppnix does.
Change-Id: I7670bec5eee1d4cd3f67a04c9a6808979fb56a8d
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10315
Reviewed-by: tazjin <tazjin@tvl.su>
Autosubmit: Adam Joseph <adam@westernsemico.com>
Tested-by: BuildkiteCI
This commit fixes builtins.filter so it propagates catchables
correctly.
Change-Id: Ib23a383bc5e272e42052205ffd1e94649a0ebc47
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10313
Autosubmit: Adam Joseph <adam@westernsemico.com>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
This adds an unimplemented placeholder for builtins.hashString.
Change-Id: Ibc770103acf5dbc3ea7589ab5ca23fe6e07bd91a
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10311
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Autosubmit: Adam Joseph <adam@westernsemico.com>
This commit fixes builtins.elemAt so it propagates catchables like
cppnix does.
Change-Id: Ieca5e128da17e78af0b14dae4a28a1ff8796e4f2
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10308
Autosubmit: Adam Joseph <adam@westernsemico.com>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
This fixes our implementation of builtins.splitVersion so it
propagates catchables like cppnix does.
Change-Id: Id5d83ea76229f8c8f202aa42353cb609e67de43f
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10305
Autosubmit: Adam Joseph <adam@westernsemico.com>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Currently this just `throw`s a message explaining that it is not
implemented. This is necessary in order to allow enumerating the
nixpkgs release attrset (afaict only one package uses this builtin).
Change-Id: I45266d46af579ddb5856b192b6be4b481369543c
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10302
Autosubmit: Adam Joseph <adam@westernsemico.com>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
... since this may import them to the store which changes their
basename.
Fixes b/350.
Change-Id: Iabd08ff4d6a424c66d6d7784d7a96b0c078f0a91
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10298
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Autosubmit: Adam Joseph <adam@westernsemico.com>
This commit adds __curPos (to the global scope, yuck) and
builtins.filterSource. These are not implemented; forcing them will
produce the same result as `throw "message"`.
Unfortunately these two post-2.3 features are used throughout
nixpkgs. Since an unresolved indentifier is a catchable error, this
breaks the entire release eval. With this commit, it simply causes
those broken packages that use these features to appear as they are:
broken.
Change-Id: Ib43dea571f6a9fab4d54869349f80ee4ec5424c2
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10297
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Autosubmit: Adam Joseph <adam@westernsemico.com>
This commit fixes our handling of `throw` within an `assert`
condition.
Fixes: b/340
Change-Id: I40a383639ec266da50a853f16216b1b7868495da
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10318
Autosubmit: Adam Joseph <adam@westernsemico.com>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
This commit fixes out `?` operator so it correctly propagates
catchables.
Change-Id: Iebaa153a8492101ee3ddd29893c98730ff331547
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10317
Autosubmit: Adam Joseph <adam@westernsemico.com>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Previously, using a catchable as either argument of OpAttrsSelect
would result in an unrecoverable error. This commit matches cppnix
behavior by propagating the catchable.
Change-Id: I4877f4068ec2b823225f185290693c101d0b9c9e
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10303
Tested-by: BuildkiteCI
Autosubmit: Adam Joseph <adam@westernsemico.com>
Reviewed-by: tazjin <tazjin@tvl.su>
When attempting to call a Value, if it is a Value::Catchable we must
not cause an uncatchable failure. This commit simply reuses the
Value::Catchable as the result of attempting to call it. This is
safe because nix is designed so that nix code cannot distinguish
between different catchable failures -- they all look the same to
the interpreted code.
This fixes b/351.
Change-Id: Ibf763a08753e541843626182ff59fdbf15ea2959
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10300
Autosubmit: Adam Joseph <adam@westernsemico.com>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
error[E006]: expected value of type 'bool', but found a 'internal[catchable]'
--> src/tests/tvix_tests/notyetpassing/eval-okay-test-catchables-in-implications.nix:1:43
|
1 | (builtins.tryEval (({ foo ? throw "up" }: foo -> true) { })).success
| ^^^^^^^^^^^
Relates to b/345
Change-Id: Ic331c32ea59bf67ae775f485b444dc6804ca13d5
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10289
Tested-by: BuildkiteCI
Autosubmit: Adam Joseph <adam@westernsemico.com>
Reviewed-by: tazjin <tazjin@tvl.su>
This commit adds Opcode::OpJumpIfCatchable, which can be inserted
ahead of most VM operations which expect a boolean on the stack, in
order to handle catchables in branching position properly.
Other than remembering to patch the jump, no other changes should be
required.
This commit also fixes b/343 by emitting this new opcode when
compiling if-then-else. There are probably other places where we
need to do the same thing.
Change-Id: I48de3010014c0bbeba15d34fc0d4800e0bb5a1ef
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10288
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Autosubmit: Adam Joseph <adam@westernsemico.com>
This is a test case for b/343, wherein tvix dies if you try to
branch on an argument whose defaulted value is a catchable.
Change-Id: I891ca825e39ad14dda9f220f06d9591874fcd45d
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10287
Reviewed-by: tazjin <tazjin@tvl.su>
Autosubmit: Adam Joseph <adam@westernsemico.com>
Tested-by: BuildkiteCI
After this commit, the only non-builtins uses of generators are:
- coerce_to_string() uses generators::request_enter_lambda()
- Thunk::force() uses generators::request_enter_lambda()
That's it! Once those two are taken care of, GenCo can become an
implementation detail of `builtins::BuiltinGen`. No more crazy
nonlocal flow control within the interpreter: if you've got a GenCo
floating around in your code it's because you're writing a builtin,
which isn't part of the core interpreter. The interpreter won't
need GenCos to talk to itself anymore.
Technically generators::request_path_import() is also used by
coerce_to_string(), but that's just because the io_handle happens to
be part of the VM. There's no recursion-depth issue there, so the
call doesn't need to go through the generator mechanism
(request_path_import() doesn't call back to the interpreter!)
Change-Id: I83ce5774d49b88fdafdd61160975b4937a435bb0
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10256
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Autosubmit: Adam Joseph <adam@westernsemico.com>
This commit implements deep_force() nonrecursively, by maintaining
an explicit stack rather than using the call stack for recursion.
As an added bonus, we don't need to pass around the SharedThunkSet
anymore, and can in fact completely eliminate SharedThunkSet.
Change-Id: I7c4f59f37834d451a28bf6be317eb0a90eac4ee6
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10252
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Autosubmit: Adam Joseph <adam@westernsemico.com>
The comment explaining ThunkSet makes it seem like it does the same
think as ThunkRepr::Blackhole. In fact neither one is a substitute
for the other. Let's explain the difference.
Change-Id: I89ceaaa9d3c499edbc7d48f70ca5d11f97666c43
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10250
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Autosubmit: Adam Joseph <adam@westernsemico.com>
LightSpan::Delayed was introduced in commit
bf286a54bc which claimed that "This
reduces the eval time for `builtins.length (builtins.attrNames
(import <nixpkgs> {}))` by *one third*!"
I am unable to reproduce this result. In fact, dropping the
LightSpan::Delayed variant of the enum makes eval of the same
expression slightly faster! I also tried a large evaluation
(pkgsCross...hello) and got similar results: slightly faster,
slightly less memory. See git footers.
I suspect that there was some unrelated horrific inefficiency that
has since been fixed. The avoided computation in `get_span()` is
nothing more than a binary search! If this were in fact a major
performance issue we could simply precompute the mapping from
CodeIdx to Span when the Chunk becomes immutable (i.e. at the end of
the compilation process, when compiler backtracking is no longer a
concern). Since a Span is just 64 bits this is not a space issue,
and since binary search is much simpler than compiling Nix
expressions it isn't a performance issue either.
Technically there is no longer any reason to have LightSpan since it
is now a single-variant enum. However there is no rush to remove
it, since Rust will optimize its representation into the same thing
you'd get if you replaced LightSpan by Span.
Prev-Benchmark: {"nixpkgs-attrnames":{"kbytes":"233824","system":"0.32","user":"2.02"}}
This-Benchmark: {"nixpkgs-attrnames":{"kbytes":"230192","system":"0.29","user":"2.00"}}
Prev-Benchmark: {"pkgsCross.aarch64-multiplatform.hello.outPath":{"kbytes":"458936","system":"0.73","user":"5.36"}}
This-Benchmark: {"pkgsCross.aarch64-multiplatform.hello.outPath":{"kbytes":"451808","system":"0.53","user":"5.10"}}
Change-Id: Ib9e04806850aa1fc4e66e2a042703986440a7b4e
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10254
Reviewed-by: tazjin <tazjin@tvl.su>
Autosubmit: Adam Joseph <adam@westernsemico.com>
Tested-by: BuildkiteCI
This is just a alias for Result<_, io::Error>, but shorter.
Change-Id: I7c22f61b85e3014885a747b5c1e5abd11b0ef17d
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10327
Tested-by: BuildkiteCI
Autosubmit: flokli <flokli@flokli.de>
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
This commit fixes b/338 by properly propagating catchables through
comparison operations.
Change-Id: I6b0283a40f228ecf9a6398d24c060bdacb1077cf
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10221
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Autosubmit: Adam Joseph <adam@westernsemico.com>
Commit 05f42519b5 fixed b/281 by
establishing a hygiene regimen to partition *catchable* errors
(i.e. those which tryEval can detect) from all other errors, like
internal VM failures or I/O errors (which Nix must not be allowed to
detect, since these errors are fundamentally impure).
Unfotunately there are still cases where tvix assumes that anything
other than Value::Bool means it should panic!(). I found another
one, and added a test case for it in:
eval_okay_src_tests_tvix_tests_eval_okay_compare_ordering_catchable_nix
Not yet passing.
Change-Id: I69c62ed9ea5c8f81870e8de5c5fe12dcde849763
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10220
Autosubmit: Adam Joseph <adam@westernsemico.com>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
This commit rewrites Value::nix_cmp_ordering() into an equivalent
nonrecursive form. Except for calls to Thunk::force(), the new form
no longer uses generators, and is async only because of the fact
that it calls Thunk::force().
I originally believed that this commit would make evaluation faster.
In fact it is slightly slower. I believe this is due to the added
vec![] allocation. I am investigating.
Prev-Nixpkgs-Benchmark: {"attrpath":"pkgsCross.aarch64-multiplatform.hello.outPath","peak-kbytes":"460048","system-seconds":"0.68","user-seconds":"5.73"}
This-Nixpkgs-Benchmark: {"attrpath":"pkgsCross.aarch64-multiplatform.hello.outPath","peak-kbytes":"460224","system-seconds":"0.67","user-seconds":"5.84"}
Change-Id: Ic627bc220d9c5aa3c5e68b9b8bf199837cd55af5
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10212
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Autosubmit: Adam Joseph <adam@westernsemico.com>
This is part of a fix for b/338.
We should never use PartialOrd::partial_cmp().
All Nix types except floats are obviously totally-ordered. In
addition, it turns out that because Nix treats division by zero
rather than producing a NaN, and because it does not support
"negative zero", even floats are in fact totally ordered in Nix.
Therefore, every call to PartialOrd::partial_cmp() in tvix is an
error. We have to *implement* this function, but we should never
call it on built-in types.
Moreover, nix_cmp_ordering() currently returns an Option<Ordering>.
I'm not sure what was going on there, since it's impossible for it
to return None. This commit fixes it to return simply Ordering
rather than Option<Ordering>.
Change-Id: If5c084164cf19cfb38c5a15554c0422faa5f895d
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10218
Autosubmit: Adam Joseph <adam@westernsemico.com>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
This commit rewrites Value::nix_eq() into an equivalent. Except for
calls to Thunk::force(), the new form no longer uses generators, and
is async only because of the fact that it calls Thunk::force().
I believed that the nonrecursive form would be faster. It is, in
fact, slightly slower. I believe this is due to the vec![]
allocation; I am investigating.
Prev-Nixpkgs-Benchmark: {"attrpath":"pkgsCross.aarch64-multiplatform.hello.outPath","peak-kbytes":"459068","system-seconds":"0.71","user-seconds":"5.39"}
This-Nixpkgs-Benchmark: {"attrpath":"pkgsCross.aarch64-multiplatform.hello.outPath","peak-kbytes":"460048","system-seconds":"0.68","user-seconds":"5.73"}
Change-Id: I10f4868891e4b7475df13f0cbc41ec78dd985dd8
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10118
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Autosubmit: Adam Joseph <adam@westernsemico.com>
This commit rewrites Thunk::force() so that it is not (directly)
self-recursive. It maintains a Vec of all the
previously-encountered thunks which point to the one it is currently
forcing, rather than recursively calling itself.
Benefits:
- Short term:
This commit saves the cost of a round-trip through the generator
machinery for the generators::request_force() which is removed by
this commit.
- Medium term:
Once a similar transformation has been applied to nix_cmp(),
nix_add(), nix_eq(), and coerce_to_string(), those four functions,
along with Thunk::force(), will make non-tail calls only to each
other. They can then be merged into a single tail-recursive
function which does not use the generator machinery at all:
enum Task { Cmp, Add, Eq, CoerceToString, Force};
fn Value::walk(task:Task, v1:Value, v2:Value) {
// ...
- Long term:
The long-term goal here is to use generators **only for builtins**
and [Marionette]-style remote control of the VM. In other words:
use `async` for things that actually involve concurrency. Calls
from the VM to builtins can then be blocking calls, because even
cppnix will overflow the stack if you make a MAX_STACK_DEPTH-deep
recursive call which passes through a builtin at every stack frame
(e.g. `{ func = builtins.sort (a: b: ... func ...) ...}`).
This way the inner "tight loop" of the interpreter doesn't pay the
costs of `async` and generators. These costs manifest in terms
of: performance, complex nonlocal control flow, and language
impediments (async Rust is a restricted subset of real Rust, and
is missing things like traits).
[Marionette]: https://firefox-source-docs.mozilla.org/testing/marionette/Intro.html
Change-Id: I6179b8abb2ea0492180fcb347f37595a14665777
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10039
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
If builtins.substring is invoked with (byte!!) offsets that aren't at
codepoint boundaries, return an error rather than panicking. This is
still incorrect (see b/337) but pushes the incorrectness forward a step.
Change-Id: I5a4261f2ff250874cd36489ef598dcf886669d04
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10199
Tested-by: BuildkiteCI
Autosubmit: grfn <grfn@gws.fyi>
Reviewed-by: sterni <sternenseemann@systemli.org>
This fixes a future clippy lint.
Change-Id: Ic830e94ef23595580c1037f10878c76bbb546dd9
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10110
Tested-by: BuildkiteCI
Reviewed-by: Adam Joseph <adam@westernsemico.com>
This commit adds Thunk::unwrap_or_clone(), which uses
Rc::try_unwrap() to avoid cloning the Value out of a an Rc which has
only one strong reference.
Change-Id: Icacefe0c823dcddf046d90c0c5cd5ed59fe976d4
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10037
Reviewed-by: tazjin <tazjin@tvl.su>
Autosubmit: Adam Joseph <adam@westernsemico.com>
Tested-by: BuildkiteCI
Change-Id: Ibddaa111a5b7a86c42dbe153ae8e53f9a5601a54
Reviewed-on: https://cl.tvl.fyi/c/depot/+/10112
Tested-by: BuildkiteCI
Reviewed-by: Adam Joseph <adam@westernsemico.com>
This *might* contain a fix for a clippy lint thrown by that crate.
Relates to b/321.
Change-Id: Ia7ebd3e26e0feb8bcc7a6c811b1e583f9016fd9e
Reviewed-on: https://cl.tvl.fyi/c/depot/+/9966
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
Autosubmit: tazjin <tazjin@tvl.su>
Uses the standard library IntoIterator trait for the construction of
our iterators. Clippy complains about duplicating this.
While doing this, I opted to rename the `IntoIter` type into something
that is more useful to users, in case somebody ends up working with
these manually.
This fixes a clippy lint, and is related to b/321.
Change-Id: I851fde0d7b8b38d182343a0fd6d9f8dd2a33ee11
Reviewed-on: https://cl.tvl.fyi/c/depot/+/9963
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
Autosubmit: tazjin <tazjin@tvl.su>
Delays allocation (through cloning) of the values to be compared
until *after* the keys have been compared.
Change-Id: I7d68c27d7a0fbcdcc387db7c092bce50ca4b94ea
Reviewed-on: https://cl.tvl.fyi/c/depot/+/9900
Autosubmit: tazjin <tazjin@tvl.su>
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Similar to `into_iter_sorted`, add a marker function for call sites
that want *borrowed* sorted iteration.
Change-Id: I7c6f14e1ac43fdb14b861b3da183eb5d12bba139
Reviewed-on: https://cl.tvl.fyi/c/depot/+/9899
Autosubmit: tazjin <tazjin@tvl.su>
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
builtins.intersectAttrs is used a _lot_ in nixpkgs eval, for whatever
reason. We previously had a very inefficient implementation that would
allocate for each comparison. It stuck out like a sore thumb in perf
analysis.
This moves to a custom algorithm with two iterators, one for the left
and one for the right side, advancing them along the (borrowed) map
keys until a match is found and allocation is required.
I've not made any effort to reduce the verbosity of this code, I don't
think it's worth it.
On my machine this reduces the mean runtime of evaluating
`nixpkgs.emacs.outPath` by ~8%.
Change-Id: Ie506d82cb8d5f45909628f771a6b73e0eca16b27
Reviewed-on: https://cl.tvl.fyi/c/depot/+/9898
Autosubmit: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
The purpose of black_box is to actually prevent the compiler from being
able to optimize computation of the benchmarked function away.
To accomplish this, we need to actually *use* black_box to blackbox the
input data away, rather than the return type.
Change-Id: I5438982f57509fbf7b85034346a2739d76aef1fa
Reviewed-on: https://cl.tvl.fyi/c/depot/+/9902
Autosubmit: flokli <flokli@flokli.de>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
tempdir pulls in remove_dir_all 0.5.3 with
https://rustsec.org/advisories/RUSTSEC-2023-0018.html, and we use
tempfile everywhere else too, so let's just migrate to that.
Change-Id: I735ade7b65e12fc26e3d43ca95fcfa07fcc64642
Reviewed-on: https://cl.tvl.fyi/c/depot/+/9565
Reviewed-by: Connor Brewster <cbrewster@hey.com>
Autosubmit: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
This commit adds a test case for b/281.
Change-Id: I8dfbfc0ff636184d7882530d8aefb329a3af9e5c
Reviewed-on: https://cl.tvl.fyi/c/depot/+/9288
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Autosubmit: Adam Joseph <adam@westernsemico.com>
Reviewed-by: flokli <flokli@flokli.de>
This commit makes catchable errors a variant of Value.
The main downside of this approach is that we lose the ability to
use Rust's `?` syntax for propagating catchable errors.
Change-Id: Ibe89438d8a70dcec29e016df692b5bf88a5cad13
Reviewed-on: https://cl.tvl.fyi/c/depot/+/9289
Reviewed-by: tazjin <tazjin@tvl.su>
Autosubmit: Adam Joseph <adam@westernsemico.com>
Tested-by: BuildkiteCI
This commit creates a separate enum for "catchable" errors (the kind
that `builtins.tryEval` can detect).
Change-Id: Ie81d1112526d852255d9842f67045f88eab192af
Reviewed-on: https://cl.tvl.fyi/c/depot/+/9287
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Autosubmit: Adam Joseph <adam@westernsemico.com>
There's some more left, but they've been renamed/refactored out of
sight.
Change-Id: I41579dedc74342b4c5f8cb39d2995b5b0c90b0f4
Reviewed-on: https://cl.tvl.fyi/c/depot/+/9372
Tested-by: BuildkiteCI
Reviewed-by: Connor Brewster <cbrewster@hey.com>
Autosubmit: flokli <flokli@flokli.de>
In cppnix 2.17, commit b72bc4a972fe568744d98b89d63adcd504cb586c, the
libexpr pretty-printing routine was fixed so that it would no longer
pretty-print attrsets with keywords in their attrnames incorrectly.
This commit implements the corresponding fix for tvix, fixes our
tests to work with cppnix>=2.17 oracles, and expands our test cases
to cover all the keywords.
Change-Id: I4b51389cd3a9c44babc8ab2a84b383b7b0b116ca
Reviewed-on: https://cl.tvl.fyi/c/depot/+/9283
Autosubmit: Adam Joseph <adam@westernsemico.com>
Reviewed-by: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
Previously, compare_strict_eval_tests() was using Strictness::Lazy.
This appears to be a minor oversight from
0ab6494286.
This commit corrects that, by changing Strictness::Lazy to
Strictness::Strict.
Change-Id: Ia2389a5d30481cd322ed55230731340b795e5d87
Reviewed-on: https://cl.tvl.fyi/c/depot/+/9282
Tested-by: BuildkiteCI
Autosubmit: Adam Joseph <adam@westernsemico.com>
Reviewed-by: sterni <sternenseemann@systemli.org>
replaceStrings would previously fail to replace the last character
in a string.
Change-Id: I43a7c960945350b2e7a5b731b7fdb617723eb38f
Reviewed-on: https://cl.tvl.fyi/c/depot/+/9151
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
This passes a unit value to the function.
Change-Id: I4df3ad8fb0f35c0f110cee3349971ae28ce2878c
Reviewed-on: https://cl.tvl.fyi/c/depot/+/9101
Autosubmit: flokli <flokli@flokli.de>
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Tested-by: BuildkiteCI
builtins.div ought to truncate towards zero so that
-(builtins.div a b) == builtins.div (-a) b
-(builtins.div a b) == builtins.div a (-b)
Change-Id: I8b7c08cd7f4fa8a1363c786d42c8d484f6cd133d
Reviewed-on: https://cl.tvl.fyi/c/depot/+/9006
Autosubmit: sterni <sternenseemann@systemli.org>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Some paths might use names that are not valid UTF-8. We should be able
to represent them.
We don't actually need to touch the PathInfo structures, as they need to
represent StorePaths, which come with their own harder restrictions,
which can't encode non-UTF8 data.
While this doesn't change any of the wire format of the gRPC messages,
it does however change the interface of tvix_eval::EvalIO - its
read_dir() method does now return a list of Vec<u8>, rather than
SmolStr. Maybe this should be OsString instead?
Change-Id: I821016d9a58ec441ee081b0b9f01c9240723af0b
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8974
Autosubmit: flokli <flokli@flokli.de>
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Tested-by: BuildkiteCI
This explicitly documents behavior of C++ Nix that goes against the
intuition you'd gather from this document: that e.g. a simple select
from an attribute set causes a value to no longer be pointer equal to
its former self.
The point of documenting this is that we can show in a to be written
section on the use of pointer equality in nixpkgs that pointer equality
is only needed in a limited sense for evaluating it (C++ Nix's exterior
pointer equality). Tvix's pointer equality is far more powerful since
value identity preserving operations also preserve pointer equality,
generally speaking (this is because we implement interior pointer
equality in my made up terminology). This should eventually also be
documented.
Change-Id: I6ce7ef2d67b012f5ebc92f9e81bba33fb9dce7d0
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8856
Tested-by: BuildkiteCI
Autosubmit: sterni <sternenseemann@systemli.org>
Reviewed-by: tazjin <tazjin@tvl.su>
This fixes a subtle issue which would occasionally lead to a crash (e.g.
when evaluating (pkgs.systemd.outPath with --trace-runtime): With each
character in the string that has a multi byte representation in UTF-8,
the actual byte position and what tvix thought it was would get out of
sync. This could either lead to
* Tvix swallowing characters or jumbling characters if multi byte
characters would cause the tracked index to become out of sync with
the byte position before the first character to be escaped, or
* Tvix crashing if (in the same situation) the out of sync index would
be within a UTF-8 byte sequence.
Luckily, std's `char_indices()` iterator implements exactly what
`nix_escape_char()`'s original author had in mind with
`.chars().enumerate()`. Using `i + 1` for continuing is safe, since all
characters that need (in fact, can) to be escaped in Nix are represented
as a single byte in UTF-8.
Change-Id: I1c836f70cde3d72db1c644e9112852f0d824715e
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8952
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Autosubmit: sterni <sternenseemann@systemli.org>
The change allows applications that use tvix_serde for parsing
nix-based configuration to extend the language with domain-specific
set of features.
Change-Id: Ia86612308a167c456ecf03e93fe0fbae55b876a6
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8848
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
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
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>
Adds new tests for foldl', intersectAttrs as well as fills in missing
.exp files.
New test cases we don't pass:
- fromTOML timestamp capabilities
- path antiquotation
- replaceStrings is lazier on C++ Nix master
The C++ Nix revision used is 7066d21a0ddb421967980094222c4bc1f5a0f45a.
Change-Id: Ic619c96e2d41e6c5ea6fa93f9402b12e564af3c5
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8778
Reviewed-by: tazjin <tazjin@tvl.su>
Autosubmit: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
genericClosure has very limited support for pointer equality: It relies
on comparison (not equality!) in C++ Nix, so as soon as C++ Nix supports
comparing lists (langVersion >= 6) we can rely on pointer equality for
key.
Since Tvix uses equality, not comparison for the insert, our behavior is
currently different, as documented by the notyetpassing tests.
Change-Id: Ifcd741ed4fc3ccc3825f7038875d56a9918b786a
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8720
Tested-by: BuildkiteCI
Autosubmit: sterni <sternenseemann@systemli.org>
Reviewed-by: tazjin <tazjin@tvl.su>
In order for the test suite we have currently to be comparable to C++
Nix, we need to display values in the same way. This was largely the
case except in some weird cases.
* <CODE> for thunks and <CYCLE> for repeated thunks (?) are already in
use. <CODE> formatting is tested by the oracle test suite already.
* Instead of lambda, we need to use <LAMBDA>
* <<primop>> and <<primop-app>> (a formatting C++ Nix uses nowhere)
now are <PRIMOP> and <PRIMOP-APP>.
We'll probably want to have a fancier display of values (in a separate
trait) down the line. This could be used for interactive usage, e.g. the
REPL or a potential debugger.
There is a peculiarity with C++ Nix 2.3 formatting primops: import is
considered a <<PRIMOP-APP>>, since it is internally implemented by means
of scopedImport. This implementation detail no longer leaks in C++ Nix
2.13 nor in Tvix.
<CYCLE> display is untested at the moment, since we exhibit a
discrepancy to C++ Nix 2.3. Our current detection is more similar to C++
Nix 2.13—luckily it is also the more consistent of the two. See also
b/245.
Change-Id: I1d534434b02e470bf5475b3758920ea81e3420dc
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8760
Reviewed-by: tazjin <tazjin@tvl.su>
Autosubmit: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
These were added by us in r/5276, so they should go into our test suite.
Change-Id: I6dc74fc242f33c22a17e0b4aee546ccae886ac85
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8774
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Autosubmit: sterni <sternenseemann@systemli.org>
C++ Nix resolves home relative paths at [parse] time. This is not an
option for us, since it prevents being able to separate the compilation
and execution phase later (e.g. precompiled nix expressions). However, a
practical consequence of this is that paths expressions are always
literals (strict) and never thunks.
[parse]: 7066d21a0d/src/libexpr/parser.y (L518-L527)
Change-Id: Ie4b9dc68f62c86d6c7fd5f1c9460c850d97ed1ca
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7041
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Unfortunately, nixpkgs has at least one case[1] where the out environment
variable is shadowed -- though it doesn't cause a problem, since it's
shadowed with the correct value, odd as this may be!
[1]: c7c2984716/pkgs/development/python-modules/pybind11/default.nix (L19)
Change-Id: Ibf6790d2484dc9cce8e424feeb5886664d498dc3
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8696
Autosubmit: tazjin <tazjin@tvl.su>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
When comparing to C++ Nix, we notice that the thunking of default
expressions in function formals corresponds to their normal thunking,
e.g. literals are not thunked. This means that we can just invoke
compile() without much of a care and trust that it will sort it out
correctly.
If function formals blow up as a result of this, it likely indicates
that the expression is treated incorrectly by compile(), not
compile_param_pattern().
Change-Id: I64acbff2f251423eb72ce43e56a0603379305e1d
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8704
Autosubmit: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
C++ Nix forces and typechecks the passed argument even if it is not
necessary in order to compute the return value of the function. I
discovered this when I thought our formals miscompilation might be that
we are too strict, but doesn't look like it in this case.
Change-Id: Ifb3c92592293052c489d1e3ae8c7c54e4b6b4dc6
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8701
Tested-by: BuildkiteCI
Autosubmit: sterni <sternenseemann@systemli.org>
Reviewed-by: tazjin <tazjin@tvl.su>
nix_oracle.rs now gives us the possibility to check this by stuffing the
expressions in a list. In fact, the incorrect behavior fixed in
- cl/8656
- cl/8655
- cl/8662
was discovered using this test suite.
Change-Id: Id0ab01ee6be0b875791214e0a72a2ac941c46c96
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8658
Autosubmit: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
This will be useful for comparing thunking behavior to C++ Nix. I
considered adding this capability to the tvix_tests/nix_tests
infrastructure, but as it would require changing the test file naming
scheme to do it in a clean way, I've postponed it–it's nice that our
tests are compatible with C++ Nix's test suite.
Change-Id: I60bcdd98ed25140e716f0858f8dc28f21ab957aa
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8657
Autosubmit: sterni <sternenseemann@systemli.org>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
HasAttrs was weird because with longer attribute paths it would
sometimes not turn out to be a thunk. If it was a thunk, it'd usually
still do some eval strictly which we'll want to avoid.
Verified against C++ Nix using a new test suite introduced in a later
CL.
Change-Id: I6d047ccc68d046bb268462f170a3c4f3c5ddeffe
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8656
Autosubmit: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Probably no real world code broken by this overzealous evaluation, but
let's be thorough!
Change-Id: Ib405a677182eab7940ace940c68e107573473a54
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8655
Reviewed-by: tazjin <tazjin@tvl.su>
Autosubmit: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
Unary operator applications are thunked which can easily be observed by
nix-instantiate --eval -E '[ (!true) (-1) ]'
Unfortunately, there are few simple expressions where this makes a
difference in the end result. Thus it only cropped up when using nixpkgs
for cross compilation: Here we would compile the expression
!(stdenv.cc.isGNU or false)
to assemble python3Minimal's passthru attribute set (at least this seems
to be the most likely explanation from the backtraces I've studied).
This means that an unthunked
<stdenv.cc.isGNU or false>
OpForce
OpInvert
would be performed in order to assemble this attribute set, causing
stdenv.cc to be evaluated too early, causing an infinite recursion.
Resolves b/273.
It seems that having a test suite that doesn't use --strict and relies
on thunks rendered as <CODE> would be beneficial for catching such
issues. I've not been able to find a test case with --strict that
demonstrates the problem fixed in this CL.
Change-Id: I640a5827b963f5b9d0f86fa2142e75e3a6bbee78
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8654
Tested-by: BuildkiteCI
Autosubmit: sterni <sternenseemann@systemli.org>
Reviewed-by: tazjin <tazjin@tvl.su>
mapAttrs, map and genList call Nix functions provided by the caller and
store the result of applying them in a Nix data structure that does not
force all of its contents when forced itself. This means that when such
a builtin application is forced, the Nix function calls performed by the
builtin should not be forced: They may be forced later, but it is also
possible that they will never be forced, e.g. in
builtins.length (builtins.map (builtins.add 2) [ 1 2 3 ])
it is not necessary to compute a single application of builtins.add.
Since request_call_with immediately performs the function call
requested, Tvix would compute function applications unnecessarily before
this change. Because this was not followed by a request_force, the
impact of this was relatively low in Nix code (most functions return a
new thunk after being applied), but it was enough to cause a lot of
bogus builtins.trace applications when evaluating anything from
`lib.modules`. The newly added test includes many cases where Tvix
previously incorrectly applied a builtin, breaking a working expression.
To fix this we add a new helper to construct a Thunk performing a
function application at runtime from a function and argument given as
`Value`s. This mimics the compiler's compile_apply(), but does itself
not require a compiler, since the necessary Lambda can be constructed
independently.
I also looked into other builtins that call a Nix function to verify
that they don't exhibit such a problem:
- Many builtins immediately use the resulting value in a way that makes
it necessary to compute all the function calls they do as soon as
the outer builtin application is forced:
* all
* any
* filter
* groupBy
* partition
- concatMap needs to (shallowly) force the returned list for
concatenation.
- foldl' is strict in the application of `op` (I added a comment that
makes this explicit).
- genericClosure needs to (shallowly) force the resulting list and some
keys of the attribute sets inside.
Resolves b/272.
Change-Id: I1fa53f744bcffc035da84c1f97ed25d146830446
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8651
Autosubmit: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
This feature allows the compiler to detect situations where the
created thunk is useless and can be merged into the parent chunk
instead.
The only case where the compiler does this initially is when
statically optimising a select expression.
For example, previously the expression `builtins.length` compiled into
two thunks:
1. An "inner" thunk which contained an `OpConstant` that had the
optimised `length` builtin in it.
2. An "outer" thunk which contained an `OpConstant` to access the
inner thunk, and the trailing OpForce of the top-level program.
With this change, the inner thunk is skipped completely and the outer
chunk directly contains the `length` builtin access.
This can be applied in several situations, some easier than others,
and we will add them in as we go along.
Change-Id: Ie44521445fce1199f99b5b17712833faea9bc357
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7959
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
This method extends the contents of one chunk with that of another,
effectively merging the thunks together.
This will be used for the upcoming "unthunking" functionality.
Change-Id: I6ad74232cd7f3eca198ed921e455205e00d76e6b
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7958
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
These two places didn't add the path from the context to the
ErrorKind, but simply relied on the
impl From<std::io::Error> for tvix_eval::ErrorKind, which doesn't add
the path.
Change-Id: Ifc7dbbe305d24242b0705de1dea34e8923e9d2cb
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8603
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Autosubmit: flokli <flokli@flokli.de>
We didn't return anything useful other than ErrorKind::IO anyways.
We can use io::ErrorKind::Unsupported for DummyIO.
Fixes b/271.
Change-Id: Icb231e9b38168e8b6fa473bfa405d160357b317f
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8602
Autosubmit: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
It's okay if these calls mutate some internal state inside an
implementation.
Change-Id: I12bb11bde0310778c3da1275696bf7de058863a3
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8571
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
This switches out the previous compressed representation (count of
instructions per span) with a representation where the chunk's span
list stores the index of the first operation that belongs to a span,
and finds the right span by using a binary search when looking them
up.
This improves the lookup complexity from O(n) to O(log n).
This improvement was suggested and (mostly) implemented by GPT-4. I
only fixed up some names and updated the logic for deleting
spans (which it only did not do because I didn't tell it about that).
The code was verified by producing a complex error before/after the
change and ensuring that all spans in the error match exactly.
Co-Authored-By: GPT-4
Change-Id: Ibfa12cc6973af1c9b0ae55bb464d1975209771f5
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8385
Reviewed-by: ezemtsov <eugene.zemtsov@gmail.com>
Tested-by: BuildkiteCI
Autosubmit: tazjin <tazjin@tvl.su>
This grows the frame stack as the call stack grows, which yields *much*
better user-facing error messages.
I haven't measured the performance impact this has yet, for now I'm
still just trying to add more information to errors and then cut down
again where necessary.
Change-Id: I89f058ef31979edacf4667775d460b60704ce4d7
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8334
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Autosubmit: tazjin <tazjin@tvl.su>
This makes it possible for callers to control whether they can receive
partially evaluated values from an evaluation or not.
We're actually flipping the default behaviour to non-strict top-level
evaluation, which means that callers have to set `strict = true` on
the Evaluation to get the previous behaviour.
Change-Id: Ic048e9ba09c88866d4c3177d5fa07db11c4eb20e
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8325
Autosubmit: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
Emits the span of the `set` that is being accessed in the `force`
operation of an attribute access.
Looking at traces, it's a lot more useful to get information about
*what* is being forced, as in cases like `foo.bar` it can be
misleading to have an error highlight `bar`, when the error occured
while forcing `foo` to be able to access `bar` in the first place.
Change-Id: Id46ff28f20c67cb4971727ac52cc4811795cea2d
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8272
Reviewed-by: flokli <flokli@flokli.de>
Autosubmit: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
This reports the span
1. of the code within a thunk,
2. of the place where the thunk was instantiated,
3. of the place where the thunk was first forced,
4. of the place where the thunk was forced again,
when yielding an infinite recursion error, which hopefully makes it
easier to debug them.
The spans are tracked in the ThunkRepr::Blackhole variant when putting
a thunk under evaluation.
Note that we currently have some loss of span precision in the VM loop
when switching between frame types, so spans 3/4 are currently a bit
wonky. Working on it.
Change-Id: Icbd2a9df903d00e8c2545b3fc46dcd2a9e3e3e55
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8270
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Autosubmit: tazjin <tazjin@tvl.su>
This is step 1 towards being able to use all 4 spans that we know when
dealing with infinite recursion. It tracks the span at which the
force of a thunk was first requested when constructing a blackhole, so
that we can highlight the spans of the first and second forces.
These are actually the least relevant spans, but the easiest to put in
place, more coming soon.
Change-Id: I4c7e82f6211b98756439d4148a4191457cc46807
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8269
Autosubmit: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
This produces traces in which we can see what kind of native code was
run. Note that these "names" are named after the generator message, so
these aren't *really* intended for end-user consumption, but we can
give them saner names later.
Example:
https://gist.github.com/tazjin/82b24e92ace8e821008954867ee05057
This already makes the traces a little easier to parse.
Change-Id: Idcd601baf84f492211b732ea0f04b377112e10d0
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8268
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Autosubmit: tazjin <tazjin@tvl.su>
When emitting an error at runtime, the VM will now use the new
`NativeError` and `BytecodeError` error kinds (which just wrap inner
errors) to create a set of diagnostics to emit.
The primary diagnostic is emitted last, with `error` type (so it will
be coloured red in terminals), the other ones will be emitted with
`note` type, highlighting the causal chain.
Example:
https://gist.github.com/tazjin/25feba7d211702453c9ebd5f8fd378e4
This is currently quite verbose, and we can cut down on this further,
but the purpose of this commit is to surface more information first of
all before worrying about the exact display.
Change-Id: I058104a178c37031c0db6b4b3e4f4170cf76087d
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8266
Autosubmit: tazjin <tazjin@tvl.su>
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
This actually uses coercion under the hood in C++ Nix. See the test
for an example.
Change-Id: Id56b364acf269225b6829d0b600e0222f8b3608d
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8322
Reviewed-by: andi <andi@notmuch.email>
Tested-by: BuildkiteCI
This was commented out and forgotten during the generator refactor, oh
well.
Change-Id: I474b685159a955a846db462da0dd0067af177b04
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8321
Autosubmit: tazjin <tazjin@tvl.su>
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Change-Id: I8224bf039f739c401900b5a2ddc839810c87cf6e
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8226
Tested-by: BuildkiteCI
Reviewed-by: Adam Joseph <adam@westernsemico.com>
We settled on this being the most reasonable name for this construct.
Change-Id: Ic31c45461a842f22aa05f4446123fe3a61dfdbc0
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8291
Tested-by: BuildkiteCI
Autosubmit: tazjin <tazjin@tvl.su>
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Given Rust's current lack of support for tail calls, we cannot avoid
using `async` for builtins. This is the only way to avoid
overflowing the cpu stack when we have arbitrarily deep
builtin/interpreted/builtin/interpreted/... "sandwiches"
There are only five `async fn` functions which are not builtins
(some come in multiple "flavors"):
- add_values
- resolve_with
- force, final_deep_force
- nix_eq, nix_cmp_eq
- coerce_to_string
These can be written iteratively rather than recursively (and in
fact nix_eq used to be written that way!). I volunteer to rewrite
them. If written iteratively they would no longer need to be
`async`.
There are two motivations for limiting our reliance on `async` to
only the situation (builtins) where we have no other choice:
1. Performance.
We don't really have any good measurement of the performance hit
that the Box<dyn Future>s impose on us. Right now all of our
large (nixpkgs-eval) tests are swamped by the cost of other
things (e.g. fork()ing `nix-store`) so we can't really measure
it. Builtins tend to be expensive operations anyways
(regexp-matching, sorting, etc) that are likely to already cost
more than the `async` overhead.
2. Preserving the ability to switch to `musttail` calls.
Clang/LLVM recently got `musttail` (mandatory-elimination tail
calls). Rust has refused to add this mainly because WASM doesn't
support, but WASM `tail_call` has been implemented and was
recently moved to phase 4 (standardization). It is very likely
that Rust will get tail calls sometime in the next year; if it
does, we won't need async anymore. In the meantime, I'd like to
avoid adding any further reliance on `async` in places where it
wouldn't be straightforward to replace it with a tail call.
https://reviews.llvm.org/D99517https://github.com/WebAssembly/proposals/pull/157
https: //github.com/rust-lang/rfcs/issues/2691#issuecomment-1462152908
Change-Id: Id15945d5a92bf52c16d93456e3437f91d93bdc57
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8290
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Autosubmit: Adam Joseph <adam@westernsemico.com>
This commit moves fetch_forced_with and fetch_captured_with into the
scope of their only caller (resolve_with).
Change-Id: I9a8bc27228888729d591e8cb021c431b2b6468f5
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8289
Autosubmit: Adam Joseph <adam@westernsemico.com>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
This rewrites nix_cmp_ordering as an iterative loop, which
eliminates the extra pinned-boxing helper function.
Change-Id: I33d0ecc913e02affd8fd4c7bc1c9ecfdf4c7deb9
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8288
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Autosubmit: Adam Joseph <adam@westernsemico.com>
* We no longer need backtrace-on-stack-overflow, as we no longer
overflow the stack with the recent eval refactorings. This was weird
voodoo anyways, introduced earlier to debug some cases where stack
overflows occured.
* default features of genawaiter crate are not needed, as we don't use
their proc macros
Change-Id: I346fc5a18d7f117ee805909a8be8f535b96be76c
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8263
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
This reorders the operations in the VM's main `match` statement while
evaluating bytecode according to the frequency with which these
operations appear in some nixpkgs evaluations.
I used raw data that looks like this:
https://gist.github.com/tazjin/63d0788a78eb8575b04defaad4ef610d
This has a small but noticeable impact on evaluation performance.
No operations have changed in any way, this is purely moving code
around.
Change-Id: Iaa4ef4f0577e98144e8905fec88149c41e8c315c
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8262
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
