Improve error messages by removing checkDependencyList in stdenv.mkDerivation

[9999years]

I recently encountered this error attempting to build a derivation:

error:
       … while calling the 'derivationStrict' builtin

         at /builtin/derivation.nix:9:12: (source not available)

       … while evaluating derivation 'infra-shell'
         whose name attribute is located at /nix/store/ppm74s0slima7385piksmdcnvcawgs1x-source/pkgs/stdenv/generic/make-derivation.nix:353:7

       … while evaluating attribute 'nativeBuildInputs' of derivation 'infra-shell'

         at /nix/store/ppm74s0slima7385piksmdcnvcawgs1x-source/pkgs/stdenv/generic/make-derivation.nix:397:7:

          396|       depsBuildBuild              = elemAt (elemAt dependencies 0) 0;
          397|       nativeBuildInputs           = elemAt (elemAt dependencies 0) 1;
             |       ^
          398|       depsBuildTarget             = elemAt (elemAt dependencies 0) 2;

       error: Dependency is not of a valid type: element 23 of nativeBuildInputs for infra-shell

This isn't very helpful; it doesn't tell me what the type of the dependency is. I'm also not directly constructing stdenv.mkDerivation { nativeBuildInputs = [...]; }, so it's not obvious what "element 23" would be (the derivation is constructed through several layers of helpers, like many derivations in nixpkgs are).

Ultimately, element 23 was a function, which surfaces another issue:

nix-repl> throw "Dependency is not of a valid type: ${a: a}"
error:
       … while evaluating a path segment

         at «string»:1:43:

            1| throw "Dependency is not of a valid type: ${a: a}"
             |                                           ^

       error: cannot coerce a function to a string

(builtins.toString produces similar results.) The Nix language doesn't provide a safe value printing function, so we can't really construct a good error message here.

The good news is that we can remove this error checking code from nixpkgs entirely without losing usability. Here's the error message on Nix 2.19.3:

error:
       … while calling the 'derivationStrict' builtin

         at /derivation-internal.nix:9:12:

            8|
            9|   strict = derivationStrict drvAttrs;
             |            ^
           10|

       … while evaluating derivation 'my-derivation'
         whose name attribute is located at /Users/wiggles/nixpkgs/pkgs/stdenv/generic/make-derivation.nix:347:7

       … while evaluating attribute 'nativeBuildInputs' of derivation 'my-derivation'

         at /Users/wiggles/nixpkgs/pkgs/stdenv/generic/make-derivation.nix:391:7:

          390|       depsBuildBuild              = elemAt (elemAt dependencies 0) 0;
          391|       nativeBuildInputs           = elemAt (elemAt dependencies 0) 1;
             |       ^
          392|       depsBuildTarget             = elemAt (elemAt dependencies 0) 2;

       error: cannot coerce a function to a string

Thanks to PRs like NixOS/nix#9754, these error messages will improve in coming Nix releases:

       … while evaluating attribute 'nativeBuildInputs' of derivation 'my-derivation'
         at /Users/wiggles/nixpkgs/pkgs/stdenv/generic/make-derivation.nix:391:7:
          390|       depsBuildBuild              = elemAt (elemAt dependencies 0) 0;
          391|       nativeBuildInputs           = elemAt (elemAt dependencies 0) 1;
             |       ^
          392|       depsBuildTarget             = elemAt (elemAt dependencies 0) 2;

       … while evaluating one element of the list

       error: cannot coerce a function to a string: «lambda @ /Users/wiggles/nixpkgs/xxx-derivation.nix:7:28»

(This shows the exact location of the failing value!)

Description of changes

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[happysalada]

Hey thanks for this PR !

The fix to pict-rs can be merged immediately, but the patching of stdenv needs to go through a lot more review and discussion.
Would you mind splitting this PR in two ?
Im happy to merge the pictrs part now and will try to reach out to more people for the stdenv fix.

[9999years]

