Drop lexically redundant traces

[rhendric]

Now let's see if I can get away with a meatier change...

Motivation

The top two trace items here are redundant.

The throw trace already points the user to the expression being evaluated. Because the throw call expression is contained in both of the operator expressions, there's no need to also have traces that point out that, to get to the throw, the evaluator had to go through the operators.

Had the expression been let x = throw "oops" + 3; in 10 - x, then the trace corresponding to the subtraction would no longer be redundant. This is because the throw call is not a subexpression of 10 - x.

The general principle here is that, if the last trace on the stack (or the error position, if no traces have been added yet) points to a location that is contained within the expression being evaluated, that expression should not add its own trace item when there is an error. We can use this principle to make traces a fair bit briefer without making things any less debuggable, which is pretty exciting, since the size of Nix traces is one of the perennial complaints about them.

The general principle seems to merit a few exceptions. Most notably, we want the repeated call site traces that are generated by a recursive function not to be dropped (as in tests/functional/lang/eval-fail-mutual-recursion) — at least, not without a ‘frames omitted’ message. So call sites get more of a pass than other expressions. I'm still working out the logic of the other edge cases.

Context

The first commit is a mechanical refactoring that allows the last PosIdx used in an error or trace to be stored in ErrorInfo. Many files were touched but it shouldn't take much brainpower to verify that the right things are happening. (Focus your brainpower on the libutil header files; I may have botched the std::shared_ptr/r-value reference juggling.)

The tricky question is whether a given position appears within a given expression. This question is answered by the new comparePos virtual method on Expr (‘compare’ because it actually returns a std::partial_ordering, indicating where the expression is relative to the position). In a world where we didn't care about increasing memory usage, comparePos could be simply implemented by adding end positions to expressions during parsing. Instead, comparePos walks the expression tree looking for matches; this is a bit gnarly and also a bit dishonest, as described in the comments. In particular, the dishonesty part means that comparePos is not suitable for, say, highlighting expressions in traces, if someone had their heart set on that. But it is honest enough to use for filtering lexically redundant traces.

With comparePos in place, most of the rest of the implementation consists of scattering hasNewPos checks before addTrace calls, mostly in src/libexpr/eval.cc. (Expr::hasNewPos is a small helper that runs comparePos against the last position to be added to an error.) The form of the checks varies, because sometimes I have to do a bit of digging to find a suitable expression to compare against the last trace position. I introduced Value::maybeGetThunkExpr() to help with a common pattern of capturing an expression from a value before it is forced, to be used in the error handler. These checks are not yet comprehensive; I did the easy ones and just enough of the trickier ones to demonstrate the value of the approach.

Things ready for review

• Overall approach
• Naming and code quality for what has been done so far
• The comparePos logic in nixexpr.cc
• That every trace removed so far in the expected test outputs is in fact redundant

Things still for me to do before you bother reviewing them

• Comprehensively search for traces and add either a filter or a comment explaining why a filter isn't desirable
• Ensure that both branches of every filter are tested; remove filters that can never fail, and remove traces that can never appear

---

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

[xokdvium]

This better be noexcept right? Because it's mostly called in catch blocks and throwing in those is a terminate().

[rhendric]

Yeah, right on.

[xokdvium]

I think I kinda like the idea. Having very detailed traces might be somewhat desirable if you really do want to trace through the forcing order and the eval stack exactly as it's written, but it's probably way too much for day-to-day use.

Also, convincing oneself that the comparators do actually correctly specify a strict weak ordering is going to be non-trivial :)