Print the value in error: cannot coerce messages

[9999years]

Motivation

This extends the error: cannot coerce a <TYPE> to a string message to print the value that could not be coerced. This helps with debugging by making it easier to track down where the value is being produced from, especially in errors with deep or unhelpful stack traces.

Context

See #561.

Here's an example of a mistake I made that would have been much easier to fix with this patch. I encountered this error while converting a NixOS configuration that relied on builtins.currentSystem to work in a flake. There were several overlays which pinned packages to specific versions, like this:

let
  nixos_master = import (
    builtins.fetchGit {
      url = "https://github.com/NixOS/nixpkgs.git";
      ref = "master";
      rev = "0c0d57d79de06b36286b8c7551dc173e372b86ad";
    }
  ) {};
in
self: super: {
  opentelemetry-collector-contrib = nixos_master.opentelemetry-collector-contrib; # version 0.78.0
}

My first try added an inherit (config.nixpkgs) localSystem; to the import arguments, but that gave me an unhelpful error:

$ nix repl nixpkgs
nix-repl> nixpkgs = legacyPackages.x86_64-linux.path
nix-repl> system = lib.systems.elaborate "x86_64-linux"
nix-repl> import nixpkgs { inherit system; }
error:
       … while evaluating a branch condition

         at /nix/store/7g8wbm1z6948x0qma4hzkkb9a3xys76w-source/pkgs/stdenv/booter.nix:64:9:

           63|       go = pred: n:
           64|         if n == len
             |         ^
           65|         then rnul pred

       … while calling the 'length' builtin

         at /nix/store/7g8wbm1z6948x0qma4hzkkb9a3xys76w-source/pkgs/stdenv/booter.nix:62:13:

           61|     let
           62|       len = builtins.length list;
             |             ^
           63|       go = pred: n:

       (stack trace truncated; use '--show-trace' to show the full trace)

       error: cannot coerce a set to a string

Note that the stack trace and error message don't make it clear which set couldn't be coerced to a string, or where the coercion failed. It's fairly easy to figure out what's gone wrong here, but in a repository with 14kloc it was significantly more difficult.

With this patch, the error message makes it clear what's happened (a system attrset has been used in place of a system string), and how to fix it (select the system attribute from config.nixpkgs.localSystem):

       error: cannot coerce a set to a string: { aesSupport = <CODE>; avx2Support = <CODE>; avx512Support = <CODE>; avxSupport = <CODE>; canExecute = <CODE>; config = <CODE>; darwinArch = <CODE>; darwinMinVersion = <CODE>; darwinMinVersionVariable = <CODE>; darwinPlatform = <CODE>; darwinSdkVersion = <CODE>; efiArch = <CODE>; emulator = <CODE>; emulatorAvailable = <CODE>; extensions = <CODE>; fma4Support = <CODE>; fmaSupport = <CODE>; gcc = <CODE>; hasSharedLibraries = <CODE>; is32bit = <CODE>; is64bit = <CODE>; isAarch = <CODE>; isAarch32 = false; isAarch64 = <CODE>; isAbiElfv2 = <CODE>; isAlpha = <CODE>; isAndroid = <CODE>; isArmv7 = <CODE>; isAvr = <CODE>; isBSD = <CODE>; isBigEndian = <CODE>; isCompatible = <CODE>; isCygwin = <CODE>; isDarwin = <CODE>; isEfi = <CODE>; isFreeBSD = <CODE>; isGenode = <CODE>; isGhcjs = <CODE>; isGnu = <CODE>; isILP32 = <CODE>; isJavaScript = <CODE>; isLinux = <CODE>; isLittleEndian = <CODE>; isLoongArch64 = <CODE>; isM68k = <CODE>; isMacOS = <CODE>; isMicroBlaze = <CODE>; isMinGW = <CODE>; isMips = <CODE>; isMips32 = <CODE>; isMips64 = <CODE>; isMips64n32 = <CODE>; isMips64n64 = <CODE>; isMmix = <CODE>; isMsp430 = <CODE>; isMusl = <CODE>; isNetBSD = <CODE>; isNone = <CODE>; isOpenBSD = <CODE>; isOr1k = <CODE>; isPower = <CODE>; isPower64 = false; isRedox = <CODE>; isRiscV = <CODE>; isRiscV32 = <CODE>; isRiscV64 = <CODE>; isRx = <CODE>; isS390 = <CODE>; isS390x = <CODE>; isSparc = <CODE>; isStatic = <CODE>; isSunOS = <CODE>; isUClibc = <CODE>; isUnix = <CODE>; isVc4 = <CODE>; isWasi = <CODE>; isWasm = <CODE>; isWindows = <CODE>; isi686 = <CODE>; isiOS = <CODE>; isx86 = <CODE>; isx86_32 = <CODE>; isx86_64 = <CODE>; libc = <CODE>; linker = <CODE>; linux-kernel = <CODE>; linuxArch = <CODE>; parsed = { _type = "system"; abi = { _type = "abi"; assertions = [ { assertion = <LAMBDA>; message = "The \"gnu\" ABI is ambiguous on 32-bit ARM. Use \"gnueabi\" or \"gnueabihf\" instead.\n"; } { assertion = <LAMBDA>; message = "The \"gnu\" ABI is ambiguous on big-endian 64-bit PowerPC. Use \"gnuabielfv2\" or \"gnuabielfv1\" instead.\n"; } ]; name = "gnu"; }; cpu = { _type = "cpu-type"; arch = "x86-64"; bits = 64; family = "x86"; name = "x86_64"; significantByte = { _type = "significant-byte"; name = "littleEndian"; }; }; kernel = { _type = "kernel"; execFormat = { _type = "exec-format"; name = "elf"; }; families = { }; name = "linux"; }; vendor = { _type = "vendor"; name = "unknown"; }; }; qemuArch = <CODE>; rust = <CODE>; rustc = <CODE>; sse3Support = <CODE>; sse4_1Support = <CODE>; sse4_2Support = <CODE>; sse4_aSupport = <CODE>; ssse3Support = <CODE>; system = "x86_64-linux"; ubootArch = <CODE>; uname = <CODE>; useAndroidPrebuilt = false; useiOSPrebuilt = false; }

