about summary refs log tree commit diff
path: root/tvix/eval/src (follow)
AgeCommit message (Collapse)AuthorFilesLines
2024-02-21 r/7591 feat(tvix/eval): Store string context alongside dataAspen Smith7-63/+365
Previously, Nix strings were represented as a Box (within Value) pointing to a tuple of an optional context, and another Box pointing to the actual string allocation itself. This is pretty inefficient, both in terms of memory usage (we use 48 whole bytes for a None context!) and in terms of the extra indirection required to get at the actual data. It was necessary, however, because with native Rust DSTs if we had something like `struct NixString(Option<NixContext>, BStr)` we could only pass around *fat* pointers to that value (with the length in the pointer) and that'd make Value need to be bigger (which is a waste of both memory and cache space, since that memory would be unused for all other Values). Instead, this commit implements *manual* allocation of a packed string representation, with the length *in the allocation* as a field past the context. This requires a big old pile of unsafe Rust, but the payoff is clear: hello outpath time: [882.18 ms 897.16 ms 911.23 ms] change: [-15.143% -13.819% -12.500%] (p = 0.00 < 0.05) Performance has improved. Fortunately this change can be localized entirely within value/string.rs, since we were abstracting things out nicely. Change-Id: Ibf56dd16c9c503884f64facbb7f0ac596463efb6 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10852 Tested-by: BuildkiteCI Reviewed-by: raitobezarius <tvl@lahfa.xyz> Autosubmit: aspen <root@gws.fyi>
2024-02-21 r/7587 fix(tvix/eval): allow reading non-UTF8 filesFlorian Klink2-6/+6
With our values using bstr now, we're not restricted to only reading files that contain valid UTF-8. Update our `read_to_string` function to `read_to_end` (named like `std::io::Read::read_to_end`), and have it return a Vec<u8>. Change-Id: I87f0291dc855a132689576559c891d66c30ddf2b Reviewed-on: https://cl.tvl.fyi/c/depot/+/11003 Tested-by: BuildkiteCI Autosubmit: flokli <flokli@flokli.de> Reviewed-by: Pádraic Ó Mhuiris <patrick.morris.310@gmail.com> Reviewed-by: flokli <flokli@flokli.de>
2024-02-20 r/7580 fix(tvix/eval): Propagate catchables in builtins.groupByAspen Smith3-4/+6
One last place where we needed to wrap the forcing of the element of a list in `try_value!`. This fixes a previously `notyetpassing` test Change-Id: I8827a3e39630e6959013b70bdfa9cbcb93f4e91c Reviewed-on: https://cl.tvl.fyi/c/depot/+/10789 Autosubmit: aspen <root@gws.fyi> Reviewed-by: sterni <sternenseemann@systemli.org> Tested-by: BuildkiteCI
2024-02-20 r/7577 fix(tvix/eval): propagate catchable errors at the top of an evalVincent Ambo3-6/+46
(Re-)Adds an error variant that wraps a catchable error kind, which is used for returning the result of an evaluation. Previously this would return the internal catchable value, which would lead to panics if users tried to use these. Somehow this was missed; I think we need error output tests. Change-Id: Id6e24aa2ce4ea4358a29b2e1cf4a6749986baf8c Reviewed-on: https://cl.tvl.fyi/c/depot/+/10991 Tested-by: BuildkiteCI Autosubmit: tazjin <tazjin@tvl.su> Reviewed-by: flokli <flokli@flokli.de>
2024-02-20 r/7574 fix(tvix/eval): make path resolution work by default in impure modeVincent Ambo1-0/+6
The previous behaviour (enabling `import`, but not allowing e.g. `<nixpkgs/lib>` to resolve) was very confusing. Now imports from NIX_PATH become enabled by default, unless the user already overrode that behaviour with something else by setting `Evaluation::nix_path` manually. Change-Id: Iad970beb633d9887be4b185b01e6f5858d81bea3 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10993 Reviewed-by: flokli <flokli@flokli.de> Autosubmit: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2024-02-20 r/7573 fix(tvix/eval): fix accidental recursion in printing errorsVincent Ambo1-1/+1
There's some code path where fancy_format_str()->Error::Display, which recurses forever and overflows the stack. This was introduced in a previous commit today. Change-Id: I87a59492099f6c138c752478901b9aa614bb57cc Reviewed-on: https://cl.tvl.fyi/c/depot/+/10990 Reviewed-by: flokli <flokli@flokli.de> Tested-by: BuildkiteCI Autosubmit: tazjin <tazjin@tvl.su> Reviewed-by: Peter Kolloch <info@eigenvalue.net>
2024-02-20 r/7572 refactor(tvix/eval): use internal SourceCode field in error printersVincent Ambo1-17/+16
Makes use of the SourceCode field now being stored directly in errors (see parent CL). With this change, the default `Display` implementation can now format errors correctly, and there is no need to keep a `SourceCode` around just for error formatting. Updates dependent crates (CLI, serde, tvixbolt) to use this correctly. Change-Id: Iddc5d7a6b4bab391f30a999e4c68aca34304c059 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10987 Tested-by: BuildkiteCI Reviewed-by: flokli <flokli@flokli.de>
2024-02-20 r/7571 refactor(tvix/eval): add SourceCode directly into error typesVincent Ambo6-40/+67
With this change it's no longer necessary to track the SourceCode struct separately from the evaluation for error reporting: It's just stored directly in the errors. This also ends up resolving an issue in compiler::bindings, where we cloned the Arc containing file references way too often. In fact those clones probably compensate for all additional SourceCode clones during error construction now. Change-Id: Ice93bf161e61f8ea3d48103435e20c53e6aa8c3a Reviewed-on: https://cl.tvl.fyi/c/depot/+/10986 Reviewed-by: flokli <flokli@flokli.de> Tested-by: BuildkiteCI
2024-02-20 r/7569 refactor(tvix/eval): add API for enabling impure evaluation featuresVincent Ambo1-10/+20
This makes some of the work of configuring an arbitrary I/O handler easier. Change-Id: I158db3235fe83df6e709578ed515e0e028c20086 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10959 Reviewed-by: flokli <flokli@flokli.de> Autosubmit: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2024-02-13 r/7509 test(tvix/eval): Add test asserting size of ValueAspen Smith1-0/+8
Now that I've done a ton of things to make sure Value is small on the stack (16 bytes, which is a perfectly reasonable size for a programming language Value enum), add a test asserting it stays that way. These size improvements have a measurable impact, too - here's the `hello outpath` benchmark compared between canon (as of r/7495) and this commit: hello outpath time: [990.56 ms 995.83 ms 1.0070 s] change: [-7.1397% -6.1302% -5.1651%] (p = 0.00 < 0.05) Performance has improved. Change-Id: If99a0976eab28eb5e516fcd2f4a0e068145af23e Reviewed-on: https://cl.tvl.fyi/c/depot/+/10799 Reviewed-by: raitobezarius <tvl@lahfa.xyz> Tested-by: BuildkiteCI Autosubmit: aspen <root@gws.fyi> Reviewed-by: sterni <sternenseemann@systemli.org>
2024-02-13 r/7508 feat(tvix/eval): Box Value::CatchableAspen Smith10-32/+37
This is now the only enum variant for Value that is larger than 8 bytes (it's 16 bytes), so boxing it (especially since it's not perf-critical) allows us to get the Value size down to only 16 bytes! Change-Id: I98598e2b762944448bef982e8ff7da6d6683c4aa Reviewed-on: https://cl.tvl.fyi/c/depot/+/10798 Tested-by: BuildkiteCI Reviewed-by: raitobezarius <tvl@lahfa.xyz> Autosubmit: aspen <root@gws.fyi>
2024-02-13 r/7507 revert(tvix/eval): Don't double-box Path valuesAspen Smith6-23/+20
This reverts commit d3d41552cf1f6485f8ebc597a2128a0d15b030a5. This was well-intentioned, but now the boxed Path values are actually the *largest* Value enum variants, at 16 bytes (because they're fat-pointers, with a len) instead of 8 bytes like all the other values. Having the double reference is a reasonable price to pay (it seems; more benchmarks may end up disagreeing) for a smaller Value repr. Change-Id: I0d3e84f646c8f5ffd0b7259c4e456637eea360f7 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10797 Tested-by: BuildkiteCI Autosubmit: aspen <root@gws.fyi> Reviewed-by: sterni <sternenseemann@systemli.org>
2024-02-13 r/7506 fix(tvix/eval): Replace inner NixString repr with Box<Bstr>Aspen Smith3-49/+79
Storing a full BString here incurs the extra overhead of the capacity for the inner byte-vector, which we basically never use as Nix strings are immutable (and we don't do any mutation / sharing analysis). Switching to a Box<BStr> cuts us from 72 bytes to 64 bytes per string (and there are a lot of strings!) Change-Id: I11f34c14a08fa02759f260b1c78b2a2b981714e4 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10794 Autosubmit: aspen <root@gws.fyi> Reviewed-by: flokli <flokli@flokli.de> Tested-by: BuildkiteCI
2024-02-10 r/7497 refactor(tvix/eval): Box the strings in CatchableErrorKindAspen Smith3-13/+14
These strings are allocated once and never changed, so they don't need the additional overhead of a capacity given by String - instead, we can use Box<str> and save on 16 bytes for each of these, *and* for each Value since this is currently the largest Value variant. Change-Id: I3e5cb070fe6c5bf82114c92d04f6bae775663a7e Reviewed-on: https://cl.tvl.fyi/c/depot/+/10796 Autosubmit: aspen <root@gws.fyi> Reviewed-by: sterni <sternenseemann@systemli.org> Tested-by: BuildkiteCI
2024-02-10 r/7496 refactor(tvix/eval): Box the inside of Value::JsonAspen Smith3-3/+3
serde_json::Value is pretty large, and is contributing (albeit not exclusively) to the large size of the Value repr. Putting it in a box is *especially* cheap (since it's rarely used) and allows us to (eventually) cut down on the size of Value. Change-Id: I005a802d8527b639beb4e938e3320b11ffa1ef23 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10795 Reviewed-by: sterni <sternenseemann@systemli.org> Autosubmit: aspen <root@gws.fyi> Tested-by: BuildkiteCI
2024-02-10 r/7495 feat(tvix/eval): strengthen significantly catchable test suiteRyan Lahfa71-0/+203
Adds a bunch (notably certain overlapping) tests for catchable situations. This should cover many scenarios, argument is catchable, element in argument is catchable, function returns catchable in the middle of the processing, etc. Co-authored-by: Aspen Smith <root@gws.fyi> Change-Id: Icd722cf8dbc91a24f45cd540a328711e5826f76c Reviewed-on: https://cl.tvl.fyi/c/depot/+/10621 Reviewed-by: aspen <root@gws.fyi> Tested-by: BuildkiteCI
2024-02-09 r/7492 fix(tvix/eval): Propagate catchables in NixAttrs::constructAspen Smith5-13/+31
Correctly propagate the case where the *key* of an attrset is a Value::Catchable (eg { "${builtins.throw "c"}" = "b"; }) in `NixAttrs::construct`, by converting the return type to `Result<Result<Self, CatchableErrorKind>, ErrorKind>` (ugh!!) and correctly handling that everywhere (including an `expect` in the Deserialize impl for NixAttrs, since afaict this is impossible to hit when deserializing from stuff like JSON). Change-Id: Ic4bc611fbfdab27c0bd8a40759689a87c4004a17 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10786 Reviewed-by: raitobezarius <tvl@lahfa.xyz> Tested-by: BuildkiteCI
2024-02-08 r/7488 fix(tvix/eval): Inline List.sort_by, and propagate errorsAspen Smith2-53/+45
In order to correctly propagate errors in the comparator passed to builtins.sort, we need to do all the sorting in a context where we can short-circuit return `Value`s (because catchables are Values on the `Ok` side of the Result , not `Err`s). Unfortunately this means we have to *inline* the List `sort_by` implementation into the builtin_sort function - fortunately this is the only place that was called so this is relatively low cost. This does that, and adds the requisite `try_value!` invocation to allow us to propagate comparator errors here. As before, this doesn't include tests, primarily since those are coming in the next commit. Change-Id: I8453c3aa2cd82299eae89828e2a2bb118da4cd48 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10754 Tested-by: BuildkiteCI Reviewed-by: raitobezarius <tvl@lahfa.xyz>
2024-02-08 r/7487 fix(tvix): Catch errors for generator in some builtinsAspen Smith1-2/+3
Nix doesn't propagate errors for the function argument to some builtins, like genList and map: ❯ nix repl Welcome to Nix version 2.3.17. Type :? for help. nix-repl> (builtins.tryEval (builtins.genList (builtins.throw "a") 10)).success true nix-repl> (builtins.tryEval (builtins.map (builtins.throw "a") [ "" ])).success true Note that this is untested as of this particular commit, only because a big test suite covering all sorts of catchable error propagation issues is coming next Change-Id: I48c8eb390a541204b1a6d438c753fa1ca9b3877e Reviewed-on: https://cl.tvl.fyi/c/depot/+/10753 Autosubmit: aspen <root@gws.fyi> Tested-by: BuildkiteCI Reviewed-by: raitobezarius <tvl@lahfa.xyz>
2024-02-08 r/7483 fix(tvix/eval): Propagate catchables in a few more placesAspen Smith1-9/+10
Propagate catchables that we get from forcing thunks in builtins in a few more places using the new try_value! macro Change-Id: I95fd41a231f877ff153f4adbabd944372d4cc7eb Reviewed-on: https://cl.tvl.fyi/c/depot/+/10738 Reviewed-by: raitobezarius <tvl@lahfa.xyz> Tested-by: BuildkiteCI
2024-02-08 r/7482 refactor(tvix/eval): Generalize propagation of catchable valuesAspen Smith1-177/+13
Rather than explicitly checking for Value::Catchable in all builtins, make the #[builtin] proc macro insert this for all strict arguments by default, with support for a #[catch] attribute on the argument to disable this behavior. That attribute hasn't actually been *used* anywhere here, primarily because the tests pass without it, even for those builtins which weren't previously checking for Value::Catchable - if some time passes without this being used I might get rid of support for it entirely. There's also a `try_value` macro in builtins directly for the places where builtins were eg forcing something, then explicitly propagating a catchable value. Change-Id: Ie22037b9d3e305e3bdb682d105fe467bd90d53e9 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10732 Tested-by: BuildkiteCI Reviewed-by: raitobezarius <tvl@lahfa.xyz>
2024-02-02 r/7467 refactor(tvix/eval): Box Value::StringAspen Smith10-63/+60
NixString is *quite* large - like 80 bytes - because of the extra capacity value for BString and because of the context. We want to keep Value small since we're passing it around a lot, so let's box the NixString inside Value::String to save on some memory, and make cloning ostensibly a little cheaper Change-Id: I343c8b4e7f61dc3dcbbaba4382efb3b3e5bbabb2 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10729 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org>
2024-02-01 r/7463 feat(tvix/eval): Don't emit OpForce for non-thunk constantsAspen Smith3-0/+26
In the compiler, skip emitting an OpForce if the last op was an OpConstant for a non-thunk constant. This gives a small (~1% on my machine) perf boost, eg when evaluating hello.outPath: ❯ hyperfine \ "./before --no-warnings -E '(import <nixpkgs> {}).hello.outPath'" \ "./after --no-warnings -E '(import <nixpkgs> {}).hello.outPath'" Benchmark 1: ./before --no-warnings -E '(import <nixpkgs> {}).hello.outPath' Time (mean ± σ): 1.151 s ± 0.022 s [User: 1.003 s, System: 0.151 s] Range (min … max): 1.123 s … 1.184 s 10 runs Benchmark 2: ./after --no-warnings -E '(import <nixpkgs> {}).hello.outPath' Time (mean ± σ): 1.140 s ± 0.022 s [User: 0.989 s, System: 0.152 s] Range (min … max): 1.115 s … 1.175 s 10 runs Summary ./after --no-warnings -E '(import <nixpkgs> {}).hello.outPath' ran 1.01 ± 0.03 times faster than ./before --no-warnings -E '(import <nixpkgs> {}).hello.outPath' Change-Id: I2105fd431d4bad699087907e16c789418e9a4062 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10714 Reviewed-by: sterni <sternenseemann@systemli.org> Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2024-02-01 r/7461 refactor(tvix/eval): Don't double-box Path valuesAspen Smith6-20/+23
PathBuf internally contains a heap pointer (an OsString), so we were in effect double-boxing here. Removing the extra layer by making Tvix::Value represented by a Box<Path> rather than a Box<PathBuf> saves us an indirection, while still avoiding the extra memory overhead of the capacity which was the reason we were boxing PathBuf in the first place. Change-Id: I8c185b9d4646161d1921917f83e87421496a3e24 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10725 Reviewed-by: sterni <sternenseemann@systemli.org> Autosubmit: aspen <root@gws.fyi> Tested-by: BuildkiteCI
2024-01-31 r/7460 fix(tvix): Represent strings as byte arraysAspen Smith13-173/+278
C++ nix uses C-style zero-terminated char pointers to represent strings internally - however, up to this point, tvix has used Rust `String` and `str` for string values. Since those are required to be valid utf-8, we haven't been able to properly represent all the string values that Nix supports. To fix that, this change converts the internal representation of the NixString struct from `Box<str>` to `BString`, from the `bstr` crate - this is a wrapper around a `Vec<u8>` with extra functions for treating that byte vector as a "morally string-like" value, which is basically exactly what we need. Since this changes a pretty fundamental assumption about a pretty core type, there are a *lot* of changes in a lot of places to make this work, but I've tried to keep the general philosophy and intent of most of the code in most places intact. Most notably, there's nothing that's been done to make the derivation stuff in //tvix/glue work with non-utf8 strings everywhere, instead opting to just convert to String/str when passing things into that - there *might* be something to be done there, but I don't know what the rules should be and I don't want to figure them out in this change. To deal with OS-native paths in a way that also works in WASM for tvixbolt, this also adds a dependency on the "os_str_bytes" crate. Fixes: b/189 Fixes: b/337 Change-Id: I5e6eb29c62f47dd91af954f5e12bfc3d186f5526 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10200 Reviewed-by: tazjin <tazjin@tvl.su> Reviewed-by: flokli <flokli@flokli.de> Reviewed-by: sterni <sternenseemann@systemli.org> Autosubmit: aspen <root@gws.fyi> Tested-by: BuildkiteCI
2024-01-31 r/7459 feat(tvix/eval/observer): Allow capturing timing of eventsAspen Smith1-0/+28
Add a new --trace-runtime-timing flag (probably a better bikeshed for this) that enables capturing the time, relative to the last event, of each event recorded with the tracing observer. This probably isn't *super* useful yet, but I'd like to start here in adding new profiling tools to the VM, specifically based on the runtime observer Change-Id: Id7f12077291c39bf3eef42ab6744bfba53687a65 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10713 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org>
2024-01-25 r/7451 feat(tvix/eval/tvix_tests): add some more xml testsFlorian Klink2-0/+48
https: //cl.tvl.fyi/c/depot/+/10686/comment/ea582dae_574d6c3f/ Change-Id: Iac74abbf2f2e0327bc9ddf9dcc6bb43f918a1c63 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10689 Autosubmit: flokli <flokli@flokli.de> Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2024-01-25 r/7449 feat(tvix/eval/tests): compare .xml outputsFlorian Klink4-20/+29
In case a .exp.xml file is provided alongside the test, compare its output with the desired state. Also, add some function .exp.xml that were presumably moved out of the way back to src/tests/nix_tests, as they now produce the correct XML output. Change-Id: Ibd8123f3e6ed7bae3a44407d2284a2b2c8ce9a28 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10687 Reviewed-by: sterni <sternenseemann@systemli.org> Autosubmit: flokli <flokli@flokli.de> Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su>
2024-01-25 r/7448 feat(tvix/eval): track pattern binding namesFlorian Klink5-5/+24
These need to be preserved at least for builtins.toXML. Also, we incorrectly only wrote an <attrspat> in case ellipsis was true, but that's not the case. Change-Id: I6bff9c47c2922f878d5c43e48280cda9c9ddb692 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10686 Tested-by: BuildkiteCI Autosubmit: flokli <flokli@flokli.de> Reviewed-by: aspen <root@gws.fyi>
2024-01-24 r/7447 fix(tvix/eval/value/function): use BTreeMap for function arg namesFlorian Klink2-5/+5
At least toXML wants to get these out in a sorted fashion. Change-Id: I6373d7488fff7c40dc2ddeeecd03ba537c92c4af Reviewed-on: https://cl.tvl.fyi/c/depot/+/10685 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2024-01-24 r/7446 feat(tvix/eval): expose value_to_xml for test casesFlorian Klink2-1/+4
It's debateable on whether the serialization code should be exposed a bit more prominently or not. Change-Id: Iff7a28f884b1490b12b145dfdadbedacb84fd387 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10684 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org> Reviewed-by: tazjin <tazjin@tvl.su>
2024-01-24 r/7445 fix(tvix/eval/tests): fix eval-okay-getenvFlorian Klink3-0/+2
This relies on TEST_VAR=foo being set to "foo". Nix does this in tests/functional/lang.sh, we do it in the test suite. Change-Id: I7ffa9ed27124530b7758aeadf07c79477656f34f Reviewed-on: https://cl.tvl.fyi/c/depot/+/10683 Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su>
2024-01-23 r/7441 docs(tvix/eval/tests): update commentFlorian Klink1-2/+1
There's no `expected_failures` feature, we run them unconditionally. Change-Id: Ibe1c93497e040d0d5b6cbfcaa043027814c191f1 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10681 Reviewed-by: raitobezarius <tvl@lahfa.xyz> Tested-by: BuildkiteCI Autosubmit: flokli <flokli@flokli.de>
2024-01-19 r/7422 chore(3p/sources): Bump channels & overlayssterni1-5/+1
- Adjust to ecl 23.9.9 release - Regenerate go protos after protoc-gen-go update - Drop dhall fork which hasn't kept up with 1.42.* - Address new clippy warnings: - Variant naming of Error::ValidationError - Simplify .try_into().unwrap() - Drop unnecessary identity function - Test module must be last in file - Drop unused `pub use` - Update agenix to 0.15.0. Current master has a installCheckPhase that doesn't work with C++ Nix 2.3.*: https://github.com/ryantm/agenix/commit/a23aa271bec82d3e962bafb994595c1c4a62b133#commitcomment-137185861 Change-Id: Ic29eef20d6fd1362ce1031364a5ca6b4edf195bd Reviewed-on: https://cl.tvl.fyi/c/depot/+/10615 Reviewed-by: aspen <root@gws.fyi> Tested-by: BuildkiteCI Autosubmit: sterni <sternenseemann@systemli.org>
2024-01-18 r/7409 refactor(tvix/eval): move Evaluation::{default,new_pure}() againFlorian Klink3-20/+19
Have a Evaluation::new() function that's used to set up the Evaluation struct initially - which is also used by both new_pure and new_impure internally. It's generic over the exact type of IO, making it easier to instantiate Evaluation with non-tvix-eval EvalIO implementations, that might not be in a Box. Change-Id: Ibf728da24aca59639c5b6df58d00ae98c99a63f5 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10640 Reviewed-by: raitobezarius <tvl@lahfa.xyz> Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2024-01-18 r/7407 refactor(tvix/eval): generalize EvalIO containerFlorian Klink5-43/+66
Don't restrict to a Box<dyn EvalIO>. There's still one or two places where we do restrict, this will be solved by b/262. Change-Id: Ic8d927d6ea81fa12d90b1e4352f35ffaafbd1adf Reviewed-on: https://cl.tvl.fyi/c/depot/+/10639 Tested-by: BuildkiteCI Reviewed-by: raitobezarius <tvl@lahfa.xyz>
2024-01-17 r/7406 fix(tvix/eval): catchable-aware `throw`Ryan Lahfa1-0/+4
`throw (throw "a")` should work and propagate the internal throw. Before this commit, it didn't work. Change-Id: Id5d46f74e484dba99e912ad9fa211f3bf1617bac Reviewed-on: https://cl.tvl.fyi/c/depot/+/10600 Tested-by: BuildkiteCI Reviewed-by: flokli <flokli@flokli.de>
2024-01-17 r/7405 fix(tvix/eval): catchable-aware `elem`Ryan Lahfa1-0/+4
`elem` did not catch the list being a catchable. This surfaced during Nixpkgs evaluation. Change-Id: Icf19b94e914e35a435c4412d769ee63ba59ab7b0 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10599 Reviewed-by: flokli <flokli@flokli.de> Tested-by: BuildkiteCI
2024-01-17 r/7404 feat(tvix/glue): introduce test suite for context stringsRyan Lahfa1-8/+0
This is an additional test suite on the top of the Nix ones for context strings matters. It already smoked out multiple mistakes and potential bugs and non-deterministic result from the evaluator. It uses a similar technology as the one in the tvix-eval albeit we instantiate a fully fledged evaluator with in-memory store. We copy the files instead of symlinking them because crates are built in isolation, so symlinks cannot work. Change-Id: I63ae225ce4f83c6e2c8ccd60d779c2f8eb9d08fb Reviewed-on: https://cl.tvl.fyi/c/depot/+/10619 Autosubmit: raitobezarius <tvl@lahfa.xyz> Tested-by: BuildkiteCI Reviewed-by: flokli <flokli@flokli.de>
2024-01-17 r/7403 fix(tvix/eval): `getContext` merges underlying valuesRyan Lahfa1-17/+65
Previously, we were assembling very naively an attribute set composed of context we saw. But it was forgetting that `"${drv}${drv.drvPath}"` would contain 2 contexts with the same key, but with different values, one with `outputs = [ "out" ];` and `allOutputs = true;`. Following this reasoning and comparing with what Nix does, we ought to merge underlying values systematically. Hence, I bring `itertools` to perform a group by on the key and merge everything on the fly, it's not beautiful but it's the best I could find, notice that I don't use `group_by` but I talk about group by, that is, because `group_by` is a `group_by_consecutive`, see https://github.com/rust-itertools/itertools/issues/374. Initially, I tried to do it without a `into_grouping_map_by`, it was akin to assemble the final `NixAttrs` directly, it was less readable and harder to pull out because we don't have a lot of in-place mutable functions on our data structures. Change-Id: I9933c9bd88ffe04de50dda14f21879b60d8b8cd4 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10620 Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su>
2024-01-17 r/7400 fix(tvix/eval): context-aware `dirOf`Ryan Lahfa1-1/+1
`dirOf` forgot to accepts contextful strings, e.g. derivations and propagates this context further. Change-Id: I6c05944a3ce5073e243e7676c9be56c48407d657 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10618 Reviewed-by: flokli <flokli@flokli.de> Tested-by: BuildkiteCI
2024-01-17 r/7399 fix(tvix/eval): context-aware… `hasContext`Ryan Lahfa1-1/+1
Yes, `hasContext e` should work where `e` is a contextful strings, otherwise, it is really useless. Change-Id: I5eb071fc257217d6e8a63fe519132ebd98186696 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10617 Tested-by: BuildkiteCI Reviewed-by: flokli <flokli@flokli.de>
2024-01-17 r/7398 feat(tvix/eval): move away from `test_generator` to `rstest`Ryan Lahfa1-28/+43
`test-generator` has not been updated in the past 2 years. `rstest` has not been updated in the past 5 months. This is an improvement in the maintenance state… I guess? We get also new features, it changes the name of the tests with numbers too. Change-Id: I5376104c7704f525dba7524da78daa09867cc669 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10623 Tested-by: BuildkiteCI Reviewed-by: flokli <flokli@flokli.de>
2024-01-16 r/7395 fix(tvix/eval): lift VM ops over Catchableedef2-115/+102
We want to handle bottoms in a consistent fashion. Previously this was handled by repetitive is_catchable checks, which were not consistently present. Change-Id: I9614c479cc6297d1f64efba22b620a26e2a96802 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10485 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2024-01-14 r/7379 fix(tvix/eval): catchable-aware builtinsRyan Lahfa5-38/+306
A bunch of operations in Tvix are not aware of catchable values and does not propagate them. In the meantime, as we wait for a better solution, we just offer this commit for moving the needle. Change-Id: Ic3f0e1550126b0847b597dfc1402c35e0eeef469 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10473 Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su>
2024-01-12 r/7375 feat(tvix/eval): make into_json publicFlorian Klink1-1/+1
Allow other crates (like tvix-glue) to look at a Value in JSON, which is used by the structured attrs feature. Change-Id: Iba02ace6e11a74c3f9b19dcbef4b008b76dec046 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10602 Reviewed-by: tazjin <tazjin@tvl.su> Reviewed-by: raitobezarius <tvl@lahfa.xyz> Tested-by: BuildkiteCI
2024-01-12 r/7369 fix(tvix/eval): fix JSON error typesFlorian Klink1-7/+7
The error message is misleading. The errors we return can happen both during serialization or deserialization, though the messages suggested the latter only. Change-Id: I2dafe17ec78ee75cab5937a3a81540fda3175eac Reviewed-on: https://cl.tvl.fyi/c/depot/+/10603 Autosubmit: flokli <flokli@flokli.de> Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2024-01-05 r/7344 fix(tvix/eval): Update eval benches for new APIAspen Smith1-2/+1
cl/10475 updated the API of tvix_eval::Evaluation, but didn't update this benchmark to use that new API. Also, fixes the docstring to no longer specify that there is a "given snippet". Change-Id: Ibb8285731849dbeec814e2585bbaa36f22368afe Reviewed-on: https://cl.tvl.fyi/c/depot/+/10542 Autosubmit: aspen <root@gws.fyi> Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su> Reviewed-by: flokli <flokli@flokli.de>
2024-01-03 r/7340 feat(tvix/eval): context-aware `split`Ryan Lahfa1-5/+14
Nix does something like: ```cpp NixStringContext context; const auto str = state.forceString(*args[1], context, pos, "while evaluating the second argument passed to builtins.split"); ``` And then do nothing with that context, therefore, we follow them and make `split` aware of the context but still do nothing with it. Change-Id: I4fee1936600ce86d99d00893ca3f64013213935b Reviewed-on: https://cl.tvl.fyi/c/depot/+/10428 Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su> Autosubmit: raitobezarius <tvl@lahfa.xyz>
2024-01-03 r/7339 feat(tvix/eval): impl `unsafeDiscardStringContext`Ryan Lahfa1-4/+19
Change-Id: I7f0cc42cbebfe5cd27bf6d4f58a4af927b83646a Reviewed-on: https://cl.tvl.fyi/c/depot/+/10423 Autosubmit: raitobezarius <tvl@lahfa.xyz> Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI