module system: type merging for enum, nullOr and listOf by ericsagnes · Pull Request #18380 · NixOS/nixpkgs

ericsagnes · 2016-09-07T01:14:11Z

Motivation for this change

This adds type merging for enum, nullOr and listOf types.

It allow to extend type definitions for enum, nullOr enum and listOf enum options over multiple modules files in a decentralized manner.

Idea come from #15096 (comment).

Example use case: Input methods

This functionality can be used extend sum types defining a choice without changing the base module.

There can be only one input method enabled system-wide, so the enabled input method option is declared as an enum in the central input method module (default.nix).
Every input method has its own set of options and config and so is managed in its own module file.

Currently, options declarations are closed so the declaration in default.nix must contains all the possible choices:

default.nix

  enabled = mkOption {
    type    = types.nullOr (types.enum [ "ibus" "fcitx" "nabi" "uim" ]);
    default = null;
    ...
  };

That means that any addition or removal of input methods result in a change in the enabled declaration in default.nix.

With type merging, it is possible to make enabled a placeholder in the main module:

default.nix:

  enabled = mkOption {
    type    = types.nullOr (types.enum [ ]);
    default = null;
    ...
  };

And to extend it in other input methods modules files:

ibus.nix:

  enabled = mkOption {
    type = types.nullOr (types.enum [ "ibus" ]);
  };

As a result, option declaration is decentralized and modularity improved.

Manual

In the manual, all the files that extends or declare the type are listed in Declared by:, sample output:

i18n.inputMethod.enabled

    Select the enabled input method. Input methods is a software to input symbols that are not available on standard input devices. Input methods are specially used to input Chinese, Japanese and Korean characters.

    Type: null or one of "uim", "nabi", "ibus", "fcitx"

    Default: null

    Example: "fcitx"

    Declared by:
    <nixpkgs/nixos/modules/i18n/input-method/uim.nix>
    <nixpkgs/nixos/modules/i18n/input-method/nabi.nix>
    <nixpkgs/nixos/modules/i18n/input-method/ibus.nix>
    <nixpkgs/nixos/modules/i18n/input-method/fcitx.nix>
    <nixpkgs/nixos/modules/i18n/input-method/default.nix>

This functionality is useful for any case where modular sum types options could make sense (display managers, window managers, desktop managers modules, ...)

cc @nbp

Things done

Tested using sandboxing
(nix.useSandbox on NixOS,
or option build-use-sandbox in nix.conf
on non-NixOS)
Built on platform(s)
- NixOS
- OS X
- Linux
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

mention-bot · 2016-09-07T01:14:13Z

@ericsagnes, thanks for your PR! By analyzing the annotation information on this pull request, we identified @edolstra, @nbp and @shlevy to be potential reviewers

ericsagnes · 2016-09-07T01:15:11Z

lib/modules.nix

@nbp It is needed to merge enums because two enum with different parameters have different names, but I am not very sure of the implications removing this can have.
Is this safe?

The goal of this check was to prevent the extension of additional option declarations with a type which is not identical. For example, submodule option has a documentation string (name), thus we can extend them with additional submodules. Another example, is with 2 identical types, such as uniq string can be merged with each others.

So this is technically a regression.

The generic question is more subtle than that, which is, is the merge procedure between these 2 types commutative? But honestly, we don't want to answer this one, because in practice this does not hold true, the reason being that the order of the merge is unspecified.

With the addition of typeMerge function, I would recommend to drop the use of these documentation strings (name), in favor of easily comparable types which can be used to uniquely identify a type, and also in a way to carry over the values out-side the comparison set.

So, one potential option would be to replace the inhabitant attribute, by a functor attribute, which can be compared and merged.

{ enum = values: mkOptionType { functor = { type = enum; wrapped = []; payload = [ values ]; }; }; loaOf = elemType: mkOptionType { functor = { type = loaOf; wrapped = [ elemType ]; payload = []; }; }; }