Priorities

Add 👍 to pull requests you find important.

[fricklerhandwerk]

What a great, simple fix for such an old issue.

Please add a release note to celebrate.

[9999years]

Added a release note!

[Ericson2314]

I think this broke the Clang on Linux build. #9564 hopefully fixes it.

[SuperSandro2000]

I just wanted to say thank you for this in the name of many people! Tiny change but it will have big impact.

[roberth]

Sorry, we had a miscommunication among the team.
This PR has a UX regression that should be addressed, as requested in the sibling PR #9554.
I'll revert this as a matter of procedure, but feel free to reopen, or add the commits to the other PR. This work is appreciated!
The revert, #9568 has a test case to illustrate the issue.

[RaitoBezarius]

It's really a shame that this incredibly useful UX improvement was reverted so quickly when solutions were being developed to address the potential UX regression. :/

While I understand the procedure, etc. I do not think this is very encouraging to new contributors of the Nix codebase, especially when I don't see what's the harm of having a UX regression on the master branch sitting while it's being fixed, it's not like master Nix is being consumed by end users AFAIK.

[roberth]

As I've already indicated, it's just procedure, and not a statement of value.
Releaseworthiness of master is important and shouldn't be dependent on anyone's commitment; not even members of the team. This helps everyone, even if it might be frustrating to the author or to you. I would argue that it even helps the author ultimately, because their second PR will be easier to review, it being a complete change from a good state to another good state.

what's the harm

Uncertainty, a need more processes, perhaps even unfairness, but I really don't want to go into detail on that.
We have to draw a line somewhere, and it's not far from here.

This work is appreciated!

a matter of procedure

Also the blame is on us for merging prematurely. @9999years did nothing wrong. Let's not make this discussion any longer than it needs to be and focus on improving Nix.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-12-08-nix-team-meeting-minutes-110/36721/1

[9999years]

@roberth You didn't really explain why printing too much text is considered a UX regression, though, especially relative to the baseline. Certainly it can be annoying to scroll through, but is it more annoying than having an error message which gives no useful information? I responded to your comment on #9554 here, but instead of replying you just reverted my PR here, with seemingly no input from the other team members.

In fact, making changes with no discussion seems to be a theme for Nix team members. It seems like a lot of the "process" is ad-hoc, undocumented, and behind closed doors, as this is now the second time a change of mine has led to a hasty addition to contribution guidelines to justify its rejection. It's intensely discouraging to attempt to make contributions that conform to rules I don't have access to.

I like Nix, but I don't feel like my contributions are welcome here and after these experiences I'm rather hesitant to try again. Why should I spend my time preparing another patch when it seems like most PRs that don't come from Nix insiders linger without reviews or get bikeshedded for months or years?

[roberth]

@9999years There was potential for non-termination, and other errors hiding the root cause. We've had regressions in error reporting before, so I wanted to prevent a situation where problems linger again.
Nix is an important infrastructure for many people, and a complicated one at that. Meanwhile we're expected to perfectly handle a massive inflow of contributions with arguably too few person hours. We really are trying our best here.

The Nix maintainer team has only existed for about a year now. We are trying to recover from a large and old backlog, while also trying to make progress in important areas for which external contributions are not made.

We are still documenting and improving our process, and we'll discuss this with the team.

I like Nix, but I don't feel like my contributions are welcome here

They are! They really are.

There's a lot more to respond to here, but I'm afraid I'll have to do so later.

[fricklerhandwerk]

I'd like to add to it that adding process documentation is something we happened to have picked up in a more consistent fashion somewhat recently, exactly to avoid that kind of frustration for contributors. We're very much aware that documentation is lacking and some things appear to be happening behind closed doors (that's certainly not on purpose, there is nothing to hide here). I very much want to change that, this is why we publish all meeting notes for example.

So this is definitely not about your particular contributions, which I personally find extraordinarily valuable. This is not the first instance where we exercised an elevated degree of asynchronous decision making that didn't run smoothly. I still think that's better than what we had a year ago, where barely any decisions were made at all, leading to a large number of stalled PRs, or half a year ago, where we made most decisions in real-time meetings that were very limited in throughput.

@roberth is right to raise stability and usability considerations, and we have to balance those conflicting interests of getting more contributions accepted, keeping maintainer workload in check, and providing a solid basis for the ecosystem to build upon; all in order to keep the project sustainable for everyone involved.

A last thought that was raised recently in discussions with @zimbatm, @infinisil, and others: Ideally we'd just merge quickly and fix forward ourselves, or fix the issues we highlight in place. It would definitely improve contributor experience a great deal (although I've seen enough instances and experienced it myself that the things you wrote being changed underneath you can be just as frustrating as a revert). And we've been trying that out in the documentation team. But given the number of contributions and the change-compile-test turnaround time on the Nix codebase, generally this is simply unrealistic given our availability. We need more capacity and more work on infrastructure and are working towards that, but it all boils down to resources that mostly have to come from the community in one form or another. You're already donating time and that's great, thank you very much. Please be patient. 🙂

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/whats-nixs-stable-api-is-code-in-nix-instantiate-part-of-it/37986/1