about summary refs log tree commit diff
path: root/tvix/eval/src/value/mod.rs
AgeCommit message (Collapse)AuthorFilesLines
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 Joseph1-83/+103
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 Joseph1-56/+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/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 Joseph1-127/+142
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-06 r/7120 feat(tvix/eval): rewrite Thunk::force() in nonrecursive formAdam Joseph1-2/+2
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-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/6955 chore(tvix): fix trivial clippy lintsVincent Ambo1-7/+6
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-09-24 r/6650 fix(tvix/eval): fix b/281 by adding Value::CatchableAdam Joseph1-7/+50
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-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-06-20 r/6336 fix(tvix/eval): only finalise formal arguments if defaultingsterni1-3/+11
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)sterni1-1/+2
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-03-17 r/6025 feat(tvix/eval): track span of first force in a thunk blackholeVincent Ambo1-2/+3
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 Joseph1-0/+5
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 Ambo1-5/+13
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/5975 refactor(tvix/eval): move `__toString` calling to a helper functionVincent Ambo1-24/+8
It turns out that this is used not just in coerceToString, but also in toJSON. Change-Id: I1c324b115a0b8bb6d83446d5bf70453c9b90685e Reviewed-on: https://cl.tvl.fyi/c/depot/+/8203 Tested-by: BuildkiteCI Reviewed-by: raitobezarius <tvl@lahfa.xyz>
2023-03-13 r/5969 refactor(tvix/eval): box PathBufVincent Ambo1-4/+4
This shaves another 8 bytes off Value. How did that type get so big?! Change-Id: I65e9b59a1636bd57e3cc4aec5fea16887070b832 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8153 Reviewed-by: raitobezarius <tvl@lahfa.xyz> Tested-by: BuildkiteCI
2023-03-13 r/5964 refactor(tvix/eval): flatten call stack of VM using generatorsVincent Ambo1-247/+154
Warning: This is probably the biggest refactor in tvix-eval history, so far. This replaces all instances of trampolines and recursion during evaluation of the VM loop with generators. A generator is an asynchronous function that can be suspended to yield a message (in our case, vm::generators::GeneratorRequest) and receive a response (vm::generators::GeneratorResponsee). The `genawaiter` crate provides an interpreter for generators that can drive their execution and lets us move control flow between the VM and suspended generators. To do this, massive changes have occured basically everywhere in the code. On a high-level: 1. The VM is now organised around a frame stack. A frame is either a call frame (execution of Tvix bytecode) or a generator frame (a running or suspended generator). The VM has an outer loop that pops a frame off the frame stack, and then enters an inner loop either driving the execution of the bytecode or the execution of a generator. Both types of frames have several branches that can result in the frame re-enqueuing itself, and enqueuing some other work (in the form of a different frame) on top of itself. The VM will eventually resume the frame when everything "above" it has been suspended. In this way, the VM's new frame stack takes over much of the work that was previously achieved by recursion. 2. All methods previously taking a VM have been refactored into async functions that instead emit/receive generator messages for communication with the VM. Notably, this includes *all* builtins. This has had some other effects: - Some test have been removed or commented out, either because they tested code that was mostly already dead (nix_eq) or because they now require generator scaffolding which we do not have in place for tests (yet). - Because generator functions are technically async (though no async IO is involved), we lose the ability to use much of the Rust standard library e.g. in builtins. This has led to many algorithms being unrolled into iterative versions instead of iterator combinations, and things like sorting had to be implemented from scratch. - Many call sites that previously saw a `Result<..., ErrorKind>` bubble up now only see the result value, as the error handling is encapsulated within the generator loop. This reduces number of places inside of builtin implementations where error context can be attached to calls that can fail. Currently what we gain in this tradeoff is significantly more detailed span information (which we still need to bubble up, this commit does not change the error display). We'll need to do some analysis later of how useful the errors turn out to be and potentially introduce some methods for attaching context to a generator frame again. This change is very difficult to do in stages, as it is very much an "all or nothing" change that affects huge parts of the codebase. I've tried to isolate changes that can be isolated into the parent CLs of this one, but this change is still quite difficult to wrap one's mind and I'm available to discuss it and explain things to any reviewer. Fixes: b/238, b/237, b/251 and potentially others. Change-Id: I39244163ff5bbecd169fe7b274df19262b515699 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8104 Reviewed-by: raitobezarius <tvl@lahfa.xyz> Reviewed-by: Adam Joseph <adam@westernsemico.com> Tested-by: BuildkiteCI
2023-03-13 r/5963 feat(tvix/eval): implement generator-based Nix equality logicVincent Ambo1-1/+181
Adds a `Value::neo_nix_eq` method (the `neo_` prefix will be dropped when we flip over to the generator implementation of the VM) which implements Nix equality semantics using async, generator-based comparisons. Instead of tracking the "kind" of equality that is being compared (see the pointer-equality doc) through a pair of booleans, I've introduced an enum that explicitly lists the possible comparisons. Change-Id: I3354cc1470eeccb3000a5ae24f2418db1a7a2edc Reviewed-on: https://cl.tvl.fyi/c/depot/+/8241 Tested-by: BuildkiteCI Reviewed-by: Adam Joseph <adam@westernsemico.com>
2023-03-07 r/5899 feat(tvix/eval): introduce generators moduleVincent Ambo1-1/+1
This module contains the request/response types for generators requesting actions from the VM. For most of these, an async helper function is added that will be used inside of generator functions to make use of these requests/responses instead of constructing them directly. Change-Id: I1e085f88adaf784a34867957a0e82532d3a83d7c Reviewed-on: https://cl.tvl.fyi/c/depot/+/8148 Tested-by: BuildkiteCI Reviewed-by: raitobezarius <tvl@lahfa.xyz>
2023-03-03 r/5870 chore(tvix/eval): fix clippy warningsVincent Ambo1-4/+4
Change-Id: I4c02f0104c455ac00a3f299c1fbf75cbb08e8972 Reviewed-on: https://cl.tvl.fyi/c/depot/+/8142 Reviewed-by: flokli <flokli@flokli.de> Tested-by: BuildkiteCI Autosubmit: tazjin <tazjin@tvl.su>
2023-02-13 r/5849 chore(tvix/eval): clippy warn is length zeroAaqa Ishtyaq1-1/+1
This CL address clippy warning about finding the zero length of something using `is_empty()` instead of `len() == 0`. Change-Id: I2b36c7c7b65b733609fc0dcd33be06f9d772bc9b Signed-off-by: Aaqa Ishtyaq <aaqaishtyaq@gmail.com> Reviewed-on: https://cl.tvl.fyi/c/depot/+/8029 Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su>
2023-02-02 r/5821 chore(tvix/eval): elaborate on internal types in Value::type_ofVincent Ambo1-6/+8
This aids in debugging quite substantially. Change-Id: Ic43232aa6165ae1c3db7ac2701938e1dfeeb418c Reviewed-on: https://cl.tvl.fyi/c/depot/+/8013 Tested-by: BuildkiteCI Reviewed-by: flokli <flokli@flokli.de> Autosubmit: tazjin <tazjin@tvl.su>
2023-01-25 r/5757 test(tvix/eval): add test for total_fmt_floatFlorian Klink1-0/+28
Change-Id: If6c478ee3d2e4ecf5ef92289614f86535ad05cb7 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7927 Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su> Autosubmit: flokli <flokli@flokli.de>
2023-01-25 r/5756 refactor(tvix/eval): extract float formatting into a helperVincent Ambo1-71/+75
This keeps the actual TotalDisplay implementation readable, as this float formatting code suddenly made up the majority of its implementation. Change-Id: I2c0d00e4a691e0b8ffbc72680f680e16feef4bee Reviewed-on: https://cl.tvl.fyi/c/depot/+/7925 Reviewed-by: flokli <flokli@flokli.de> Autosubmit: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2023-01-25 r/5753 feat(tvix/eval): use lexical-core to format floatFlorian Klink1-2/+86
Apparently our naive implementation of float formatting, which simply used {:.5}, and trimmed trailing "0" strings not sufficient. It wrongly trimmed numbers with zeroes but no decimal point, like `10000` got trimmed to `1`. Nix uses `std::to_string` on the double, which according to https://en.cppreference.com/w/cpp/string/basic_string/to_string is equivalent to `std::sprintf(buf, "%f", value)`. https://en.cppreference.com/w/cpp/io/c/fprintf mentions this is treated like this: > Precision specifies the exact number of digits to appear after > the decimal point character. The default precision is 6. In the > alternative implementation decimal point character is written even if > no digits follow it. For infinity and not-a-number conversion style > see notes. This doesn't seem to be the case though, and Nix uses scientific notation in some cases. There's a whole bunch of strategies to determine which is a more compact notation, and which notation should be used for a given number. https://github.com/rust-lang/rust/issues/24556 provides some pointers into various rabbit holes for those interested. This gist seems to be that currently a different formatting is not exposed in rust directly, at least not for public consumption. There is the [lexical-core](https://github.com/Alexhuszagh/rust-lexical) crate though, which provides a way to format floats with various strategies and formats. Change our implementation of `TotalDisplay` for the `Value::Float` case to use that. We still need to do some post-processing, because Nix always adds the sign in scientific notation (and there's no way to configure lexical-core to do that), and lexical-core in some cases keeps the trailing zeros. Even with all that in place, there as a difference in `eval-okay- fromjson.nix` (from tvix-tests), which I couldn't get to work. I updated the fixture to a less problematic number. With this, the testsuite passes again, and does for the upcoming CL introducing builtins.fromTOML, and enabling the nix testsuite bits for it, too. Change-Id: Ie6fba5619e1d9fd7ce669a51594658b029057acc Reviewed-on: https://cl.tvl.fyi/c/depot/+/7922 Tested-by: BuildkiteCI Autosubmit: flokli <flokli@flokli.de> Reviewed-by: tazjin <tazjin@tvl.su>
2023-01-20 r/5715 refactor(tvix/eval): keep globals alive through VM structVincent Ambo1-4/+24
This forces users to pass the fully constructed set of globals to the VM, making it harder to accidentally "lose" the set while weak references to it still exist. This doesn't modify any functionality, but is laying the foundation for simplifying some of the builtins behaviour that has grown more complex again. Change-Id: I5120f97861c65dc46d90b8a4e2c92ad32cc53e03 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7877 Autosubmit: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI Reviewed-by: flokli <flokli@flokli.de>
2023-01-17 r/5670 refactor(tvix/value): use proptest strategies from imbl crateVincent Ambo1-6/+0
Instead of going through Vec/BTreeMap for generating our internal types, use the proptest strategies from imbl. The one thing I couldn't figure out in the previous implementation is where the ranges/sizes of generated collections came from. The strategies in proptest use different types (Range, with an unknown default value, and SizeRange with 0..100). I've opted to specify 0..100 directly, but we can probably make it configurable. Change-Id: I749bc4c703fe424099240cab822b1642e5216361 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7791 Autosubmit: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI Reviewed-by: flokli <flokli@flokli.de>
2023-01-16 r/5663 chore(tvix/eval): add other required items to public APIVincent Ambo1-2/+2
External implementors of builtins must be able to force values, which necessitates publishing a bunch more items from the crate. Change-Id: I8f6b8ae88156aae417dbe630a698d123d0c1c8d4 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7830 Tested-by: BuildkiteCI Reviewed-by: flokli <flokli@flokli.de>
2023-01-12 r/5652 feat(tvix/eval): implement builtins.toJSONVincent Ambo1-3/+4
Implements `Serialize` for `tvix_eval::Value`. Special care is taken with serialisation of attribute sets, and forcing of thunks. The tests should cover both cases well. Change-Id: I9bb135bacf6f87bc6bd0bd88cef0a42308e6c335 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7803 Reviewed-by: flokli <flokli@flokli.de> Tested-by: BuildkiteCI Autosubmit: tazjin <tazjin@tvl.su>
2023-01-10 r/5640 feat(tvix/eval): implement serde::Deserialize for ValueRyan Lahfa1-42/+17
Co-Authored-By: Vincent Ambo <tazjin@tvl.su> Change-Id: Ib6f7d1f4f4faac36b44f5f75cccc57bf912cf606 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7626 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2023-01-08 r/5629 fix(tvix/eval): fix last uses of Vec<Value> -> NixList in builtinsVincent Ambo1-11/+6
Change-Id: I0d71b82eb7ddc1e457b0996b0668006f55f56751 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7790 Autosubmit: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI Reviewed-by: sterni <sternenseemann@systemli.org>
2022-12-29 r/5542 refactor(tvix/eval): remove extra Rc<..> around Value::AttrsVincent Ambo1-3/+3
The `im::OrdMap` is already small and cheap to copy while sharing memory, so this is not required anymore. Only the `KV` variant may have slightly larger content, but in practice this doesn't seem to make a difference when comparing the two variants and this one is less complicated. Change-Id: I64a563b209a2444125653777551373cb2989ca7d Reviewed-on: https://cl.tvl.fyi/c/depot/+/7677 Reviewed-by: sterni <sternenseemann@systemli.org> Tested-by: BuildkiteCI
2022-12-29 r/5541 refactor(tvix/eval): persistent, memory-sharing OrdMap for NixAttrsVincent Ambo1-1/+1
This uses the `im::OrdMap` for `NixAttrs` to enable sharing of memory between different iterations of a map. This slightly speeds up eval, but not significantly. Future work might include benchmarking whether using a `HashMap` and only ordering in cases where order is actually required would help. This switches to a fork of `im` that fixes some bugs with its OrdMap implementation. Change-Id: I2f6a5ff471b6d508c1e8a98b13f889f49c0d9537 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7676 Reviewed-by: sterni <sternenseemann@systemli.org> Tested-by: BuildkiteCI
2022-12-29 r/5540 refactor(tvix/eval): use im::Vector directly where possibleVincent Ambo1-3/+4
The conversion from im::Vector -> Vec is cheaper for NixList construction (of course), so where possible we should make use of that. This updates most builtins dealing with lists to use Vector directly, and marks the function constructing NixList from Vec as deprecated so that we get appropriate warnings in places where it's still in use. These places are currently inside of JSON serialisation logic which is in flux right now, so lets leave them as-is until it's stabilised. Change-Id: I037f12a2800f2576db4d9526bd935efd079163f0 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7671 Reviewed-by: sterni <sternenseemann@systemli.org> Tested-by: BuildkiteCI
2022-12-25 r/5486 fix(tvix/eval): fix current clippy warningsVincent Ambo1-2/+2
It's been a while since the last time, so quite a lot of stuff has accumulated here. Change-Id: I0762827c197b30a917ff470fd8ae8f220f6ba247 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7597 Reviewed-by: grfn <grfn@gws.fyi> Autosubmit: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
2022-12-25 r/5484 feat(tvix/eval): implement From<f64> for ValueRyan Lahfa1-0/+6
Change-Id: I287282a195d6f752260242739332b2357791974a Reviewed-on: https://cl.tvl.fyi/c/depot/+/7625 Tested-by: BuildkiteCI Reviewed-by: tazjin <tazjin@tvl.su>
2022-12-22 r/5477 feat(tvix/eval): display function names in documentationVincent Ambo1-1/+8
... if they are known. We currently do not propagate names correctly for curried functions. Change-Id: I19d57fb30a5c0000ccdf690b91076f6b2191de23 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7596 Tested-by: BuildkiteCI Reviewed-by: grfn <grfn@gws.fyi>
2022-12-22 r/5475 feat(tvix/eval): add Value::explain methodVincent Ambo1-0/+33
This value creates a human-readable explanation of a value. This can be used to implement documentation related functionality. For some values, the amount of information displayed can be expanded quite a bit. Change-Id: Ie8c400feae909e7680af163596f99060262e4241 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7592 Reviewed-by: grfn <grfn@gws.fyi> Tested-by: BuildkiteCI
2022-12-21 r/5467 feat(tvix/eval): use `EvalIO::import_path` when coercing pathsVincent Ambo1-1/+2
This "ties the knot" of importing files into a store when referring to them through path literals, e.g. inside of strings. I'm not yet sure if this interface is sufficient for builtins.path (which we haven't implemented at all yet), but it's enough to wire up eval & store initially. In the default implementations nothing interesting happens in this function at all. Change-Id: Ie01ff4161617d1e743a68dbd1a5e54c1b40c0990 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7582 Reviewed-by: grfn <grfn@gws.fyi> Tested-by: BuildkiteCI
2022-12-21 r/5459 feat(tvix/eval): add EvalIO to public crate APIVincent Ambo1-4/+4
This lets users set the `io_handle` field on an `Evaluation`, which is then propagated to the VM. Change-Id: I616d7140724fb2b4db47c2ebf95451d5303a487a Reviewed-on: https://cl.tvl.fyi/c/depot/+/7566 Reviewed-by: grfn <grfn@gws.fyi> Tested-by: BuildkiteCI
2022-12-21 r/5452 feat(tvix/eval): wrap Closure in Rc<> to match cppnix semanticsAdam Joseph1-2/+8
Change-Id: I595087eff943d38a9fc78a83d37e207bb2ab79bc Reviewed-on: https://cl.tvl.fyi/c/depot/+/7443 Reviewed-by: grfn <grfn@gws.fyi> Tested-by: BuildkiteCI
2022-12-03 r/5379 feat(tvix/eval): Continue removing leakage of BTreeMap.Lyle Mantooth1-2/+3
Fixes b/212. Based on feedback in https://cl.tvl.fyi/c/depot/+/7492, all uses of `NixAttrs::from_map` have been removed. Only `from_iter` and `from_kv` remain. Change-Id: I52e25f73018c2aa1843197427516b7a852503e2c Reviewed-on: https://cl.tvl.fyi/c/depot/+/7500 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI Autosubmit: IslandUsurper <lyle@menteeth.us>
2022-11-28 r/5349 feat(tvix/eval): add CoercionKind::ThunksOnlyAdam Joseph1-4/+11
Signed-off-by: Adam Joseph <adam@westernsemico.com> Change-Id: I92acb7e6099a4796d953b2d4d02cca4076ed0fb1 Reviewed-on: https://cl.tvl.fyi/c/depot/+/7426 Reviewed-by: tazjin <tazjin@tvl.su> Tested-by: BuildkiteCI
generated by cgit-pink 1.4.1 (git 2.44.2) at 2024-12-19T00·38+0000