lib: add missing removeAttrs builtin

[zimbatm]

Description of changes

I'm expecting all the builtins.* functions to be available in lib.*

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• 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/)
• 23.05 Release Notes (or backporting 22.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.

[infinisil]

Coincidentally very relevant is @roberth's comment here:

Instead of worrying or breaking something, let's just establish that the builtins suck. They're our equivalent of the kernel syscall interface. Have you seen the number of stat calls?

So lib can choose its own sensible name; problem solved.

And I agree with this. So instead of just propagating builtins I'd rather make sure the name and interface of the function is correct, and only using the builtin for speed underneath.

And this is a great example, because removeAttrs's argument ordering is not intuitive, I frequently make this mistake:

nix-repl> removeAttrs [ "x" ] { x = 0; } 
error: value is a list while a set was expected

       at «string»:1:1:

            1| removeAttrs [ "x" ] { x = 0; }
             | ^

nix-repl> removeAttrs { x = 0; } [ "x" ] 
{ }

Unfortunately just flipping around the order for lib.attrsets.removeAttrs would be way more confusing. But let's think about alternatives a bit.

[zimbatm]

To me, lib is also a dumpster fire, but shorter to type. It's weird to have functions only available in builtins and not in lib. Especially removeAttrs that has been around for so long, I keep bumping against this.

I agree that the ordering of the arguments is not intuitive. There are always weird corners in languages and historical accidents. You learn them by heart and live with them, it's not a big deal. As you noted, it would be even worse if the ordering was reversed in lib.

[zimbatm]

Here is an effort I started a while back to re-think the stdlib entirely, with a proper categorization effort. And reversed removeAttrs arguments: https://github.com/numtide/nix-stdlib/blob/main/src/types/attrs.nix#L21-L22

[roberth]

Not curry friendly, I agree.
Even better is to design your expressions so that you don't need removeAttrs. It's usually for separating things that should have been separated from the start. Wishful thinking though.

How about a new name for the flipped version?
deleteAttrs, removeAttrsByNames, elideAttrs, omitAttrs, excludeAttrs. Plenty of choice.

[zimbatm]

I'm sorry; this PR was supposed to be a trivial change that would be merged in. We already agree that removeAttrs is what it is. This PR addresses a specific issue; having it in builtins and not in lib is surprising. Renaming it doesn't solve that issue. I agree that removeAttrs is imperfect, but many have learned to use it. If you disagree with that premise, I will just close the PR.

If you want to re-think how the lib is structured, then let's have that conversation. But holistically and in a separate space. For example, if we had per-type prefixes like lib.attrs, lib.lists, lib.strings, ... then it becomes possible to add common nouns to these namespaces like find, id, map that makes sense with each respective types. We could also look at what Haskell and Idris are doing and draw inspiration from there.

[roberth]

I'm sorry;

No need for anyone to feel sorry or apologize. It's good to have a conversation, so that we've considered the broader context of the change. I think this PR is a good step forward. Using a different name is just a suggestion that can be executed on independently of this PR if you prefer it that way.

[roberth]

We've considered the alternatives, and I think by adding a function with a new name we can solve the currying issue of the existing, established function. As mentioned, changing the meaning without changing the name would be rather confusing, so this PR is a net positive and does not lead to an unnecessarily local optimum.

[infinisil]

Fair points, agreed :)

[github-actions]

Successfully created backport PR for release-22.11:

• [Backport release-22.11] lib: add missing removeAttrs builtin #227316