Make TypeError structured
#10199
Make TypeError structured
#10199
Conversation
2a62e05
to
776e652
Compare
776e652
to
1de04fd
Compare
| @@ -57,18 +57,18 @@ std::pair<Value *, PosIdx> findAlongAttrPath(EvalState & state, const std::strin | |||
| Value * vNew = state.allocValue(); | |||
| state.autoCallFunction(autoArgs, *v, *vNew); | |||
| v = vNew; | |||
| state.forceValue(*v, noPos); | |||
| state.forceValue(*v, pos); | |||
There was a problem hiding this comment.
Are you sure? FWIW it came from c1ca4f0
| @@ -500,15 +500,15 @@ std::shared_ptr<AttrCursor> AttrCursor::maybeGetAttr(Symbol name, bool forceErro | |||
| // evaluate to see whether 'name' exists | |||
| } else | |||
| return nullptr; | |||
| //error<TypeError>("'%s' is not an attribute set", getAttrPathStr()).debugThrow(); | |||
| //error<EvalError>("'%s' is not an attribute set", getAttrPathStr()).debugThrow(); | |||
There was a problem hiding this comment.
| //error<EvalError>("'%s' is not an attribute set", getAttrPathStr()).debugThrow(); | |
| //error<TypeError>("nAttrs, getAttrPathStr()).debugThrow(); |
Is this better?
There was a problem hiding this comment.
Oh I guess getAttrPathStr is not the value, however
There was a problem hiding this comment.
Still, unless we are sure we never catch a TypeError, I am a bit wary of changing this
| @@ -574,14 +574,15 @@ std::string AttrCursor::getString() | |||
| debug("using cached string attribute '%s'", getAttrPathStr()); | |||
| return s->first; | |||
| } else | |||
| root->state.error<TypeError>("'%s' is not a string", getAttrPathStr()).debugThrow(); | |||
| // TODO: Is this an internal error? Where is literally any documentation for the eval cache? | |||
There was a problem hiding this comment.
There probably is no documentation :(
From seeing how this is used, I think this cursor thing can either just traverse a regular "uncached" value, or traverse a cache entry's serialized value. The getString part is just the caller asking for a specific type; it's not an internal error.
| [[nodiscard, gnu::noinline]] EvalErrorBuilder<T> & withFrameTrace(PosIdx pos, const std::string_view text); | ||
|
|
| std::string_view showType(ValueType type, bool article); | ||
|
|
There was a problem hiding this comment.
| std::string_view showType(ValueType type, bool article); |
It's in value.hh right?
| case nExternal: return WA("an", "external value"); | ||
| case nFloat: return WA("a", "float"); | ||
| case nThunk: return WA("a", "thunk"); | ||
| } |
There was a problem hiding this comment.
If we stick with macro
| } | |
| } | |
| #undef WA |
|
|
||
| std::string_view showType(ValueType type, bool withArticle) | ||
| { | ||
| #define WA(a, w) withArticle ? a " " w : w |
There was a problem hiding this comment.
I wonder if constexpr stuff would work for this or not
| // Allow selecting a subset of enum values | ||
| #pragma GCC diagnostic push | ||
| #pragma GCC diagnostic ignored "-Wswitch-enum" |
There was a problem hiding this comment.
Can we just list the ones we want to combine below with follow-through? There is a attribute thing to say "this follow-through is intentional".
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2024-03-11-nix-team-meeting-132/42960/1 |
Motivation
With #9834 merged, we can now start converting stringly-typed errors (which just contain a string message) to structured errors (which contain the data to format the string message).
This is good because it means we can just change the formatting code in one place. Structured error classes increase the consistency of error messages and makes refactors like #9754 much easier. Structured error classes also helps us clarify our exception hierarchy. (I've found several separate classes of error while preparing this PR — I've converted these to
EvalErrorfor now and will leave them for future PRs.) Finally, structured error classes lay the groundwork for unique error codes (like many other programming languages support).Priorities and Process
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.