Improve location reporting for type and type coercion errors

[layus]

Describe the bug

Nix sometimes reports imprecise error location when a value has the wrong type for an operation.

In this example, builtins.hasAttr expects a string and receives a boolean as first argument. But the caret points at the builtin itself, making the error message misleading.

error: value is a Boolean while a string was expected

       at /nix/store/1bicq9whypjqyy2j6b7qw58jqm9f1py3-source/lib.nix:75:12:

           74|         let inputName = declaredInputs."${name}"; in
           75|         if builtins.hasAttr inputName nickelInputs then
             |            ^
           76|           let input = nickelInputs."${inputName}"; in

In the code, we see that we coerce the first arg to a string, and pass as trace position the pos of the builtin itsefl (the only we can have at this point in evaluation).

https://github.com/NixOS/nix/blob/3144b373a486bf0d5ff35d3202a658311b26c6f0/src/libexpr/primops.cc#L2206

Expected behavior

Each time we have a coercion, there is a potential for failure. We should ensure that the message is correct by pointing at the right location. From my analysis, it is next to impossible, as the information is lost by the time we know that we have to coerce.

The next best thing is to amend the error message to mention how the error relates to the error location. In this case, say it is the first argument to the builtin under evaluation. This way, the caret can remain on the builtin itself.

error: in the first argument of builtins.hasAttr: value is a Boolean while a string was expected

Thus, coercion functions such as coerceToString, evalAttrs and the like should take an extra, mandatory error message describing what the coerced value means for the current Expr node under evaluation.

We should also ensure that each Expr node that may fail in such a way gets a proper error context. That is, we should ensure that a caret points to the expression we refer to in the error message. This is the issue encountered by https://github.com/NixOS/nix/issues/6191.

Additional context

https://github.com/NixOS/nix/issues/6191

https://github.com/NixOS/nix/commit/9d67332e4b8d75a101b8cf6e1d7e4dc8e99e964a (thanks @greedy !)

[layus]

/cc @kamadorueda . Someone told me you are working on improving error messages, and could want to have a look at this one ;-)

[layus]

Well, I am wrong, we do have an Expr, but not all Expr have a position to use for error reporting. I wonder if it is a deliberate design decision. I wonder if Pos *pos could be moved to Expr mother class :thinking:

[roberth]

My working hypothesis is that the ^ points (correctly) at the start of the Expr. If that's an ExprCall, the start of the call is equal to the start of the fun subexpression, which leads to confusion. If the Pos would include the end of the expression as well, we could show the difference between the ExprCall and its fun subexpression, solving this issue.

It would then look like:

error: value is a Boolean while a string was expected

       at /nix/store/1bicq9whypjqyy2j6b7qw58jqm9f1py3-source/lib.nix:75:12:

           74|         let inputName = declaredInputs."${name}"; in
           75|         if builtins.hasAttr inputName nickelInputs then
             |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
           76|           let input = nickelInputs."${inputName}"; in

Not 100% there, but already an improvement.

Ideally, the primops can get a Pos for each argument.

error: value is a Boolean while a string was expected

       at /nix/store/1bicq9whypjqyy2j6b7qw58jqm9f1py3-source/lib.nix:75:12:

           74|         let inputName = declaredInputs."${name}"; in
           75|         if builtins.hasAttr inputName nickelInputs then
             |                             ^^^^^^^^^
           76|           let input = nickelInputs."${inputName}"; in

or perhaps even printing the function and argument Pos both, separately. Here the function context is quite clear, but that may not always be the case?

[layus]

My working hypothesis is that the ^ points (correctly) at the start of the Expr. If that's an ExprCall, the start of the call is equal to the start of the fun subexpression, which leads to confusion. If the Pos would include the end of the expression as well, we could show the difference between the ExprCall and its fun subexpression, solving this issue.

It would then look like:

error: value is a Boolean while a string was expected

       at /nix/store/1bicq9whypjqyy2j6b7qw58jqm9f1py3-source/lib.nix:75:12:

           74|         let inputName = declaredInputs."${name}"; in
           75|         if builtins.hasAttr inputName nickelInputs then
             |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
           76|           let input = nickelInputs."${inputName}"; in

Not 100% there, but already an improvement.

Not really valid. the result of the call builtins.hasAttr inputName nickelInputs is not the problem here. It is builtins.hasAttr that takes a string as first argument. The problem here is that, albeit the code does have an Expr to evaluate as first argument, not all Expr have a position. So the position passed in the debug message is taken from the builtin, which is just wrong.

So either add a Pos to every Expr, or provide a message that works in the absence of a proper pos.

Ideally, the primops can get a Pos for each argument.

error: value is a Boolean while a string was expected

       at /nix/store/1bicq9whypjqyy2j6b7qw58jqm9f1py3-source/lib.nix:75:12:

           74|         let inputName = declaredInputs."${name}"; in
           75|         if builtins.hasAttr inputName nickelInputs then
             |                             ^^^^^^^^^
           76|           let input = nickelInputs."${inputName}"; in

or perhaps even printing the function and argument Pos both, separately. Here the function context is quite clear, but that may not always be the case?

If we print the argument position, we will automatically have the function position, as each function call gets a trace frame in the debug.

[layus]

Another example;

nix-instantiate --eval --expr 'if "hello" then true else false'                                                                                                                                          (nix)
error: value is a string while a Boolean was expected

       at «string»:1:1:

            1| if "hello" then true else false
             | ^

[layus]

Oh, actually first intuition was right. We only have an Expr when we use evalXXX fucntions, but forceXXX functions do not have that property. Back to the first idea then.