Fix, improve and test error traces#15832
Conversation
- Extract computeTraceDisplay() from showErrorInfo() for testable trace logic - Add 14 unit tests in libutil-tests/error-traces.cc for truncation, dedup, ordering - Add 13 eval-level trace tests in libexpr-tests/eval-traces.cc - Tests enforce 'read from the end' output layout - Fix tracesSeen pass-by-value bug in printSkippedTracesMaybe
The truncation now drops outer (earlier) trace frames and keeps inner
(later) frames closest to the error. The truncation message appears at
the top of the trace output, where the omitted frames would have been.
This makes the output read naturally from the bottom:
error:
(stack trace truncated; use '--show-trace' ...)
… kept outer traces
… inner traces (closest to error)
error: actual error message
The error message and its immediate context are always visible.
Three exact-match tests pinning the rendered output format: - ErrorTraceGolden.noTraces: simple error with no trace frames - ErrorTraceGolden.threeTracesNoTruncation: 3 traces within limit - ErrorTraceGolden.sixTracesWithTruncation: 6 traces, both truncated and full --show-trace variants
Set evaluation context at: - InstallableFlake::toDerivedPaths (nix build/develop with flakes) - InstallableAttrPath::toDerivedPaths (nix build -f, nix-build -A) - nix eval command - nix-build / nix-shell main eval loop - nix-instantiate processExpr - REPL evalString and loadFile
The outermost guard reflects the top-level user action (e.g. 'nix build nixpkgs#hello') which is the most useful context. Inner guards from sub-evaluations don't overwrite it. The destructor only unsets if it was the guard that originally set the context.
Separates the evaluation context mechanism from error.hh. Call sites now include nix/util/eval-context.hh instead of nix/util/error.hh, since the guard is about context, not errors. error.hh still includes eval-context.hh for currentEvalContext() used in BaseError constructors.
The guard was placed in CmdEval::run(), but --expr evaluation happens earlier during parseInstallables(). Move the guard there so that errors thrown during initial expression evaluation carry the context. The outermost-wins semantics ensure that the parseInstallables guard takes precedence over any later guard in run().
- Add functional smoke test for evalContext display with regex patterns for actual file paths and expressions - Add integration test with complex nested functions showing trace truncation persists with evalContext - Fix stack trace truncation message formatting with proper blank line separation from following trace frames
|
|
||
| std::optional<std::string> ErrorInfo::programName = std::nullopt; | ||
|
|
||
| static thread_local std::optional<std::string> evalContextStr; |
There was a problem hiding this comment.
This seems quite bad, we'd rather avoid more globals. This can be achieved by just inserting a catch block in the relevant code? Also seems quite similar to what traces at the top (outermost) can include additionally?
xokdvium
left a comment
There was a problem hiding this comment.
Overall the fix with reversing the order is great, thanks! @roberth tells me that this got mangled by a concurrent PR back when the reversal was done?
I think in general we aligned on having more characterisation tests in the lang test suite for error traces, as that's much more maintainable then handwritten unit tests that also hardcode the ANSI escapes. The coverage there is pretty good overall, and maybe a good starting point to land this would be to add a bad test, which gets fixed in a subsequent commit/PR?
There was a problem hiding this comment.
(Comment in a somewhat arbitrary place to allow for threading.)
Suggestion from the peanut gallery, since you're working on how traces are filtered and presented: how about always including the while evaluating derivation '%s' trace from src/libexpr/primops.cc, as a way of implementing #10061? If this is too unrelated, I could offer it in a separate PR, but I didn't want to step on your toes here.
Motivation
I did some heavy lifting work on the traces output some time (years?) ago, but in the meantime some of the design principles I tried to apply were reverted.
This time, I reintroduce the opinionated changes, together with a proper testing framework and tests.
Context
We already display the trace from outer to inner, in order to have the innermost trace closer to the actual error message, at the bottom of the terminal, supposedly closer to where the user lands after the error.
At the time I ensured that the innermost error traces were kept while truncating, but it got somehow reversed, and now only the outermost ones are displayed.
So I returned to display innermost ones, add tests, and moved the truncation warning at the top, where the truncation actually happens.
I have also take the opportunity to add a better error message at the top, saying what we were doing while erroring. Hopefully this can help to know which attribute failed in situation where we build multiple installables for example.
Now, I am not too sure about the tests locations, but otherwise I think it is a much needed improvement test-wise.
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.