Towards structured error classes #9834
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,113 @@ | ||
| #include "eval-error.hh" | ||
|
Member
There was a problem hiding this comment. question about why not just write the implementation in header file, maybe
Contributor
Author
There was a problem hiding this comment. Reduces compile times, at least to an extent. I can put the implementation in the header file if that's what's preferred, though.
Member
There was a problem hiding this comment.
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.
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).
Member
There was a problem hiding this comment. I don't mind this so much. If it becomes annoying it's easy to change.
Contributor
Author
There was a problem hiding this comment. I went to refactor this and remembered why I had it like this -- the
Member
There was a problem hiding this comment.
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.
Contributor
Author
There was a problem hiding this comment. Unfortunately even if you move
Member
There was a problem hiding this comment.
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 :(. |
||
| #include "eval.hh" | ||
| #include "value.hh" | ||
|
|
||
| namespace nix { | ||
|
|
||
| template<class T> | ||
| EvalErrorBuilder<T> & EvalErrorBuilder<T>::withExitStatus(unsigned int exitStatus) | ||
| { | ||
| error.withExitStatus(exitStatus); | ||
| return *this; | ||
| } | ||
|
|
||
| template<class T> | ||
| EvalErrorBuilder<T> & EvalErrorBuilder<T>::atPos(PosIdx pos) | ||
| { | ||
| error.err.pos = error.state.positions[pos]; | ||
| return *this; | ||
| } | ||
|
|
||
| template<class T> | ||
| EvalErrorBuilder<T> & EvalErrorBuilder<T>::atPos(Value & value, PosIdx fallback) | ||
| { | ||
| return atPos(value.determinePos(fallback)); | ||
| } | ||
|
|
||
| template<class T> | ||
| EvalErrorBuilder<T> & EvalErrorBuilder<T>::withTrace(PosIdx pos, const std::string_view text) | ||
| { | ||
| error.err.traces.push_front( | ||
| Trace{.pos = error.state.positions[pos], .hint = hintfmt(std::string(text)), .frame = false}); | ||
| return *this; | ||
| } | ||
|
|
||
| template<class T> | ||
| EvalErrorBuilder<T> & EvalErrorBuilder<T>::withFrameTrace(PosIdx pos, const std::string_view text) | ||
| { | ||
| error.err.traces.push_front( | ||
| Trace{.pos = error.state.positions[pos], .hint = hintformat(std::string(text)), .frame = true}); | ||
| return *this; | ||
| } | ||
|
|
||
| template<class T> | ||
| EvalErrorBuilder<T> & EvalErrorBuilder<T>::withSuggestions(Suggestions & s) | ||
| { | ||
| error.err.suggestions = s; | ||
| return *this; | ||
| } | ||
|
|
||
| template<class T> | ||
| EvalErrorBuilder<T> & EvalErrorBuilder<T>::withFrame(const Env & env, const Expr & expr) | ||
| { | ||
| // NOTE: This is abusing side-effects. | ||
| // TODO: check compatibility with nested debugger calls. | ||
| // TODO: What side-effects?? | ||
| error.state.debugTraces.push_front(DebugTrace{ | ||
| .pos = error.state.positions[expr.getPos()], | ||
| .expr = expr, | ||
| .env = env, | ||
| .hint = hintformat("Fake frame for debugging purposes"), | ||
| .isError = true}); | ||
| return *this; | ||
| } | ||
|
|
||
| template<class T> | ||
| EvalErrorBuilder<T> & EvalErrorBuilder<T>::addTrace(PosIdx pos, hintformat hint, bool frame) | ||
| { | ||
| error.addTrace(error.state.positions[pos], hint, frame); | ||
| return *this; | ||
| } | ||
|
|
||
| template<class T> | ||
| template<typename... Args> | ||
| EvalErrorBuilder<T> & | ||
| EvalErrorBuilder<T>::addTrace(PosIdx pos, std::string_view formatString, const Args &... formatArgs) | ||
| { | ||
|
|
||
| addTrace(error.state.positions[pos], hintfmt(std::string(formatString), formatArgs...)); | ||
| return *this; | ||
| } | ||
|
|
||
| template<class T> | ||
| void EvalErrorBuilder<T>::debugThrow() | ||
| { | ||
| if (error.state.debugRepl && !error.state.debugTraces.empty()) { | ||
| const DebugTrace & last = error.state.debugTraces.front(); | ||
| const Env * env = &last.env; | ||
| const Expr * expr = &last.expr; | ||
| error.state.runDebugRepl(&error, *env, *expr); | ||
| } | ||
|
|
||
| // `EvalState` is the only class that can construct an `EvalErrorBuilder`, | ||
| // and it does so in dynamic storage. This is the final method called on | ||
| // any such instance and must delete itself before throwing the underlying | ||
| // error. | ||
| auto error = std::move(this->error); | ||
| delete this; | ||
|
Member
There was a problem hiding this comment. OK I see this is the
Member
There was a problem hiding this comment. https://isocpp.org/wiki/faq/freestore-mgmt#delete-this OK it appears to be. Still, I am tempted to say we should be using |
||
|
|
||
| throw error; | ||
9999years marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| template class EvalErrorBuilder<EvalError>; | ||
|
Member
There was a problem hiding this comment. If you do this in |
||
| 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>; | ||
|
Comment on lines
+102
to
+111
Member
There was a problem hiding this comment. Are these all That would help make clear what exactly the "closed world' of instantiations that we need is.
Contributor
Author
There was a problem hiding this comment. Yes, they're all subtypes of These are all the error types we use
Member
There was a problem hiding this comment. OK the |
||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| #pragma once | ||
|
|
||
| #include <algorithm> | ||
|
|
||
| #include "error.hh" | ||
| #include "pos-idx.hh" | ||
|
|
||
| namespace nix { | ||
|
|
||
| struct Env; | ||
| struct Expr; | ||
| struct Value; | ||
|
|
||
| class EvalState; | ||
| template<class T> | ||
| class EvalErrorBuilder; | ||
|
|
||
| class EvalError : public Error | ||
| { | ||
| template<class T> | ||
| friend class EvalErrorBuilder; | ||
| public: | ||
| EvalState & state; | ||
|
|
||
| EvalError(EvalState & state, ErrorInfo && errorInfo) | ||
| : Error(errorInfo) | ||
| , state(state) | ||
| { | ||
| } | ||
|
|
||
| template<typename... Args> | ||
| explicit EvalError(EvalState & state, const std::string & formatString, const Args &... formatArgs) | ||
| : Error(formatString, formatArgs...) | ||
| , state(state) | ||
| { | ||
| } | ||
| }; | ||
|
|
||
| MakeError(ParseError, Error); | ||
| MakeError(AssertionError, EvalError); | ||
| MakeError(ThrownError, AssertionError); | ||
| MakeError(Abort, EvalError); | ||
| MakeError(TypeError, EvalError); | ||
| MakeError(UndefinedVarError, EvalError); | ||
| MakeError(MissingArgumentError, EvalError); | ||
| MakeError(CachedEvalError, EvalError); | ||
| MakeError(InfiniteRecursionError, EvalError); | ||
|
|
||
| struct InvalidPathError : public EvalError | ||
| { | ||
| public: | ||
| Path path; | ||
| InvalidPathError(EvalState & state, const Path & path) | ||
| : EvalError(state, "path '%s' is not valid", path) | ||
| { | ||
| } | ||
| }; | ||
|
|
||
| /** | ||
| * `EvalErrorBuilder`s may only be constructed by `EvalState`. The `debugThrow` | ||
| * method must be the final method in any such `EvalErrorBuilder` usage, and it | ||
| * handles deleting the object. | ||
| */ | ||
| template<class T> | ||
| class EvalErrorBuilder final | ||
| { | ||
| friend class EvalState; | ||
|
|
||
| template<typename... Args> | ||
| explicit EvalErrorBuilder(EvalState & state, const Args &... args) | ||
| : error(T(state, args...)) | ||
| { | ||
| } | ||
|
|
||
| public: | ||
| T error; | ||
|
|
||
| [[nodiscard, gnu::noinline]] EvalErrorBuilder<T> & withExitStatus(unsigned int exitStatus); | ||
9999years marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| [[nodiscard, gnu::noinline]] EvalErrorBuilder<T> & atPos(PosIdx pos); | ||
|
|
||
| [[nodiscard, gnu::noinline]] EvalErrorBuilder<T> & atPos(Value & value, PosIdx fallback = noPos); | ||
|
|
||
| [[nodiscard, gnu::noinline]] EvalErrorBuilder<T> & withTrace(PosIdx pos, const std::string_view text); | ||
|
|
||
| [[nodiscard, gnu::noinline]] EvalErrorBuilder<T> & withFrameTrace(PosIdx pos, const std::string_view text); | ||
|
|
||
| [[nodiscard, gnu::noinline]] EvalErrorBuilder<T> & withSuggestions(Suggestions & s); | ||
|
|
||
| [[nodiscard, gnu::noinline]] EvalErrorBuilder<T> & withFrame(const Env & e, const Expr & ex); | ||
|
|
||
| [[nodiscard, gnu::noinline]] EvalErrorBuilder<T> & addTrace(PosIdx pos, hintformat hint, bool frame = false); | ||
|
|
||
| template<typename... Args> | ||
| [[nodiscard, gnu::noinline]] EvalErrorBuilder<T> & | ||
| addTrace(PosIdx pos, std::string_view formatString, const Args &... formatArgs); | ||
|
|
||
| /** | ||
| * Delete the `EvalErrorBuilder` and throw the underlying exception. | ||
| */ | ||
| [[gnu::noinline, gnu::noreturn]] void debugThrow(); | ||
| }; | ||
|
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UndefinedVarErrorused to be a subclass ofError. This PR changes it to be a subclass ofEvalErrorinstead. There's already acatchclause forEvalErrorhere, so theUndefinedVarErrorclause is no longer needed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK sounds good.