lib/types: better error when trying to order submodules and optionTypes

[ncfavier]

The following contrived example:

with lib; (evalModules {
  modules = [ {
    options.foo = mkOption {
      type = types.submodule { };
    };
    config.foo = mkBefore { };
  } ];
}).config.foo

currently fails with error: anonymous function at nixpkgs/lib/types.nix:608:33 called with unexpected argument 'priority'.

This is because sortProperties adds a priority attribute to mkOrdered definitions for sorting purposes, but some consumers (here the submodule merge function) only expect value and file attributes.

While there is usually no reason to use ordering on submodule definitions, there is no reason it should fail either.

The other solution would be to discard the priority after sortProperties is done sorting, but I think it's more future-proof to allow definition values to come with arbitrary metadata.

[roberth]

While there is usually no reason to use ordering on submodule definitions, there is no reason it should fail either.

The alternative is to silently ignore the mkBefore context, which isn't propagated into the submodule's options. I don't think submodule should implement the ordering contract in any way shape or form, because it will eventually lead to submodule options having mkBefore (mkAfter x)) which is meaningless without resorting to lexicographic ordering, assuming lexicographic ordering even has the intended effect.

allow definition values to come with arbitrary metadata.

Often less is more. By allowing ignored metadata to seep into the module system implementation, we create a situation where it isn't clear which information is used, required or discarded, and where consumers of such information can pop up in unexpected places without having to consider the overall design of the module system, including bad interactions such as the one I just mentioned. We're finding out about it because it triggers an error, which is a better than allowing all sorts of ambiguities to build up over time.

That said, I am interested in carrying _module and options metadata through submodule and attrsOf for the purpose of allowing the module author to wire up standardized warnings and assertions, something like #97023 (comment), to revive that idea in a way that doesn't add strictness.

[roberth]

Regarding the linked home manager PR, it'd be wise to contribute the dag type, or a similar type to nixpkgs where it can be unit tested with the rest of the modules system, so that we won't accidentally break it in the future.

[ncfavier]

It's not exactly silently ignored: you could still order the submodule definitions, but this order would only matter in unspecified ways (for example, if you ensure that one submodule definition A containing a list definition comes before another submodule definition B, then the lists will actually be merged in the reverse order B ++ A). So I probably agree that an error is good, although this could really use a better error message.

On the other hand, I now think we should keep the priority attribute in definitions, because I was able to use it in nix-community/home-manager#2950 (comment) to make DAG definitions like dagEntry = mkBefore value; work by pushing down the priority from the submodule definition to the data definition in the submodule. (I only did this when value is an actual data value rather than a submodule, so that we don't run into things like dagEntry = mkAfter (dag.entryAnywhere (mkBefore value)); like you described. In this case the outer priority is ignored.)