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
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
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
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
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>
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
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
This adds a function NixList::force_elements() which forces each
element of the list shallowly. This behavior is needed for
`builtins.replaceStrings`, and probably a few other builtins as
well.
Signed-off-by: Adam Joseph <adam@westernsemico.com>
Change-Id: I3f0681acbbfe50e781b5f07b6a441647f5e6f8da
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7094
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Rather than implementing all of the interesting semantics of value
comparison with a macro bound to the VM, implement the bulk of the logic
with a method on Value itself that returns an Ordering, and then use the
macro to implement the comparison against that Ordering. This has no
functional change, but paves the way to implementing lexicographic
comparison of list values, which is supported in the latest version of
upstream nix.
Change-Id: I8af1a020b41577021af5939f5edc160c407d4a9e
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7069
Autosubmit: grfn <grfn@gws.fyi>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
I played around a little bit with doing this in-place, but ended up
going with this perhaps slightly clone-heavy approach for now because
ideally most clones on Value are cheap - but later we should benchmark
alternate approaches that get to reuse allocations better if necessary
or possible.
Change-Id: If998eb2056cedefdf2fb480b0568ac8329ccfc44
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7068
Autosubmit: grfn <grfn@gws.fyi>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
When compiling a lambda, take the name of the outer slot (if
available) and store it as the name on the lambda.
These names are then shown in the observer, and nowhere else (so far).
It is of course common for these things to thread through many
different context levels (e.g. `f = a: b: c: ...`), in this setup only
the outermost closure or thunk gains the name, but it's better than
nothing.
Change-Id: I681ba74e624f2b9e7a147144a27acf364fe6ccc7
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7065
Reviewed-by: grfn <grfn@gws.fyi>
Autosubmit: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Using the same method as in Thunk::deep_force, detect cycles when
printing values by maintaining a set of already seen thunks.
With this, display of infinite values matches that of Nix:
> nix-instantiate --eval --strict -E 'let as = { x = 123; y = as; }; in as'
{ x = 123; y = { x = 123; y = <CYCLE>; }; }
> tvix-eval -E 'let as = { x = 123; y = as; }; in as'
=> { x = 123; y = { x = 123; y = <CYCLE>; }; } :: set
Change-Id: I007b918d5131d82c28884e46e46ff365ef691aa8
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7056
Tested-by: BuildkiteCI
Reviewed-by: grfn <grfn@gws.fyi>
This is done via a new `deepForce` function on Value. Since values can
be cyclical (for example, see the test-case), we need to do some extra
work to avoid RefCell borrow errors if we ever hit a graph cycle:
While deep-forcing values, we keep a set of thunks that we have
already seen and avoid doing any work on the same thunk twice. The set
is encapsulated in a separate type to stop potentially invalid
pointers from leaking out.
Finally, since deep_force is conceptually similar to
`VM::force_for_output` (but more suited to usage in eval since it
doesn't clone the values) this removes the latter, replacing it with
the former.
Co-Authored-By: Vincent Ambo <tazjin@tvl.su>
Change-Id: Iefddefcf09fae3b6a4d161a5873febcff54b9157
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7000
Tested-by: BuildkiteCI
Reviewed-by: grfn <grfn@gws.fyi>
Reviewed-by: tazjin <tazjin@tvl.su>
This commit deduplicates the Thunk-like functionality from Closure
and unifies it with Thunk.
Specifically, we now have one and only one way of breaking reference
cycles in the Value-graph: Thunk. No other variant contains a
RefCell. This should make it easier to reason about the behavior of
the VM. InnerClosure and UpvaluesCarrier are no longer necessary.
This refactoring allowed an improvement in code generation:
`Rc<RefCell<>>`s are now created only for closures which do not have
self-references or deferred upvalues, instead of for all closures.
OpClosure has been split into two separate opcodes:
- OpClosure creates non-recursive closures with no deferred
upvalues. The VM will not create an `Rc<RefCell<>>` when executing
this instruction.
- OpThunkClosure is used for closures with self-references or
deferred upvalues. The VM will create a Thunk when executing this
opcode, but the Thunk will start out already in the
`ThunkRepr::Evaluated` state, rather than in the
`ThunkRepr::Suspeneded` state.
To avoid confusion, OpThunk has been renamed OpThunkSuspended.
Thanks to @sterni for suggesting that all this could be done without
adding an additional variant to ThunkRepr. This does however mean
that there will be mutating accesses to `ThunkRepr::Evaluated`,
which was not previously the case. The field `is_finalised:bool`
has been added to `Closure` to ensure that these mutating accesses
are performed only on finalised Closures. Both the check and the
field are present only if `#[cfg(debug_assertions)]`.
Change-Id: I04131501029772f30e28da8281d864427685097f
Signed-off-by: Adam Joseph <adam@westernsemico.com>
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7019
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
In `a++b`, the previous implementation would move `b` (i.e. memcpy
its elements) twice. Let's do that only once.
We sure do call NixList.clone() a whole lot. At some point in the
future we probably want to do a SmolStr-type split for NixList into
a two-variant enum where one side is an Rc<Vec<Value>> for lists
longer than a certain length.
Signed-off-by: Adam Joseph <adam@westernsemico.com>
Change-Id: I32154d18785a1f663454a8b9d4afd3e78bffdf9c
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7040
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Now that we're tracking formals on Lambda this ends up being quite easy;
we just pull them off of the Lambda for the argument closure and use
them to construct the result attribute set.
Change-Id: I811cb61ec34c6bef123a4043000b18c0e4ea0125
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7003
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Validate "closed formals" (formal parameters without an ellipsis) via a
new ValidateClosedFormals op, which checks the arguments (in an attr set
at the top of the stack) against the formal parameters on the Lambda in
the current frame, and returns a new UnexpectedArgument error (including
the span of the formals themselves!!) if any arguments aren't allowed
Change-Id: Idcc47a59167a83be1832a6229f137d84e426c56c
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7002
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
In preparation for both implementing the `functionArgs` builtin and
adding support for validating closed formals, record information about
the formal arguments to a function *on the Lambda itself*. This may seem
a little odd for the purposes of just closed formal checking, but is
something we have to have anyway for builtins.functionArgs so I figured
I'd do it this way to kill both birds with one stone.
Change-Id: Ie3770a607bf352a1eb395c79ca29bb25d5978cd8
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7001
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
To assert that OpFindFile is only emitted for specially compiled SPATH
expressions, as well as make sure it doesn't accidentally operate on
“ordinary values”, introduce an UnresolvedPath internal value. If
OpFindFile sees a non-UnresolvedPath value, it'll crash.
Note that this change is not done purely for OpFindFile: We may want to
compile SPATH expressions as function calls to __findFile (like C++ Nix
does) in the future, so the UnresolvedPath value would definitely need
to be an ordinary string again then. Rather, this change is done in
preparation for resolving home dir relative paths at runtime (since they
depend on the environment) for which we'll need a similar mechanism to
OpFindFile.
Change-Id: I6acf287f35197cd9e13377079f972b9d36e5b22e
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7023
Autosubmit: sterni <sternenseemann@systemli.org>
Reviewed-by: Adam Joseph <adam@westernsemico.com>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Lambda has a quite large and variable-sized runtime representation,
unlike Rc<Lambda>. It would be easy to accidentally call clone() on
this and create input-dependent performance regressions.
Nothing in the codebase is currently using Lambda.clone(). Let's
remove the derived instance. If it's really needed it is very easy
to add it back in, but whoever does that will have to look at the
struct they're adding Clone to, which will hopefully prompt them to
think about whether or not that's really what they want to do.
Change-Id: I7806a741862ab4402229839ce3f4ea75aafd6d12
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7029
Autosubmit: Adam Joseph <adam@westernsemico.com>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
I believe this variant is left over from a previous implementation.
If not, please let me know.
Signed-off-by: Adam Joseph <adam@westernsemico.com>
Change-Id: I02a3bf2f63794d09e96a5a92a034c0ad3d1ff221
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7027
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Using `serde_json` for parsing JSON here, plus an `impl FromJSON for
Value`. The latter is primarily to stay "dependency light" for now -
likely going with an actual serde `Deserialize` impl in the future is
going to be way better as it allows saving significantly on intermediary
allocations.
Change-Id: I152a0448ff7c87cf7ebaac927c38912b99de1c18
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6920
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
This commit causes the test eval-okay-builtins.nix to pass.
It also adds tests/tvix_tests/eval-okay-dirof.nix which has better
coverage than the nix tests for this builtin.
Signed-off-by: Adam Joseph <adam@westernsemico.com>
Change-Id: I71d96b48680696fd6e4fea3a9861742b35cfaa66
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6987
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Right now we're pretending that the Rust library path_clean does the
same thing that cppnix's canonPath() does. This is not true. It's
close enough for the test suite, but may come back to bite us.
Let's create our own canon_path() function and call that in all the
places where we intend to match the behavior of cppnix's
canonPath(). That way when we fix this we can fix it once, in one
place.
Signed-off-by: Adam Joseph <adam@westernsemico.com>
Change-Id: Ia6f9577f62f49ef352ff9cfa5efdf37c32d31b11
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6993
Tested-by: BuildkiteCI
Reviewed-by: grfn <grfn@gws.fyi>
I broke the build.
I didn't understand that before hitting "submit" you need to re-test
your changes on latest HEAD (and that CI doesn't do that for you); I
failed to re-test cl/6912 following the merge of cl/6907.
This commit fixes the build by removing the overlapping instances.
Change-Id: I2a720d2c60cc7103b350f78102a8998f93bac828
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6965
Tested-by: BuildkiteCI
Autosubmit: Adam Joseph <adam@westernsemico.com>
Reviewed-by: tazjin <tazjin@tvl.su>
Currently, the span on *all* thunk force errors is the span at which the
thunk is forced, which for recursive thunk forcing ends up just being
the same span over and over again. This changes the span on thunk force
errors to be the span at which point the thunk is *created*, which is a
bit more helpful (though the printing atm is a little... crowded). To
make this work, we have to thread through the span at which a thunk is
created into a field on the thunk itself.
Change-Id: I81474810a763046e2eb3a8f07acf7d8ec708824a
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6932
Autosubmit: grfn <grfn@gws.fyi>
Reviewed-by: Adam Joseph <adam@westernsemico.com>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Previously the various call functions either returned `EvalResult<()>`
or `EvalResult<Value>`, which was confusing.
Now only vm::call_with returns a Value directly, and other parts of
the API just leave the stack top in the post-call state.
This makes it easier to reason about what's going on in non-tail-call
cases (which are making a comeback).
Change-Id: I264ffc683a11aca72dd06e2220a5ff6e7c5fc2b0
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6936
Tested-by: BuildkiteCI
Reviewed-by: grfn <grfn@gws.fyi>
This commit implements (lazy) resolution of `<...>` paths via either the
NIX_PATH environment variable, or the -I command-line flag - both
handled via EvalOptions. As a result, EvalOptions can no longer derive
Copy, meaning we have to clone it at each line of the repl - this is
probably not a huge deal as repl performance is not exactly an inner
loop and we're not cloning very much.
Internally, this works by creating a thunk which pushes a constant
containing the string inside the brackets to the stack, then a new
opcode to resolve that path via the `NixPath`. To get that opcode to
work, we now have to pass in the NixPath when constructing the VM.
This (intentionally) leaves out proper implementation of path resolution
via `findFile` (cppnix just calls whatever identifier called findFile is
in scope!!!) as that's widely considered a bit of a misfeature, but if
we do decide to implement that down the road it likely wouldn't be more
than a few extra ops within the thunk introduced here.
Change-Id: Ibc979b7e425b65cbe88599940520239a4a10cee2
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6918
Autosubmit: grfn <grfn@gws.fyi>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Implement adding paths and strings via OpAdd. Since the nix rules are
quite obscure, I'm electing to test this one with an oracle test to
avoid the danger of getting the actual asserted result wrong.
Change-Id: Icdcca3690ca2e8459e386c1f29cc48eaaa39e9a3
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6914
Autosubmit: grfn <grfn@gws.fyi>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Factor out the construction of Value::Attrs (including the Rc) into a
new `attrs` constructor function, to abstract away the presence of the
Rc itself.
Change-Id: I42fd4c3841e1db368db999ddd651277ff995f025
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6892
Autosubmit: grfn <grfn@gws.fyi>
Reviewed-by: sterni <sternenseemann@systemli.org>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Thunks correctly force when comparing for equality against other thunks,
but weren't being forced correctly when comparing against non-thunk
values, in either direction.
Change-Id: Ia03702895ec4d70aed3445c1b0a9a7a641d1a300
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6897
Autosubmit: grfn <grfn@gws.fyi>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
For some upcoming builtins (notably, import) we need to capture
arguments in the builtin's implementation.
To allow this, we can no longer use function pointers for builtins,
but must use a reference-counted closure object instead.
Unfortunately this adds an extra pointer operation to every builtin
call. We should benchmark this later against having a split builtin
representation.
Change-Id: I109d98d0e25998870542f47573eb1ec2e546f2a2
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6856
Tested-by: BuildkiteCI
Reviewed-by: grfn <grfn@gws.fyi>
This is the same code as before, just moved into a trait impl to gain
access to stuff that needs IntoIterator
Change-Id: Iff9375cd05593dd2681fa85ccc7f4554bf944a02
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6842
Autosubmit: tazjin <tazjin@tvl.su>
Reviewed-by: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
This implementation, which only ever worked for non-recursive
attribute sets, is no longer needed and thus removed here.
We have a new implementation of these nested keys coming up instead.
Change-Id: I0c2875154026a4f5f6e0aa038e465f54444bf721
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6783
Reviewed-by: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
This makes it possible to call a callable value (builtin or
closure/lambda) directly, without unwrapping it first. This is needed
for pretty much all higher-order functions to work correctly.
This is mostly equivalent to the previous code in coerce_to_string for
calling `__toString`, except it expects the argument(s) to already be
placed on the stack.
Note that the span for the `NotCallable` error is not currently
guaranteed to make any sense, will experiment with this.
Change-Id: I821224368d438a28900858b343defc1817e46a0a
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6717
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
Generate smaller recursive values for generated Values, and run fewer
cases for the attrs proptests which are particularly egregious.
Change-Id: Ia35c7c120270feaf045be1deb440c87ebb185c27
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6716
Autosubmit: grfn <grfn@gws.fyi>
Reviewed-by: tazjin <tazjin@tvl.su>
Reviewed-by: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
Instead of arity, we pass a array reference to Builtin::new that
describes how many arguments there are and which of them need to be
forced, eliminating the need to force manually.
Note that this change doesn't fix some of the instances where the the
Builtin doesn't consider that the value could be a Thunk.
Change-Id: Iadb58bb79886c30dc6b09dcf0ffad8abf28036a1
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6662
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Refactor the `force!` macro to a method on `Value` which returns a
smart-pointer-esque type, which simplifies the callsite and eliminates
rightward drift, especially for high-arity builtins.
Change-Id: I97a7837580accfb4bbd03b24f2acdbd38645efa5
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6656
Autosubmit: grfn <grfn@gws.fyi>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Thunks might be encountered deep in equality comparison (eg nested
inside a list or attr-set), at which point we need to force them in
order to compare them for equality (or else we panic when trying to get
at their value).
Fixes: b/192
Change-Id: I912151085f8298f30d5214c7965251c9266443f2
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6652
Autosubmit: grfn <grfn@gws.fyi>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Pass in, but ignore, a mutable reference to the VM to the `nix_eq`
functions, in preparation for using that VM to force thunks during
comparison.
Change-Id: I565435d8dfb33768f930fdb5a6b0fb1365d7e161
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6651
Autosubmit: grfn <grfn@gws.fyi>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Using rust's PartialEq trait to implement Nix equality semantics is
reasonably fraught with peril, both because the actual laws are
different than what nix expects, and (more importantly) because certain
things actually require extra context to compare for equality (for
example, thunks need to be forced). This converts the manual PartialEq
impl for Value (and all its descendants) to a *derived* PartialEq
impl (which requires a lot of extra PartialEq derives on miscellanious
other types within the codebase), and converts the previous
nix-semantics equality comparison into a new `nix_eq` method. This
returns an EvalResult, even though it can't currently return an error,
to allow it to fail when eg forcing thunks (which it will do soon).
Since the PartialEq impls for Value and NixAttrs are now quite boring,
this converts the generated proptests for those into handwritten ones
that cover `nix_eq` instead
Change-Id: If3da7171f88c22eda5b7a60030d8b00c3b76f672
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6650
Autosubmit: grfn <grfn@gws.fyi>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
As before, this limits the cases to a relatively small number because
otherwise things get quite large.
Change-Id: I5371dc56418fca52e1dd1d905b20868f647091ba
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6649
Autosubmit: grfn <grfn@gws.fyi>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Only running 20 cases for now, since Value can get quite big if you let
it run for a while.
Change-Id: I09ef19da22c789c4869793836c98937c44595340
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6648
Autosubmit: grfn <grfn@gws.fyi>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
this was found by proptests!
Change-Id: I16d6a6ece3b20cdddd6f78c94cc87befb1b651e6
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6647
Autosubmit: grfn <grfn@gws.fyi>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Impl Arbitrary for Value (and NixAttrs and NixList) in the same way we
did for NixString. Value currently only generates non-"internal"
values (no thunks, AttrNotFound, etc.) and can't generate
functions (builtins or closures), because those'd require full
generation of tvix bytecode, which is a bit more work than I'd like to
do now - there's a `todo!` left in the code for a place where we could
allow opting-in to internal values and functions later.
Change-Id: I07a59e2b1d89cfaa912d4ecebd642caf4ddb040a
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6627
Autosubmit: grfn <grfn@gws.fyi>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Add a suite of proptests covering the laws of the handwritten stdlib
trait impls (Eq, Ord, and Hash) for String, generated from a new set of
macros for generating those tests which can be applied to other types.
Change-Id: Ib3276c9e96fca497aece094e5612707d3dc77ccd
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6626
Autosubmit: grfn <grfn@gws.fyi>
Reviewed-by: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
The From<String> impl for NixString only generates StringRepr::Heap
strings, but we want to make sure we're testing StringRepr::Smol too
Change-Id: I6d04b9cf12ef8462fe2788e0c6414b165f40311d
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6629
Autosubmit: grfn <grfn@gws.fyi>
Reviewed-by: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
This function is necessary for all builtins that expect some form of
path as an argument. It is merely a wrapper around coerce_to_string that
can shortcut if we already have a path. The absolute path check is done
in the same way as in C++ Nix for compatibility, although it should
probably be revised in the long term (think about Windows, for example).
Since coercing to a path is not an operation possible in the language
directly, this function can live in the builtins module as the only
place it is required.
Change-Id: I69ed5455c00d193fea88b8fa83e28907a761cab5
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6574
Autosubmit: sterni <sternenseemann@systemli.org>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Define `.len()` method on `NixAttrs` to preallocate the capacity of the result
vector.
Also anchor an errant comment to its context (I think).
Change-Id: I268f15025d453d7b3ae1146558c80e51433dd2a8
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6546
Reviewed-by: wpcarro <wpcarro@gmail.com>
Reviewed-by: sterni <sternenseemann@systemli.org>
Autosubmit: wpcarro <wpcarro@gmail.com>
Tested-by: BuildkiteCI
Implement C++ Nix's `EvalState::coerceToString` minus some of the Path
/ store handling. This is currently only used for `toString` which does
all possible coercions, but we've already prepared the weaker coercion
variant which is e.g. used for builtins that expect string arguments.
`EvalState::coerceToPath` is still missing for builtins that need a
path, but it'll be easy to build on top of this.
Change-Id: I78d15576b18921791d04b6b1e964b951fdef22c6
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6571
Autosubmit: sterni <sternenseemann@systemli.org>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Pointed out by sterni in cl/6370
Change-Id: I324d8049a2702ced8f30ad43a64d63ae79dd0eab
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6569
Reviewed-by: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
This very closely follows the way it's done for warnings, but errors
have a lot more information available in some cases which we do not
surface yet.
Note also that due to requiring the `CodeMap`, this is not yet called
from eval.rs as the way that is threaded through needs to be
refactored, so only the method for reporting these errors as strings
is implemented so far.
Next steps for this will be to add a generic diagnostics module that
reduces some of the boilerplate for this between warnings & errors,
and which will also give us a good point in the future to switch to a
fancier diagnostics crate.
Change-Id: If6bb209f8e7a568d866e516a90335b9b2afbf66d
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6534
Reviewed-by: grfn <grfn@gws.fyi>
Tested-by: BuildkiteCI
This struct will be responsible for tracking upvalues (and is a
convenient place to introduce optimisations for reducing value clones)
instead of a plain value vector.
The main motivation for this is that the upvalues will have to capture
the `with`-stack fully and I want to avoid duplicating the logic for
this between the two capturing types.
Change-Id: I6654f8739fc2e04ca046e6667d4a015f51724e99
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6485
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
As pointed out by sterni in cl/6205, this is actually possible in
syntactically valid expressions like
{ ${12 + 13} = 12; }
Change-Id: Id8a1e3aceb551f288f9050c4eea563eb6572f1a7
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6461
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
As pointed out by grfn on cl/6091
Change-Id: I28308577b7cf99dffb4a4fd3cc8783eb9ab4d0d6
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6460
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
At the point where control flow exits Thunk::force (which may be due
to recursing), it is vital that there is no longer a borrow to the
inner thunk representation, otherwise this can cause accidental
infinite recursion (which will be detected, but cause failures on
valid code).
Change-Id: I2846f3142830ae3110a4f5d2299e9d7928634504
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6436
Reviewed-by: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
This could occur when the disassembler is enabled and tracing the
runtime while a thunk is being evaluated, as it would not be possible
for the *tracer* to borrow the thunk at this exact moment.
However, we know that if the borrowing fails here we are dealing with
a not-fully evaluated thunk (blackhole), which should just print the
internal representation.
Change-Id: I4bdb4f17818d55795368e3d28842048f488f0a91
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6416
Reviewed-by: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
Previously, "calling" (setting up the VM run loop for executing a call
frame) and "running" (running this loop to completion) were separate
operations.
This was basically an attempt to avoid nesting `VM::run` invocations.
However, doing things this way introduced some tricky bugs for exiting
out of the call frames of thunks vs. builtins & closures.
For now, we unify the two operations and always return the value to
the caller directly. For now this makes calls a little less effective,
but it gives us a chance to nail down some other strange behaviours
and then re-optimise this afterwards.
To make sure we tackle this again further down I've added it to the
list of known possible optimisations.
Change-Id: I96828ab6a628136e0bac1bf03555faa4e6b74ece
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6415
Reviewed-by: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
The casting methods of `Value` are pretty verbose, and actually
incorrect before this commit as they did not account for inner thunk
values.
To address this, we first attempt to make them correct by introducing
a standard macro to generate them and traverse the inner thunk(s) if
necessary.
This is likely to be a performance hit as it will now involve more
cloning of values. We can do multiple things to alleviate this, but
should do some measurements first.
Change-Id: If315d6e2afe7b69db727df535bc6cbfb89a691aa
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6412
Reviewed-by: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
This makes it possible for builtins to force values on their own,
without the VM having to apply a strictness mask to the arguments
first.
Change-Id: Ib49a94e56ca2a8d515c39647381ab55a727766e3
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6411
Reviewed-by: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
With this, if an error occurs while forcing a thunk (which is very
likely) it is threaded through to the top by wrapping it in the
ErrorKind::ThunkForce variant.
We could use this to generate "stacktrace-like" error output if we
wanted, or simply jump through and discard everything except the
innermost error.
Change-Id: I3c1c8708c2f73ae062815adf490ce935b1979da8
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6409
Reviewed-by: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
Previously error spans were optional because the information about
code spans was not available at runtime. Now that this information has
been added, the error type will always carry a span.
This change is very invasive all throughout the codebase. This is due
to the fact that many functions that are called *by* the VM expected
to return `EvalResult`, but this no longer works as the span
information is not available to those functions - only to the VM
itself.
To work around this the majority of these functions have been changed
to return `Result<T, ErrorKind>` instead and an accompanying macro in
the VM constructs the "real" error.
Note that this implementatino currently has a bug where errors
occuring within thunks will yield the location at which the thunk was
forced, not the location at which the error occured within the code.
This will be fixed soon, but the commit is large enough as is.
Change-Id: Ib1ecb81a4d09d464a95ea7ea9e589f3bd08d5202
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6408
Reviewed-by: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
Get the length of a list
Change-Id: I41d91e96d833269541a1b3c23b7cc879f96d1e5a
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6407
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
Note that I've allowed `needless_lifetimes` for the attribute set
iterator, as I find the type easier to understand with these
annotations present.
Change-Id: I33abb17837ee4813076cdb9a87f54bac4a37044e
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6373
Reviewed-by: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
This does not require a custom iterator type (for now?)
Change-Id: I5beb194bd8629571bd4040c69c977c27149807fa
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6371
Reviewed-by: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
If a thunk is already evaluated, there are cases where due to the
memoisation implementation something might observe a value wrapped in
a thunk.
In these cases, the implementation of `Display` and `PartialEq` must
delegate to the underlying value.
Note that there are a handful of other cases like these which we need
to cover.
It is a little tricky to write integration tests for these directly,
especially as some of the open-upvalue optimisations coming down the
pipe will reduce the number of observable thunks.
One test that covers a part of this behaviour is currently
disabled (needs some more machinery), but it's being brought back in
the next commits.
Change-Id: Iaa8cd338c12236af844bbc99d8cec2205f0d0095
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6370
Reviewed-by: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
Implementing iteration over NixAttrs requires a custom iterator type
in order to encapsulate the different representations. The BTreeMap
for example has its own iterator type which needs to be encapsulated.
This is mostly boilerplate code, but for a change some simple unit
tests have been added in.
Change-Id: Ie13b063241d461b810876f95f53878388e918ef2
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6367
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
These static strings show up a bunch when dealing with the internals
of attribute sets, and having them available as static references is
required.
Due to the way const expressions are evaluated, taking a reference to
the existing NixString::NAME / NixString::VALUE items does not work
and the references themselves need to be const-evaluated.
Change-Id: If6e75847af978118a3b266fe6a3242321722434d
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6366
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
The VM previously took care of repeatedly forcing a thunk until it
reached an evaluated state. This logic is now encapsulated inside of
the `Thunk::force` implementation.
In addition, force no longer returns a reference to the value by
default, leaving it up to callers to decide whether they want to
borrow the value or not (a helper is provided for this).
Change-Id: I2aa7da922058ad1c57fbf8bfc7785aab7971c02b
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6365
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
This operation forces the evaluation of a thunk.
There is some potential here for making an implementation that avoids
some copies, but the thunk machinery is tricky to get right so the
first priority is to make sure it is correct by keeping the
implementation simple.
Change-Id: Ib381455b02f42ded717faff63f55afed4c8fb7e3
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6352
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
The capacity (i.e. number of builtins) is known from the lambda, so we
can size it correctly right away.
Change-Id: Iab0b5a3f47d450fa9866c091ebbbed935b934907
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6351
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
This trait abstracts over the commonalities of upvalue handling
between closures and thunks.
It allows the VM to simplify the code used for setting up upvalues,
without duplicating between the two different types.
Note that this does not yet refactor the VM code to optimally make use
of this.
Change-Id: If8de5181f26ae1fa00d554f1ae6ea473ee4b6070
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6347
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
For now, do not distinguish between closing and non-closing thunks, it
will make the initial implementation easier. See Knuth etc.
Change-Id: I0bd51e0f89f2c77e90bac63b507e5027b649e3d8
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6346
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
Introduces the representation of runtime thunks, that is lazily
evaluated values. Their representation is very similar to closures.
Change-Id: I24d1ab7947c070ae72ca6260a7bbe6198bc8c7c5
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6343
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
This instruction finalises the initialisation of deferred upvalues in
closures (and soon, thunks).
The compiler does not yet emit this instruction, some more accounting
is needed for that.
Change-Id: Ic4181b26e19779e206f51e17388559400da5f93a
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6337
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
This will leave a sentinel value in the upvalue slot in which the
actual value is to be captured after resolution once a scope is fully
set up.
Change-Id: I12b37b0dc8d32603b03e675c3bd039468e70b354
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6336
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
The previous closure refactoring introduced a bug in which the same
closure object would get mutated constantly for each instance of a
closure, which is incorrect behaviour.
This commit instead introduces an explicit new Value variant for the
internal "blueprint" that the compiler generates (essentially just the
lambda) and uses this variant to construct the closure at runtime.
If the blueprint ever leaks out to a user somehow that is a critical
bug and tvix-eval will panic.
As a ~treat~ test for this, the fibonacci function is being used as it
is a self-recursive closure (i.e. different instantiations of the same
"blueprint") getting called with different values and it's good to
have it around.
Change-Id: I485de675e9bb0c599ed7d5dc0f001eb34ab4c15f
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6323
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
This puts together the puzzle pieces for threading dynamic
upvalues (that is, upvalues resolved from the `with`-stack) all the
way through.
Reading the test case enclosed in this commit and walking through it
is recommended to understand what problem is being tackled here.
In short, because the compiler can not statically know *which*
with-scope a dynamic argument is resolved from it needs to lay the
groundwork for resolving from *all* possible scopes.
There are multiple different approaches to doing this. The approach
chosen in this commit is that if a dynamic upvalue is detected, the
compiler will emit instructions to close over this dynamic value
in *all* enclosing lambda contexts.
It uses a new instruction for this that will leave around a sentinel
value in case an identifier could not be resolved, and wire the
location of this found value (or sentinel) up through the upvalues to
the next level of nesting.
In this tradeoff, tvix potentially closes over more upvalues than are
needed (but in practice, how often do people create *really* deep
`with`-stacks? and in *this* kind of code situation? maybe we should
even warn for this!) but avoids keeping the entire attribute sets
themselves around.
Looking at the test case, each surrounding closure will close
over *all* dynamic identifiers that are referenced later on visible to
it, but only the last one for each identifier will actually end up
being used.
This also covers our bases for an additional edge-case this creates,
in which an identifier potentially resolves to a dynamic upvalue *and*
to a dynamic value within the function's own scope (again, would
anyone really do this?) by introducing a resolution instruction for
that particular case.
There is likely some potential for cleaning up this code which is
quite ugly in some parts, but as this implementation is now carefully
calibrated to work I decided it is time to commit it and clean it up
in subsequent commits.
Change-Id: Ib701e3e6da39bd2c95938d1384036ff4f9fb3749
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6322
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
This is required to efficiently construct the upvalue array at
runtime, as there are situations where during Closure construction
multiple things already have a reference to the closure (e.g. a
self-reference).
Change-Id: I35263b845fdc695dc873de489f5168d39b370f6a
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6312
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
These need to be handled specially by the runtime if the compiler
determines that a given local must be resolved via `with`.
Note that this implementation has a bug: It currently allows `with`
inside of nested lambdas to shadow statically known identifiers. This
will be cleaned up in the next commit.
Change-Id: If196b99cbd1a0f2dbb4a40a0e88cdb09a009c6b9
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6299
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
... same as the others
Change-Id: I9c8868388c10b0b6484c5bdd3799d801296c6979
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6292
Tested-by: BuildkiteCI
Reviewed-by: grfn <grfn@gws.fyi>
Fully implements the instructions for compiling closure
objects (without runtime handling yet).
Closure (and thunk) objects are created at runtime by capturing all
known upvalues. To represent this, the instructions for creating them
need to have a variable number of arguments. Due to that, this commit
introduces new variants in OpCode that are not actually operations,
but data.
If the VM is implemented correctly, the instruction pointer should
never point at these. Due to this, the VM will panic if it sees a data
operand during an execution run.
Change-Id: Ic56b49b3a42736dc437751e76df0e89c8d0619c6
Reviewed-on: https://cl.tvl.fyi/c/depot/+/6291
Tested-by: BuildkiteCI
Reviewed-by: grfn <grfn@gws.fyi>
