There's some more left, but they've been renamed/refactored out of
sight.
Change-Id: I41579dedc74342b4c5f8cb39d2995b5b0c90b0f4
Reviewed-on: https://cl.tvl.fyi/c/depot/+/9372
Tested-by: BuildkiteCI
Reviewed-by: Connor Brewster <cbrewster@hey.com>
Autosubmit: flokli <flokli@flokli.de>
In cppnix 2.17, commit b72bc4a972fe568744d98b89d63adcd504cb586c, the
libexpr pretty-printing routine was fixed so that it would no longer
pretty-print attrsets with keywords in their attrnames incorrectly.
This commit implements the corresponding fix for tvix, fixes our
tests to work with cppnix>=2.17 oracles, and expands our test cases
to cover all the keywords.
Change-Id: I4b51389cd3a9c44babc8ab2a84b383b7b0b116ca
Reviewed-on: https://cl.tvl.fyi/c/depot/+/9283
Autosubmit: Adam Joseph <adam@westernsemico.com>
Reviewed-by: sterni <sternenseemann@systemli.org>
Tested-by: BuildkiteCI
Previously, compare_strict_eval_tests() was using Strictness::Lazy.
This appears to be a minor oversight from
0ab6494286.
This commit corrects that, by changing Strictness::Lazy to
Strictness::Strict.
Change-Id: Ia2389a5d30481cd322ed55230731340b795e5d87
Reviewed-on: https://cl.tvl.fyi/c/depot/+/9282
Tested-by: BuildkiteCI
Autosubmit: Adam Joseph <adam@westernsemico.com>
Reviewed-by: sterni <sternenseemann@systemli.org>
replaceStrings would previously fail to replace the last character
in a string.
Change-Id: I43a7c960945350b2e7a5b731b7fdb617723eb38f
Reviewed-on: https://cl.tvl.fyi/c/depot/+/9151
Tested-by: BuildkiteCI
Reviewed-by: sterni <sternenseemann@systemli.org>
This passes a unit value to the function.
Change-Id: I4df3ad8fb0f35c0f110cee3349971ae28ce2878c
Reviewed-on: https://cl.tvl.fyi/c/depot/+/9101
Autosubmit: flokli <flokli@flokli.de>
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Tested-by: BuildkiteCI
builtins.div ought to truncate towards zero so that
-(builtins.div a b) == builtins.div (-a) b
-(builtins.div a b) == builtins.div a (-b)
Change-Id: I8b7c08cd7f4fa8a1363c786d42c8d484f6cd133d
Reviewed-on: https://cl.tvl.fyi/c/depot/+/9006
Autosubmit: sterni <sternenseemann@systemli.org>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Some paths might use names that are not valid UTF-8. We should be able
to represent them.
We don't actually need to touch the PathInfo structures, as they need to
represent StorePaths, which come with their own harder restrictions,
which can't encode non-UTF8 data.
While this doesn't change any of the wire format of the gRPC messages,
it does however change the interface of tvix_eval::EvalIO - its
read_dir() method does now return a list of Vec<u8>, rather than
SmolStr. Maybe this should be OsString instead?
Change-Id: I821016d9a58ec441ee081b0b9f01c9240723af0b
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8974
Autosubmit: flokli <flokli@flokli.de>
Reviewed-by: raitobezarius <tvl@lahfa.xyz>
Tested-by: BuildkiteCI
This explicitly documents behavior of C++ Nix that goes against the
intuition you'd gather from this document: that e.g. a simple select
from an attribute set causes a value to no longer be pointer equal to
its former self.
The point of documenting this is that we can show in a to be written
section on the use of pointer equality in nixpkgs that pointer equality
is only needed in a limited sense for evaluating it (C++ Nix's exterior
pointer equality). Tvix's pointer equality is far more powerful since
value identity preserving operations also preserve pointer equality,
generally speaking (this is because we implement interior pointer
equality in my made up terminology). This should eventually also be
documented.
Change-Id: I6ce7ef2d67b012f5ebc92f9e81bba33fb9dce7d0
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8856
Tested-by: BuildkiteCI
Autosubmit: sterni <sternenseemann@systemli.org>
Reviewed-by: tazjin <tazjin@tvl.su>
This fixes a subtle issue which would occasionally lead to a crash (e.g.
when evaluating (pkgs.systemd.outPath with --trace-runtime): With each
character in the string that has a multi byte representation in UTF-8,
the actual byte position and what tvix thought it was would get out of
sync. This could either lead to
* Tvix swallowing characters or jumbling characters if multi byte
characters would cause the tracked index to become out of sync with
the byte position before the first character to be escaped, or
* Tvix crashing if (in the same situation) the out of sync index would
be within a UTF-8 byte sequence.
Luckily, std's `char_indices()` iterator implements exactly what
`nix_escape_char()`'s original author had in mind with
`.chars().enumerate()`. Using `i + 1` for continuing is safe, since all
characters that need (in fact, can) to be escaped in Nix are represented
as a single byte in UTF-8.
Change-Id: I1c836f70cde3d72db1c644e9112852f0d824715e
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8952
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Autosubmit: sterni <sternenseemann@systemli.org>
The change allows applications that use tvix_serde for parsing
nix-based configuration to extend the language with domain-specific
set of features.
Change-Id: Ia86612308a167c456ecf03e93fe0fbae55b876a6
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8848
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
I've noticed this behavior when writing the admittedly cursed test case
included in this CL. Alternatively we could use some sort of machinery
using `builtins.trace`, but I don't think we capture stderr anywhere.
I've elected to put this into the eval cache itself while C++ Nix does
it in builtins.import already, namely via `realisePath`. We don't have
an equivalent for this yet, since we don't support any kind of IfD, but
we could revise that later. In any case, it seems good to encapsulate
`ImportCache` in this way, as it'll also allow using file hashes as
identifiers, for example.
C++ Nix also does our equivalent of canon_path in `builtins.import`
which we still don't, but I suspect it hardly makes a difference.
Change-Id: I05004737ca2458a4c67359d9e7d9a2f2154a0a0f
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8839
Autosubmit: sterni <sternenseemann@systemli.org>
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
When dealing with a formal argument in a function argument pattern that
has a default expression, there are two different things that can happen
at runtime: Either we select its value from the passed attribute
successfully or we need to use the default expression. Both of these may
be thunks and both of these may need finalisers. However, in the former
case this is taken care of elsewhere, the value will always be finalised
already if necessary. In the latter case we may need to finalise the
thunk resulting from the default expression. However, the thunk
corresponding to the expression may never end up in the local's stack
slot. Since finalisation goes by stack slot (and not constants), we need
to prevent a case where we don't fall back to the default expression,
but finalise anyways.
Previously, we worked around this by making `OpFinalise` ignore
non-thunks. Since finalisation of already evaluated thunks still
crashed, the faulty compilation of function pattern arguments could
still cause a crash.
As a new approach, we reinstate the old behavior of `OpFinalise` to
crash whenever encountering something that is either not a thunk or
doesn't need finalisation. This can also help catching (similar)
miscompilations in the future. To then prevent the crash, we need to
track whether we have fallen back or not at runtime. This is done using
an additional phantom on the stack that holds a new `FinaliseRequest`
value. When it comes to finalisation we check this value and
conditionally execute `OpFinalise` based on its value.
Resolves b/261 and b/265 (partially).
Change-Id: Ic04fb80ec671a2ba11fa645090769c335fb7f58b
Reviewed-on: https://cl.tvl.fyi/c/depot/+/8705
Reviewed-by: tazjin <tazjin@tvl.su>
Tested-by: BuildkiteCI
Autosubmit: sterni <sternenseemann@systemli.org>
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>
