Adds new tests for foldl', intersectAttrs as well as fills in missing
.exp files.
New test cases we don't pass:
- fromTOML timestamp capabilities
- path antiquotation
- replaceStrings is lazier on C++ Nix master
The C++ Nix revision used is 7066d21a0ddb421967980094222c4bc1f5a0f45a.
Change-Id: Ic619c96e2d41e6c5ea6fa93f9402b12e564af3c5
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8778
Reviewed-by: tazjin <tazjin@tvl.su>
Autosubmit: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
genericClosure has very limited support for pointer equality: It relies
on comparison (not equality!) in C++ Nix, so as soon as C++ Nix supports
comparing lists (langVersion >= 6) we can rely on pointer equality for
key.
Since Tvix uses equality, not comparison for the insert, our behavior is
currently different, as documented by the notyetpassing tests.
Change-Id: Ifcd741ed4fc3ccc3825f7038875d56a9918b786a
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8720
Tested-by: BuildkiteCI
Autosubmit: sterni <sternenseemann@systemli.org>
Reviewed-by: tazjin <tazjin@tvl.su>
In order for the test suite we have currently to be comparable to C++
Nix, we need to display values in the same way. This was largely the
case except in some weird cases.
* <CODE> for thunks and <CYCLE> for repeated thunks (?) are already in
use. <CODE> formatting is tested by the oracle test suite already.
* Instead of lambda, we need to use <LAMBDA>
* <<primop>> and <<primop-app>> (a formatting C++ Nix uses nowhere)
now are <PRIMOP> and <PRIMOP-APP>.
We'll probably want to have a fancier display of values (in a separate
trait) down the line. This could be used for interactive usage, e.g. the
REPL or a potential debugger.
There is a peculiarity with C++ Nix 2.3 formatting primops: import is
considered a <<PRIMOP-APP>>, since it is internally implemented by means
of scopedImport. This implementation detail no longer leaks in C++ Nix
2.13 nor in Tvix.
<CYCLE> display is untested at the moment, since we exhibit a
discrepancy to C++ Nix 2.3. Our current detection is more similar to C++
Nix 2.13—luckily it is also the more consistent of the two. See also
b/245.
Change-Id: I1d534434b02e470bf5475b3758920ea81e3420dc
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8760
Reviewed-by: tazjin <tazjin@tvl.su>
Autosubmit: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
These were added by us in r/5276, so they should go into our test suite.
Change-Id: I6dc74fc242f33c22a17e0b4aee546ccae886ac85
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8774
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Autosubmit: sterni <sternenseemann@systemli.org>
C++ Nix resolves home relative paths at [parse] time. This is not an
option for us, since it prevents being able to separate the compilation
and execution phase later (e.g. precompiled nix expressions). However, a
practical consequence of this is that paths expressions are always
literals (strict) and never thunks.
[parse]: 7066d21a0d/src/libexpr/parser.y (L518-L527)
Change-Id: Ie4b9dc68f62c86d6c7fd5f1c9460c850d97ed1ca
Reviewed-on: https://cl.tvl.fyi/c/depot/+/7041
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
Unfortunately, nixpkgs has at least one case[1] where the out environment
variable is shadowed -- though it doesn't cause a problem, since it's
shadowed with the correct value, odd as this may be!
[1]: c7c2984716/pkgs/development/python-modules/pybind11/default.nix (L19)
Change-Id: Ibf6790d2484dc9cce8e424feeb5886664d498dc3
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8696
Autosubmit: tazjin <tazjin@tvl.su>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
When comparing to C++ Nix, we notice that the thunking of default
expressions in function formals corresponds to their normal thunking,
e.g. literals are not thunked. This means that we can just invoke
compile() without much of a care and trust that it will sort it out
correctly.
If function formals blow up as a result of this, it likely indicates
that the expression is treated incorrectly by compile(), not
compile_param_pattern().
Change-Id: I64acbff2f251423eb72ce43e56a0603379305e1d
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8704
Autosubmit: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
Reviewed-by: tazjin <tazjin@tvl.su>
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>
