While preparing PRs like #9753, I've had to change error messages in
dozens of code paths. It would be nice if instead of
EvalError("expected 'boolean' but found '%1%'", showType(v))
we could write
TypeError(v, "boolean")
or similar. Then, changing the error message could be a mechanical
refactor with the compiler pointing out places the constructor needs to
be changed, rather than the error-prone process of grepping through the
codebase. Structured errors would also help prevent the "same" error
from having multiple slightly different messages, and could be a first
step towards error codes / an error index.
This PR reworks the exception infrastructure in `libexpr` to
support exception types with different constructor signatures than
`BaseError`. Actually refactoring the exceptions to use structured data
will come in a future PR (this one is big enough already, as it has to
touch every exception in `libexpr`).
The core design is in `eval-error.hh`. Generally, errors like this:
state.error("'%s' is not a string", getAttrPathStr())
.debugThrow<TypeError>()
are transformed like this:
state.error<TypeError>("'%s' is not a string", getAttrPathStr())
.debugThrow()
The type annotation has moved from `ErrorBuilder::debugThrow` to
`EvalState::error`.
This extends the `error: cannot coerce a TYPE to a string` message
to print the value that could not be coerced. This helps with debugging
by making it easier to track down where the value is being produced
from, especially in errors with deep or unhelpful stack traces.
Low-hanging fruit in the spirit of #9753 and #9754 (means 9999years did
all the hard work already).
This basically prints out what was attempted to be called as function,
i.e.
map (import <nixpkgs> {}) [ 1 2 3 ]
now gives the following error message:
error:
… while calling the 'map' builtin
at «string»:1:1:
1| map (import <nixpkgs> {}) [ 1 2 3 ]
| ^
… while evaluating the first argument passed to builtins.map
error: expected a function but found a set: { _type = "pkgs"; AAAAAASomeThingsFailToEvaluate = «thunk»; AMB-plugins = «thunk»; ArchiSteamFarm = «thunk»; BeatSaberModManager = «thunk»; CHOWTapeModel = «thunk»; ChowCentaur = «thunk»; ChowKick = «thunk»; ChowPhaser = «thunk»; CoinMP = «thunk»; «18783 attributes elided»}
This does not yet resolve the coupling between packages and
derivations, but it makes the code more consistent with the
terminology, and it accentuates places where the coupling is
obvious, such as
auto drvPath = packageInfo.queryDrvPath();
if (!drvPath)
throw Error("'%s' is not a derivation", what());
... which isn't wrong, and in my opinion, doesn't even look
wrong, because it just reflects the current logic.
However, I do like that we can now start to see in the code that
this coupling is perhaps a bit arbitrary.
After this rename, we can bring the DerivingPath concept into type
and start to lift this limitation.
these symbols are used a *lot*, so it makes sense to cache them. this
mostly increases clarity of the code (however clear one may wish to call
the parser desugaring here), but it also provides a small performance
benefit.
there's no reason the parser itself should be doing semantic analysis
like bindVars. split this bit apart (retaining the previous name in
EvalState) and have the parser really do *only* parsing, decoupled from
EvalState.
most EvalState and Expr members defined here could be elsewhere, where
they'd be easier to maintain (not being embedded in a file with arcane
syntax) and *somewhat* more faithfully placed according to the path of
the file they're defined in.
most instances of this being used do not refer to the "current"
position, sometimes not even to one reasonably close by. it could also
be called `makePos` instead, but `at` seems clear in context.
ParserState better describes what this struct really is. the parser
really does modify its state (most notably position and symbol tables),
so calling it that rather than obliquely "data" (which implies being
input only) makes sense.
since nix doesn't use the bison `error` terminal anywhere any invocation
of yyerror will immediately cause a failure. since we're *already*
leaking tons of memory whatever little bit bison allocates internally
doesn't much matter any more, and we'll be replacing the parser soon anyway.
coincidentally this now also matches the error behavior of URIs when
they are disabled or ~/ paths in pure eval mode, duplicate attr
detection etc.
`FLOAT`, `INT`, and `IN` are identifers taken by macros.
The name `IN_KW` is chosen to match `OR_KW`, which is presumably named
that way for the same reason of dodging macros.
When returning a 0-length substring, avoid calling coerceToString,
since it returns a string_view with the string's length, which is
expensive to compute for large strings.
Also fingerprint and some preparatory improvements.
Testing is still not up to scratch because lots of logic is duplicated
between the workdir and commit cases.
Enabled for fetchGit, which historically had this behavior,
among other behaviors we do not want in fetchGit.
fetchTree disables this parameter by default. It can choose the
simpler behavior, as it is still experimental.
I am not confident that the filtering implementation is future
proof. It should reuse a source filtering wrapper, which I believe
Eelco has already written, but not merged yet.
The Nix team has requested that this output format remain unchanged.
I've added a warning to the man page explaining that `nix-instantiate
--eval` output will not parse correctly in many situations.
Previously, there were two mostly-identical value printers -- one in
`libexpr/eval.cc` (which didn't force values) and one in
`libcmd/repl.cc` (which did force values and also printed ANSI color
codes).
This PR unifies both of these printers into `print.cc` and provides a
`PrintOptions` struct for controlling the output, which allows for
toggling whether values are forced, whether repeated values are tracked,
and whether ANSI color codes are displayed.
Additionally, `PrintOptions` allows tuning the maximum number of
attributes, list items, and bytes in a string that will be displayed;
this makes it ideal for contexts where printing too much output (e.g.
all of Nixpkgs) is distracting. (As requested by @roberth in
https://github.com/NixOS/nix/pull/9554#issuecomment-1845095735)
Please read the tests for example output.
Future work:
- It would be nice to provide this function as a builtin, perhaps
`builtins.toStringDebug` -- a printing function that never fails would
be useful when debugging Nix code.
- It would be nice to support customizing `PrintOptions` members on the
command line, e.g. `--option to-string-max-attrs 1000`.
Also move `SourcePath` into `libutil`.
These changes allow `error.hh` and `error.cc` to access source path and
position information, which we can use to produce better error messages
(for example, we could consider omitting filenames when two or more
consecutive stack frames originate from the same file).
This avoids a Value allocation for empty list constants. During a `nix
search nixpkgs`, about 82% of all thunked lists are empty, so this
removes about 3 million Value allocations.
Performance comparison on `nix search github:NixOS/nixpkgs/e1fa12d4f6c6fe19ccb59cac54b5b3f25e160870 --no-eval-cache`:
maximum RSS: median = 3845432.0000 mean = 3845432.0000 stddev = 0.0000 min = 3845432.0000 max = 3845432.0000 [rejected?, p=0.00000, Δ=-70084.00000±0.00000]
soft page faults: median = 965395.0000 mean = 965394.6667 stddev = 1.1181 min = 965392.0000 max = 965396.0000 [rejected?, p=0.00000, Δ=-17929.77778±38.59610]
system CPU time: median = 1.8029 mean = 1.7702 stddev = 0.0621 min = 1.6749 max = 1.8417 [rejected, p=0.00064, Δ=-0.12873±0.09905]
user CPU time: median = 14.1022 mean = 14.0633 stddev = 0.1869 min = 13.8118 max = 14.3190 [not rejected, p=0.03006, Δ=-0.18248±0.24928]
elapsed time: median = 15.8205 mean = 15.8618 stddev = 0.2312 min = 15.5033 max = 16.1670 [not rejected, p=0.00558, Δ=-0.28963±0.29434]
since `up` and `values` are both pointer-aligned the type field will
also be pointer-aligned, wasting 48 bits of space on most machines. we
can get away with removing the type field altogether by encoding some
information into the `with` expr that created the env to begin with,
reducing the GC load for the absolutely massive amount of single-entry
envs we create for lambdas. this reduces memory usage of system eval by
quite a bit (reducing heap size of our system eval from 8.4GB to 8.23GB)
and gives similar savings in eval time.
running `nix eval --raw --impure --expr 'with import <nixpkgs/nixos> {}; system'`
before:
Time (mean ± σ): 5.576 s ± 0.003 s [User: 5.197 s, System: 0.378 s]
Range (min … max): 5.572 s … 5.581 s 10 runs
after:
Time (mean ± σ): 5.408 s ± 0.002 s [User: 5.019 s, System: 0.388 s]
Range (min … max): 5.405 s … 5.411 s 10 runs
This fixes a segfault on infinite function call recursion (rather than
infinite thunk recursion) by tracking the function call depth in
`EvalState`.
Additionally, to avoid printing extremely long stack traces, stack
frames are now deduplicated, with a `(19997 duplicate traces omitted)`
message. This should only really be triggered in infinite recursion
scenarios.
Before:
$ nix-instantiate --eval --expr '(x: x x) (x: x x)'
Segmentation fault: 11
After:
$ nix-instantiate --eval --expr '(x: x x) (x: x x)'
error: stack overflow
at «string»:1:14:
1| (x: x x) (x: x x)
| ^
$ nix-instantiate --eval --expr '(x: x x) (x: x x)' --show-trace
error:
… from call site
at «string»:1:1:
1| (x: x x) (x: x x)
| ^
… while calling anonymous lambda
at «string»:1:2:
1| (x: x x) (x: x x)
| ^
… from call site
at «string»:1:5:
1| (x: x x) (x: x x)
| ^
… while calling anonymous lambda
at «string»:1:11:
1| (x: x x) (x: x x)
| ^
… from call site
at «string»:1:14:
1| (x: x x) (x: x x)
| ^
(19997 duplicate traces omitted)
error: stack overflow
at «string»:1:14:
1| (x: x x) (x: x x)
| ^
Previously, IFDs would be built within the eval store, even though one
is typically using `--eval-store` precisely to *avoid* local builds.
Because the resulting Nix expression must be copied back to the eval
store in order to be imported, this requires the eval store to trust
the build store's signatures.
There's no good reason to deprecate it:
- For consistency reasons it should continue to exist, such that all
primitive types have a corresponding `builtins.is*` primop.
- There's no implementation cost to continuing to have this function
- It costs users time to try to migrate away from it, e.g.
https://github.com/NixOS/nixpkgs/pull/219747 and https://github.com/NixOS/nixpkgs/pull/275548
- Using it can give easier-to-read code like `all isNull list`
Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
this also reduces forceValue code size and removes the need for
hideInDiagnostics. coopting thunk forcing like this has the additional
benefit of clarifying how these errors can happen in the first place.
forceValue is extremely hot. interestingly adding likeliness annotations
to the branches does not seem to make a difference.
before:
Time (mean ± σ): 4.224 s ± 0.005 s [User: 3.711 s, System: 0.512 s]
Range (min … max): 4.218 s … 4.234 s 10 runs
after:
Time (mean ± σ): 4.140 s ± 0.009 s [User: 3.647 s, System: 0.492 s]
Range (min … max): 4.130 s … 4.152 s 10 runs
almost all uses of this are interactive, except for deepSeq. deepSeq is
going to be expensive and rare enough to not care much about, and
Value::determinePos should usually be cheap enough to not be too much of
a burden in any case.
~1% parser speedup from not using TLS indirections, less on system eval.
this could have also gone in flex yyextra data, but that's significantly
slower for some reason (albeit still faster than thread locals).
before:
Time (mean ± σ): 4.231 s ± 0.004 s [User: 3.725 s, System: 0.504 s]
Range (min … max): 4.226 s … 4.240 s 10 runs
after:
Time (mean ± σ): 4.224 s ± 0.005 s [User: 3.711 s, System: 0.512 s]
Range (min … max): 4.218 s … 4.234 s 10 runs
~2% speedup on parsing without eval, less (but still significant) on
system eval. having flex generate faster parsers leads to very strange
misparses. maybe re2c is worth investigating.
before:
Time (mean ± σ): 4.260 s ± 0.003 s [User: 3.754 s, System: 0.505 s]
Range (min … max): 4.257 s … 4.266 s 10 runs
after:
Time (mean ± σ): 4.231 s ± 0.004 s [User: 3.725 s, System: 0.504 s]
Range (min … max): 4.226 s … 4.240 s 10 runs
checking for isBlackhole in the forceValue hot path is rather more
expensive than necessary, and with a little bit of trickery we can move
such handling into the isApp case. small performance benefit, but under
some circumstances we've seen 2% improvement as well.
〉 nix eval --raw --impure --expr 'with import <nixpkgs/nixos> {}; system'
before:
Time (mean ± σ): 4.429 s ± 0.002 s [User: 3.929 s, System: 0.500 s]
Range (min … max): 4.427 s … 4.433 s 10 runs
after:
Time (mean ± σ): 4.396 s ± 0.002 s [User: 3.894 s, System: 0.501 s]
Range (min … max): 4.393 s … 4.399 s 10 runs
`eval-system` option overrides just the value of `builtins.currentSystem`.
This is more useful than overriding `system` since you can build these
derivations on remote builders which can work on the given system.
Co-authored-by: John Ericson <John.Ericson@Obsidian.Systems>
Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
* document `fetchTree`
* display experimental feature note at the top
we have to enable the new `fetchTree` experimental feature to render it
at all. this was a bug introduced when adding that new feature flag.
Co-authored-by: tomberek <tomberek@users.noreply.github.com>
Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
Co-authored-by: Silvan Mosberger <github@infinisil.com>
According https://en.cppreference.com/w/cpp/io/strstream, it has been
deprecated since C++98! The Clang + Linux build systems to not have it
at all, or at least be hiding it.
We can just use `std::stringstream` instead, I think.
* Print the value in `error: cannot coerce` messages
This extends the `error: cannot coerce a TYPE to a string` message
to print the value that could not be coerced. This helps with debugging
by making it easier to track down where the value is being produced
from, especially in errors with deep or unhelpful stack traces.
Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
This includes position information in more places, making debugging
easier.
Before:
```
$ nix-instantiate --show-trace --eval tests/functional/lang/eval-fail-using-set-as-attr-name.nix
error:
… while evaluating an attribute name
at «none»:0: (source not available)
error: value is a set while a string was expected
```
After:
```
error:
… while evaluating an attribute name
at /pwd/lang/eval-fail-using-set-as-attr-name.nix:5:10:
4| in
5| attr.${key}
| ^
6|
error: value is a set while a string was expected
```
Today, with the tests inside a `tests` intermingled with the
corresponding library's source code, we have a few problems:
- We have to be careful that wildcards don't end up with tests being
built as part of Nix proper, or test headers being installed as part
of Nix proper.
- Tests in libraries but not executables is not right:
- It means each executable runs the previous unit tests again, because
it needs the libraries.
- It doesn't work right on Windows, which doesn't want you to load a
DLL just for the side global variable . It could be made to work
with the dlopen equivalent, but that's gross!
This reorg solves these problems.
There is a remaining problem which is that sibbling headers (like
`hash.hh` the test header vs `hash.hh` the main `libnixutil` header) end
up shadowing each other. This PR doesn't solve that. That is left as
future work for a future PR.
Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
