Towards structured error classes #9834

9999years · 2024-01-23T00:15:46Z

Motivation

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).

Notes for reviewers

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:

EvalErrorBuilder<TypeError>(state, "'%s' is not a string", getAttrPathStr())
  .debugThrow()

Priorities and Process

Add 👍 to pull requests you find important.

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

inclyc · 2024-01-25T06:30:46Z

src/libexpr/eval-error.cc

@@ -0,0 +1,99 @@
+#include "eval-error.hh"


question about why not just write the implementation in header file, maybe eval-error.hh?

Reduces compile times, at least to an extent. I can put the implementation in the header file if that's what's preferred, though.

Reduces compile times, at least to an extent

If you only have implememtations in .cc files(formally translation unit, TUs), other libexpr users may COPY the source code again and do same instantiation.

Reduces compile times

IMHO, I think it is not a good option to do any 'optimization' without dedicated profiling. This may reduce compile time, but do make things hard somehow(as I commented above).

I don't mind this so much. If it becomes annoying it's easy to change.

I went to refactor this and remembered why I had it like this -- the PosIdx type is forward-declared (it comes from nixexpr.hh, which itself depends on eval-error.hh).

I went to refactor this and remembered why I had it like this -- the PosIdx type is forward-declared (it comes from nixexpr.hh, which itself depends on eval-error.hh).

Hmm, so how about split PosIdx definition from nixexpr.hh , to resolve this circular #includes?

Maybe we can do this in next patches, for not just keep this as-is.

Unfortunately even if you move PosIdx out of nixexpr.hh, the definition of EvalErrorBuilder<T>::withFrame depends on EvalState :(

Unfortunately even if you move PosIdx out of nixexpr.hh, the definition of EvalErrorBuilder<T>::withFrame depends on EvalState :(

That's quite unfortunate. Ideally all headers are 'self-contained', looks like those many 'forward declaration's lead to such isssue. This cannot be trivially solved, then let's keep it as-is :(.

inclyc · 2024-01-25T06:32:28Z

src/libexpr/eval-error.cc

+    throw error;
+}
+
+template class EvalErrorBuilder<EvalError>;


If you do this in .cc TUs, all instantiated classes must be listed here which may lead to extra maintenance issue.

src/libexpr/eval-error.hh

Ericson2314 · 2024-01-30T17:38:12Z

src/libexpr/eval-error.cc

+template class EvalErrorBuilder<EvalError>;
+template class EvalErrorBuilder<AssertionError>;
+template class EvalErrorBuilder<ThrownError>;
+template class EvalErrorBuilder<Abort>;
+template class EvalErrorBuilder<TypeError>;
+template class EvalErrorBuilder<UndefinedVarError>;
+template class EvalErrorBuilder<MissingArgumentError>;
+template class EvalErrorBuilder<InfiniteRecursionError>;
+template class EvalErrorBuilder<CachedEvalError>;
+template class EvalErrorBuilder<InvalidPathError>;


Are these all EvalError or subtypes of it? It would be nice to add some sort of static_assert to EvalErrorBuilder if so.

That would help make clear what exactly the "closed world' of instantiations that we need is.

Yes, they're all subtypes of EvalError.

These are all the error types we use EvalErrorBuilder with. If they're not declared here it becomes a link error. The methods on EvalErrorBuilder require a state member on the error, so in practice it'll become a compile error to use EvalErrorBuilder with an error type that's not a subtype of EvalError.

OK the state thing is good to hear.

Ericson2314 · 2024-01-30T17:43:48Z

src/libcmd/repl.cc

-        } catch (UndefinedVarError & e) {
-            // Quietly ignore undefined variable errors.


What's this?

UndefinedVarError used to be a subclass of Error. This PR changes it to be a subclass of EvalError instead. There's already a catch clause for EvalError here, so the UndefinedVarError clause is no longer needed.

OK sounds good.

Ericson2314 · 2024-01-30T17:47:50Z

Sometimes throw becomes debugThrow(). Is that fine?

Ericson2314 · 2024-01-30T17:48:58Z

In general, I like this :). But I would like to get the opinion of @pennae, and @layus (who wrote the original ErrorBuilder) too.

9999years · 2024-01-30T18:38:20Z

Sometimes throw becomes debugThrow(). Is that fine?

Yes, this will make the debugger work in more places.

layus · 2024-01-30T18:46:32Z

I wrote the builder for the same reason as this PR, to simplify the complex code that existed before. That being said, the reason for that ugly code was that building error messages from a template using fmt/format reserves a lot of stack space for a code path that is seldom used. I think @pennae could elaborate more. So, the difficulty is to avoid allocating _any_ (or just one word?) stack space for the error message. This is why all error formatting was moved to noinline, noreturn functions. My change reused a pointer to an error builder located in State. It was a bit ugly as the same memory was used by all error constructions, but hopefully they are never nested. This PR reintroduces a stack-allocated error builder that performs formatting. Meaning that it probably allocates all the formatter memory on the stack too. Now, I do not know how to test the performance impact, but at least you have the background to understand the reasons behind the previous code. Le 30 janvier 2024 18:49:10 GMT+01:00, John Ericson ***@***.***> a écrit :

…

In general, I like this :). But I would like to get the opinion of @pennae, and @layus (who wrote the original `ErrorBuilder`) too. -- Reply to this email directly or view it on GitHub: #9834 (comment) You are receiving this because you were mentioned. Message ID: ***@***.***>