Thus merging option declarations will recursively check for if the functions are identical, and if we can zip the arguments, otherwise we should throw.

What do you think?

nbp · 2016-09-07T18:52:07Z

@ericsagnes , Thanks.
I will carefully review these changes on Saturday, ping me on irc if I forgot.

nbp · 2016-09-10T14:48:10Z

Even with the comment above which explains why we cannot merge it as-is, this is still some nice work!

ericsagnes · 2016-09-10T17:43:42Z

@nbp

Indeed, the functor idea is a great one. Thanks!
I like the new code a lot more :)

I slightly adapted your suggestion to something I found more practical to implement:

wrapped and payload are not lists but just the values or elemType of the type to simplify the default merging
added a binary operation to combine payloads values, binOp, to make the default type merging function as generic as possible

Other changes:

the removed check is back and augmented with a condition that verifies that types are not mergeable
I took the freedom to refactor a little the check condition to make it easier for the eyes

I tried to play with nix-repl but couldn't find a way to check that two functions are identical, so I restored the previous check for the moment.

> let a = x: 1; b = y: 1; in [ (a == a) (a == b) ]
[ false false ]

Maybe I am missing something obvious here?

There is always the possibility to add a string representation of the type function name to the functor in case function check is not possible, but I would prefer to have your opinion before going this way.

Note: I let the changes in a separate commit for easier review, but will squash when the PR will be ready.

ericsagnes · 2016-09-11T07:02:52Z

@nbp

I added a new commit to separate type names and descriptions.
That should address the problems that name ambiguity had.

I also changed the fixupOptionType in lib/module.nix to make it work with the type name and functor as it was relying on the type name only.

The check removed in the first commit is now restored in its original state (beside the second commit refactor).

ericsagnes · 2016-09-12T06:46:06Z

update:

fixed a bug with the bothHave function.
removed unneeded checks.

nbp · 2016-09-12T18:43:51Z

lib/options.nix

Identically, update https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/installer/tools/nixos-option.sh#L259

nbp · 2016-09-12T19:16:48Z

@ericsagnes

let a = x: 1; b = y: 1; in [ (a == a) (a == b) ]
[ false false ]
Maybe I am missing something obvious here?

Hum … Last time I tried that the result was [ true false ]. Maybe this is a regression in the Nix language. (@edolstra ?)

ericsagnes · 2016-09-13T00:32:31Z

@nbp

Thanks for the review!
I added a commit that should address all the points, let me know if I missed something.

ericsagnes · 2016-09-18T01:36:26Z

update: added some documentation.

The commits are kept for review, and will be squashed when the PR is ready.

@nbp Could you review the recent changes?

ericsagnes · 2016-09-27T12:08:05Z

