Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make TypeError structured #10199

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Make TypeError structured #10199

wants to merge 1 commit into from

Conversation

9999years
Copy link
Contributor

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 EvalError for 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.

@github-actions github-actions bot added new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority labels Mar 9, 2024
@9999years 9999years force-pushed the structured-type-error branch 3 times, most recently from 2a62e05 to 776e652 Compare March 9, 2024 04:48
@9999years 9999years force-pushed the structured-type-error branch from 776e652 to 1de04fd Compare March 9, 2024 04:50
Ericson2314
Ericson2314 reviewed Apr 1, 2024
View reviewed changes
src/libexpr/attr-path.cc
@@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure? FWIW it came from c1ca4f0

Ericson2314
Ericson2314 reviewed Apr 1, 2024
View reviewed changes
src/libexpr/eval-cache.cc
@@ -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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//error<EvalError>("'%s' is not an attribute set", getAttrPathStr()).debugThrow();
//error<TypeError>("nAttrs, getAttrPathStr()).debugThrow();

Is this better?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I guess getAttrPathStr is not the value, however

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, unless we are sure we never catch a TypeError, I am a bit wary of changing this

Ericson2314
Ericson2314 reviewed Apr 1, 2024
View reviewed changes
src/libexpr/eval-cache.cc
@@ -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?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Ericson2314
Ericson2314 reviewed Apr 1, 2024
View reviewed changes
src/libexpr/eval-error.hh
Comment on lines -86 to -87
[[nodiscard, gnu::noinline]] EvalErrorBuilder<T> & withFrameTrace(PosIdx pos, const std::string_view text);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated cleanup?

Ericson2314
Ericson2314 reviewed Apr 1, 2024
View reviewed changes
src/libexpr/eval-error.hh
Comment on lines +51 to +52
std::string_view showType(ValueType type, bool article);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
std::string_view showType(ValueType type, bool article);

It's in value.hh right?

Ericson2314
Ericson2314 reviewed Apr 1, 2024
View reviewed changes
src/libexpr/value.cc
case nExternal: return WA("an", "external value");
case nFloat: return WA("a", "float");
case nThunk: return WA("a", "thunk");
}
Copy link
Member

@Ericson2314 Ericson2314 Apr 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we stick with macro

Suggested change
}
}
#undef WA

Ericson2314
Ericson2314 reviewed Apr 1, 2024
View reviewed changes
src/libexpr/value.cc

std::string_view showType(ValueType type, bool withArticle)
{
#define WA(a, w) withArticle ? a " " w : w
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if constexpr stuff would work for this or not

Ericson2314
Ericson2314 reviewed Apr 1, 2024
View reviewed changes
src/libexpr/value.cc
Comment on lines +36 to +38
// Allow selecting a subset of enum values
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wswitch-enum"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Ericson2314
Ericson2314 reviewed Apr 1, 2024
View reviewed changes
Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry with my traveling I did not see this sooner. Just a few small things / questions. Excited to see this work continue!

@nixos-discourse
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ericson2314 Ericson2314 Ericson2314 left review comments

@roberth roberth Awaiting requested review from roberth roberth is a code owner

@edolstra edolstra Awaiting requested review from edolstra edolstra is a code owner

@fricklerhandwerk fricklerhandwerk Awaiting requested review from fricklerhandwerk fricklerhandwerk is a code owner

Assignees

@Ericson2314 Ericson2314

Labels
new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@9999years @nixos-discourse @Ericson2314