about summary refs log tree commit diff
path: root/tvix/eval/src/value
AgeCommit message (Collapse)AuthorFilesLines
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-03 r/7335 feat(tvix/eval): contextful coercion of filesRyan Lahfa1-0/+5
In the past reference tracking system, `tvix-io` glue was appending plain paths in the known path state. Now, we make up for this by just making contextful coercion of file imports. Change-Id: Ieb9b04dd83302c77909252d5f7733857ac3cf8fd Reviewed-on: https://cl.tvl.fyi/c/depot/+/10443 Tested-by: BuildkiteCI Autosubmit: raitobezarius <tvl@lahfa.xyz> Reviewed-by: tazjin <tazjin@tvl.su>
2024-01-03 r/7333 feat(tvix/eval): contextful == of derivationsRyan Lahfa1-2/+6
Otherwise, you just fail because they are not... contextless strings! Change-Id: I0b8f63a18cd89c3841b613d41c12ec4ee336f953 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10442 Reviewed-by: tazjin <tazjin@tvl.su> Autosubmit: raitobezarius <tvl@lahfa.xyz> Tested-by: BuildkiteCI
2024-01-03 r/7331 feat(tvix/eval): implement `getContext` primopRyan Lahfa1-0/+4
Change-Id: I2c5068a28f9883a01b0ff80a5e5ab32ba18bfc1a Reviewed-on: https://cl.tvl.fyi/c/depot/+/10437 Reviewed-by: tazjin <tazjin@tvl.su> Autosubmit: raitobezarius <tvl@lahfa.xyz> Tested-by: BuildkiteCI
2024-01-03 r/7317 feat(tvix/eval): context-aware `coerce_to_string`Ryan Lahfa2-2/+18
I am still undecided whether we need a CoercionKind to control the coerced context, here's a simple attempt. Change-Id: Ibe59d09ef26c519a6acfdfe392014446646dd6d8 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10426 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI Autosubmit: raitobezarius <tvl@lahfa.xyz>
2023-12-29 r/7282 feat(tvix/eval): context-aware casting to stringsRyan Lahfa1-2/+23
By default, we don't want contextful strings and we almost always want contextless strings. To this end, we make taking a contextful string a very explicit operation under `to_contextful_str` and we implement manually the `to_str` cast which requires a `if !s.has_context()` guard that the macro cannot cover. Change-Id: I7aae8e57a7d73e547e62b1edb0b1cc7e8c0c69b6 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10425 Autosubmit: raitobezarius <tvl@lahfa.xyz> Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2023-12-29 r/7280 feat(tvix/eval): introduce `NixContext`Ryan Lahfa2-7/+192
This prepares the data structures to implement string contexts in Nix. Change-Id: Idd913c9c881daeb8d446907f4b940e462e730978 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10420 Autosubmit: raitobezarius <tvl@lahfa.xyz> Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su>
2023-12-29 r/7274 fix(tvix/eval): catchable in type field of nix_eq()Adam Joseph1-2/+10
Change-Id: I165ff77764e272cc94d18cb03ad6cbc9a8ebefde Reviewed-on: https://cl.tvl.fyi/c/depot/+/10348 Autosubmit: Adam Joseph <adam@westernsemico.com> Reviewed-by: sterni <sternenseemann@systemli.org> Tested-by: BuildkiteCI
2023-12-29 r/7272 refactor(tvix/eval): let OpCoerceToString select the CoercionKindAdam Joseph1-1/+1
Change-Id: I92d58ef216d7e0766af70f019b3dcd445284a95d Reviewed-on: https://cl.tvl.fyi/c/depot/+/10344 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2023-12-25 r/7264 fix(tvix/eval/value): correctly emit spaces when coercing listssterni1-2/+17
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
2023-12-14 r/7218 fix(tvix/eval): remove incorrect imports when coercingsterni2-18/+43
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>
2023-12-12 r/7178 fix(tvix/eval): fix branching on catchable defaults (b/343)Adam Joseph1-0/+1
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>
2023-12-12 r/7176 feat(tvix/eval): nonrecursive coerce_to_string()Adam Joseph3-111/+113
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>
2023-12-12 r/7175 feat(tvix/eval): nonrecursive deep_force()Adam Joseph2-67/+69
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>
2023-12-12 r/7174 docs(tvix/eval): clarify difference between ThunkSet and BlackholeAdam Joseph1-2/+5
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>
2023-12-12 r/7169 fix(tvix/eval): preserve catchables in nix_cmp_ordering(), fix b/338Adam Joseph1-4/+6
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>
2023-12-12 r/7167 feat(tvix/eval): nonrecursive nix_cmp_ordering(), fixes b/339Adam Joseph1-40/+69
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>
2023-12-12 r/7166 fix(tvix/eval): never use partial_cmp() (partial fix b/338)Adam Joseph1-18/+11
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
2023-12-12 r/7164 feat(tvix/eval): nonrecursive nix_eq()Adam Joseph2-129/+151
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>
2023-12-09 r/7134 refactor(tvix/eval): address clippy lintsFlorian Klink1-1/+1
Change-Id: Ic2bd4e8291b30ceac9fa0e88a4f56e61ae99b603 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10227 Reviewed-by: tazjin <tazjin@tvl.su> Autosubmit: flokli <flokli@flokli.de> Tested-by: BuildkiteCI
2023-12-09 r/7130 feat(tvix/eval): impl DoubleEndedIter for OwnedAttrsIteratorAdam Joseph1-0/+10
Change-Id: I4bd85dbe9c27047f4abbdeff4e2b796e9bcab3a1 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10211 Reviewed-by: tazjin <tazjin@tvl.su> Autosubmit: Adam Joseph <adam@westernsemico.com> Tested-by: BuildkiteCI
2023-12-06 r/7120 feat(tvix/eval): rewrite Thunk::force() in nonrecursive formAdam Joseph2-63/+96
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
2023-12-05 r/7119 fix(tvix/eval): Return error rather than panicking on bad substringAspen Smith1-0/+9
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>
2023-11-25 r/7064 fix(tvix): ensure PartialOrd/Ord agree for StorePath & NixStringVincent Ambo1-1/+1
This fixes a *future* clippy lint: https://rust-lang.github.io/rust-clippy/master/index.html#/incorrect_partial_ord_impl_on_ord_type In essence, because the implementation of *both* Ord and PartialOrd implies that ordering is not partial, all results of PartialOrd should simply be those of Ord. This is to avoid subtle bugs in future refactorings. Change-Id: I8fc6694010208752dd47746a2aaaeca0c788d574 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10109 Tested-by: BuildkiteCI Reviewed-by: flokli <flokli@flokli.de>
2023-11-25 r/7055 refactor(tvix/eval): add ThunkRepr::is_forced()Adam Joseph1-5/+9
Change-Id: I4eab5c81fb82337da06327248845cd2f3a4490d3 Reviewed-on: https://cl.tvl.fyi/c/depot/+/10038 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI Autosubmit: Adam Joseph <adam@westernsemico.com>
2023-11-25 r/7054 feat(tvix/eval): add Thunk::unwrap_or_clone()Adam Joseph1-1/+34
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
2023-11-05 r/6958 chore(tvix): add missing clippy attributes & configVincent Ambo1-0/+1
For cases where clippy lints don't apply to us, or something is misfiring, add appropriate configuration. Relates to b/321. Change-Id: I0af453910b4a4112bf685b2a8e9a73de10ec87ea Reviewed-on: https://cl.tvl.fyi/c/depot/+/9965 Reviewed-by: flokli <flokli@flokli.de> Autosubmit: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2023-11-05 r/6957 refactor(tvix/eval): use IntoIterator trait for owned NixAttrs iterVincent Ambo1-15/+22
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>
2023-11-05 r/6955 chore(tvix): fix trivial clippy lintsVincent Ambo5-18/+17
Relates to b/321. Change-Id: I37284f89b186e469eb432e2bbedb37aa125a6ad4 Reviewed-on: https://cl.tvl.fyi/c/depot/+/9961 Tested-by: BuildkiteCI Reviewed-by: flokli <flokli@flokli.de> Autosubmit: tazjin <tazjin@tvl.su>
2023-11-03 r/6932 refactor(tvix/eval): delay allocation when comparing attr valuesVincent Ambo1-4/+4
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
2023-11-03 r/6931 chore(tvix/eval): add a marker for sorted borrowed attrs iterationVincent Ambo1-0/+6
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
2023-09-24 r/6650 fix(tvix/eval): fix b/281 by adding Value::CatchableAdam Joseph3-14/+65
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
2023-09-22 r/6624 docs(tvix/eval): fix some broken docstr referencesFlorian Klink2-5/+6
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>
2023-09-15 r/6589 fix(tvix/eval): update identifier quoting to match cppnix 2.17Adam Joseph1-1/+12
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
2023-08-20 r/6509 refactor(tvix/eval): don't use `format!` in `write!` argsFlorian Klink1-1/+1
Change-Id: I0b2fdb418c2e36280d5c551a81634e1742193903 Reviewed-on: https://cl.tvl.fyi/c/depot/+/9105 Tested-by: BuildkiteCI Autosubmit: flokli <flokli@flokli.de> Reviewed-by: raitobezarius <tvl@lahfa.xyz>
2023-08-20 r/6502 refactor(tvix/eval): cargo clippy &GenCoFlorian Klink2-5/+5
Change-Id: I6b1b902ccbc12bf2acdb0fdf406d6ef336ff0b2f Reviewed-on: https://cl.tvl.fyi/c/depot/+/9098 Reviewed-by: raitobezarius <tvl@lahfa.xyz> Tested-by: BuildkiteCI Autosubmit: flokli <flokli@flokli.de>
2023-08-20 r/6501 refactor(tvix/eval): derive default for value::AttrsRep enumFlorian Klink1-7/+2
Change-Id: Iaeb9ed7024c2ce85373f8aec0d223f52e1a3a539 Reviewed-on: https://cl.tvl.fyi/c/depot/+/9097 Autosubmit: flokli <flokli@flokli.de> Reviewed-by: raitobezarius <tvl@lahfa.xyz> Tested-by: BuildkiteCI
2023-08-13 r/6482 fix(tvix/eval): fix a comment position in value::jsonVincent Ambo1-2/+2
Change-Id: Ic58653254b36694f1991a18db9ceea0d532c2e31 Reviewed-on: https://cl.tvl.fyi/c/depot/+/9068 Autosubmit: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI Reviewed-by: flokli <flokli@flokli.de>
2023-07-11 r/6404 fix(tvix/eval): use byte, not codepoint index for slicing in escapesterni1-1/+5
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>
2023-06-20 r/6336 fix(tvix/eval): only finalise formal arguments if defaultingsterni2-4/+15
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>
2023-06-15 r/6308 fix(tvix/eval): make tvix display values like nix-instantiate(1)sterni2-3/+4
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
2023-06-07 r/6243 fix(tvix/eval): type check function argument with set patternsterni1-0/+1
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>
2023-05-26 r/6207 fix(tvix): don't call function eagerly in genList, map & mapAttrssterni1-0/+31
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>
2023-03-22 r/6036 fix(tvix/eval): print unevaluated thunks like Nix doesVincent Ambo1-0/+1
Change-Id: Ie4c563e933f571f45cb4f4efe650d1b65f119e8d Reviewed-on: https://cl.tvl.fyi/c/depot/+/8324 Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org> Autosubmit: tazjin <tazjin@tvl.su>
2023-03-17 r/6026 feat(tvix/eval): report all known spans on infinite recursionVincent Ambo1-4/+38
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>
2023-03-17 r/6025 feat(tvix/eval): track span of first force in a thunk blackholeVincent Ambo2-8/+11
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>
2023-03-13 r/5991 chore(tvix/eval): mark async functions which are called by the VMAdam Joseph2-0/+6
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/D99517 https://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>
2023-03-13 r/5989 feat(tvix/eval): rewrite nix_cmp_ordering to be nonrecursiveAdam Joseph1-45/+46
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>
2023-03-13 r/5979 fix(tvix/eval): implement cppnix JSON-serialisation semanticsVincent Ambo5-38/+100
This drops the usage of serde::Serialize, as the trait can not be used to implement the correct semantics (function colouring!). Instead, a manual JSON serialisation function is written which correctly handles toString, outPath and other similar weirdnesses. Unexpectedly, the eval-okay-tojson test from the C++ Nix test suite now passes, too. This fixes an issue where serialising data structures containing derivations to JSON would fail. Change-Id: I5c39e3d8356ee93a07eda481410f88610f6dd9f8 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8209 Reviewed-by: raitobezarius <tvl@lahfa.xyz> Tested-by: BuildkiteCI
2023-03-13 r/5978 feat(tvix/eval): give generators human-readable namesVincent Ambo1-2/+2
This adds static strings to generator frames that describe the generator in a human-readable fashion, which are then logged in observers. This makes runtime traces very precise, explaining exactly what is being requested from where. Change-Id: I695659a6bd0b7b0bdee75bc8049651f62b150e0c Reviewed-on: https://cl.tvl.fyi/c/depot/+/8206 Tested-by: BuildkiteCI Reviewed-by: raitobezarius <tvl@lahfa.xyz>
generated by cgit-pink 1.4.1 (git 2.44.2) at 2024-11-16T05·27+0000