Ericson2314 · 2024-01-30T19:01:18Z

I will say an ultimate goal with the structured errors stuff is to not do any formatting until the exception is rendered. The exception itself should just contain the raw structured data, no hintfmt or similar.

So while we might indeed have a problem in the short term --- thanks for pointing this out @layus --- I am confident the problem is gone in the final destination.

9999years · 2024-01-30T19:03:40Z

The exception itself should just contain the raw structured data, no hintfmt or similar.

Hmm. I agree this is a good idea but it will be challenging to implement. Currently the ErrorInfo that all errors contain includes a hintfmt.

layus

Otherwise, I have nothing against this change.

src/libexpr/eval-error.hh

Ericson2314 · 2024-01-30T19:23:27Z

The exception itself should just contain the raw structured data, no hintfmt or similar.

Hmm. I agree this is a good idea but it will be challenging to implement. Currently the ErrorInfo that all errors contain includes a hintfmt.

https://github.com/ericson2314/nix/tree/more-structured-errors I started fixing this here. I think it would be good to pick up that work in conjunction with this.

The basic idea was that the non-structured errors can still inherit from a type with the hintfmt like today, so except for some core infra things, the conversion should be pay-as-you-go.

pennae

this looks good :) would have been easier to go through if it had been split into more specific commits though.

there doesn't seem to be an eval perf impact from these changes, with or without nonlines on the builder when built with LTO enabled (haven't tried with LTO turned off).

src/libexpr/eval-error.cc

src/libexpr/eval-error.hh

pennae · 2024-01-30T20:42:49Z

src/libexpr/eval.cc

-    state.error("infinite recursion encountered")
-        .debugThrow<InfiniteRecursionError>();
+    EvalErrorBuilder<InfiniteRecursionError>(state, "infinite recursion encountered")
+        .atPos(v.determinePos(noPos))


since this position will ne overwritten anyway, is this needed?

src/libexpr/eval.cc

src/libexpr/primops.cc

pennae · 2024-01-30T21:11:52Z

src/libexpr/primops.cc

-                .msg = hintfmt("memory limit exceeded by regular expression '%s'", re),
-                .errPos = state.positions[pos]
-            }));
+            EvalErrorBuilder<EvalError>(state, "memory limit exceeded by regular expression '%s'", re)


unrelated nitpick: we're not sure it's wise to print regexes like this without escaping them as a nix string again first.

src/libexpr/primops/context.cc

src/libexpr/primops.cc

pennae

unfortunately we have to retract our performance claims due to errors in the benchmarking setup, the PR as it stands today gives a rather massive 3.5% eval perf hit. that's not good and has to be rectified :(

9999years · 2024-02-01T21:21:16Z