Sure, split the pict-rs changes off here: #293608

[happysalada]

@infinisil ignoring the pict-rs changr, there is a proposition of a fix to stdenv, what do you think ?

[infinisil]

How is the error for Nix 2.3 users? That's the minimum version that Nixpkgs supports.

[9999years]

@infinisil Looks OK:

error: while evaluating the attribute 'buildInputs' of the derivation 'my-derivation' at /Users/wiggles/nixpkgs/pkgs/stdenv/generic/make-derivation.nix:347:7:
cannot coerce a function to a string, at /Users/wiggles/nixpkgs/pkgs/stdenv/generic/make-derivation.nix:347:7

[infinisil]

While we should worry about older Nix versions too, I don't think this is worth it even for newer Nix versions. See also #24462 and the original motivation from https://unix.stackexchange.com/questions/350997/in-a-new-nixos-derivation-based-on-a-binary-distribution-why-am-i-getting-an-e?stw=2

Even if newer Nix versions show the location better, the underlying error message seems worse:

cannot coerce a X to a string

Compared to the old

Dependency is not of a valid type: element 23 of nativeBuildInputs for infra-shell

Especially for newcomers it's not easy to figure out why something even gets coerced to a string in the first place (and what "coerce" even means!).

While the custom error message isn't perfect either, we could easily improve it, e.g. by saying what the actual type is and what it should be, additionally printing a bit of the value (e.g. like the module system, though I also wrote this nice streaming toPretty at some point which would work better]).

[9999years]

@infinisil I'm not sure I understand -- even with Nix 2.3, the error message clearly states the derivation name (my-derivation) and the attribute that fails to coerce (buildInputs). The only thing that's really removed here is element 23, which I'm not convinced is very useful -- derivations are frequently composed through helpers and overrides such that it's not obvious which index corresponds to which element in the source.

Especially for newcomers it's not easy to figure out why something even gets coerced to a string in the first place (and what "coerce" even means!).

I think newcomers will be well-served by the improved coercion error messages in recent Nix versions (see NixOS/nix#9754) as well as the improved source location information available in recent Nix versions. (And newcomers will tend to run recent Nix versions in general.)

Moreover, having the custom-constructed error message in nixpkgs actually prevents Nix from providing a better error message, which is how I discovered this issue in the first place — I saw the bad error message, attempted to run it with a newer Nix to debug it, and wasn't able to get any additional information from the improvements I've made to error messages. This logic has a cost, and it's not just a maintenance cost!

[SuperSandro2000]

I must say that when you have lists of a hundred packages with some optionals inbetween I usually reside to binary search because not having any information about the wrong element makes it almost impossible to know what to fix even as an advanced nix user. Even just having the name of the package in the error message would eliminate that and save a bunch of time for many people.
Now that I know why this is broken, I don't want to keep it like this any longer and do something about it.

[infinisil]

I think it's important to distinguish between developer-facing errors and user-facing errors:

• User-facing errors are caused by the user using some function in the wrong way. These should be understandable and fixable by just reading a custom error message given with throw and adjusting the function call. Think of these like compiler errors. These messages can be tested as well to ensure that we can guarantee good error messages for certain cases. Users shouldn't have to enable and read stack traces to understand what went wrong, that gets tricky really quickly, especially because error traces can change considerably over time. It's a bit unfortunate how newer Nix versions print part of the stack trace by default, adding a lot of noise but rarely being useful.

• Developer-facing errors are for mistakes not caused by the user, internal to the function. These can be very hard to fix, so you need a lot of context where --show-trace can help a lot. They need to be fixed by changing the function and it doesn't make sense to test for these.

Check out how lib.fileset does it for example: All user errors are throws with pretty good context to fix the problem.

While it's nice that stack traces now show the location of unexpected values, it would be even better if this would be exposed with e.g. builtins.unsafeGetPos, such that it can be used for all user-facing error messages, instead of functions having to give up on user-facing errors just for that feature.