Skip to content

Change Error message @checkOverlay #5147

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

Closed
bryanhonof wants to merge 2 commits into from
Closed

Change Error message @checkOverlay #5147

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9e9a024
Change Error message @checkOverlay
bryanhonof Aug 17, 2021
f0fc2a1
Update error messages
bryanhonof Aug 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions src/nix/flake.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,11 +333,15 @@ struct CmdFlakeCheck : FlakeCommand
auto checkOverlay = [&](const std::string & attrPath, Value & v, const Pos & pos) {
try {
state->forceValue(v, pos);
if (!v.isLambda() || v.lambda.fun->matchAttrs || std::string(v.lambda.fun->arg) != "final")
throw Error("overlay does not take an argument named 'final'");
if (!v.isLambda())
throw Error("Expected a lambda for the overlay attribute, but got something else");
if (v.lambda.fun->matchAttrs)
throw Error("By convention the attribute 'overlay' is expected to be a simple lambda starting with 'final: ', not an attribute set matching lambda");
Copy link
Member

@edolstra edolstra Sep 14, 2021

Choose a reason for hiding this comment

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

Suggested change
throw Error("By convention the attribute 'overlay' is expected to be a simple lambda starting with 'final: ', not an attribute set matching lambda");
throw Error("the attribute 'overlay' must a function that takes a single argument named 'final ', not an attribute set");

It's not a convention but a requirement. Also instead of "lambda" it's better to say "function".

Copy link
Member

@roberth roberth Sep 14, 2021

Choose a reason for hiding this comment

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

What is responsible for this "requirement"?

Assuming we do need this, why say "function", a general term, when the value must be provided by a lambda? Or does Nix have another implementation of "function" that is acceptable here, but is not a lambda?

Copy link
Member

@edolstra edolstra Sep 14, 2021

Choose a reason for hiding this comment

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

This exact check is the requirement.

if (std::string(v.lambda.fun->arg) != "final")
throw Error("By convention the attribute 'overlay' expects the argument 'final' not '" + std::string(v.lambda.fun->arg) + "'");
Copy link
Member

@roberth roberth Aug 30, 2021

Choose a reason for hiding this comment

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

This message isn't right for "matchAttrs" type lambdas, such as { foo, bar }: x. That case should have its own message.
Same below.

Copy link
Member Author

@bryanhonof bryanhonof Aug 30, 2021

Choose a reason for hiding this comment

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

@roberth Do you have a proposal for what error it should throw?

Copy link
Member

@roberth roberth Aug 30, 2021

Choose a reason for hiding this comment

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

By convention the attribute 'overlay' is expected to be a simple lambda starting with 'final: ', not an attribute set matching lambda.

I guess you could add a representation of it too, but a source position Pos would be even better.

Copy link
Member

@edolstra edolstra Aug 30, 2021

Choose a reason for hiding this comment

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

Position info is already added to the error context by the try { ... } catch { ... }.

Copy link
Member

@edolstra edolstra Aug 30, 2021

Choose a reason for hiding this comment

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

This will crash if v is not a lambda.

Copy link
Member Author

@bryanhonof bryanhonof Aug 31, 2021

Choose a reason for hiding this comment

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

In the new commit the !v.isLambda() check is now isolated, and also throws a more appropriate error message, telling the user the attribute should be a lambda.

auto body = dynamic_cast<ExprLambda *>(v.lambda.fun->body);
if (!body || body->matchAttrs || std::string(body->arg) != "prev")
throw Error("overlay does not take an argument named 'prev'");
throw Error("By convention the attribute 'overlay' expects the argument 'prev' not '" + std::string(body->arg) + "'");
// FIXME: if we have a 'nixpkgs' input, use it to
// evaluate the overlay.
} catch (Error & e) {
Expand Down