lib.strings.concatMapAttrsStringSep: init

[ShamrockLee]

Description of changes

This pull request introduces lib.concatMapAttrsStringSep, the attribute set version of lib.concatMapAttrsStringSet. This function is initially defined locally in the Nix expression of apptainer and singularity packages. It increases the readability of attribute-set-to-string expressions.

Before

lib.concatStringsSep "\n" (lib.attrValues (lib.mapAttrs (name: package: "${name}: ${package.version}") packages))

or

lib.concatMapStringsSep "\n" (name: "${name}: ${packages.${name}.version}") (lib.attrNames packages)

After

lib.concatMapAttrsStringSep "\n" (name: package: "${name}: ${package.version}") packages

Cc:
@infinisil for the new library function
@roberth for the restructuring of tests.overriding

Things done

• Built on platform(s)
  • x86_64-linux (only the manuals are rebuilt)
  • 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.11 Release Notes (or backporting 23.11 and 24.05 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.

[ShamrockLee]

Result of nixpkgs-review run on x86_64-linux 1

1 package blacklisted:

• nixos-install-tools

[roberth]

concatMapStringsSep has put us on a slippery slope, but I have to acknowledge that this is a very very common pattern that always causes some dissonance when trying to figure out how to actually do this, so I'm not opposed.
I'm wondering though, whether this name actually makes it easier to remember.

Brainstorming

I'm trying to think what's the problem we're generally trying to solve. I think it's

1. calling multiple functions is annoying
2. writing trivial folds is annoying, because you have to write both pieces: the operator and the null element, or the operator and empty case
3. have a coherent scheme for composing all these operations

(1) could be solved with the pipe operator
(2) could be solved with a little monoid-inspired library, whose use would look like

reduce.fromAttrs (reduce.concatStrings) attrs
reduce.mapAttrs (reduce.concatSep "-") (k: v: f k v) attrs
reduce.fromList reduce.concatStrings [ "a;" "b;" ]
reduce.fromList reduce.lines [ "a" "b" ]
reduce.map reduces.lines (x: "${x};") [ "a" "b" ]

The reduce library consists of two functions and constants that produce "reducers" and a set of functions that take a data structure and apply a reducer.
A reducer could be a pair of a binary operation and the empty value, or a binary operation and the initial value.

However, this is not actually more expressive than a pipe like this

attrs
  |> mapAttrs (k: v: f k v)
  |> concatStringsSep "\n"

or potentially

concatStringsSep "\n" <| mapAttrs (k: v: f k v) <| attrs

compare:

concatMapStringAttrsSep "\n" (k: v: f k v) attrs

So it is shorter, but it is also harder to change the processing pipeline.
For example, making the strings unique can be easy:

concatStringsSep "\n" <| unique <| mapAttrs (k: v: f k v) <| attrs

Whereas with concatMapStringAttrsSep my brain would enter a pipeline stall, having to rethink the whole expression.

Of course we don't quite have pipe operators yet.

1. Merge as experimental feature (links)
2. Release Nix (Nix 2.24 due now)
3. Try it out, agree on syntax and semantics, merge RFC 148, release as non-experimental
4. Wait for Nixpkgs to bump lib/minver.nix. There's 2.3 hold-outs in the community, so may have to compromise on a backport and point release.

So until we have that, we need parentheses

concatStringsSep "\n" (mapAttrs (k: v: f k v) attrs)
# or
concatMapStringAttrsSep "\n" (k: v: f k v) attrs
# and, for reference
concatStringsSep "\n" (unique (mapAttrs (k: v: f k v) attrs))

Conclusion

Not a big fan, but I'd like to move forward with this in the meanwhile, I will probably use it, but eventually I'd like to get rid of it.

(At least in terms of actual use; not sure about deprecation. I'd prefer for a linter to suggest it, rather than spewing out warnings)

[ShamrockLee]

concatMapStringsSep has put us on a slippery slope

@roberth Thank you for sharing insight into the structural problem of adding all these composite functions. Providing reusable and composable parts is indeed better than pouring all the combinations of concat[I][Map]String[Attr]s[Sep] into lib.strings.

4. Wait for Nixpkgs to bump lib/minver.nix. There's 2.3 hold-outs in the community, so may have to compromise on a backport and point release.

I didn't find explicit documentation about the lifecycle of each Nix version and the backport policy, and Nixpkgs's lib/minver.nix seems to have stayed at 2.3 for years. (Is it a result of the 2.4 CLI changing controversy?) Does Nixpkgs adopt any new Language features developed after Nix 2.3?

concatStringsSep "\n" <| unique <| mapAttrs (k: v: f k v) <| attrs

As for the current standard library, mapAttrs maps between attribute sets, and the equivalent representation of concatMapStringAttrsSep (k: v: f k v) attrs with mapAttrs would be

concatStringsSep "\n" (attrValues (mapAttrs (k: v: f k v) attrs))

With the pipe operator, it would be

concatStringsSep "\n" <| attrValues <| mapAttrs  (k: v: f k v) <| attrs

Or the other direction.

I'll restructure the tests.overriding.tests with the mapAttrs-based pattern in another PR so that the test writing experience of overriding-related features would be smoother.

[ShamrockLee]

@fricklerhandwerk I just remembered that I need to add double stars at the beginning of the comment block (/**) to make the documentation comment in the new format build. Sorry for the noise.

[infinisil]

Needs a test, but other than that I'm totally in favor of having this!

@roberth Let's keep the model of diverging, then converging in mind. I think we are in the diverging phase with a lot of lib, where we don't have good answers to everything, so it's fine to experiment a bit, even in the stable namespace (but only with long, unambiguous identifiers!). Though having an experimental lib would be great too for this, but that's a future topic :)

[ShamrockLee]

Needs a test, but other than that, I'm totally in favor of having this!

Glad you like it! There is a test inside lib/tests/misc.nix.

[ShamrockLee]

I renamed it concatMapAttrsStringSep as @infinisil suggested.

I noticed that lib.mapAttrs is implemented by a built-in function. Should I reimplement lib.concatMapAttrsStringSep with lib.mapAttrs? The "before" part of the issue description shows both candidate implementations.

[infinisil]

I think this is good other than the one comment I left!

[ShamrockLee]

@infinisil Thank you for your approval.

I rebased the feature branch to resolve conflicts, adopted the mapAttrs-based implementation from @roberth, and adjusted some wording in the argument documentation.

The mapAttrs-based implementation is shorter and more readable. Besides, mapAttrs, attrValues, and concatStringsSep are all backed by builtins, making it potentially more efficient (I haven't benchmarked it yet).

[Gerg-L]

Shouldn't this have been named concatMapAttrsStringsSep to be consistent with other lib functions

[ShamrockLee]

@infinisil Do you think we should add the extra "s"?

Initial naming: #330010 (comment)

[ShamrockLee]

Update:

Just realized that this was merged a year ago. I had thought it was just merged due to the notification.

There is no compelling reason to rename.

I apologise for the noise caused by pinging.