I've added a pre-allocated buffer to EvalState to construct errors in. This replaces the old pre-allocated pointer used for the same purpose.

pennae

we cannot reproduce nix search being that much faster (it's still about 2% slower for us), but other evaluations are under 0.5% from the merge-base value. instead of placement new we should probably be using regular new as the error paths aren't hot enough that avoiding malloc will be enough of a benefit to justify the correctness implications.

(we're not a huge fan of the chained builder pattern here and would instead move all the builder methods into EvalError, give them all a && ref qualifiers, and wrap the throwing/debugRepl interaction into a macro to stick the whole construction into a noinline function. but that can just as well be future work)

src/libexpr/eval.hh

src/libexpr/eval-error.cc

While preparing PRs like NixOS#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`.

We're on C++ 20 now, we don't need this

layus · 2024-02-02T20:14:22Z

Hi @pennae, I may have asked before, but can you share the benchmark you use ? I think we should have benchmark "tests" or at least benchmark targets for the build. If you cannot or will not share it, I will consider writing some myself. It feels wrong to use you as an oracle, and it feels odd to get performance feedback at the end of a long development process. It can bring frustration for those who do not know performance is such a strict constraint. Le 31 janvier 2024 23:41:41 GMT+01:00, pennae ***@***.***> a écrit :

…

@pennae requested changes on this pull request. unfortunately we have to retract our performance claims due to errors in the benchmarking setup, the PR as it stands today gives a rather massive 3.5% eval perf hit. that's not good and has to be rectified :( -- Reply to this email directly or view it on GitHub: #9834 (review) You are receiving this because you were mentioned. Message ID: ***@***.***>

9999years · 2024-02-02T20:19:52Z

@layus

In private correspondence, @pennae said:

[We gather performance metrics by] running nix eval --raw --impure --expr 'with import <nixpkgs/nixos> {}; system' with a vetted nixpkgs pin and NixOS config for the most part, since that tends to exercise all the bits that go into a real-world eval
hyperfine is our current favourite for gathering statistical data, and for reproducibility of results the whole things runs in a taskset -c2,3 chrt -f50 with cpu boost clocks turned off
we also run each of them on a temporary store to minimize interference with the rest of the system

pennae · 2024-02-03T01:52:57Z

we think we've described the method in detail but always avoided posting the exact inputs because we had never taken the time to clean our test system config up enough to post, but now we did. it's remarkably simple, and for best results run with cpu boosting disabled since that can introduce a bunch of uncertainty. the nixpkgs revision changes occasionally to keep up with the world, the only important thing for both nixpkgs and the system config is to remain stable (and recent/large enough) during a benchmark run.

https://gist.github.com/pennae/43ebf7709d5e13ece3912e5233ce44d9

pennae

much better now! performance impacts on system eval are down in the noise now, nix search is slightly slower but not enoguh to be bothered by it (less than 1%).

src/libexpr/eval.hh

Ericson2314 · 2024-02-06T21:53:18Z

src/libexpr/eval-error.cc

+    // any such instancve and must delete itself before throwing the underlying
+    // error.
+    auto error = std::move(this->error);
+    delete this;


OK I see this is the delete corresponding to the new. @tfc is this legal?

https://isocpp.org/wiki/faq/freestore-mgmt#delete-this OK it appears to be.

Still, I am tempted to say we should be using std::unique and debugThrow can be a && method.

tests/functional/fetchGit.sh

nixos-discourse · 2024-02-19T10:25:29Z

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-54/39990/1

github-actions bot added new-cli Relating to the "nix" command repl The Read Eval Print Loop, "nix repl" command and debugger labels Jan 23, 2024

9999years force-pushed the structured-errors branch from a4fe892 to 673703e Compare January 23, 2024 01:18

github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Jan 23, 2024

9999years force-pushed the structured-errors branch 4 times, most recently from 8d0c4e2 to bb7b162 Compare January 24, 2024 02:14

inclyc reviewed Jan 25, 2024

View reviewed changes

9999years mentioned this pull request Jan 26, 2024

Increase stack size on macOS as well as Linux #9860

Merged

9999years force-pushed the structured-errors branch 3 times, most recently from 5a14403 to 6784cf4 Compare January 26, 2024 18:14

9999years marked this pull request as ready for review January 26, 2024 18:16

9999years requested review from thufschmitt, roberth and edolstra as code owners January 26, 2024 18:16

9999years force-pushed the structured-errors branch 2 times, most recently from 4a301f3 to 3ec4cf3 Compare January 29, 2024 22:27

Ericson2314 reviewed Jan 30, 2024

View reviewed changes

layus reviewed Jan 30, 2024

View reviewed changes

src/libexpr/eval-error.hh Show resolved Hide resolved

pennae approved these changes Jan 30, 2024

View reviewed changes

pennae suggested changes Jan 31, 2024

View reviewed changes

9999years force-pushed the structured-errors branch from 3ec4cf3 to 30eda09 Compare February 1, 2024 20:30

Move PodIdx to pos-idx.hh and PosTable to pos-table.hh

c62c21e

9999years force-pushed the structured-errors branch from 30eda09 to 58206ef Compare February 1, 2024 21:13

pennae reviewed Feb 2, 2024

View reviewed changes

src/libexpr/eval.hh Outdated Show resolved Hide resolved

src/libexpr/eval.hh Outdated Show resolved Hide resolved

src/libexpr/eval-error.cc Show resolved Hide resolved

9999years added 3 commits February 1, 2024 16:39

Attach positions to errors in derivationStrict

87dc4bc

Remove EXCEPTION_NEEDS_THROW_SPEC

faaccec

We're on C++ 20 now, we don't need this

9999years force-pushed the structured-errors branch from 58206ef to faaccec Compare February 2, 2024 00:39

9999years requested review from pennae, Ericson2314 and inclyc February 2, 2024 17:46

pennae approved these changes Feb 3, 2024

View reviewed changes

Ericson2314 reviewed Feb 6, 2024

View reviewed changes

src/libexpr/eval.hh Show resolved Hide resolved

Ericson2314 reviewed Feb 6, 2024

View reviewed changes

tests/functional/fetchGit.sh Show resolved Hide resolved

9999years added 2 commits February 6, 2024 16:49

Add comments

474fc40

Add comment

9723f53

Ericson2314 approved these changes Feb 8, 2024

View reviewed changes

thufschmitt merged commit 1ba9780 into NixOS:master Feb 8, 2024
8 checks passed

9999years deleted the structured-errors branch February 8, 2024 19:43

thufschmitt mentioned this pull request Feb 14, 2024

Continuous benchmarks #4897

Open

9999years mentioned this pull request Mar 9, 2024

Make TypeError structured #10199

Open

		} catch (UndefinedVarError & e) {
		// Quietly ignore undefined variable errors.

Towards structured error classes #9834

Towards structured error classes #9834

Conversation

9999years commented Jan 23, 2024 • edited Loading

Motivation

Notes for reviewers

Priorities and Process

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inclyc Jan 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 commented Jan 30, 2024

Ericson2314 commented Jan 30, 2024

9999years commented Jan 30, 2024

layus commented Jan 30, 2024 via email

Ericson2314 commented Jan 30, 2024 • edited Loading

9999years commented Jan 30, 2024

layus left a comment

Choose a reason for hiding this comment

Ericson2314 commented Jan 30, 2024

pennae left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pennae left a comment

Choose a reason for hiding this comment

9999years commented Feb 1, 2024 • edited Loading

pennae left a comment

Choose a reason for hiding this comment

layus commented Feb 2, 2024 via email

9999years commented Feb 2, 2024 • edited Loading

pennae commented Feb 3, 2024

pennae left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

nixos-discourse commented Feb 19, 2024

9999years commented Jan 23, 2024 •

edited

Loading

inclyc Jan 29, 2024 •

edited

Loading

Ericson2314 commented Jan 30, 2024 •

edited

Loading

9999years commented Feb 1, 2024 •

edited

Loading

9999years commented Feb 2, 2024 •

edited

Loading

Ericson2314 Feb 6, 2024 •

edited

Loading