C++ Nix forces and typechecks the passed argument even if it is not
necessary in order to compute the return value of the function. I
discovered this when I thought our formals miscompilation might be that
we are too strict, but doesn't look like it in this case.
Change-Id: Ifb3c92592293052c489d1e3ae8c7c54e4b6b4dc6
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8701
Tested-by: BuildkiteCI
Autosubmit: sterni <sternenseemann@systemli.org>
Reviewed-by: tazjin <tazjin@tvl.su>
nix_oracle.rs now gives us the possibility to check this by stuffing the
expressions in a list. In fact, the incorrect behavior fixed in
- cl/8656
- cl/8655
- cl/8662
was discovered using this test suite.
Change-Id: Id0ab01ee6be0b875791214e0a72a2ac941c46c96
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8658
Autosubmit: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
This will be useful for comparing thunking behavior to C++ Nix. I
considered adding this capability to the tvix_tests/nix_tests
infrastructure, but as it would require changing the test file naming
scheme to do it in a clean way, I've postponed it–it's nice that our
tests are compatible with C++ Nix's test suite.
Change-Id: I60bcdd98ed25140e716f0858f8dc28f21ab957aa
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8657
Autosubmit: sterni <sternenseemann@systemli.org>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
HasAttrs was weird because with longer attribute paths it would
sometimes not turn out to be a thunk. If it was a thunk, it'd usually
still do some eval strictly which we'll want to avoid.
Verified against C++ Nix using a new test suite introduced in a later
CL.
Change-Id: I6d047ccc68d046bb268462f170a3c4f3c5ddeffe
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8656
Autosubmit: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Probably no real world code broken by this overzealous evaluation, but
let's be thorough!
Change-Id: Ib405a677182eab7940ace940c68e107573473a54
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8655
Reviewed-by: tazjin <tazjin@tvl.su>
Autosubmit: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
Unary operator applications are thunked which can easily be observed by
nix-instantiate --eval -E '[ (!true) (-1) ]'
Unfortunately, there are few simple expressions where this makes a
difference in the end result. Thus it only cropped up when using nixpkgs
for cross compilation: Here we would compile the expression
!(stdenv.cc.isGNU or false)
to assemble python3Minimal's passthru attribute set (at least this seems
to be the most likely explanation from the backtraces I've studied).
This means that an unthunked
<stdenv.cc.isGNU or false>
OpForce
OpInvert
would be performed in order to assemble this attribute set, causing
stdenv.cc to be evaluated too early, causing an infinite recursion.
Resolves b/273.
It seems that having a test suite that doesn't use --strict and relies
on thunks rendered as <CODE> would be beneficial for catching such
issues. I've not been able to find a test case with --strict that
demonstrates the problem fixed in this CL.
Change-Id: I640a5827b963f5b9d0f86fa2142e75e3a6bbee78
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8654
Tested-by: BuildkiteCI
Autosubmit: sterni <sternenseemann@systemli.org>
Reviewed-by: tazjin <tazjin@tvl.su>
mapAttrs, map and genList call Nix functions provided by the caller and
store the result of applying them in a Nix data structure that does not
force all of its contents when forced itself. This means that when such
a builtin application is forced, the Nix function calls performed by the
builtin should not be forced: They may be forced later, but it is also
possible that they will never be forced, e.g. in
builtins.length (builtins.map (builtins.add 2) [ 1 2 3 ])
it is not necessary to compute a single application of builtins.add.
Since request_call_with immediately performs the function call
requested, Tvix would compute function applications unnecessarily before
this change. Because this was not followed by a request_force, the
impact of this was relatively low in Nix code (most functions return a
new thunk after being applied), but it was enough to cause a lot of
bogus builtins.trace applications when evaluating anything from
`lib.modules`. The newly added test includes many cases where Tvix
previously incorrectly applied a builtin, breaking a working expression.
To fix this we add a new helper to construct a Thunk performing a
function application at runtime from a function and argument given as
`Value`s. This mimics the compiler's compile_apply(), but does itself
not require a compiler, since the necessary Lambda can be constructed
independently.
I also looked into other builtins that call a Nix function to verify
that they don't exhibit such a problem:
- Many builtins immediately use the resulting value in a way that makes
it necessary to compute all the function calls they do as soon as
the outer builtin application is forced:
* all
* any
* filter
* groupBy
* partition
- concatMap needs to (shallowly) force the returned list for
concatenation.
- foldl' is strict in the application of `op` (I added a comment that
makes this explicit).
- genericClosure needs to (shallowly) force the resulting list and some
keys of the attribute sets inside.
Resolves b/272.
Change-Id: I1fa53f744bcffc035da84c1f97ed25d146830446
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8651
Autosubmit: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
This feature allows the compiler to detect situations where the
created thunk is useless and can be merged into the parent chunk
instead.
The only case where the compiler does this initially is when
statically optimising a select expression.
For example, previously the expression `builtins.length` compiled into
two thunks:
1. An "inner" thunk which contained an `OpConstant` that had the
optimised `length` builtin in it.
2. An "outer" thunk which contained an `OpConstant` to access the
inner thunk, and the trailing OpForce of the top-level program.
With this change, the inner thunk is skipped completely and the outer
chunk directly contains the `length` builtin access.
This can be applied in several situations, some easier than others,
and we will add them in as we go along.
Change-Id: Ie44521445fce1199f99b5b17712833faea9bc357
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7959
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
This method extends the contents of one chunk with that of another,
effectively merging the thunks together.
This will be used for the upcoming "unthunking" functionality.
Change-Id: I6ad74232cd7f3eca198ed921e455205e00d76e6b
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7958
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
These two places didn't add the path from the context to the
ErrorKind, but simply relied on the
impl From<std::io::Error> for tvix_eval::ErrorKind, which doesn't add
the path.
Change-Id: Ifc7dbbe305d24242b0705de1dea34e8923e9d2cb
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8603
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Autosubmit: flokli <flokli@flokli.de>
We didn't return anything useful other than ErrorKind::IO anyways.
We can use io::ErrorKind::Unsupported for DummyIO.
Fixes b/271.
Change-Id: Icb231e9b38168e8b6fa473bfa405d160357b317f
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8602
Autosubmit: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
It's okay if these calls mutate some internal state inside an
implementation.
Change-Id: I12bb11bde0310778c3da1275696bf7de058863a3
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8571
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
This switches out the previous compressed representation (count of
instructions per span) with a representation where the chunk's span
list stores the index of the first operation that belongs to a span,
and finds the right span by using a binary search when looking them
up.
This improves the lookup complexity from O(n) to O(log n).
This improvement was suggested and (mostly) implemented by GPT-4. I
only fixed up some names and updated the logic for deleting
spans (which it only did not do because I didn't tell it about that).
The code was verified by producing a complex error before/after the
change and ensuring that all spans in the error match exactly.
Co-Authored-By: GPT-4
Change-Id: Ibfa12cc6973af1c9b0ae55bb464d1975209771f5
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8385
Reviewed-by: ezemtsov <eugene.zemtsov@gmail.com>
Tested-by: BuildkiteCI
Autosubmit: tazjin <tazjin@tvl.su>
This grows the frame stack as the call stack grows, which yields *much*
better user-facing error messages.
I haven't measured the performance impact this has yet, for now I'm
still just trying to add more information to errors and then cut down
again where necessary.
Change-Id: I89f058ef31979edacf4667775d460b60704ce4d7
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8334
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Autosubmit: tazjin <tazjin@tvl.su>
This makes it possible for callers to control whether they can receive
partially evaluated values from an evaluation or not.
We're actually flipping the default behaviour to non-strict top-level
evaluation, which means that callers have to set `strict = true` on
the Evaluation to get the previous behaviour.
Change-Id: Ic048e9ba09c88866d4c3177d5fa07db11c4eb20e
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8325
Autosubmit: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
Emits the span of the `set` that is being accessed in the `force`
operation of an attribute access.
Looking at traces, it's a lot more useful to get information about
*what* is being forced, as in cases like `foo.bar` it can be
misleading to have an error highlight `bar`, when the error occured
while forcing `foo` to be able to access `bar` in the first place.
Change-Id: Id46ff28f20c67cb4971727ac52cc4811795cea2d
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8272
Reviewed-by: flokli <flokli@flokli.de>
Autosubmit: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
This reports the span
1. of the code within a thunk,
2. of the place where the thunk was instantiated,
3. of the place where the thunk was first forced,
4. of the place where the thunk was forced again,
when yielding an infinite recursion error, which hopefully makes it
easier to debug them.
The spans are tracked in the ThunkRepr::Blackhole variant when putting
a thunk under evaluation.
Note that we currently have some loss of span precision in the VM loop
when switching between frame types, so spans 3/4 are currently a bit
wonky. Working on it.
Change-Id: Icbd2a9df903d00e8c2545b3fc46dcd2a9e3e3e55
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8270
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Autosubmit: tazjin <tazjin@tvl.su>
This is step 1 towards being able to use all 4 spans that we know when
dealing with infinite recursion. It tracks the span at which the
force of a thunk was first requested when constructing a blackhole, so
that we can highlight the spans of the first and second forces.
These are actually the least relevant spans, but the easiest to put in
place, more coming soon.
Change-Id: I4c7e82f6211b98756439d4148a4191457cc46807
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8269
Autosubmit: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
This produces traces in which we can see what kind of native code was
run. Note that these "names" are named after the generator message, so
these aren't *really* intended for end-user consumption, but we can
give them saner names later.
Example:
https://gist.github.com/tazjin/82b24e92ace8e821008954867ee05057
This already makes the traces a little easier to parse.
Change-Id: Idcd601baf84f492211b732ea0f04b377112e10d0
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8268
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Autosubmit: tazjin <tazjin@tvl.su>
When emitting an error at runtime, the VM will now use the new
`NativeError` and `BytecodeError` error kinds (which just wrap inner
errors) to create a set of diagnostics to emit.
The primary diagnostic is emitted last, with `error` type (so it will
be coloured red in terminals), the other ones will be emitted with
`note` type, highlighting the causal chain.
Example:
https://gist.github.com/tazjin/25feba7d211702453c9ebd5f8fd378e4
This is currently quite verbose, and we can cut down on this further,
but the purpose of this commit is to surface more information first of
all before worrying about the exact display.
Change-Id: I058104a178c37031c0db6b4b3e4f4170cf76087d
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8266
Autosubmit: tazjin <tazjin@tvl.su>
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
This actually uses coercion under the hood in C++ Nix. See the test
for an example.
Change-Id: Id56b364acf269225b6829d0b600e0222f8b3608d
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8322
Reviewed-by: andi <andi@notmuch.email>
Tested-by: BuildkiteCI
This was commented out and forgotten during the generator refactor, oh
well.
Change-Id: I474b685159a955a846db462da0dd0067af177b04
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8321
Autosubmit: tazjin <tazjin@tvl.su>
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Change-Id: I8224bf039f739c401900b5a2ddc839810c87cf6e
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8226
Tested-by: BuildkiteCI
Reviewed-by: Adam Joseph <adam@westernsemico.com>
We settled on this being the most reasonable name for this construct.
Change-Id: Ic31c45461a842f22aa05f4446123fe3a61dfdbc0
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8291
Tested-by: BuildkiteCI
Autosubmit: tazjin <tazjin@tvl.su>
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Given Rust's current lack of support for tail calls, we cannot avoid
using `async` for builtins. This is the only way to avoid
overflowing the cpu stack when we have arbitrarily deep
builtin/interpreted/builtin/interpreted/... "sandwiches"
There are only five `async fn` functions which are not builtins
(some come in multiple "flavors"):
- add_values
- resolve_with
- force, final_deep_force
- nix_eq, nix_cmp_eq
- coerce_to_string
These can be written iteratively rather than recursively (and in
fact nix_eq used to be written that way!). I volunteer to rewrite
them. If written iteratively they would no longer need to be
`async`.
There are two motivations for limiting our reliance on `async` to
only the situation (builtins) where we have no other choice:
1. Performance.
We don't really have any good measurement of the performance hit
that the Box<dyn Future>s impose on us. Right now all of our
large (nixpkgs-eval) tests are swamped by the cost of other
things (e.g. fork()ing `nix-store`) so we can't really measure
it. Builtins tend to be expensive operations anyways
(regexp-matching, sorting, etc) that are likely to already cost
more than the `async` overhead.
2. Preserving the ability to switch to `musttail` calls.
Clang/LLVM recently got `musttail` (mandatory-elimination tail
calls). Rust has refused to add this mainly because WASM doesn't
support, but WASM `tail_call` has been implemented and was
recently moved to phase 4 (standardization). It is very likely
that Rust will get tail calls sometime in the next year; if it
does, we won't need async anymore. In the meantime, I'd like to
avoid adding any further reliance on `async` in places where it
wouldn't be straightforward to replace it with a tail call.
https://reviews.llvm.org/D99517https://github.com/WebAssembly/proposals/pull/157
https: //github.com/rust-lang/rfcs/issues/2691#issuecomment-1462152908
Change-Id: Id15945d5a92bf52c16d93456e3437f91d93bdc57
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8290
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Autosubmit: Adam Joseph <adam@westernsemico.com>
This commit moves fetch_forced_with and fetch_captured_with into the
scope of their only caller (resolve_with).
Change-Id: I9a8bc27228888729d591e8cb021c431b2b6468f5
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8289
Autosubmit: Adam Joseph <adam@westernsemico.com>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
This rewrites nix_cmp_ordering as an iterative loop, which
eliminates the extra pinned-boxing helper function.
Change-Id: I33d0ecc913e02affd8fd4c7bc1c9ecfdf4c7deb9
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8288
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Autosubmit: Adam Joseph <adam@westernsemico.com>
* We no longer need backtrace-on-stack-overflow, as we no longer
overflow the stack with the recent eval refactorings. This was weird
voodoo anyways, introduced earlier to debug some cases where stack
overflows occured.
* default features of genawaiter crate are not needed, as we don't use
their proc macros
Change-Id: I346fc5a18d7f117ee805909a8be8f535b96be76c
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8263
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
This reorders the operations in the VM's main `match` statement while
evaluating bytecode according to the frequency with which these
operations appear in some nixpkgs evaluations.
I used raw data that looks like this:
https://gist.github.com/tazjin/63d0788a78eb8575b04defaad4ef610d
This has a small but noticeable impact on evaluation performance.
No operations have changed in any way, this is purely moving code
around.
Change-Id: Iaa4ef4f0577e98144e8905fec88149c41e8c315c
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8262
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
The name of this was not accurate anymore after all the recent
shuffling, as noted by amjoseph. Conceptual tail calls here only occur
for Nix bytecode calling Nix bytecode, but things like a builtin call
actually push a new native frame.
Change-Id: I1dea8c9663daf86482b8c7b5a23133254b5ca321
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8256
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
... except now the tests fail, but at least it works
Change-Id: I05e86c173f40533ae65548585c1ddaa200ac5235
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8214
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Tested-by: BuildkiteCI
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
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>
These are serialised as the serialisation of the value of that field.
Change-Id: Ida51708b1f43ce09b0ec835f4e265918aa31dd09
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8205
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Tested-by: BuildkiteCI
These must be serialised to a JSON string of the *result* of coercing
the function application to a string.
Change-Id: Ib7f49ccd950503ddbdbf99643cd59565e26b50da
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8204
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Tested-by: BuildkiteCI
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>
Do not print the entire value (they're likely to be thunks anyways).
This is useful because there *can* be cases where something like
`nixpkgs` itself is sent through one of these messages, in which case
the observer trying to print it will just blow up.
Change-Id: I1fa37ea071d75efa0eb3428c6e2fe4351c62be6b
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8202
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Print only the top 6 values of the stack, not the entire stack.
There's very few operations that deal with more values anyways, so the
rest are not likely to be useful.
This gets us one step closer to tracing VERY large executions without
blowing up.
Change-Id: I97472321b0321b25d534d9f53b3aadfacc2318fa
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8201
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
This can actually blow up when tracing arbitrary execution, as some of
the data structures just get too large to run through a tabwriter.
Change-Id: I6ec4c30ee48655b8a62954ca219107404fb2c256
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8200
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Formals can be initialised with deferred default values (see the test
cases), in which case they need an extra thunk to have something that
can be finalised appropriately when the setup is done.
Fixes: b/255
Change-Id: I380e3770be68eaa83ace96d450c7cead32dacc9f
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8196
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
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
No longer needed, and in some cases caused some extra work.
Change-Id: I64e8e7292573bdc92a9c7a8e470e33f8c526f311
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8152
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Tested-by: BuildkiteCI
Instead of the two different representations (which we don't really
use much), use a `Box<str>` (which potentially shaves another 8 bytes
off `Value`).
NixString values themselves are immutable anyways (which was a
guarantee we already had with `SmolStr`), so this doesn't change
anything else.
Change-Id: I1d8454c056c21ecb0aebc473cfb3ae06cd70dbb6
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8151
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
The size of a `Vector<Value>` is 64 *bytes*, which is quite large, and
it bloated the entire Value type to this size.
This change adds an indirection for the inner vector through Rc.
Initially I tried to use a Box, but this breaks pointer equality
guarantees for the Vector when it is small enough to be inlined.
This reduces the size of Value from 64 to 32 bytes.
Change-Id: Ic3211e861b1966c78b2c3d536ba291fea92647fd
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8150
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Tested-by: BuildkiteCI
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
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>
These functions will be used by the changes in the VM to observe the
runtime execution of generator frames, and provide a more linear view
of the execution of the Tvix VM.
Change-Id: I10b1b1933dedc065e7c61d5d6062f0aaeee0097e
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8240
Tested-by: BuildkiteCI
Reviewed-by: Adam Joseph <adam@westernsemico.com>
In order to implement an asynchronous builtins.sort (required for
moving builtins to generators), we need an `async` sorting algorithm
as our comparators involve invoking a Nix function.
This commit implements a fairly simple, optimised bubble sort as the
sorting algorithm used in our `async fn sort_by`.
There don't seem to be any crates providing async versions of things
like this, and they might actually be pretty hard to implement
generically due to some constraints about how `async` works.
Note that this algorithm is less efficient than the hybrid
"timsort/mergesort/insert sort" used in the Rust standard library. I
tried to write a merge sort implementation, but ran into isuses with
the sort becoming unstable because our comparators can not yield
equality. This is the simplest implementation which I know to be
correct.
Note that as of this commit this is *not* covered by the Tvix test
suite, but it will be as soon as the rest of the generator code lands.
Change-Id: Ia9a604f7dd941d6acc9212c902e0e637ed75bebc
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8239
Reviewed-by: Adam Joseph <adam@westernsemico.com>
Tested-by: BuildkiteCI
We currently send two warnings in case of detecting dead code - W008
inside compile_dead_code, and a more detailed warning in all places that
invoke compile_dead_code:
```
warning[W007]: useless operation on boolean: this expression is always false
--> /nix/store/qz3gjn95gazab4fkb7s8lm6hz17rdzzy-414z9nnj1wy66ymq6vgb693x9xjz6hf2-nixpkgs-src/pkgs/top-level/perl-packages.nix:12079:15
|
12079 | doCheck = false && !stdenv.isDarwin;
| ^^^^^^^^^^^^^^^^^^^^^^^^^
warning[W008]: this code will never be executed
--> /nix/store/qz3gjn95gazab4fkb7s8lm6hz17rdzzy-414z9nnj1wy66ymq6vgb693x9xjz6hf2-nixpkgs-src/pkgs/top-level/perl-packages.nix:12079:24
|
12079 | doCheck = false && !stdenv.isDarwin;
| ^^^^^^^^^^^^^^^^
```
The place invoking `compile_dead_code` has more context to why the code
is unused, so it's error message is much more useful.
Stop emitting the less informative warning inside compile_dead_code
(W008), and update the comment that we expect the caller to emit a
warning.
I kept W008 itself still around, in case we end up having places this
will get used again.
Change-Id: I2c5d84fc0cb4035872cd4b71cc3e9e34e120eb37
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8024
Tested-by: BuildkiteCI
Autosubmit: flokli <flokli@flokli.de>
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
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>
As applies are thunked, there was no situation where OpCall could be
emitted. In practice, all calls were already tail calls.
Change-Id: Id0d441dcdd86f804d7cddd0cc14f589bbfc75e5b
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8147
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Because they do not use it, and it can not be passed with the coming
generator refactoring.
Change-Id: I0d96f2357a7ee79cd8a0f401583d4286230d4a6b
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8146
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Instead of using a suspended native thunk, calculate and optionally
insert the storeDir builtin when the VM is constructed.
We already have the IO handle available at this point and can just
check whether a storeDir is present, and insert its absolute value as
a builtin.
Change-Id: If966eee6ff26dc888b6e888e7c46170c0c346b05
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8145
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Tested-by: BuildkiteCI
This is a ThunkSet wrapped to be shareable, which will be required
once ThunkSets are embedded in futures.
Change-Id: I5a067b7972ac86e4d354c75ef05c86b2284c1137
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8144
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Again simplifying some code down the line, where bits of code that
construct attribute sets already have the final structure available.
Change-Id: I0bb7a1daa63298122b51be73d35d695a4f73f8b0
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8140
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Tested-by: BuildkiteCI
This simplifies some code down the line.
Change-Id: I58dd71e796e11479f44516cf24932f8061843d23
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8139
Tested-by: BuildkiteCI
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
This adds addresses of thunk and closure chunks to the debug output
displayed when dumping bytecode.
This makes it possible to see in the dump which thunks are referenced
by constants in other thunks.
Change-Id: I2c98de5227e7cb415666cd3134c947a56979dc80
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8137
Autosubmit: tazjin <tazjin@tvl.su>
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
This CL removes redundant clone from value which is
going to be dropped without further use.
Change-Id: Ibd2a724853c5cfbf8ca40bf0b3adf0fab89b9be5
Signed-off-by: Aaqa Ishtyaq <aaqaishtyaq@gmail.com>
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8125
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
We want the address that the Rc is pointing to, not the address of the
Rc.
Change-Id: I8eba21677f242bbe4166c74d4aa4269c316076e3
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8045
Reviewed-by: flokli <flokli@flokli.de>
Autosubmit: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
This CL address clippy warning which expects to
use `writeln` instead of `write` for strings with
new line.
Change-Id: Ia72a07502c60cfd489ecf1e3833b9d42d44a8b17
Signed-off-by: Aaqa Ishtyaq <aaqaishtyaq@gmail.com>
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8030
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
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>
This branch was missing, and an assumption elsewhere just executed the
returned (broken) bytecode.
This fixes b/253.
Change-Id: I015023ba921bc08ea03882167f1f560feca25e50
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8090
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
Autosubmit: tazjin <tazjin@tvl.su>
These should be inspectable by callers.
Change-Id: Ia9ef871aa63958d06066aaea61b2aecbd217369b
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8089
Autosubmit: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
Plain paths like `foo/bar.nix` are also allowed, so we can not
determine this based on the prefix.
The upstream PR that is referenced in a comment here has a
significantly different interface than we expected, so I'm not
touching that comment yet in this CL before I've had more time to
digest it.
Change-Id: Iea33bbb35de9c00a7d7fedf64d02253c75c1cc9e
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8032
Autosubmit: tazjin <tazjin@tvl.su>
Reviewed-by: Alyssa Ross <hi@alyssa.is>
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
We only use Rc in `impl EvalIO for StdIO`, which is only included when
building with the "impure" feature.
Change-Id: Id29d647c899cbfcdda11abfb9fabd5aa7e24299f
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8025
Autosubmit: flokli <flokli@flokli.de>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
This reduces the size of `Builtin` from 88 (!) bytes to 8, and as the
largest variant of `Value`, the size of that type from 96 to 64.
The next largest type is NixList, clocking in at 64 bytes.
This has noticeable performance impact. In an implementation without
disk I/O, evaluating nixpkgs.stdenv looks like this:
Benchmark 1: tvix -E '(import <nixpkgs> {}).stdenv.drvPath'
Time (mean ± σ): 1.151 s ± 0.003 s [User: 1.041 s, System: 0.109 s]
Range (min … max): 1.147 s … 1.155 s 10 runs
After this change, it looks like this:
Benchmark 1: tvix -E '(import <nixpkgs> {}).stdenv.drvPath'
Time (mean ± σ): 1.046 s ± 0.004 s [User: 0.954 s, System: 0.092 s]
Range (min … max): 1.041 s … 1.053 s 10 runs
Change-Id: I5ab7cc02a9a450c0227daf1f1f72966358311ebb
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8027
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
When resolving a select expression (`attrs.name` or `attrs.name or
default`), if the set compiles to a constant attribute set (as is most
notably the case with `builtins`) we can backtrack and replace that
attribute set directly with the compiled value.
For something like `builtins.length`, this will directly emit an
`OpConstant` that leaves the `length` builtin on the stack.
Change-Id: I639654e065a06e8cfcbcacb528c6da7ec9e513ee
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7957
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
This fixes a very complicated bug (b/246). Evaluation
progresses *much* further after this, leading to several less
complicated bugs likely being uncovered by this
What was the problem?
=====================
Previously, when evaluating a thunk, we had a code path that looked
like this:
match *thunk {
ThunkRepr::Evaluated(Value::Thunk(ref inner_thunk)) => {
let inner_repr = inner_thunk.0.borrow().clone();
drop(thunk);
self.0.replace(inner_repr);
}
/* ... */
}
This code path created a copy of the inner `ThunkRepr` of a nested
thunk, and moved that copy into the `ThunkRepr` of the parent.
The effect of this was that the original `ThunkRepr` (unforced!) lived
on in the original thunk, without the memoization of the subsequent
forcing applying to it.
This had the result that Tvix would repeatedly evaluate these thunks
without ever memoizing them, if they occured repeatedly as shared
inner thunks. Most notably, this would *always* occur when
builtins.import was used.
What's the solution?
====================
I have completely rewritten `Thunk::force_trampoline_self` to make all
flows that can occur in it explicit. I have also removed the outer
loop inside of that function, and resorted to more use of trampolining
instead.
The function is now well-commented and it should be possible to read
it from top-to-bottom and get a general sense of what is going on,
though the trampolining itself (which is implemented in the VM) needs
to be at least partially understood for this.
What's the new problem(s)?
==========================
One new (known) problem is that we have to construct `Error` instances
in all error types here, but we do not have spans available in some
thunk-related situations. Due to b/238 we cannot ask the VM for an
arbitrary span from the callsite leading to the force. This means that
there are now code paths where, under certain conditions, causing an
evaluation error during thunk forcing will panic.
To fix this we will need to investigate and fix b/238, and/or add a
span tracking mechanism to thunks themselves.
What other impacts does this have?
==================================
With this commit, eval of nixpkgs mostly succeeds (things like stdenv
evaluate to the same hashes for us and C++ Nix, meaning we now
construct identical derivations without eval breaking).
Due to this we progress much further into nixpkgs, which lets us
uncover more additional bugs. For example, after this commit we can
quickly see that cl/7949 introduces some kind of behavioural issue and
should not be merged as-is (this was not apparent before).
Additionally, tvix-eval is now seemingly very fast. When doing
performance analysis of a nixpkgs eval, we now mostly see the code
path for shelling out to C++ Nix to add things to the store in there.
We still need those code paths, so we can not (yet) do a performance
analysis beyond that.
Change-Id: I738525bad8bc5ede5d8c737f023b14b8f4160612
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8012
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
... not just a TODO.
Most use-cases of unsafeDiscardStringContext are for cases where a
string is processed in some ways and no longer contains a "physical"
reference, but still has its context attached in C++ Nix.
We don't need to do this. This does diverge in behaviour in use-cases
related to build scheduling, but that whole behaviour will be
different in Tvix.
Change-Id: I4056d4c09f62d44d6bd52b791db03fe5556672b5
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8016
Reviewed-by: flokli <flokli@flokli.de>
Autosubmit: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
... instead of a BTreeMap, as we do not need ordering guarantees here.
HashMaps are noticeably faster here (especially as we've been sorting
essentially random data!).
Change-Id: Ie92d74286df9f763c04c9b226ef1066ee8484c13
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8014
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Autosubmit: tazjin <tazjin@tvl.su>
This adds a fake argument name to builtins.toXML which allows toXML to
serialise any value instead of panicking on functions. We do still
have to fix the value itself, eventually, though.
Change-Id: I2e330ecddcd80442b4fac5eced64431ac86123ba
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7962
Autosubmit: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
Formals can depend on each other when using another formal as a
default value.
This test ensures that the compiler's declaration and initialisation
order of formals is consistent with what actually happens in the VM.
Change-Id: Ibdabe262554e8066d67fac1ebc3b5a48ef626e18
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7948
Reviewed-by: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
While it is in the given example, i.e. for integer addition, to claim
that they are equivalent is a bit misleading: builtins.add is less
overloaded than +, i.e. builtins.add "foo" "bar" will fail whereas
"foo" + "bar" performs string concatenation.
Change-Id: Ib52d530d1ab289b367565b286f06a76dd518d4fb
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7929
Autosubmit: sterni <sternenseemann@systemli.org>
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
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
This allows parsing TOML from Tvix. We can enable the eval-okay-fromTOML
testcase from nix_tests. It uses the `toml` crate, and the serde
integration it brings with it.
Change-Id: Ic6f95aacf2aeb890116629b409752deac49dd655
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7920
Autosubmit: flokli <flokli@flokli.de>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
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>
Two main reasons:
1. Traversing the structure to do this optimisation is
actually *slower* than not optimising it.
2. There are literally hundreds of thousands of incidences of this in
nixpkgs, and with some of the weird code there some of
these (functionally) useless parens are actually required for
readability reasons.
Change-Id: I1044b1c5f9fe20df4b6085851fc3b191277c65dc
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7917
Autosubmit: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
call_value in the VM expects the callable to be forced when calling
it, which was not the case for functors.
Change-Id: Id55a2fe32a9573be42aef8669e268df519a989cd
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7909
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
This makes it possible to inject builtins into the builtin set that
are written in Nix code, and which at runtime are represented by a
thunk that will compile them the first time they are used.
Change-Id: Ia632367328f66fb2f26cb64ae464f8f3dc9c6d30
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7891
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
While moving the CLI out of the evaluator, we forgot to update the
README in //tvix/eval. Move this up to //tvix, so people know where
to start.
Keep the instructions on how to build only `//tvix/eval` in `//tvix/
eval/README.md`.
Change-Id: Ie2755e8b5a0056225dbf3a0ee040f70f7f6a1f27
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7887
Reviewed-by: sterni <sternenseemann@systemli.org>
Autosubmit: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
The codebase contains a lot of complexity and odd roundabout
handling for shadowing globals. I'm pretty sure none of this is
necessary, and all of it disappears if you simply make the globals
part of the ordinary identifier resolution chain, with their own
scope up above the root scope. Then the ordinary shadowing routines
do the right thing, and no special cases or new terminology are
required.
This commit does that.
Note by tazjin: This commit was originally abandoned when Adam decided
not to take away reviewer bandwidth for this at the time (eval was
still in a much earlier stage). As we've recently done some
significant refactoring of globals initialisation this came up again,
and it seems we can easily cover the use-cases of the poison tracking
in other ways now, so I've rebased, updated and resurrected the CL.
Co-Authored-By: Vincent Ambo <tazjin@tvl.su>
Signed-off-by: Adam Joseph <adam@westernsemico.com>
Change-Id: Ib3309a47a7b31fa5bf10466bade0d876b76ae462
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7089
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
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>
This adds a feature to the `#[builtins]` macro which lets users
specify an additional state type to (optionally) thread through to
builtins when constructing them.
This makes it possible for builtins-macro users to pass external state
handles (specifically, in our case, known path tracking) into a set of
builtins.
Change-Id: I3ade20d333fc3ba90a80822cdfa5f87a9cfada75
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7840
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
All invocations of the builtin macro had to previously filter through
the `builtin_tuple` function, but it's more sensible to directly
return these from the macro.
Change-Id: I45600ba84d56c9528d3e92570461c319eea595ce
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7825
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
This makes it possible for users to add additional context to an
error, which will then be rendered as an additional secondary span in
the formatted error output.
We should strive to do this basically anywhere errors are raised that
can occur multiple times, *especially* during type casts. This was
triggered by me debugging a type cast error attached to a fairly
large-ish span (a builtin invocation).
Change-Id: I51be41fabee00cf04de973935daf34fe6424e76f
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7849
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
Instead of having a representation of suspended native thunks that
involves constructing a fake code chunk, make these thunks a
first-class part of the internal thunk representation.
The previous code was not that simple to understand, and actually
contained a critical bug which could lead to Tvix crashes. This
version fixes the particular instance of that bug, but instead
uncovers another (b/238) which can still lead to Tvix crashes.
Fixes: b/237.
Change-Id: I771d03864084d63953bdbb518fec94487481f839
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7750
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
This is unnecessary, Rc already provides all the boxing we need.
Change-Id: I08cf0939c48da43f04c847526c7e5dae5336d528
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7749
Tested-by: BuildkiteCI
Reviewed-by: grfn <grfn@gws.fyi>
Reviewed-by: sterni <sternenseemann@systemli.org>
This is a somewhat terrifying hack that enables us to support
`builtins.builtins`, by running a "fake compilation" inside of a
suspended native thunk that can resolve the weak pointer to the
globals.
With this implementation, the thunk at `builtins.builtins` actually
resolves to the "real" `builtins` (verified with a new test).
This is kind of ugly, and it's something users shouldn't use, but
bubbling a warning out of this is difficult at the moment due to a
little bit of trickery with how the spans in suspended native thunks
work (they don't) (see b/237, b/238)
Change-Id: I67d0e93246dd5b279c960aeda00402031aa12af3
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7748
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
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>
This variant is required for external builtins (which in our case
includes `derivation`) to thread through reasonable error messages.
This has some potential for improvement, but it's an improvement over
the status quo of panicking in the external builtins when no
appropriate error is available.
Change-Id: I7e4bdb0a156c7717092dde30aa4785192182dc66
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7841
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
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>
This CL addresses clippy warning len_without_is_empty
which expects `.is_empty()` method to be present when
implementing `.len()` method for an item.
Change-Id: I8878db630b9ef5853649a906b764a33299bb5dc8
Signed-off-by: Aaqa Ishtyaq <aaqaishtyaq@gmail.com>
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7806
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
This explains my current thinking on string contexts. Thanks to
everyone who gave input so far.
Change-Id: I773219402a79a9d4753b4e7cfbf3a4a751a993a3
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7807
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
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>
This CL address clippy warnings related to use of 'format!' macro
to return unmodified 'String'.
Change-Id: I88726e59d8f39f6a455a8c1f48075b52d167e489
Signed-off-by: Aaqa Ishtyaq <aaqaishtyaq@gmail.com>
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7804
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
This just shuffles the Display implementations around so that
ErrorKind itself is displayable, which is useful in some situations
where errors under construction need to be type-converted.
Change-Id: I7b633d03d0dc34f345c4f20676e0023ecb1db0c4
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7802
Autosubmit: tazjin <tazjin@tvl.su>
Reviewed-by: edef <edef@edef.eu>
Tested-by: BuildkiteCI
This placeholder should not live in the main crate anymore as we will
be injecting the real one from outside of eval, but there are still
language tests that depend on a (simple, mockable) version of it.
Change-Id: I68ea169db15cbdbeed320930d3069e21e376c90d
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7783
Reviewed-by: flokli <flokli@flokli.de>
Autosubmit: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
This is marginally more efficient and has simpler bytecode.
Change-Id: Iad37c9aeef24583e8f696911bcd83d43639f2e36
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7769
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
This adds a mechanism to the compiler to compile an expression without
emitting any code. This allows for detected dead code to still be
compiled to detect errors & warnings inside of it.
Change-Id: Ie78479173570e9c819d8f32ae683ce34234a4c5d
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7767
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
This optimiser can rewrite some expressions into more efficient forms,
and warn users about those cases.
As a proof-of-concept, only some simple boolean comparisons are
supported for now.
Change-Id: I7df561118cfbad281fc99523e859bc66e7a1adcb
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7766
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
This adds a very minimal amount of additional Rc-increments (~1 per
compilation), but makes it a lot easier to add an AST-optimising
compiler pass without incurring a lot of extra cost.
Change-Id: I57208bdfc8882e3ae21c5850e14aa380d3ccea36
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7765
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
This would make it possible to implement something like a linter based
on the tvix-eval compiler warnings.
Change-Id: I1feb4e7c4a44be7d1204b0a962ab522fd32b93c6
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7763
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
There was probably a misunderstanding somewhere about the
with_stack_size being related to how far away it is from the with, but
it is about whether there is a with at all.
This broke a warning (`UselessInherit`), and may actually have let to
more inefficient codegen in some cases.
Change-Id: I08338ea59ae39dad01ca8a4e09d934a936cdea2f
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7762
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
... without them, using the new Builtins API is basically impossible
for library consumers.
Change-Id: Ice0557a2e55e12d812f51bf5a99e6b8c91ad1b91
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7755
Autosubmit: tazjin <tazjin@tvl.su>
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
Code probably rarely relies on these, but it's not hard to support them.
Change-Id: I8499fec34efaf031f9c013bbd370a13db929a2a3
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7772
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Autosubmit: sterni <sternenseemann@systemli.org>
This will eventually force us to have a base builtins set in common with
C++ Nix, i.e. all 2.3 builtins except the controversial
builtins.valueSize.
Change-Id: I2c767f07d6a14711911658e87da9f18ede57a143
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7747
Autosubmit: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Implements externally tagged enum deserialisation. Other serialisation
methods are handled by serde internally using the existing methods.
See the tests for examples.
Change-Id: Ic4a9da3b5a32ddbb5918b1512e70c3ac5ce64f04
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7721
Tested-by: BuildkiteCI
Autosubmit: tazjin <tazjin@tvl.su>
Reviewed-by: flokli <flokli@flokli.de>
With this is_valid_nix_identifier should line up with the upstream lexer
definition:
ID [a-zA-Z\_][a-zA-Z0-9\_\'\-]*
While we're working on this, add a simple test checking the various
formatting rules. Interestingly, it would not be suitable as an identity
test, since you have to write
{ "assert" = null; }
in order to avoid an evaluation error, but C++ Nix is happy to print
this as
{ assert = null; }
– maybe should be considered to be a bug.
Change-Id: I0a4e1ccb5033a80f3767fb8d1c4bba08d303c5d8
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7744
Autosubmit: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Previously the construction of globals (a compiler-only concept) and
builtins (a (now) user-facing API) was intermingled between multiple
different modules, and kind of difficult to understand.
The complexity of this had grown in large part due to the
implementation of `builtins.import`, which required the notorious
"knot-tying" trick using Rc::new_cyclic (see cl/7097) for constructing
the set of globals.
As part of the new `Evaluation` API users should have the ability to
bring their own builtins, and control explicitly whether or not impure
builtins are available (regardless of whether they're compiled in or
not).
To streamline the construction and allow the new API features to work,
this commit restructures things by making these changes:
1. The `tvix_eval::builtins` module is now only responsible for
exporting sets of builtins. It no longer has any knowledge of
whether or not certain sets (e.g. only pure, or pure+impure) are
enabled, and it has no control over which builtins are globally
available (this is now handled in the compiler).
2. The compiler module is now responsible for both constructing the
final attribute set of builtins from the set of builtins supplied
by a user, as well as for populating its globals (that is
identifiers which are available at the top-level scope).
3. The `Evaluation` API now carries a `builtins` field which is
populated with the pure builtins by default, and can be extended by
users.
4. The `import` feature has been moved into the compiler, as a
special case. In general, builtins no longer have the ability to
reference the "fix point" of the globals set.
This should not change any functionality, and in fact preserves minor
differences between Tvix/Nix that we already had (such as
`builtins.builtins` not existing).
Change-Id: Icdf5dd50eb81eb9260d89269d6e08b1e67811a2c
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7738
Reviewed-by: sterni <sternenseemann@systemli.org>
Autosubmit: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
This makes it easier to interface this error with other crates.
Change-Id: I4947ea6097608f8c0427fb94a819ef748d94ea4b
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7711
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
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
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
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
This is a persistent, structurally sharing data structure which is
more efficient in some of our use-cases. I have verified the
efficiency improvement using `hyperfine` repeatedly over expressions
on nixpkgs.
Lists are not the most performance-critical structure in Nix (that
would be attribute sets), but we can already see a small (~5-10%)
improvement.
Note that there are a handful of cases where we still go via `Vec`
that need to be fixed, most notable for `builtins.sort` which can not
currently be implemented directly using `im::Vector` because of a
restrictive type bound.
Change-Id: I237cc50cbd7629a046e5a5e4601fbb40355e551d
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7670
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
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
Introduces continuation-passing-based trampolining of thunk forcing to
avoid recursing when forcing deeply nested expressions.
This is required for evaluating large expressions.
This change was extracted out of cl/7362.
Co-authored-by: Vincent Ambo <tazjin@tvl.su>
Co-authored-by: Griffin Smith <grfn@gws.fyi>
Change-Id: Ifc1747e712663684b2fff53095de62b8459a47f3
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7551
Reviewed-by: grfn <grfn@gws.fyi>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
... 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>
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
This type allows for temporarily compatibility with the C++ Nix store,
specifically (for now) it gives us the store directory used by Nix and
imports files the same way.
Change-Id: I4767794ef2863eba49661315c63c4e17de946d60
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7587
Reviewed-by: grfn <grfn@gws.fyi>
Tested-by: BuildkiteCI
Having a multi-line docstring yields multiple doc-attributes in order,
however we were previously discarding all but the first one.
This reduces them into a single string instead, which can then be
displayed as multi-line documentation.
Change-Id: I1f237956cdea2e4c746d3f13744e0373c1c645a6
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7594
Reviewed-by: grfn <grfn@gws.fyi>
Autosubmit: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
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
Returns the store directory through EvalIO::store_dir.
Note that this is _optional_ in Tvix, as an evaluation can occur in a
context where there simply is no store directory. In those contexts,
`builtins.storeDir` returns `null` in Tvix.
This would only happen in contexts like Tvixbolt (or completely
unrelated use-cases) in practice.
Co-Authored-By: Vincent Ambo <tazjin@tvl.su>
Change-Id: I5a752c7e89b2f75bd7efb082dbfa5b25e3b1ff3b
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7452
Autosubmit: Adam Joseph <adam@westernsemico.com>
Tested-by: BuildkiteCI
Reviewed-by: grfn <grfn@gws.fyi>
This shouldn't be available if we've built a "pure" crate.
Change-Id: I7c85827ee212890252ff7e0b6242e2c52618cba5
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7572
Tested-by: BuildkiteCI
Reviewed-by: grfn <grfn@gws.fyi>
With this change, the behaviour of reading a string from a file path
is controlled by the provided `EvalIO` structure.
This is a huge step towards abstracting away I/O behaviour correctly.
Change-Id: Ifde8e46cd863b16e0301dca45a434ad27560399f
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7567
Tested-by: BuildkiteCI
Reviewed-by: grfn <grfn@gws.fyi>
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
This trait is going to be used to abstract filesystem interactions in
Tvix.
For now, it only contains a `read_to_string` method that closely
mirrors `std::fs::read_to_string`.
As a first step, to see how this works in practice, we will thread
through only this function to the various relevant parts.
Two implementations are provided in tvix-eval itself: A dummy
implementation (which just returns ErrorKind::NotImplemented for all
operations), and a std implementation which delegates to `std`
functions.
Change-Id: Ied3e3bf4bd0e874dd84e166190e3873a0f923ddb
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7565
Reviewed-by: grfn <grfn@gws.fyi>
Tested-by: BuildkiteCI
This type carries the information required for calculating a
span (i.e. the chunk and offset), instead of the span itself. The span
is then only calculated in cases where it is required (when throwing
errors).
This reduces the eval time for
`builtins.length (builtins.attrNames (import <nixpkgs> {}))` by *one
third*!
The data structure in chunks that carries span information reduces
in-memory size by trading off the speed of retrieving span
information. This is because the span information is only actually
required when throwing errors (or emitting warnings).
However, somewhere along the way we grew a dependency on carrying span
information in thunks (for correctly reporting error chains). Hitting
the code paths for span retrieval was expensive, and carrying the
spans in a different way would still be less cache-efficient. This
change is the best tradeoff I could come up with.
Refs: b/229.
Change-Id: I27d4c4b5c5f9be90ac47f2db61941e123a78a77b
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7558
Reviewed-by: grfn <grfn@gws.fyi>
Tested-by: BuildkiteCI
Having thunks which, when forced, execute native Rust code rather
than interpreted opcodes lets us avoid having to bundle
`src/libexpr/primops/derivation.nix` like cppnix does by implementing
it in Rust instead.
Change-Id: If91d77a6736234321eee87ba4b4777eed5a3fe1c
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7450
Reviewed-by: grfn <grfn@gws.fyi>
Tested-by: BuildkiteCI
Instead of finding locals by doing 2x O(n) walks over the compiler's
locals list, use a secondary name-based index for resolving locals by
name.
Previously, almost 60% (!!) of eval time on some expressions over
nixpkgs was spent in `Local::has_name`. This function doesn't even
exist anymore now, and eval speed about doubles as a result.
Note that this doesn't exactly make the locals code easier to read,
but I'm also not sure what we can simplify in there in general.
This fixes b/227.
Change-Id: I29ce5eb9452b02d3b358c673e1f5cf8082e2fef9
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7560
Reviewed-by: grfn <grfn@gws.fyi>
Tested-by: BuildkiteCI
With this change, it should be possible to have both existing
use-cases (CLI & Tvixbolt) use the same API.
Change-Id: I2195264f08cc892177b559a28660dc5f98e48e41
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7545
Autosubmit: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Reviewed-by: grfn <grfn@gws.fyi>
This is required for passing through NIX_PATH from the CLI.
Change-Id: If129df79ef9c3ffab31408adb85679909276c4f0
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7544
Reviewed-by: grfn <grfn@gws.fyi>
Autosubmit: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
This should make no difference in Nix builds, but allows running tests
locally again with `cargo test` for //tvix/eval.
Change-Id: I97d61840143d5c14db61d5862781bf635f9a28e7
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7590
Reviewed-by: grfn <grfn@gws.fyi>
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
In //tvix/eval:
* criterion bumped to 4.0, which at least depends on clap 3.x instead
of 2.x, which is less incompatible
In //tvix/cli:
* no changes required
In //tvix/nix_cli:
* some minor changes for compatibility with clap 4.0, no functionality
changes
Change-Id: If793f64b59fcaa2402d3d483ddbab4092f32df03
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7588
Reviewed-by: grfn <grfn@gws.fyi>
Tested-by: BuildkiteCI
The tvix-eval project is independent from any *uses* of the evaluator,
such as the tvix-repl.
This functionality has been split out into a separate "tvix-cli"
crate. Note that this doesn't have to mean that this CLI crate is the
"final" CLI crate for tvix, the point of this is not "getting the CLI
structure right" but rather "getting the evaluator structure right".
This reshuffling is part of restructuring the way that functionality
like store communication is injected into language evaluation.
Note that at this commit the new CLI crate is not at feature-parity.
Change-Id: Id0af03dc8e07ef09a9f882a89612ad555eca8f93
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7541
Autosubmit: tazjin <tazjin@tvl.su>
Reviewed-by: grfn <grfn@gws.fyi>
Tested-by: BuildkiteCI
This removes internal uses of the previous crate::eval module, which
is being removed.
Change-Id: I5fb3c53460a9c5381853d0258f9ed074ab23c630
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7543
Tested-by: BuildkiteCI
Autosubmit: tazjin <tazjin@tvl.su>
Reviewed-by: grfn <grfn@gws.fyi>
A step towards something more like how I imagine the future public API
for tvix-eval. Please note that this is definitely not the final
version yet, but it's better than the previous API that either exposed
a side-effecting blackbox, or a very low-level "interface".
The basic idea is that an evaluation of some Nix code is requested by
a caller with various parameters, but not all callers are interested
in all of these parameters.
There are also some bits of information that are returned from an
evaluation that are not necessarily relevant to all callers.
To support this somewhat ergonomically, the API is built around an
`Evaluation` struct that is configured by the caller with the various
parameters and then "executed".
Change-Id: I71826f3897126898adc2873d31c44d3eaf5c2be0
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7542
Reviewed-by: grfn <grfn@gws.fyi>
Autosubmit: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Introduces granular dependency builds using crate2nix, bootstrapped
off the generated configuration from the newly introduced
workspace (see cl/7533).
This commit checks in the generated Cargo.nix file which can be
regenerated with a parameterless invocation of `crate2nix generate` in
`//tvix`. I tried generating this in IFD, but it turned out to be
harder than what seemed worthwhile for now.
In this setup, the various build targets for Rust projects end up
being attributes of the imported `Cargo.nix` file at the `tvix.crates`
attribute. These still lack configuration, however, which has been
fixed in the various `default.nix` files of individual projects.
Note that we (temporarily) lose the ability to build tvix-eval's
benchmarks in CI. I haven't figured out what magic incantation summons
them from the void again ...
The `eval-okay-readDir` tests from both test suites have been disabled
because they fail for unknown reasons when run in this new derivation.
Somebody will have to debug it!
Change-Id: I2014614ccb9c8951aedbd71df7966ca191a13695
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7538
Autosubmit: tazjin <tazjin@tvl.su>
Reviewed-by: flokli <flokli@flokli.de>
Tested-by: BuildkiteCI
This relates to the (abandoned) cl/7256.
Introduces a Cargo workspace at //tvix that is primarily intended to
be used as a workaround for the annoying Nix+Rust tooling while having
a consistent set of dependencies.
This is driven in part by a desire to adopt crate2nix and get more
granular Nix builds for Tvix's Rust projects, and in part by a need to
split //tvix/eval into something providing the CLI (REPL etc.), and a
library providing eval, without significantly altering the structure
of build targets.
To accomplish this the workspace has been designed to allow projects
to remain independent build targets. I want to avoid lumping all the
projects together - something like //tvix/eval should always be
independent of other parts of tvix.
A helper function in //tvix/default.nix lets downstream naersk
projects construct a sparse root for the project which combines the
workspace's `Cargo.lock` with the project's own `Cargo.toml`.
Note that cargo commands in the workspace itself require the build
dependencies of _all_ projects to be present, which is currently a bit
annoying to accomplish.
This introduces some breakage:
1. It breaks usage of rust-analyser without being in a shell with the
dependencies of *all* Tvix projects, as it is not capable of
respecting only the subset of dependencies for a part of the
workspace.
2. It is no longer possible to run tests using `cargo test`, as the
test generation crate we use does not work with workspaces:
https://github.com/frehberg/test-generator/issues/6
This still works in the Nix build as we construct a Cargo project
that looks like it's not in a workspace there. Until somebody fixes
that crate / writes a new macro / does something else with the test
suite, the way to run the tests is through the Nix build.
Long-term we'll probably want to get rid of cargo completely, it's
just a big wart and most tooling works without it if correctly
configured, but we don't have time for that now.
Change-Id: I846bff7a8429a25c077fd1e9ef4e3c34a299a4a1
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7533
Reviewed-by: flokli <flokli@flokli.de>
Autosubmit: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
A few weeks ago, oberblastmeister did a release to crates.io so we can
stop importing it via GitHub.
Change-Id: I9d5fa5cd281685779c71b12fed45ed201a1db17e
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7532
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
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>
Allows for the removal of some BTreeMap usage when constructing NixAttrs
by allowing any iterator over 2-tuples to build a NixAttrs. Some
instances of BTreeMap didn't have anything to do with making NixAttrs,
and some were just the best tool for the job, so they are left using the
old `from_map` interface.
Change-Id: I668ea600b0d93eae700a6b1861ac84502c968d78
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7492
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Maybe counter-intuitively the inner elements of a list or the
attribute values of an attribute set will be forced despite
pointer equality (but only one layer deep).
Change-Id: I485d96452fb56f5fb342d39039c9137725b33d3f
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7371
Reviewed-by: Adam Joseph <adam@westernsemico.com>
Tested-by: BuildkiteCI
This came up in the Nix Language channel today and I thought it
warranted a test case.
We did actually implement this correctly.
Change-Id: I4b37c92d06eb6e3a7f59ea3d10af38f2b0a93d53
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7493
Reviewed-by: sterni <sternenseemann@systemli.org>
Autosubmit: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Rust doesn't do tail-call elimination (still!) so the best we can
hope for here is to inline non-recursive invocations.
Change-Id: I78949967e48b006fcbf31786d8f6281cd122f36f
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7360
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Autosubmit: Adam Joseph <adam@westernsemico.com>
Before this, tvix was spending most of its time furiously re-parsing
and re-compiling nixpkgs, each time hoping to get a different result...
Change-Id: I1c0cfbf9af622c276275b1f2fb8d4e976f1b5533
Signed-off-by: Adam Joseph <adam@westernsemico.com>
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7361
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
With this change, the test introduced by cl/7370 passes.
Change-Id: Ie7d2f02a59d61151f14ebd328e6cfa5892cacfb0
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7375
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
Autosubmit: Adam Joseph <adam@westernsemico.com>
This passes all the function/thunk-pointer-equality tests in
cl/7369.
Change-Id: Ib47535ba2fc77a4f1c2cc2fd23d3a879e21d8b4c
Signed-off-by: Adam Joseph <adam@westernsemico.com>
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7358
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
The backtrace-on-stack-overflow create provides best-effort stack
traces when a stack overflow happens. Since it's running on the
(usually tiny) signal alternate stack this isn't easy.
This is guarded by a new `backtrace_overflow` feature flag and never
enabled (even if that feature is selected) for release builds. This
is strictly for debugging; there's crazy unsafe voodoo in there.
https://lib.rs/crates/backtrace-on-stack-overflow
Example output:
```
Stack Overflow:
0: backtrace_on_stack_overflow::handle_sigsegv
at /home/amjoseph/.cargo/registry/src/github.com-1ecc6299db9ec823/backtrace-on-stack-overflow-0.2.0/src/lib.rs:93:40
1: <unknown>
2: __rust_probestack
3: tvix_eval::vm::VM::run_op
at src/vm.rs:399
4: tvix_eval::vm::VM::run
at src/vm.rs:388:23
5: tvix_eval::vm::VM::enter_frame
at src/vm.rs:360:22
6: tvix_eval::value::thunk::Thunk::force
at src/value/thunk.rs:116:25
7: tvix_eval::vm::VM::run_op
at src/vm.rs:801:37
8: tvix_eval::vm::VM::run
at src/vm.rs:388:23
9: tvix_eval::vm::VM::enter_frame
at src/vm.rs:360:22
10: tvix_eval::value::thunk::Thunk::force
at src/value/thunk.rs:116:25
...
```
Signed-off-by: Adam Joseph <adam@westernsemico.com>
Change-Id: I1d8a2017f836be7bf91a2223e7adacb86fa1dbb2
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7354
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
See cl/7368
Change-Id: I97630994c3d65f4d16414a0da236ce000a5b6d33
Signed-off-by: Adam Joseph <adam@westernsemico.com>
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7374
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
See cl/7372; Nix equality semantics require the ability to track
pointer equality of upvalue-sets.
Signed-off-by: Adam Joseph <adam@westernsemico.com>
Change-Id: I82ba517499cf370189a80355e4e46a5caaab7153
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7373
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
This test case checks two things:
* A sanity check that "pointer equality for functions" means not
just the lambda, but also the upvalues.
* To be pointer-equal, it is not enough for the upvalues to be
normal-form equal (i.e. `nix_eq()`-equal); the upvalues must be
*pointer*-equal. The second part of the test case checks for
this.
Signed-off-by: Adam Joseph <adam@westernsemico.com>
Change-Id: I4e59327a6f199b8212e97197b212e3c3934bb3f0
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7372
Reviewed-by: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
The "dep:" syntax in Cargo.toml is very new; crate2nix master has
support for it, but they have not yet made a release with this
update, and therefore the crate2nix in nixpkgs does not yet support
it.
Could we avoid using "dep:" for a few weeks to give crate2nix a
chance to release so I can bump the version in nixpkgs? I've opened
an issue asking crate2nix to make a release:
https://github.com/kolloch/crate2nix/issues/264
I propose that if they haven't acted within a month we stop waiting
and revert this at that time.
Change-Id: I999a72429db667bedf4b2cdba27cb63b3f3d9657
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7350
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
When we start unrecursivifying (sp?) things, Rust's borrow checker
is going to be a headache; its magic only works when you use the CPU
stack as your call stack.
Fixing the borrow checker issues usually involves adding lots of
`clone()`s. Right now `NixList` is the only variant of `Value` that
isn't cheap to clone() -- all the others are either a wrapper around
Rc or else are of bounded size.
Note that this requires dropping the `DerefMut for NixList` instance
and using `Vec<Value>` instead in those situations.
Change-Id: I5a47df66855342aa2064f8f3cb7934ff422d26bd
Signed-off-by: Adam Joseph <adam@westernsemico.com>
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7359
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
When comparing Nix values for equality, an issue can occur where
recursive values contain thunks to themselves which causes borrow
errors when forcing them for comparison later down the line.
To work around this we clone the values for now. There might be some
optimisations possible like checking for thunk equality directly and
short-circuiting on that (we have to check what Nix does).
Change-Id: I7e75c992ea68f100058f52b4b46168da7d671994
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7314
Reviewed-by: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
When passing multiple arguments, every intermediate callable needs to
be forced as this is expected by the VM's call_value function.
Also adds a debug assertion for this which makes it easier to spot
exactly what went wrong.
Change-Id: I3aa519cb6cdaab713bd18282bef901c4cd77c535
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7312
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
This function covers builtins.genericClosure, seemingly including
weird behaviour around the order in which the work set is processed.
For some reason, in C++ Nix the test expectation is written in XML
which we do not yet support, so I have created a new expectation file
using `nix-instantiate --eval --strict` on the file (yes, using C++
Nix).
Change-Id: Id90e7117d120dc66d963a51083c4d8e8f2d9f181
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7311
Reviewed-by: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
This implementation closely follows the original implementation in
Nix, including the use of an equality-based "set" structure to track
keys that have already been processed.
Note that this test does not yet enable the `notyetpassing` test for
builtins.genericClosure because (for as of yet unknown reasons) this
test compares against XML output (however, evaluating the test case
actually does work).
This takes us one step closer to nixpkgs eval.
This commit was written somewhere in the North Sea.
Co-Authored-By: Griffin Smith <root@gws.fyi>
Change-Id: I450a866e6f2888b27c2fe7c7f77ce0f79bfe3e6c
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7310
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
Both //tvix/eval and //tvix/nix_cli have need to for rust tooling available
in $PATH.
Move this one level up, so it's accessible in all subdirectories.
Change-Id: I0763bbe9cefdc962f3a8f86c51e8f67cde8b4b04
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7248
Autosubmit: flokli <flokli@flokli.de>
Reviewed-by: sterni <sternenseemann@systemli.org>
Reviewed-by: grfn <grfn@gws.fyi>
Tested-by: BuildkiteCI
This detects if the second argument of a division is a zero (either as integer
or as float). If so, an error message is displayed.
This fixes b/219.
Change-Id: I50203d14a71482bc757832a2c8dee08eb7d35c49
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7258
Tested-by: BuildkiteCI
Reviewed-by: flokli <flokli@flokli.de>
Add a new `documentation: Option<&'static str>` field to Builtin, and
populate it in the `#[builtins]` macro with the docstring of the builtin
function, if any.
Change-Id: Ic68fdf9b314d15a780731974234e2ae43f6a44b0
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7205
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Refactor the arguments of a Builtin to be a vec of a new BuiltinArgument
struct, which contains the old strictness boolean and also a static
`name` str - this is automatically determined via the ident for the
corresponding function argument in the proc-macro case, and passed in in
the cases where we're still manually calling Builtin::new.
Currently this name is unused, but in the future this can be used as
part of a documentation system for builtins.
Change-Id: Ib9dadb15b69bf8c9ea1983a4f4f197294a2394a6
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7204
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Some new top-level re-exports (specifically VM, Builtin, and ErrorKind)
were added to lib.rs in tvix/eval to allow the builtin-macros tests to
work - we should be clear which of these are part of the public
interface (I think it's reasonable for ErrorKind to be) and which
aren't (specifically I'm not sure VM and Builtin necessarily should be,
at least yet).
Change-Id: I3bbeaa63cdda9227224cd3bc298a9bb8da4deb7c
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7203
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Similar to what we did with pure builtins, define the impure builtins
within a module at the top-level using the new #[builtins] attribute
macro
Change-Id: Ie5d5135d00bb65e651531df6eadba642cd4eb08e
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7202
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Break out all pure builtin functions to top-level functions defined
within the `pure_builtins` module in `builtins/mod.rs`.
Change-Id: I9a10660446d557b1a86da4c45a463e9a1a9b4f2d
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7201
Reviewed-by: tazjin <tazjin@tvl.su>
Reviewed-by: Adam Joseph <adam@westernsemico.com>
Tested-by: BuildkiteCI
Mostly as a proof-of-concept of the new proc-macros for defining
builtins, define a single builtin (the first in the list, `abort`) at
the top-level of a child module within builtins/mod.rs, and add it to
the list of builtins returned from `pure_builtins`.
If this works nicely, we can start breaking out the rest of the builtins
into the top-level too, in addition to introducing additional sets of
builtins (to differentiate between pure and impure builtins).
Change-Id: I5bdd57c57fecf8d63c9fed4fc6b1460f533b20f2
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7199
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Add a single new proc macro to a new proc-macro crate,
`tvix-eval-proc-macros` for defining an inline module containing nix
builtins, and automatically generating a function within that module
which returns a list of those builtins as `tvix_eval::value::Builtin`.
Change-Id: Ie4afae438914d2af93d15637151a49b4c68aa352
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7198
Reviewed-by: tazjin <tazjin@tvl.su>
Reviewed-by: Adam Joseph <adam@westernsemico.com>
Tested-by: BuildkiteCI
This commit adds a markdown document which explains how the
thread-local VM infrastructure works, in case it is useful in the
future.
Change-Id: Id10e32a9e3c5fa38a15d4bec9800f7234c59234a
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7193
Tested-by: BuildkiteCI
Autosubmit: Adam Joseph <adam@westernsemico.com>
Reviewed-by: tazjin <tazjin@tvl.su>
This is more generally useful than just inside the VM, until it is
stabilised in Rust itself.
Change-Id: Id9aa3d5b533ff38e3d2c6b85ad484394fdd05dcf
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7186
Tested-by: BuildkiteCI
Autosubmit: tazjin <tazjin@tvl.su>
Reviewed-by: grfn <grfn@gws.fyi>
Reviewed-by: Adam Joseph <adam@westernsemico.com>
This fixes a mistake I made in d978b556e6.
Change-Id: I88db697105a7149e9785f6aface03bff68566d2b
Signed-off-by: Adam Joseph <adam@westernsemico.com>
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7085
Reviewed-by: grfn <grfn@gws.fyi>
Tested-by: BuildkiteCI
Scope_depth and with_stack_depth were being reset to zero for nested
function abstractions. Fortunately nothing depends on them being
computed correctly in these cases, but it sure was confusing.
Change-Id: I59980b6a5aff043f60079f97211220b0086eb97d
Signed-off-by: Adam Joseph <adam@westernsemico.com>
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7091
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
It is very confusing that this opcode is called DataLocalIdx, but it
carries a StackIdx rather than a LocalIdx. It seems like this
really ought to be called DataStackIdx, but maybe I've
misunderstood; if so please explain it to me.
Change-Id: I91f6ffa759412beef0b91d3c19ec0d873fe51b99
Signed-off-by: Adam Joseph <adam@westernsemico.com>
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7088
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
This implements builtins.split, and passes eval-okay-regex-split.nix
(which is moved out of notyetpassing).
Signed-off-by: Adam Joseph <adam@westernsemico.com>
Change-Id: Ieb0975da2058966c697ee0e2f5b3f26ccabfae57
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7143
Tested-by: BuildkiteCI
Reviewed-by: grfn <grfn@gws.fyi>
We have to be careful implementing `builtins.groupBy`, since the
list may contain thunks, and tvix's to_xxx() functions do not work
on thunks.
Signed-off-by: Adam Joseph <adam@westernsemico.com>
Change-Id: I182b6fc2d4296f864ed16744ef70b153e8e6978a
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7039
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
The impl Display for NixAttrs needs to wrap double quotes around any
keys which are not valid Nix identifiers. This commit does that,
and adds a test (which fails prior to this commit and passes after
this commit).
Change-Id: Ie31ce91e8637cb27073f23f115db81feefdc6424
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7084
Autosubmit: Adam Joseph <adam@westernsemico.com>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
The variable name `local_idx` is used here for a StackIdx, which invites
confusion.
Signed-off-by: Adam Joseph <adam@westernsemico.com>
Change-Id: I2e22db90acdc0d29586ee5b72ea18d42d93badcb
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7086
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
If self.depth > other.depth then self is deeper than other, so self
is *below* other, not above it. Let's just inline the function.
Change-Id: I8dda3d90cbc86c8a6fa01bc4a5e506a2e403bd20
Signed-off-by: Adam Joseph <adam@westernsemico.com>
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7090
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
It isn't possible to implement PartialEq properly for Value, because
any sensible implementation needs to force() thunks, which cannot be
done without a `&mut VM`.
The existing derive(PartialEq) has false negatives, which caused the
bug which cl/7142 fixed. Fortunately that bug was easy to find, but
a silent false negative deep within the bowels of nixpkgs could be a
real nightmare to hunt down.
Let's just remove the PartialEq impl for Value, and the other
derive(PartialEq)'s that depend on it.
Signed-off-by: Adam Joseph <adam@westernsemico.com>
Change-Id: Iacd3726fefc7fc1edadcd7e9b586e04cf8466775
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7144
Reviewed-by: kanepyork <rikingcoding@gmail.com>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
I think we should bring this into $PATH too.
Change-Id: Ie31ac558355b7c4ed9dcd3dd60e1b03f141d1178
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7166
Autosubmit: flokli <flokli@flokli.de>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
The current implementation of nix_eq will force one level of thunks
and then switch to the (non-forcing) rust Eq::eq() method. This
gives incorrect results for lists-of-thunks.
This commit changes nix_eq() to be recursive.
A regression test (which fails prior to this commit) is included.
This fix also causes nix_tests/eval-okay-fromjson.nix to pass, so it
is moved out of notyetpassing.
Change-Id: I655fd7a5294208a7b39df8e2c3c12a8b9768292f
Signed-off-by: Adam Joseph <adam@westernsemico.com>
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7142
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
We're getting close to the finish line, folks.
I went through the list of builtins and there are only 33 that
remain unimplemented. I've marked them, and indicated which are
ready to be implemented vs which are waiting for other things.
We can delete this column from the table once everything is
implemented.
Signed-off-by: Adam Joseph <adam@westernsemico.com>
Change-Id: Idfaef93283536288b12e59aef5c3e1cd139bd133
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7140
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
I believe that the currentTime, findFile, hashFile, pathExists,
readDir, path (unless ?sha256), and readFile builtins are impure.
This commit marks them as such in docs/builtins.md.
Change-Id: Ib1b59fe643dde73cb2b00050b4ef9d3401ad22eb
Signed-off-by: Adam Joseph <adam@westernsemico.com>
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7139
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
This is a bit tricky because the comparator can throw errors, so we
need to propagate them out if they exist and try to avoid sorting
forever by returning a reasonable ordering in this case (as
short-circuiting is not available).
Co-Authored-By: Vincent Ambo <tazjin@tvl.su>
Change-Id: Icae1d30f43ec1ae64b2ba51e73ee467605686792
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7072
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
Lists are compared lexicographically in C++ nix as of [0], and our
updated nix test suites depend on this. This implements comparison of
list values in `Value::nix_cmp` using a very similar algorithm to what
C++ does - similarly to there, this requires passing in the VM so we can
force thunks in the list elements as we go.
[0]: 09471d2680#
Change-Id: I5d8bb07f90647a1fec83f775243e21af856afbb1
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7070
Autosubmit: grfn <grfn@gws.fyi>
Reviewed-by: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
CL/7034 looks great, except that for a length-N target string it
will perform N deep copies of each of the from and to-lists. Let's
use references instead of clones.
Signed-off-by: Adam Joseph <adam@westernsemico.com>
Change-Id: Icd341213a9f0e728f9c8453cec6d23af5e1dea91
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7095
Reviewed-by: wpcarro <wpcarro@gmail.com>
Reviewed-by: j4m3s <james.landrein@gmail.com>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
I assumed that AttrsRep::KV represented attrsets with a single
attribute as a Key-Value pair. That is not the case. Let's warn
other people about this.
Signed-off-by: Adam Joseph <adam@westernsemico.com>
Change-Id: Ie3d2765fcc1ab705c153ab94ffe77bbd6d4ab39e
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7093
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Rustc uses wasm32-unknown-unknown, which is rejected by config.sub,
for wasm-in-the-browser environments. Rustc should be using
wasm32-unknown-none, which config.sub accepts. Hopefully the rustc
people will change their triple before stabilising this triple. In
the meantime, we fix it here in order to unbreak tvixbolt.
https://doc.rust-lang.org/beta/nightly-rustc/rustc_target/spec/wasm32_unknown_unknown/index.html
Signed-off-by: Adam Joseph <adam@westernsemico.com>
Change-Id: I941fd8d6f3db4e249901772fd79321ad88cd9cc6
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7107
Reviewed-by: tazjin <tazjin@tvl.su>
Reviewed-by: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
