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>
