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

Print the value in value is X while a Y is expected error #9753

Merged
merged 1 commit into from
Jan 22, 2024
Merged

Print the value in value is X while a Y is expected error #9753

merged 1 commit into from
Jan 22, 2024

Conversation

9999years
Copy link
Contributor

Motivation

Adds the failing value to value is <TYPE> while a <TYPE> is expected error messages.

Context

Take two of #9554 now that #9606 is merged.
Partial fix for #561, in particular #561 (comment).

Priorities

Add 👍 to pull requests you find important.

@9999years 9999years changed the title Print value on type error Print the value in value is X while a Y is expected error Jan 12, 2024
@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 Jan 12, 2024
roberth
roberth reviewed Jan 13, 2024
View reviewed changes
doc/manual/rl-next/print-value-in-type-error.md Outdated Show resolved Hide resolved
doc/manual/rl-next/print-value-in-type-error.md Outdated Show resolved Hide resolved
doc/manual/rl-next/print-value-in-type-error.md Outdated Show resolved Hide resolved
src/libexpr/print-options.hh Outdated Show resolved Hide resolved
src/libexpr/print-options.hh Outdated Show resolved Hide resolved
@roberth roberth mentioned this pull request Jan 13, 2024
Merged
@roberth roberth self-assigned this Jan 13, 2024
Ma27
Ma27 reviewed Jan 15, 2024
View reviewed changes
Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

This is a very nice change!

I played a bit around with this and it seems to be working fine. I even through this against some more complex evaluations involving flake-parts with custom modules and that seems to be producing helpful results.

From what I can tell, this looks OK code-wise from what I can tell - haven't been doing contributing much to the Nix codebase recently.

One issue I noticed however is that the opening bracket of a list/attr-set is magenta, the closing bracket isn't, i.e.

"[...] while a set  was expected: " ANSI_MAGENTA "[" ANSI_CYAN " 2 3 " ANSI_NORMAL "]"

E.g. reproducible with ./result/bin/nix-instantiate --eval -E '[ 1 2 ] // {}'.

Also, I'm wondering if it'd make sense to do a little bit of pretty-printing for lists and attr-sets, though that'd require changes on existing code, so that's a follow-up I guess.

Ma27
Ma27 reviewed Jan 20, 2024
View reviewed changes
src/libexpr/eval.cc Outdated
#include <fstream>
#include <functional>
#include <strstream>
Copy link
Member

Choose a reason for hiding this comment

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

Btw any particular reason for using strstream here? That is deprecated for quite a while and I just realized that it doesn't build with clang (used gcc last time).
Also, reversing this change seems fine as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, fixed this to use iostream instead.

Ma27 added a commit to Ma27/nix that referenced this pull request Jan 20, 2024
@Ma27
libexpr: strstream -> iostream
46b05bc 
Needed to fix the build with clang.
See also NixOS#9753 (comment)
Ma27 added a commit to Ma27/nix that referenced this pull request Jan 20, 2024
@Ma27
libexpr: print value of what is attempted to be called as function
6d22d72 
Low-hanging fruit in the spirit of NixOS#9753 and NixOS#9754 (means 9999years did
all the hard work already).

This basically prints out what was attempted to be called as function,
i.e.

  map (import <nixpkgs> {}) [ 1 2 3 ]

now gives the following error message:

    error:
           … while calling the 'map' builtin
             at «string»:1:1:
                1| map (import <nixpkgs> {}) [ 1 2 3 ]
                 | ^

           … while evaluating the first argument passed to builtins.map

           error: expected a function but found a set: { _type = "pkgs"; AAAAAASomeThingsFailToEvaluate = «thunk»; AMB-plugins = «thunk»; ArchiSteamFarm = «thunk»; BeatSaberModManager = «thunk»; CHOWTapeModel = «thunk»; ChowCentaur = «thunk»; ChowKick = «thunk»; ChowPhaser = «thunk»; CoinMP = «thunk»;  «18783 attributes elided»}
@Ma27 Ma27 mentioned this pull request Jan 20, 2024
Merged
@9999years
Print value on type error
cb7fbd4 
Adds the failing value to `value is <TYPE> while a <TYPE> is expected`
error messages.
@roberth
Copy link
Member

roberth commented Jan 22, 2024

One issue I noticed however is that the opening bracket of a list/attr-set is magenta, the closing bracket isn't, i.e.

"[...] while a set  was expected: " ANSI_MAGENTA "[" ANSI_CYAN " 2 3 " ANSI_NORMAL "]"

E.g. reproducible with ./result/bin/nix-instantiate --eval -E '[ 1 2 ] // {}'.

This is caused by the formatting logic behind hintformat - specifically how the default item type is printed via yellowtxt (bad name; was about warnings, and those are magenta now).
It tries to print the argument in magenta, but printValue does not know that it "should" reset to that color state. I'm not sure that it should. It might be nice to reset to bold, to distinguish it from the fixed parts of the error message, but that might be over-engineering it.