@nbp ping (sorry for spamming but I don't know who could review this PR beside you)

joachifm · 2016-10-01T15:46:31Z

cc @Ericson2314 might be able to provide some feedback

Ericson2314 · 2016-10-14T21:19:32Z

Hmm me? I'm not very familiar with the inner workings of the module system. From a general PL perspective, I'd consider making extensibililty opt-in--i.e. have closedEnum and openEnum--but I haven't thought about the specifics of the nixpkgs usecase.

Also, while I'm in general happy to review things note I don't have push access.

nbp

Sorry for the delay.

I think these changes are going in the right direction. Still, I do not think it is yet acceptable.

nbp · 2016-10-31T16:47:23Z

lib/types.nix

Shouldn't this just return null, as defined as part of the typeMerge property? Also returning null will display the generic error message about non merge-able options.

Merging 2 identical simple type should be accepted. bool and bool should merge fine without returning null. I suggest you change the default value of typeMerge to defaultTypeMerge functor.

nbp · 2016-10-31T16:50:59Z

lib/types.nix

We should also guard that f'.wrapped is not null and return null. Otherwise this function would fail with an error message which would mention that we expected wrapped to be an attribute set, while we got null, which is not as helpful as the error message related to the fact that we cannot merge both types.

Do the same for the payload attribute below.

nbp · 2016-10-31T16:52:51Z

lib/types.nix

nit: The comment should distinguish the two, by mentioning that the description is defined recursively, by embedding the type of the wrapped type, if any.

nbp · 2016-10-31T17:00:48Z

lib/types.nix

Testing the name in the either type should no longer be necessary iff this is being handler by the defaultTypeMerge function. In which case, only mt1 == null || mt2 == null would be necessary.

nbp · 2016-10-31T17:06:29Z

lib/types.nix

nit: Should we avoid duplicates in the enumerated values?

nbp · 2016-10-31T17:14:56Z

lib/types.nix

Why should these be re-computed, as we already know them as t1 and t2 from the either function arguments?

nbp · 2016-10-31T17:16:32Z

lib/types.nix

We should check that f.type.name == name before interpreting f'.wrapped as being a list.

nbp · 2016-10-31T17:21:11Z

lib/types.nix

This is not a commutative behaviour, either both types should be mergeable, or we should not be able to merge this either type.

ericsagnes · 2016-11-01T05:10:30Z

Thanks for the review!

I addressed all the points in the latest commit, it looks cleaner now.

We should check that f.type.name == name before interpreting f'.wrapped as being a list.

There was a problem with that as f.type is the unevaluated type function, so it is not easy to extract name from it.

I chose to add the name to the functor but let me know if there is a better way.

The commit also introduce a few other changes:

defaultFunctor is now a function taking the type name.
defaultFunctor type is automatically derivated from name (when possible).
As typeMerge is now set by default, type merging was explicitly disabled for submodule
explicit typeMerge for separatedString as string types with a different separator should not be merged
more checks in defaultTypeMerge

nbp

Apart from the submodule type typeMerge function, which would fail on NixOS, this patch looks perfect.

This feature would be awesome once we could use it for all the examples listed in the documentation nit below.

nbp · 2016-11-01T15:51:08Z

lib/modules.nix

nit: sameType is unused, remove it.

nbp · 2016-11-01T15:52:50Z

lib/modules.nix

nit: add parenthesis around this expression. (bothHave "type" && ...)

nbp · 2016-11-01T15:54:30Z

lib/types.nix

nit: remove useless parenthesis.

nbp · 2016-11-01T16:01:09Z

lib/types.nix

note: This assumes that null is not a valid payload, but I am fine with this restriction.

nbp · 2016-11-01T16:04:17Z

lib/types.nix

sep does not have any meaning as part of a functor. I think what you could do is:

{ functor = (defaultFunctor name) // { payload = sep; binOp = sepLhs: sepRhs: if sepLhs == sepRhs then sepLhs else null; }; }

nbp · 2016-11-01T16:05:57Z

lib/types.nix

With the new functor from above, the defaultTypeMerge should be enough. So removing this code should be fine.

nbp · 2016-11-01T16:14:39Z

nixos/doc/manual/development/option-declarations.xml

nit: we cannot remove, unless we do not include a module.

nbp · 2016-11-01T16:18:00Z

nixos/doc/manual/development/option-declarations.xml

foo, bar, and baz might not be easy to follow, maybe taking a concrete example might help get a better understanding of the feature provided by extensible option types.

For examples:

service: audio; backends: alsa, pulseaudio.

service: webserver; backends: apache, nginx.

service: init-system; backends: upstart, systemd, initd.

service: display manager; backends: kdm, slim.

nbp · 2016-11-01T16:24:15Z

nixos/doc/manual/development/option-declarations.xml

nit: I would replace Extended by Extensible, and do the same for all the references below as well.
Extended would apply to the result, not the property of a type.

nbp · 2016-11-01T16:45:35Z

lib/types.nix

I highly doubt this would work with the current NixOS modules, as we at least use that for the fileSystems.

Currently the merging process of submodules is handled by mergeOptionDecls, but we still expects to be able to merge the type. The opts value is similar to the list of enumerated values of the enum type, but as the submodules are extracted, I guess you could just define the following functor:

{ functor = (defaultFunctor name) // { # Merging of submodules is done as part of mergeOptionDecls, as we have to annotate # each submodule with its location. payload = []; binOp = lhs: rhs: []; }; }

ericsagnes · 2016-11-04T02:52:50Z

Thanks again for the review!

Addressed all the points in the last commit and added a changelog.

nbp

Apart from the documentation nits, the technical part is good and mergeable.

nbp · 2016-11-05T20:37:51Z

nixos/doc/manual/development/option-declarations.xml

nit: namely enum and submodules and any composed forms of them.

nbp · 2016-11-05T20:38:59Z

nixos/doc/manual/development/option-types.xml

nit: separatedString and submodules are also value types.

nbp · 2016-11-05T20:42:08Z

nixos/doc/manual/release-notes/rl-1703.xml

nit: to extend certain types, such as enum, through multiple option declarations of the same option across multiple modules.

nbp · 2016-11-05T20:42:52Z

nixos/doc/manual/development/option-types.xml

nit: not the only type.

ericsagnes · 2016-11-05T21:38:30Z

Thanks! Updated the documentation and squashed the commits so this should be ready to merge.

nbp · 2016-11-05T23:06:23Z

@ericsagnes Awesome work! Thanks for your dedication and patience :)

ericsagnes · 2016-11-06T00:57:20Z

Thanks for merging!

ericsagnes reviewed Sep 7, 2016
View reviewed changes

nbp added the 0.kind: enhancement Add something new or improve an existing system. label Sep 10, 2016

nbp reviewed Sep 12, 2016
View reviewed changes

ericsagnes force-pushed the feat/typeMerge branch 2 times, most recently from 58683dc to d1cccae Compare September 18, 2016 01:33

ericsagnes force-pushed the feat/typeMerge branch from d1cccae to 373ccdf Compare October 1, 2016 23:47

nbp suggested changes Oct 31, 2016

View reviewed changes

nbp suggested changes Nov 1, 2016

View reviewed changes

nbp added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 9.needs: changelog This PR needs a changelog entry labels Nov 1, 2016

nbp added this to the 17.03 milestone Nov 1, 2016

ericsagnes force-pushed the feat/typeMerge branch from 9424bd9 to c34a62f Compare November 4, 2016 02:48

nbp approved these changes Nov 5, 2016

View reviewed changes

nbp added 8.has: changelog This PR adds or changes release notes and removed 9.needs: changelog This PR needs a changelog entry labels Nov 5, 2016

module system: extensible option types

10bc213

ericsagnes force-pushed the feat/typeMerge branch from c34a62f to 10bc213 Compare November 5, 2016 21:36

nbp merged commit e14de56 into NixOS:master Nov 5, 2016

nbp mentioned this pull request May 26, 2017

displayManager, windowManager, desktopManager: use extensible option types #20271

Closed

6 tasks

Zocker1999NET mentioned this pull request Oct 5, 2024

lib.types: add mergeTypes, a commutative type merger #346674

Open

13 tasks

Uh oh!

Conversation

ericsagnes commented Sep 7, 2016

Motivation for this change

Example use case: Input methods

Manual

Things done

Uh oh!

mention-bot commented Sep 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nbp commented Sep 7, 2016

Uh oh!

nbp commented Sep 10, 2016

Uh oh!

ericsagnes commented Sep 10, 2016

Uh oh!

ericsagnes commented Sep 11, 2016

Uh oh!

ericsagnes commented Sep 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nbp commented Sep 12, 2016

Uh oh!

ericsagnes commented Sep 13, 2016

Uh oh!

ericsagnes commented Sep 18, 2016

Uh oh!

ericsagnes commented Sep 27, 2016

Uh oh!

joachifm commented Oct 1, 2016

Uh oh!

Ericson2314 commented Oct 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nbp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericsagnes commented Nov 1, 2016

Uh oh!

nbp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Ericson2314 commented Oct 14, 2016 •

edited

Loading