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`.
(cherry picked from commit c6a89c1a1659b31694c0fbcd21d78a6dd521c732)
Change-Id: Iced91ba4e00ca9e801518071fb43798936cbd05a
Enter debugger more reliably in `let` expressions and function calls
(cherry picked from commit c4ed92fa6f836d3d8eb354a48c37a2f9eeecc3aa)
Change-Id: I16d0cad7e898feecd2399723b92ba8df67222fb4
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.
(cherry picked from commit 09a1128d9e2ff0ae6176784938047350d6f8a782)
Change-Id: I73d9f66be4555168e048cb2d542277251580c2d1
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.
(cherry picked from commit b596cc9e7960b9256bcd557334d81e9d555be5a2)
Change-Id: I481a7623afc783e9d28a6eb4627552cf8a780986
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.
(cherry picked from commit e1aa585964c3d864ebff0030584f3349a539d615)
Change-Id: Ibc704567462bb40f37cda05d8fadd465519db5f5
Print the value in `error: cannot coerce` messages
(cherry picked from commit 5b7bfd2d6b89d7dd5f54c1ca6c8072358d31a84e)
===
test taken from 6e8d5983143ae576e3f4b1d2954a5267f2943a49; it was added
previously (and not backported because its pr was a mostly-revert), but
it's useful to have around.
Change-Id: Icbd14b55e3610ce7b774667bf14b82e6dc717982
libexpr: print value of what is attempted to be called as function
(cherry picked from commit 50e5d7b883042852538371237e32a66bb22f0485)
Change-Id: I7cb6290bd8f244e83bfce3b2eed2a4c8b4f16a83
Print the value in `value is X while a Y is expected` error
(cherry picked from commit 5f72a97092da6af28a7d2b2a50d74e9d34fae7e1)
Change-Id: Idb4bc903ae59a0f5b6fb3b1da4d47970fe0a6efe
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`.
(cherry picked from commit 0fa08b451682fb3311fe58112ff05c4fe5bee3a4, )
===
Restore ambiguous value printer for `nix-instantiate`
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.
(cherry picked from commit df84dd4d8dd3fd6381ac2ca3064432ab31a16b79)
Change-Id: I7cca6b4b53cd0642f2d49af657d5676a8554c9f8
Combine `AbstractPos`, `PosAdapter`, and `Pos`
(cherry picked from commit 113499d16fc87d53b73fb62fe6242154909756ed)
===
this is a bit cursed because originally it was based on InputAccessor
code that we don't have and moved/patched features we likewise don't
have (fetchToStore caching, all the individual accessors,
ContentAddressMethod). the commit is adjusted accordingly to
match (remove caching, ignore accessors, use FileIngestionMethod).
note that `state.rootPath . CanonPath == abs` and
computeStorePathForPath works relative to cwd, so the slight rewrite in
the moved fetchToStore is legal.
Change-Id: I05fd340c273f0bcc8ffabfebdc4a88b98083bce5
Add position information to `while evaluating the attribute` errors in the debugger
(cherry picked from commit ffe67c86a8ef3695e5c8b9c9800c192ac633dded)
Change-Id: I177ea5ec60898abe09fb9d80d9602b2a32ff8f44
Fix segfault on infinite recursion in some cases
(cherry picked from commit bf1b294bd81ca76c5ec9fe3ecd52196bf52a8300)
Change-Id: Id137541426ec8536567835953fccf986a3aebf16
Revert use of boost::container::small_vector in the evaluator
(cherry picked from commit 6832d18ac734f4b855f97c07b158491dd01cefcd)
Change-Id: I7f10af0c8b8a8beb4b1e36424120995f4ed82738
This function is now trivial enough that it doesn't need to exist.
`EvalState` can still be initialized with a custom search path, but we
don't have a need to mutate the search path after it has been
constructed, and I don't see why we would need to in the future.
Fixes#8229
Types converted:
- `NixStringContextElem`
- `OutputsSpec`
- `ExtendedOutputsSpec`
- `DerivationOutput`
- `DerivationType`
Existing ones mostly conforming the pattern cleaned up:
- `ContentAddressMethod`
- `ContentAddressWithReferences`
The `DerivationGoal::derivationType` field had a bogus initialization,
now caught, so I made it `std::optional`. I think #8829 can make it
non-optional again because it will ensure we always have the derivation
when we construct a `DerivationGoal`.
See that issue (#7479) for details on the general goal.
`git grep 'Raw::Raw'` indicates the two types I didn't yet convert
`DerivedPath` and `BuiltPath` (and their `Single` variants) . This is
because @roberth and I (can't find issue right now...) plan on reworking
them somewhat, so I didn't want to churn them more just yet.
Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
`EvalState::mkSingleDerivedPathString` previously contained its own
inverse (printing, rather than parsing) in order to validate what was
parsed. Now that is pulled out into its own separate function:
`EvalState::coerceToSingleDerivedPath`.
In additional that pulled out logic is deduplicated with
`EvalState::mkOutputString` via `EvalState::mkOutputStringRaw`, which is
itself deduplicated (and generalized) with
`DownstreamPlaceholder::mkOutputStringRaw`.
All these changes make the unit tests simpler.
(We would ideally write more unit tests for `mkSingleDerivedPathString`
`coerceToSingleDerivedPath` directly, but we cannot yet do that because
the IO in reading the store path won't work when the dummy store cannot
hold anything. Someday we'll have a proper in-memory store which will
work for this.)
Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
We want to be able to write down `foo.drv^bar.drv^baz`:
`foo.drv^bar.drv` is the dynamic derivation (since it is itself a
derivation output, `bar.drv` from `foo.drv`).
To that end, we create `Single{Derivation,BuiltPath}` types, that are
very similar except instead of having multiple outputs (in a set or
map), they have a single one. This is for everything to the left of the
rightmost `^`.
`NixStringContextElem` has an analogous change, and now can reuse
`SingleDerivedPath` at the top level. In fact, if we ever get rid of
`DrvDeep`, `NixStringContextElem` could be replaced with
`SingleDerivedPath` entirely!
Important note: some JSON formats have changed.
We already can *produce* dynamic derivations, but we can't refer to them
directly. Today, we can merely express building or example at the top
imperatively over time by building `foo.drv^bar.drv`, and then with a
second nix invocation doing `<result-from-first>^baz`, but this is not
declarative. The ethos of Nix of being able to write down the full plan
everything you want to do, and then execute than plan with a single
command, and for that we need the new inductive form of these types.
Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
- Better types
- Own header / C++ file pair
- Test factored out methods
- Pass parsed thing around more than strings
Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
This is done in roughly the same way builtin functions are documented.
Also auto-link experimental features for primops, subsuming PR #8371.
Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
This fixes a bug in commands like `nix eval' which would emit invalid attribute
sets if they contained reserved keywords such as "assert", "let", etc.
These keywords will not be quoted when printed, making them valid expressions.
All keywords recognized by the lexer are quoted except "or", which does not
require quotation.
Motivation
`PathSet` is not correct because string contexts have other forms
(`Built` and `DrvDeep`) that are not rendered as plain store paths.
Instead of wrongly using `PathSet`, or "stringly typed" using
`StringSet`, use `std::std<StringContextElem>`.
-----
In support of this change, `NixStringContext` is now defined as
`std::std<StringContextElem>` not `std:vector<StringContextElem>`. The
old definition was just used by a `getContext` method which was only
used by the eval cache. It can be deleted altogether since the types are
now unified and the preexisting `copyContext` function already suffices.
Summarizing the previous paragraph:
Old:
- `value/context.hh`: `NixStringContext = std::vector<StringContextElem>`
- `value.hh`: `NixStringContext Value::getContext(...)`
- `value.hh`: `copyContext(...)`
New:
- `value/context.hh`: `NixStringContext = std::set<StringContextElem>`
- `value.hh`: `copyContext(...)`
----
The string representation of string context elements no longer contains
the store dir. The diff of `src/libexpr/tests/value/context.cc` should
make clear what the new representation is, so we recommend reviewing
that file first. This was done for two reasons:
Less API churn:
`Value::mkString` and friends did not take a `Store` before. But if
`NixStringContextElem::{parse, to_string}` *do* take a store (as they
did before), then we cannot have the `Value` functions use them (in
order to work with the fully-structured `NixStringContext`) without
adding that argument.
That would have been a lot of churn of threading the store, and this
diff is already large enough, so the easier and less invasive thing to
do was simply make the element `parse` and `to_string` functions not
take the `Store` reference, and the easiest way to do that was to simply
drop the store dir.
Space usage:
Dropping the `/nix/store/` (or similar) from the internal representation
will safe space in the heap of the Nix programming being interpreted. If
the heap contains many strings with non-trivial contexts, the saving
could add up to something significant.
----
The eval cache version is bumped.
The eval cache serialization uses `NixStringContextElem::{parse,
to_string}`, and since those functions are changed per the above, that
means the on-disk representation is also changed.
This is simply done by changing the name of the used for the eval cache
from `eval-cache-v4` to eval-cache-v5`.
----
To avoid some duplication `EvalCache::mkPathString` is added to abstract
over the simple case of turning a store path to a string with just that
string in the context.
Context
This PR picks up where #7543 left off. That one introduced the fully
structured `NixStringContextElem` data type, but kept `PathSet context`
as an awkward middle ground between internal `char[][]` interpreter heap
string contexts and `NixStringContext` fully parsed string contexts.
The infelicity of `PathSet context` was specifically called out during
Nix team group review, but it was agreeing that fixing it could be left
as future work. This is that future work.
A possible follow-up step would be to get rid of the `char[][]`
evaluator heap representation, too, but it is not yet clear how to do
that. To use `NixStringContextElem` there we would need to get the STL
containers to GC pointers in the GC build, and I am not sure how to do
that.
----
PR #7543 effectively is writing the inverse of a `mkPathString`,
`mkOutputString`, and one more such function for the `DrvDeep` case. I
would like that PR to have property tests ensuring it is actually the
inverse as expected.
This PR sets things up nicely so that reworking that PR to be in that
more elegant and better tested way is possible.
Co-authored-by: Théophane Hufschmitt <7226587+thufschmitt@users.noreply.github.com>
eldritch horrors
Yorick
Robert Hensing