Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
roberth opened this issue Sep 26, 2023 · 4 comments
Labels
feature Feature request or proposal language The Nix expression language; parser, interpreter, primops, evaluation, etc

Comments

@roberth
Copy link
Member

roberth commented Sep 26, 2023

Is your feature request related to a problem? Please describe.

I'd expect the following to work,

lib.removeAttrs { a = 1; b = 2; } { "b" = throw "ignored"; } 
 == builtins.intersectAttrs ["a"] { "a" = 1; }

but instead I have to write this:

lib.removeAttrs { a = 1; b = 2; } [ "b" ]
 == builtins.intersectAttrs { "a" = throw "ignored"; } { "a" = 1; }

This is arbitrary. Let's just make it work.

Describe the solution you'd like

The second argument of removeAttrs is allowed to be an attrset where the values are ignored.

The first argument of intersectAttrs is allowed to be a list where the items are interpreted as attribute names.

Describe alternatives you've considered

  • Keep as is. People won't write expressions that only work in recent Nix versions.
  • Implement this logic in Nixpkgs lib.

Additional context

Priorities

Add 👍 to issues you find important.

@roberth roberth added feature Feature request or proposal language The Nix expression language; parser, interpreter, primops, evaluation, etc labels Sep 26, 2023
@infinisil
Copy link
Member

Not a fan of this, because:

  • Builtins don't need to have a good user interface, they just need to provide the primitive to allow doing basic operations efficiently. Nixpkgs' lib is a much better place for this.

  • I'd also rather avoid functions that accept multiple types, because it makes type annotations non-trivial and harder to understand:

    removeAttrs :: (List | Attrs) -> Attrs -> Attrs
    # Or:
    removeAttrs :: HasKeys ks => ks -> Attrs -> Attrs
    
  • In general it sounds like a design smell for functions to only use parts of their arguments. This leads to surprises, because users expect changes to arguments cause changes to the result.

    E.g. one might think that removeAttrs { a = 10; } { a = 10; b = 20; } == { b = 20; } compares the values of a to determine whether it should be removed, leading to surprises when changing the first a doesn't change the result.

  • It's not obvious which attributes removeAttrs fooAttrs barAttrs persist and which ones does it removes. This is worsened by intersectAttrs fooAttrs barAttrs working exactly the opposite way.

Imo the path forward is to leave the builtins alone and instead implement better functions in lib, e.g.:

  • lib.attrsets.removeKeys :: List -> Attrs -> Attrs
  • lib.attrsets.selectKeys :: List -> Attrs -> Attrs

Such that a == lib.attrsets.unionOfDisjoint (removeKeys k a) (selectKeys k a).

@roberth
Copy link
Member Author

roberth commented Mar 6, 2024

  • Builtins don't need to have a good user interface

True, but implemented in C++ would perform slightly better.

  • In general it sounds like a design smell for functions to only use parts of their arguments.

Should be somewhat expected in a lazy language, but I'm biased; I'm rather comfortable with laziness. That's rare.

removeKeys

We never use "keys". It's always something like attributes, attrNames, attrs.

List ->

Conversions to list tend to allocate more, and the algorithms can't take advantage of the list's properties, because it has basically none.

@infinisil
Copy link
Member

The performance argument is a really good one.

Here's another thought: These two operations (selecting attributes and removing attributes) can probably be done more efficiently together (which is often also what you actually need). So how about something like this:

builtins.diffAttrs { a = 0; b = 0; } { b = 1; c = 1; }
-> {
  only.left = { a = 0; };
  both.left = { b = 0; };
  both.right = { b = 1; };
  only.right = { c = 1; };
}

This does cost extra attribute allocations (though this could be optimised with an idea like #7676). However this cost is constant, compared to the cost of having to run both intersectAttrs and removeAttrs.

@roberth
Copy link
Member Author

roberth commented Apr 22, 2024

That's a lot like a zipAttrsWith, but limited to two arguments.

Another option is

combineAttrs (name: left: left) (name: left: right: left + right) (name: right: right) { a = 0; b = 1; } { b = 10; c = 20; }
{ a = 0; b = 11; c = 20; }

or if you want more flexibility in the style of lib.concatMapAttrs

mergeZipAttrs (name: left: { ${name} = left; }) (name: left: right: { ${name} = left + right; ${name}-left = left; ${name}-right = right; }) (name: right: { ${name} = right; }) { a = 0; b = 1; } { b = 10; c = 20; }
{ a = 0; b = 11; c = 20; b-left = 1; b-right = 10; }

Idk what's better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature request or proposal language The Nix expression language; parser, interpreter, primops, evaluation, etc
Projects
None yet
Development

No branches or pull requests

2 participants