As for the magenta token, you could wrap all expressions in normaltxt, or perhaps by add some extra overload of hintformat::operator%. In the latter case, you might make the call sites a bit more concise.

@9999years
Copy link
Contributor Author

That's a good idea. Can I leave it to a follow-up PR to keep this focused?

@roberth roberth merged commit 5f72a97 into NixOS:master Jan 22, 2024
8 checks passed
@roberth
Copy link
Member

roberth commented Jan 22, 2024

follow-up PR

👍

Ma27 added a commit to Ma27/nix that referenced this pull request Jan 22, 2024
@Ma27
libexpr: print value of what is attempted to be called as function
81499a0 
Low-hanging fruit in the spirit of NixOS#9753 and NixOS#9754 (means 9999years did
all the hard work already).

This basically prints out what was attempted to be called as function,
i.e.

  map (import <nixpkgs> {}) [ 1 2 3 ]

now gives the following error message:

    error:
           … while calling the 'map' builtin
             at «string»:1:1:
                1| map (import <nixpkgs> {}) [ 1 2 3 ]
                 | ^

           … while evaluating the first argument passed to builtins.map

           error: expected a function but found a set: { _type = "pkgs"; AAAAAASomeThingsFailToEvaluate = «thunk»; AMB-plugins = «thunk»; ArchiSteamFarm = «thunk»; BeatSaberModManager = «thunk»; CHOWTapeModel = «thunk»; ChowCentaur = «thunk»; ChowKick = «thunk»; ChowPhaser = «thunk»; CoinMP = «thunk»;  «18783 attributes elided»}
@9999years 9999years deleted the print-value-on-type-error branch January 22, 2024 22:41
@9999years 9999years mentioned this pull request Jan 23, 2024
Merged
9999years added a commit to 9999years/nix that referenced this pull request Jan 26, 2024
@9999years
libexpr: Support structured error classes
5a14403 
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:

    EvalErrorBuilder<TypeError>(state, "'%s' is not a string", getAttrPathStr())
      .debugThrow()
9999years added a commit to 9999years/nix that referenced this pull request Jan 26, 2024
@9999years
libexpr: Support structured error classes
6784cf4 
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:

    EvalErrorBuilder<TypeError>(state, "'%s' is not a string", getAttrPathStr())
      .debugThrow()
9999years added a commit to 9999years/nix that referenced this pull request Jan 26, 2024
@9999years
libexpr: Support structured error classes
4a301f3 
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:

    EvalErrorBuilder<TypeError>(state, "'%s' is not a string", getAttrPathStr())
      .debugThrow()
9999years added a commit to 9999years/nix that referenced this pull request Jan 29, 2024
@9999years
libexpr: Support structured error classes
3ec4cf3 
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:

    EvalErrorBuilder<TypeError>(state, "'%s' is not a string", getAttrPathStr())
      .debugThrow()
9999years added a commit to 9999years/nix that referenced this pull request Feb 1, 2024
@9999years
libexpr: Support structured error classes
30eda09 
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`.
9999years added a commit to 9999years/nix that referenced this pull request Feb 1, 2024
@9999years
libexpr: Support structured error classes
f2f90e3 
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`.
9999years added a commit to 9999years/nix that referenced this pull request Feb 2, 2024
@9999years
libexpr: Support structured error classes
c6a89c1 
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`.
9999years added a commit to 9999years/nix that referenced this pull request Feb 4, 2024
@9999years
Don't print values in magenta
e351f5e 
This fixes the opening bracket of lists/attrsets being printed in
magenta, unlike the closing bracket.

NixOS#9753 (comment)
@9999years 9999years mentioned this pull request Feb 4, 2024
Merged
9999years added a commit to 9999years/nix that referenced this pull request Feb 4, 2024
@9999years
Don't print values in magenta
9646d62 
This fixes the opening bracket of lists/attrsets being printed in
magenta, unlike the closing bracket.

NixOS#9753 (comment)
@9999years 9999years mentioned this pull request Feb 5, 2024
Open
tebowy pushed a commit to tebowy/nix that referenced this pull request Jul 11, 2024
Merge pull request NixOS#9753 from 9999years/print-value-on-type-error
87e6ac5 
Print the value in `value is X while a Y is expected` error

(cherry picked from commit 5f72a97)
Change-Id: Idb4bc903ae59a0f5b6fb3b1da4d47970fe0a6efe
tebowy pushed a commit to tebowy/nix that referenced this pull request Jul 11, 2024
libexpr: Support structured error classes
0825296 
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`.

(cherry picked from commit c6a89c1)
Change-Id: Iced91ba4e00ca9e801518071fb43798936cbd05a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ma27 Ma27 Ma27 left review comments

@roberth roberth roberth approved these changes

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

Assignees

@roberth roberth

Labels
new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority
Projects
Nix team
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@9999years @roberth @Ma27