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

init: lib.{collect',flattenAttrs} #221608

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 99 additions & 0 deletions lib/attrsets.nix
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,105 @@ rec {
else
[];

/* Recursively collect values produced by function `f` that verify a given
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change the format to commonmark. This PR is open for so long... conventions beeing changed in master branch already.

https://github.com/NixOS/nixpkgs/blob/master/lib/attrsets.nix#L65

Suggested change
/* Recursively collect values produced by function `f` that verify a given
/** Recursively collect values produced by function `f` that verify a given

predicate named `pred` from the set `attrs`. The recursion is stopped
when the predicate is verified.

If `pred` verifies the given `attrs` set, the result is a list with a
single element `f [ ] attrs`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Order of recursion? Looks like it traverse the leafs first then going up? Sometimes this can be important to know.


Attention:
Using this function on attribute sets that refer to themselves will cause
infinite recursion if the predicate allows that.

Example:
collect'
(_: v: isDerivation v)
(path: value: { inherit path value; })
{
a = «derivation 1»;
b = {
c = «derivation 2»;
};
not-a-derivation = 42;
}
=> [
{ path = [ "a" ]; value = «derivation 1»; }
{ path = [ "b" "c" ]; value = «derivation 2»; }
]

Type:
collect' :: ([String] -> a -> Bool) -> ([String] -> a -> b) -> AttrSet -> [b]
*/
collect' =
# Given an attribute's path and value, determine if recursion should stop.
pred:
# Given an attribute's path and value, produce the value to collect.
f:
# The attribute set to recursively collect.
attrs:
let
recurse = prefix: attrs:
concatMap
(name: visit (prefix ++ [ name ]) attrs.${name})
(attrNames attrs);
visit = path: value:
if pred path value then
[ (f path value) ]
else if isAttrs value then
recurse path value
else
[ ];
in
visit [ ] attrs;

/* Flatten an attribute set `attrs` with nested attributes where leaf nodes
verify a given predicate named `pred`. That is, a flattened set will have
all leaf values at top level. Attribute paths are transformed to names
using the function `f`.
Comment on lines +499 to +502
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/* Flatten an attribute set `attrs` with nested attributes where leaf nodes
verify a given predicate named `pred`. That is, a flattened set will have
all leaf values at top level. Attribute paths are transformed to names
using the function `f`.
/* Consider the final argument to be the root of a tree.
Every node that satisfies `isLeaf` or is not an attribute set is considered a leaf.
Derive an attribute name from each path using `f`, and return them as a single attribute set.

More declarative, less control flow.


Attention:
Using this function on attribute sets that refer to themselves will cause
infinite recursion if the predicate allows that.
Comment on lines +505 to +506
Copy link
Member

@roberth roberth Mar 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Using this function on attribute sets that refer to themselves will cause
infinite recursion if the predicate allows that.
If your input is cyclic or diverging, make sure that `isLeaf` only returns `true` for a finite part of your input.
The returned flat attribute set does not exhibit the laziness of the original input.
Specifically, `flattenAttrs` can not return before evaluating all leaves (except when `f` behaves as `x: true`, in which case `flattenAttrs f` is a convoluted attribute renaming function).

Diverging inputs also need a good isLeaf.
Might want to simplify my phrasing though; not sure.


Example:
flattenAttrs
isDerivation
(concatStringsSep "-")
{
foo = «derivation 1»;
bar = {
linux = «derivation 2»;
darwin = «derivation 3»;
};
baz = {
brr = 42;
};
}
=> {
foo = «derivation 1»;
bar-linux = «derivation 2»;
bar-darwin = «derivation 3»;
baz-brr = 42;
}

Type:
flattenAttrs :: (Any -> Bool) -> ([String] -> String) -> AttrSet -> AttrSet
*/
flattenAttrs =
# Given an attribute's value, determine if recursion should stop.
pred:
# Given an attribute's path, produce the top-level name in new attribute set.
f:
# The attribute set to recursively flatten.
attrs:
if pred attrs then attrs
else
listToAttrs (map (x: nameValuePair (f x.path) x.value) (collect'
(_: v: pred v || !isAttrs v)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the tree only contains the empty path, and the root isn't an attrset?
I think you could come up with this in the tests by thinking more about the possible inputs, besides the desired results.

(path: value: { inherit path value; })
attrs));

/* Return the cartesian product of attribute set value combinations.

Example:
Expand Down
8 changes: 4 additions & 4 deletions lib/default.nix
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,10 @@ let
composeManyExtensions makeExtensible makeExtensibleWithCustomName;
inherit (self.attrsets) attrByPath hasAttrByPath setAttrByPath
getAttrFromPath attrVals attrValues getAttrs catAttrs filterAttrs
filterAttrsRecursive foldlAttrs foldAttrs collect nameValuePair mapAttrs
mapAttrs' mapAttrsToList concatMapAttrs mapAttrsRecursive mapAttrsRecursiveCond
genAttrs isDerivation toDerivation optionalAttrs
zipAttrsWithNames zipAttrsWith zipAttrs recursiveUpdateUntil
filterAttrsRecursive foldlAttrs foldAttrs collect collect' flattenAttrs
nameValuePair mapAttrs mapAttrs' mapAttrsToList concatMapAttrs
mapAttrsRecursive mapAttrsRecursiveCond genAttrs isDerivation toDerivation
optionalAttrs zipAttrsWithNames zipAttrsWith zipAttrs recursiveUpdateUntil
recursiveUpdate matchAttrs overrideExisting showAttrPath getOutput getBin
getLib getDev getMan chooseDevOutputs zipWithNames zip
recurseIntoAttrs dontRecurseIntoAttrs cartesianProductOfSets
Expand Down
109 changes: 109 additions & 0 deletions lib/tests/misc.nix
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,115 @@ runTests {
};
};

# code from the example
testCollect'Example = let
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe

Suggested change
testCollect'Example = let
testCollectPrimeExample = let

Since your test seems to be the first one that tests a function ending in "prime", following test contributions might the naming convention.

These are a little awkward to type:

mapAttrs'Example
overrideScope'Example

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a PrimeExample of an unnecessary rule that may lead to more discomfort rather than less.

To be fair, I wouldn't actually call it a prime example, but I couldn't resist the pun, and it actually makes a point!
However, test names are a bit inconsequential.

drvf = name: derivation { inherit name; builder = "builder"; system = "system"; };
drv1 = drvf "drv1";
drv2 = drvf "drv2";
in {
expr = collect'
(_: v: isDerivation v)
(path: value: { inherit path value; })
{
a = drv1;
b = {
c = drv2;
};
not-a-derivation = 42;
};
expected = [
{ path = [ "a" ]; value = drv1; }
{ path = [ "b" "c" ]; value = drv2; }
];
};

testCollect'TopLevel = let
attrs = {
a = 1;
b = {
c = 2;
};
};
in {
expr = collect'
(_: _: true)
(path: value: { inherit path value; })
attrs;
expected = [
{ path = [ ]; value = attrs; }
];
};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would you expect:

collect' (_: v: !isAttrs v) (path: value: { inherit path value; }) { a = 1; b = {}; }

# code from the example
testFlattenAttrsExample = let
drvf = name: derivation { inherit name; builder = "builder"; system = "system"; };
drv1 = drvf "drv1";
drv2 = drvf "drv2";
drv3 = drvf "drv3";
in {
expr = flattenAttrs
isDerivation
(concatStringsSep "-")
{
foo = drv1;
bar = {
linux = drv2;
darwin = drv3;
};
baz = {
brr = 42;
};
};
expected = {
foo = drv1;
bar-linux = drv2;
bar-darwin = drv3;
baz-brr = 42;
};
};

testFlattenAttrsShortestDuplicateTakesPrecedence = {
expr = flattenAttrs
(_: false) # always recurse
(concatStringsSep "-")
{
foo = {
bar = 1;
};
foo-bar = 2;

a = {
b = {
c = 1;
};
b-c = 2;
};
a-b-c = 3;
};
expected = {
foo-bar = 1;
a-b-c = 1;
};
};

testFlattenAttrsNoRecurse = let
attrs = {
a = {
b = {
c = 1;
};
b-c = 2;
};
a-b-c = 3;
};
in {
expr = flattenAttrs
(_: true)
(throw "never recurses")
attrs;
expected = attrs;
};

# code from the example
testRecursiveUpdateUntil = {
expr = recursiveUpdateUntil (path: l: r: path == ["foo"]) {
Expand Down