Lib.types.anything's merge function behavior regarding lists

Hello!

In lib.types.anything definition, I’m having trouble understanding the purpose of lines 258 to 261.

From what I understand:

  • if the number of option definitions is less than or equal to 1, the function simply “merges” (i.e., keeps) the value.
  • otherwise, it throws an error due to conflicting definitions, avoiding the complexity and potential risks of merging multiple lists, which might lead to unwanted or unsafe behavior.

What are the benefits of this approach compared to removing those lines entirely (so that lib.types.mergeEqualOption would be called instead)? This alternative seems to retain the behavior of rejecting the merge of different lists (i.e., lists that don’t match the != operator) while also handling options that contain the same values, which, as far as I can see, wouldn’t introduce any unsafe or unwanted behavior.

Am I missing something?

Thank you for your help

2 Likes

Author of that type here. Nice catch, I think you’re right, there’s no reason not to have it default to mergeEqualOption! Feel free to PR that :smiley:

1 Like

Thank you for your super fast and clear answer, here is the PR.

It goes straight to the point of removing the few lines I mentioned above.
Do I need to do other things ? Its my first contribution to nixpkgs other than packages :slightly_smiling_face:.
I didn’t write no tests, nor executes anything to run existing ones.

Thank you for your help

Thanks! Yeah tests would be good, I left a comment in the PR :slight_smile:

I’ve updated the PR.

I’m struggling to run the tests locally with the script where a lot of tests fails for what seems to be unrelated reasons

====== module tests ======
174 Pass
79 Fail

and with nix-build lib/tests/release.nix which takes forever (basically 40mn+ before I cut) and spams crazy long messages like

trace: evaluation warning: tryEval failed at: freshBootstrapTools.test-pkgs.buildPackages.buildPackages.buildPackages.buildPackages.buildPackages.buildPackages.buildPackages.buildPackages.buildPackages.buildPackages.buildPackages.buildPackages.buildPackages.buildPackages.buildPackages.buildPackages.buildPackages.buildPackages.buildPackages.buildPackages.buildPackages.buildPackages.buildPackages.buildPackages.buildPackages.buildPackages.buildPackages.buildPackages.buildPackages.buildPackages.........

and the next one containing one more .buildPackages and so one.

I’ll see tomorrow to try to fix that and make the test run locally.

Oh that’s odd. I recommend just running the script directly:

lib/tests/modules.sh

Is there a reason why this type has different semantics to listOf anything for merging?

@jakehamilton The preference should always be to define a specific type that matches what the module expects. types.anything’s main purpose is to be a “safe” fallback when it’s unclear what the type should be, with behavior that’s as generic (accepting all values) and predictable (having expected merge results) as possible. I intended for types.anything to eventually replace types.unspecified (which has terrible merge behavior!) as the default option type.

The problem with listOf anything’s merging behavior is that it merges by concatenating values in somewhat non-deterministic order (it depends on imports order), which can cause problems when the order is important (and it has before!). Furthermore, merging by concatenation can break stuff, such as when lists should have a fixed length (e.g. a transformation Matrix), or if the list represents one “thing” by itself (e.g. interpolation points for a gradient). That’s why it’s not fit as a safe default for types.anything, even though it would work in most cases.

1 Like

Gotcha, thanks for the insight! I think I have just been surprised in the past when the two look like they should operate the same, but don’t.

I feel that, for list types, order is unimportant and it is fine to merge things via concatenation. Assuming, of course, that an ordered alternative exists. I don’t think nixpkgs has something like that outside of ordered strings. Perhaps Home-Manager’s DAG type should be used in these places instead? I’ve had some great success working with it to implement much more complex behaviors where order is important.