lib: let getAttrs ignore nonexistent keys

[h7x4]

This PR contains an alternative implementation of lib.attrsets.getAttrs which ignores keys which do not exist in the attrset. It now behaves more similarly to lib.removeAttrs, which I've considered to be it's opposite. I am not aware of any usage where we depend on getAttrs throwing in absence of expected attributes (at least not solely), but I've only looked at in-tree usage.

I'm not really sure that this is the behavior we want for this function considering it's named getAttrs, but it would be useful to have a function that mirrors removeAttrs more accurately. I don't mind making this into a new function if we want to keep the old behavior for this one.

I believe this increases the time complexity. As far as I can tell, the old implementation was O(n) where n is the size of the key list, while this is O(n) + O(x log y) where n is the key list and x and y depends on which is large of the key list and the input attrs (according to builtins.intersectAttrs).

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.

[roberth]

I recognize the problem, but I'm not happy about the solution.

• This blurs the intent of calling getAttrs

• This change delays errors to the moment where the result is accessed instead of the getAttrs call.
  In general, reporting errors sooner makes them easier to understand because it is closer to the caller's context. Some error context will literally be lost if the code can be abstracted to reduce (produce x) when produce x returns successfully and the trace of the produce part would have shown the info. The error trace of produce x is particularly relevant if it contains addErrorContext calls.

• removeAttrs, which I've considered to be its opposite.

  This might be unnecessary, but let's formalize it a little bit.

  removeAttrs a b can be seen as difference $$A \setminus B$$ by forgetting the attribute values.
  Taking the complement of the second operand, we get $$A \setminus B^{\complement}$$, which is equivalent to the intersection of $$A$$ and $$B$$, i.e., $$A \cap B$$.

  To summarize, removeAttrs can be seen as a form of set difference, and then the complement operation transforms it into an intersection.

  So from that I conclude that we should just call it lib.intersectAttrs. This already exists in builtins, but doesn't accept the exact same types you'd like to use.

  I've proposed to change that in Nix, but it could certainly be done in lib instead, as far as I'm concerned, and that might be a better first step, or even a better approach altogether.

  The builtin is right, biased, ignoring the left-hand values, so that's also the operand that where we can allow a list of names instead of an attrset.

  See also removeAttrs: allow removals to be specified as attrset, and conversely for intersectAttrs nix#9050

---

As far as I can tell, the old implementation was O(n) where n is the size of the key list,

Probably needs to sort the keys internally anyway. A log factor is a lot like a constant factor though, so if we really want to say something about performance, we'll have to measure it.

[h7x4]

Just to make sure I'm understanding you correctly, you're suggesting we change lib.intersectAttrs (and possibly lib.removeAttrs) to differ from their builtins counterparts and act like Attrs -> Attrs -> Attrs set functions?

Changing the builtins variant from its lib variant feels illegal, but I've never seen any written rule saying it's a no go. I suppose the only one I've seen that's currently deviating is lib.foldl' (but only due to a bug in nix?).

Would it be a bad idea to create a [String] -> Attrs -> Attrs variant for the time being, or as an alternative? I suspect there are a lot of areas where people might want to just remove/keep a few attrs, and having them create an attrset with a bunch of null values as input seems like a worse developer experience. Maybe we could create a better name for it so the intention is clearer than with getAttrs. If the name is a bit longer it might feel like you're opting in to postponing the errors as getAttrs will be the quicker one to reach for?

[roberth]

for the time being

If you need it for other code, I recommend putting it locally in a let binding for the time being, with a comment pointing to this discussion.
That way we keep lib simple by default.
I'd like to hear @infinisil's thoughts, and I hope we can decide something soon.

[infinisil]

Check out #269586, which is essentially the same idea but has a lot of discussion already.