lib.mapDataRecursiveCond: init

[ibizaman]

Description of changes

This new 'mapDataRecursiveCond' function is like 'mapAttrsRecursiveCond' and additionally recurses on lists.

This function will be used with user-provided freeform variables - arbitrary combinations of
attrset, lists and values - in the upcoming secret management feature. The PR #328472 shows how it will be used.

The test testMapAttrRecursiveDoc is taken verbatim from the documentation of that function. I added it as some kind of regression test.

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.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.

[ibizaman]

@ofborg test misc

[AndersonTorres]

I purposely created a new function instead of updating the existing 'mapAttrsRecursiveCond' one because to avoid changing the implementation of such a base function.

It makes few to no sense.

You are creating a function that supersedes a function that already exists.
Your function covers - and should cover - the use cases of the existing function.
You are even using a similar identical signature.
Your function can or should replace the previous one.

Why the duplication?

[ibizaman]

@AndersonTorres to be honest, I didn't know how averse we - the nix community - were towards changing such a base function. It's my first time touching that part of the code. Essentially, I feared that this would introduce breaking changes because the function will return something else.

I agree though that having only one function is better for plenty of reasons and I'm happy you're pushing towards to that! So I'll update the PR.

[ibizaman]

Checked locally by running nix-build --arg pkgs "import ./. {}" ./lib/tests/release.nix and indeed the failure in https://gist.github.com/GrahamcOfBorg/3552cb4663303e0afaefaf3fec69050f comes from the ifList branch. I commented those two lines out and the test pass. Still trying to figure out what's going on.

[roberth]

@AndersonTorres Nixpkgs lib is like the libc of the Nix ecosystem. We should be conservative about changes, particularly in the attrsets and lists sub-libraries, which have well defined, bounded behavior that everyone should be able to rely on.

Your function covers - and should cover - the use cases of the existing function.
You are even using a similar identical signature.

Similar does not mean equivalent, and identical signatures are not that significant.

It makes few to no sense.

This could be considered disrespectful.

@ibizaman Your intuition about changes in lib was right, and I apologize for the confusion that was caused.

[ibizaman]

@roberth I don't mind at all reverting to my original PR. It's going to be less scary, for sure. Especially with the failing test there. Should I wait for more feedback though?

[infinisil]

As another main lib maintainer, I fully agree with @roberth that we should be conservative with changes to such core functions. In particular I'm also arguing for an explicitly specified stability guarantee.

Changing mapAttrsRecursiveCond like this actually has the chance to change evaluation results and cause breakages. As an example

lib.mapAttrsRecursive (as: value:
  if lib.isList value then
    value ++ [ 3 ]
  else
    value + 3
) { a = [ 10 ]; b = 20; }

Before this PR, this expression evaluates to:

{ a = [ 10 3 ]; b = 23; }

After:

{ a = [ 13 ]; b = 23; }

So I definitely don't think this would be okay.

[infinisil]

I'm not a fan of using ' for naming. How about just lib.mapRecursiveCond? Because it won't be specific to attrsets anymore. Still fine to put it under lib.attrsets though, we don't really have a better place for it right now :)

[infinisil]

Suggested change

	then imap0 (i: v: recurse (path ++ [(toString i)]) v) value
	then imap0 (i: v: recurse (path ++ [i]) v) value

Otherwise we can't distinguish between { "0" = 1; } and [ 1 ] when recursing, because both would have "0" in the path. Passing through the integer without converting to a string makes it clear that integer path entries come from lists.

[AndersonTorres]

@AndersonTorres Nixpkgs lib is like the libc of the Nix ecosystem. We should be conservative about changes, particularly in the attrsets and lists sub-libraries, which have well defined, bounded behavior that everyone should be able to rely on.

Understood.

It makes few to no sense.

This could be considered disrespectful.

I was not meant to be disrespectful.
I was pointing out that the redundancy implied by this implementation was confusing.

Regardless, apologies for my bad take.

[roberth]

How about just lib.mapRecursiveCond?

There's still potential for more to be transformed: function return values, so this name may be too general. I don't think it aligns particularly well with the goal of the function, so how about lib.mapDataRecursiveCond instead?
It does cover all the non-"atomic" Nix types that are data.

fine to put it under lib.attrsets though

I agree. I wouldn't mind something like lib.data or lib.generic, but that needs a bit more thought put into it: which other kinds of functions would fit there?
Something to think about, but perhaps not part of this PR.

[roberth]

Could you pass path to cond as well? It's helpful for things like recurseForDerivations = true; which need to be skipped based on their attribute name, but perhaps not their value.
Easy to add now, and avoids yet another function later.

[infinisil]

This is looking great overall, I'm happy to merge once the feedback is addressed :D

Sidenote: To avoid further minor review iterations once the feedback is addressed, I'd like to promote the workflow of reviewers pushing to PRs directly if the PR is almost ready, but they're still unhappy with something minor :)

[roberth]

I'd like to promote the workflow of reviewers pushing to PRs directly

I like that too, but it's not always the best option

• can look too authoritative
• can break things - reviewer doesn't always know best
• can take more reviewer time, which is in short supply
• can be force-pushed away by accident

Sometimes a branch can't be pushed to by maintainers. Look for "Maintainers are allowed to edit this pull request" on the PR page.

All that said, it is generally a nicer experience 👍

[ibizaman]

@infinisil @roberth I should have answered to all your comments. I did create small commits instead of rebasing for a better review experience in GitHub but I'll squash it all when we're done.