lib/types: init taggedSubmodule #254790
lib/types: init taggedSubmodule #254790
Conversation
github-actions
bot
added
6.topic: nixos
8.has: documentation
6.topic: module system
Lassulus
force-pushed
the
tagged_submodule
branch
from
a206d46
to
eb312d4
Compare
There was a problem hiding this comment.
The biggest problem with this might be options rendering in the manual, because it's not obvious how that should look like, and then it might also not be obvious how it should be used.
Can you link to the code in disko that uses such a type? I'd like to see some good use cases for it where the alternative would be worse.
|
yeah, the rendering in the manual is the biggest talking point for now. We discussed something like but this breaks for nesting, but maybe this is already an issue? these nested types are used everywhere in disko. Just look at any example: https://github.com/nix-community/disko/tree/master/example like here: https://github.com/nix-community/disko/blob/master/example/simple-efi.nix#L8 |
|
I think this would be very nice to have. Regarding documentation, this would increase the scope a fair bit but I'd love to see a "types" section in the manual which is separate from the options section. This would not only present a solution for arbitrarily nested types (as provided by disko, which supports various "container" types that can contain filesystems or further containers), but allow referencing types in multiple places much more elegantly. We already have various modules that reexport the options of an instance of the nginx virtualHost submodule, and having them all link to a single type documentation section would be awesome. |
|
@Lassulus I see thanks, so you have definitions like these: {
disk.foo = {
type = "disk";
content = { ... };
};
disk.bar = {
type = "zpool";
content = { ... };
};
}where depending on the Here's an alternate proposal that would be fairly idiomatic in the current module system and work with the current documentation rendering by default: {
disk = { ... };
zpool = { ... };
}where the attribute name determines the type/tag. This could use some new type to support that, e.g. This type would then essentially be implemented as |
|
ah that is not quite right. the type has nothing to do with the content. content is just an option which is another taggedSubmodule as type. so setting something like We can rename the type to tag, but IMHO I find type to be clearer, although it could be confused with the type of lib.mkOption |
|
so with your idea, the implementation of simple-efi would look something like this I guess? So the current types would just be some options? I have to play around with that and see what problems I encounter when implementing it. although I think it could be a bit harder. The hard part I'm not sure about is reusing of types. since something like the current SO how would I reuse the luks "type" here? currently it's a submodule but not sure if I can do this with the proposed type? |
This would then have to take into account priorities as well. Specifically the priority of the whole submodule would have to be used, but I normally only associate priorities with typical options, and I'm not even sure if priorities on entire submodules behave sensibly anyway... |
There was a problem hiding this comment.
Reviewed with the assumption that making the type tag look like an option and behave like an option is ok.
I guess what's not been addressed is your use of recursion. As a purely technical solution I've proposed to use visible = shallow, but perhaps we should instead make it easy to document a (sub)module separately, and refer to it from the NixOS options docs. In a way that seems cleanest, but it also diverges a lot from what we currently do for options, and making it work on search.nixos.org/options may be tricky.
As a workaround, perhaps the option where recursion happens could document something like
"This submodule accepts the same options as disk.<name>.content"
(or something like that)
| @@ -710,6 +710,23 @@ rec { | |||
| in mergedOption.type; | |||
| }; | |||
|
|
|||
| # A type that is one of several submodules, similiar to types.oneOf but is usable inside attrsOf or listOf | |||
| # submodules need an option with a type str which is used to find the corresponding type | |||
| taggedSubmodules = | |||
There was a problem hiding this comment.
| taggedSubmodules = | |
| taggedSubmodule = |
When used by itself, the option value is just a single submodule.
The naming convention describes the option value rather than the definition syntax (or whether multiple defs are ok).
Also apply to name below.
| , specialArgs ? {} | ||
| }: mkOptionType rec { | ||
| name = "taggedSubmodules"; | ||
| description = "one of ${concatStringsSep "," (attrNames types)}"; |
There was a problem hiding this comment.
| description = "one of ${concatStringsSep "," (attrNames types)}"; | |
| description = "submodule with type tag"; | |
| descriptionClass = "composite"; |
The valid type values can be documented in a generated type option.
| }: mkOptionType rec { | ||
| name = "taggedSubmodules"; | ||
| description = "one of ${concatStringsSep "," (attrNames types)}"; | ||
| check = x: if x ? type then types.${x.type}.check x else throw "No type option set in:\n${lib.generators.toPretty {} x}"; |
There was a problem hiding this comment.
check should return a bool, as it's used by either to do rudimentary but lazy type checks.
Assuming we keep the syntax as { type = "gpt"; partitions = ...; }, this should do an evalModules to get type option value, with something like freeformType = lazyAttrsOf raw; to ignore the other options. Alternatively we could handle the module arguments in a custom manner if that leads to better error messages.
| merge = loc: foldl' | ||
| (res: def: types.${def.value.type}.merge loc [ | ||
| (lib.recursiveUpdate { value._module.args = specialArgs; } def) | ||
| ]) | ||
| { }; |
There was a problem hiding this comment.
Same solution as in check should be used to get the type here.
| ]) | ||
| { }; | ||
| nestedTypes = types; | ||
| }; |
There was a problem hiding this comment.
TODO
- return the options for documentation
getSubOptions. Return anenumoption for the type tag, or maybe it should be more recognizable. - type merging would be nice, but we can scope that out. If not, we should probably make it behave like
enum, as in applying the union to the type tag, and merging modules for any overlapping definitions.
|
Also don't forget to write tests in |
roberth
changed the title
|
I'm gonna really have to see some examples and tests, the examples from disko are very hard to understand, regarding what is an option and what isn't and which options are allowed in which cases. |
| merge = loc: foldl' | ||
| (res: def: types.${def.value.type}.merge loc [ | ||
| (lib.recursiveUpdate { value._module.args = specialArgs; } def) | ||
| ]) |
There was a problem hiding this comment.
This makes it impossible btw to use mkMerge if one of the items to be merged is missing a type. Minimal example in disko where I encountered this ~yesterday:
{ lib, ... }:
with lib;
let
mkDataset = cfg: mkMerge [ cfg { type = "zfs_fs"; } ];
mkLegacyMount = p: { mountpoint = p; options.mountpoint = "none"; };
in {
disko.devices.zpool.tank.datasets = mapAttrs (const mkDataset) {
"foo" = mkLegacyMount "/foo";
"bar" =mkLegacyMount "/bar";
};
}This gives the following error:
error: No type option set in:
{
mountpoint = "/bar";
options = {
mountpoint = "none";
};
}
AFAIU this happens because each declaration on its own is passed into the wrapped type, i.e. the stuff from mkLegacyMount is passed unmerged, i.e. without the type from mkDataset.
Anyways, having attr-sets inside the module system where the surroundings (i.e. mkIf/mkMerge/etc) "silently" stop working[1] isn't very nice and it would be cool if this could get fixed. Alternatively I'd also be fine with a better error message that catches this issue properly (it's already rather simple to break a module evaluation in a way that it's hard to figure out the culprit and I'd rather not have yet another case).
[1] well, it fails, but it doesn't tell you that it fails because of mkMerge.
There was a problem hiding this comment.
This could probably be solved by doing an evalModules just to figure out the type and then evaluate the submodule again. (See https://github.com/NixOS/nixpkgs/pull/254790/files#r1323465281)
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/problems-with-types-oneof-and-submodules/15197/4 |
|
I've needed something like this multiple times in my projects, just stumbled upon this. Will enable me to write cleaner and more concise modules for NixNG (yes it's still alive, I'm in the process of supporting Forgejo nicely) |
|
@Lassulus I have a module called lib.flatten
[
''\newenvironment{${cfg.name}}''
"{% begin"
(lib.optional (options.begin.isDefined) cfg.begin)
"}%"
"{% end"
(lib.optional (options.end.isDefined) cfg.end)
"}%"
]So if they are undefined, I get back an empty list which gets removed via However, if I import this as a tagged submodule into another modules (in my case the module is called error: The option `test.templates.basic.content.blockquote.end` is used but not defined.Full Tracewarning: Git tree '/home/bakerdn/dev/djacu/nixcv' is dirty
error:
… while calling 'g'
at /nix/store/4fgs7yzsy2dqnjw8j42qlp9i1vgarzy0-source/lib/attrsets.nix:715:19:
714| g =
715| name: value:
| ^
716| if isAttrs value && cond value
… from call site
at /nix/store/4fgs7yzsy2dqnjw8j42qlp9i1vgarzy0-source/lib/attrsets.nix:718:20:
717| then recurse (path ++ [name]) value
718| else f (path ++ [name]) value;
| ^
719| in mapAttrs g;
… while calling anonymous lambda
at /nix/store/4fgs7yzsy2dqnjw8j42qlp9i1vgarzy0-source/lib/modules.nix:242:72:
241| # For definitions that have an associated option
242| declaredConfig = mapAttrsRecursiveCond (v: ! isOption v) (_: v: v.value) options;
| ^
243|
… while evaluating the option `test.templates.basic._out.latex':
… while calling anonymous lambda
at /nix/store/4fgs7yzsy2dqnjw8j42qlp9i1vgarzy0-source/lib/modules.nix:824:28:
823| # Process mkMerge and mkIf properties.
824| defs' = concatMap (m:
| ^
825| map (value: { inherit (m) file; inherit value; }) (builtins.addErrorContext "while evaluating definitions from `${m.file}':" (dischargeProperties m.value))
… while evaluating definitions from `/nix/store/z90fx0q5zl0gyisyw7bpcbbvvllff8bs-source/modules/templates/templates.nix':
… from call site
at /nix/store/4fgs7yzsy2dqnjw8j42qlp9i1vgarzy0-source/lib/modules.nix:825:137:
824| defs' = concatMap (m:
825| map (value: { inherit (m) file; inherit value; }) (builtins.addErrorContext "while evaluating definitions from `${m.file}':" (dischargeProperties m.value))
| ^
826| ) defs;
… while calling 'dischargeProperties'
at /nix/store/4fgs7yzsy2dqnjw8j42qlp9i1vgarzy0-source/lib/modules.nix:896:25:
895| */
896| dischargeProperties = def:
| ^
897| if def._type or "" == "merge" then
… from call site
at /nix/store/z90fx0q5zl0gyisyw7bpcbbvvllff8bs-source/modules/templates/templates.nix:34:10:
33| "\n\n"
34| (cfg.outOrdered "latex");
| ^
35| };
… while calling 'outOrdered'
at /nix/store/z90fx0q5zl0gyisyw7bpcbbvvllff8bs-source/modules/components/orderedTaggedContent.nix:53:18:
52| );
53| outOrdered = out:
| ^
54| lib.flatten (
… from call site
at /nix/store/z90fx0q5zl0gyisyw7bpcbbvvllff8bs-source/modules/components/orderedTaggedContent.nix:54:7:
53| outOrdered = out:
54| lib.flatten (
| ^
55| builtins.map
… while calling 'flatten'
at /nix/store/4fgs7yzsy2dqnjw8j42qlp9i1vgarzy0-source/lib/lists.nix:186:13:
185| */
186| flatten = x:
| ^
187| if isList x
… while calling anonymous lambda
at /nix/store/4fgs7yzsy2dqnjw8j42qlp9i1vgarzy0-source/lib/lists.nix:188:21:
187| if isList x
188| then concatMap (y: flatten y) x
| ^
189| else [x];
… from call site
at /nix/store/4fgs7yzsy2dqnjw8j42qlp9i1vgarzy0-source/lib/lists.nix:188:24:
187| if isList x
188| then concatMap (y: flatten y) x
| ^
189| else [x];
… while calling 'flatten'
at /nix/store/4fgs7yzsy2dqnjw8j42qlp9i1vgarzy0-source/lib/lists.nix:186:13:
185| */
186| flatten = x:
| ^
187| if isList x
… while calling anonymous lambda
at /nix/store/z90fx0q5zl0gyisyw7bpcbbvvllff8bs-source/modules/components/orderedTaggedContent.nix:56:10:
55| builtins.map
56| (x: x._out.${out})
| ^
57| cfg.contentsOrdered
… while calling anonymous lambda
at /nix/store/4fgs7yzsy2dqnjw8j42qlp9i1vgarzy0-source/lib/types.nix:527:14:
526| merge = loc: defs:
527| map (x: x.value) (filter (x: x ? value) (concatLists (imap1 (n: def:
| ^
528| imap1 (m: def':
… from call site
at /nix/store/4fgs7yzsy2dqnjw8j42qlp9i1vgarzy0-source/lib/modules.nix:846:59:
845| if isDefined then
846| if all (def: type.check def.value) defsFinal then type.merge loc defsFinal
| ^
847| else let allInvalid = filter (def: ! type.check def.value) defsFinal;
… while calling 'merge'
at /nix/store/4fgs7yzsy2dqnjw8j42qlp9i1vgarzy0-source/lib/types.nix:556:20:
555| check = isAttrs;
556| merge = loc: defs:
| ^
557| mapAttrs (n: v: v.value) (filterAttrs (n: v: v ? value) (zipAttrsWith (name: defs:
… from call site
at /nix/store/4fgs7yzsy2dqnjw8j42qlp9i1vgarzy0-source/lib/types.nix:557:35:
556| merge = loc: defs:
557| mapAttrs (n: v: v.value) (filterAttrs (n: v: v ? value) (zipAttrsWith (name: defs:
| ^
558| (mergeDefinitions (loc ++ [name]) elemType defs).optionalValue
… while calling 'filterAttrs'
at /nix/store/4fgs7yzsy2dqnjw8j42qlp9i1vgarzy0-source/lib/attrsets.nix:395:5:
394| # The attribute set to filter
395| set:
| ^
396| listToAttrs (concatMap (name: let v = set.${name}; in if pred name v then [(nameValuePair name v)] else []) (attrNames set));
… while calling anonymous lambda
at /nix/store/4fgs7yzsy2dqnjw8j42qlp9i1vgarzy0-source/lib/attrsets.nix:396:29:
395| set:
396| listToAttrs (concatMap (name: let v = set.${name}; in if pred name v then [(nameValuePair name v)] else []) (attrNames set));
| ^
397|
… from call site
at /nix/store/4fgs7yzsy2dqnjw8j42qlp9i1vgarzy0-source/lib/attrsets.nix:396:62:
395| set:
396| listToAttrs (concatMap (name: let v = set.${name}; in if pred name v then [(nameValuePair name v)] else []) (attrNames set));
| ^
397|
… while calling anonymous lambda
at /nix/store/4fgs7yzsy2dqnjw8j42qlp9i1vgarzy0-source/lib/types.nix:557:51:
556| merge = loc: defs:
557| mapAttrs (n: v: v.value) (filterAttrs (n: v: v ? value) (zipAttrsWith (name: defs:
| ^
558| (mergeDefinitions (loc ++ [name]) elemType defs).optionalValue
… while calling anonymous lambda
at /nix/store/4fgs7yzsy2dqnjw8j42qlp9i1vgarzy0-source/lib/types.nix:557:86:
556| merge = loc: defs:
557| mapAttrs (n: v: v.value) (filterAttrs (n: v: v ? value) (zipAttrsWith (name: defs:
| ^
558| (mergeDefinitions (loc ++ [name]) elemType defs).optionalValue
… while calling anonymous lambda
at /nix/store/4fgs7yzsy2dqnjw8j42qlp9i1vgarzy0-source/lib/modules.nix:824:28:
823| # Process mkMerge and mkIf properties.
824| defs' = concatMap (m:
| ^
825| map (value: { inherit (m) file; inherit value; }) (builtins.addErrorContext "while evaluating definitions from `${m.file}':" (dischargeProperties m.value))
… while evaluating definitions from `/nix/store/z90fx0q5zl0gyisyw7bpcbbvvllff8bs-source/modules/templates/templates.nix':
… from call site
at /nix/store/4fgs7yzsy2dqnjw8j42qlp9i1vgarzy0-source/lib/modules.nix:825:137:
824| defs' = concatMap (m:
825| map (value: { inherit (m) file; inherit value; }) (builtins.addErrorContext "while evaluating definitions from `${m.file}':" (dischargeProperties m.value))
| ^
826| ) defs;
… while calling 'dischargeProperties'
at /nix/store/4fgs7yzsy2dqnjw8j42qlp9i1vgarzy0-source/lib/modules.nix:896:25:
895| */
896| dischargeProperties = def:
| ^
897| if def._type or "" == "merge" then
… while calling 'g'
at /nix/store/4fgs7yzsy2dqnjw8j42qlp9i1vgarzy0-source/lib/attrsets.nix:715:19:
714| g =
715| name: value:
| ^
716| if isAttrs value && cond value
… from call site
at /nix/store/4fgs7yzsy2dqnjw8j42qlp9i1vgarzy0-source/lib/attrsets.nix:718:20:
717| then recurse (path ++ [name]) value
718| else f (path ++ [name]) value;
| ^
719| in mapAttrs g;
… while calling anonymous lambda
at /nix/store/4fgs7yzsy2dqnjw8j42qlp9i1vgarzy0-source/lib/modules.nix:242:72:
241| # For definitions that have an associated option
242| declaredConfig = mapAttrsRecursiveCond (v: ! isOption v) (_: v: v.value) options;
| ^
243|
… while evaluating the option `test.templates.basic.content.blockquote.end':
error: The option `test.templates.basic.content.blockquote.end' is used but not defined. |
|
I want to highlight Robert's new PR, which implements what I mentioned earlier and is much more solid imo: #284551 |
Yeah I saw that and it looks really cool! I couldn't tell if it's a complete replacement for this PR or not and this line from the body made me think it solves a similar but different problem. Or is that not true?
|
|
I'll respond to this comment by @Ma27 from #284551 here, since it's more relevant here:
I'm not really convinced of that being a good type design. If we compare it to structs and enums in Haskell/Rust/Swift/..., using # Type
attrTag {
sqlite = submodule {
options.file = lib.mkOption { type = path; };
};
mysql = submodule {
options.host = lib.mkOption { type = str; };
options.user = lib.mkOption { type = str; };
options.password = lib.mkOption { type = str; };
};
postgresql = submodule {
options.host = lib.mkOption { type = str; };
options.user = lib.mkOption { type = str; };
options.password = lib.mkOption { type = str; };
};
}
# Values
## Valid
{
mysql = {
host = "...";
user = "...";
password = "...";
};
}
## Invalid
{
sqlite.file = "...";
postgresql.user = "...";
}
# Usage
## This function doesn't exist yet, but should, it's very easy to implement
lib.match value {
sqlite = { file }: { /* ... */ };
mysql = { host, user, password }: { /* ... */ };
postgresql = { host, user, password }: { /* ... */ };
}And the equivalent in Rust: // Type
enum Database {
Sqlite {
file: File,
},
MySQL {
host: String,
user: String,
password: String,
},
Postgres {
host: String,
user: String,
password: String,
},
}
// Values
/// Valid
MySQL {
host: "...",
user: "...",
password: "...",
}
/// Invalid (compilation error)
Sqlite {
file: "...",
user: "...",
}
// Usage
match value {
Sqlite { file } => { /* ... */ },
MySQL { host, user, password } => { /* ... */ },
Postgresql { host, user, password } => { /* ... */ },
}Note how this is effectively a direct conversion. If you want, you can also introduce a variable in Nix or another type in Rust to share the field definitions between MySQL and Postgresql: # Type
let
uri = submodule {
options.host = lib.mkOption { type = str; };
options.user = lib.mkOption { type = str; };
options.password = lib.mkOption { type = str; };
};
in
attrTag {
sqlite = submodule {
options.file = lib.mkOption { type = path; };
};
# You can also do `mysql = uri`, though that would be more trouble if there's a field specific for only one of mysql/postgresql
mysql = submodule {
options.uri = lib.mkOption { type = uri; };
};
postgresql = submodule {
options.uri = lib.mkOption { type = uri; };
};
}Or Rust: // Type
struct URI {
host: String,
user: String,
password: String,
}
enum Database {
Sqlite {
file: File,
},
MySQL {
uri: URI,
},
Postgres {
uri: URI,
},
}In comparison, assuming I understood it correctly, using # Type
taggedSubmodule {
types = {
sqlite = submodule {
options.type = lib.mkOption { type = str; };
options.file = lib.mkOption { type = path; };
};
mysql = submodule {
options.type = lib.mkOption { type = str; };
options.host = lib.mkOption { type = str; };
options.user = lib.mkOption { type = str; };
options.password = lib.mkOption { type = str; };
};
postgresql = submodule {
options.type = lib.mkOption { type = str; };
options.host = lib.mkOption { type = str; };
options.user = lib.mkOption { type = str; };
options.password = lib.mkOption { type = str; };
};
};
}
# Values
## Valid
{
type = "mysql";
host = "...";
user = "...";
password = "...";
}
## Invalid
{
type = "sqlite";
file = "...";
user = "...";
}
# Usage
## We could also have a more convenient `match` function, though it's not really necessary
(value: {
sqlite = { /* value.file */ };
mysql = { /* value.{host,user,password} */ };
postgresql = { /* value.{host,user,password} */ };
}.${value.type})In Rust, this would look like this: Rust: // Type
enum DatabaseType {
Sqlite,
MySQL,
Postgresql,
}
struct Database {
type: DatabaseType,
file: Option<String>,
host: Option<String>,
user: Option<String>,
password: Option<String>,
}
// Values
/// Valid
Database {
type: MySQL,
file: None,
host: Some("..."),
user: Some("..."),
password: Some("..."),
}
/// Invalid (runtime error!)
Database {
type: Sqlite,
file: Some("..."),
host: Some("..."),
user: Some("..."),
password: Some("..."),
}
/// Invalid (runtime error!)
Database {
type: Sqlite,
file: None,
host: None,
user: Some("..."),
password: None,
}
// Usage
match value.type {
Sqlite => { /* database.file.unwrap() */ },
MySQL => { /* database.{host,user,password}.unwrap() */ },
Postgresql => { /* database.{host,user,password}.unwrap() */ },
}The This is also the reason why it's hard to render documentation for such a type. Furthermore it's also just less elegant and harder to implement correctly. This reminds me of null pointers or |
|
The problem is the intersection of tagged enum types with RFC42 -- when we're exposing an application's config structure directly rather than wrapping it with our own thing, we need to be able to type config without deciding its shape ourselves. |
I do still feel that way, although I have underestimated the difficulty of getting this one right. I don't really know what level of "right" we should go for, so I'll just share my thoughts. Maybe I should just encourage you to implement the previously suggested reviewed instead, but... It'd be a lot easier if we could improve the Now this approach is a lot to ask, and still has development risk, so I'm a little unsure whether to recommend taking some steps back like that, or continue to fix individual issues in the current implementation. I suppose another approach is to give up on type = taggedRecord {
bike = { handlebars = mkOption { ... }; };
car = { motor = mkOption { ... }; };
}This would mean giving up on the ability to use It's not very harmonious with the existing idioms at all that way, but maybe that's ok? |
That's the first time I hear that; this PR just uses disko as a motivating example which could also work with #284551. Can I see a RFC42 |
AFAIU #284551 (comment) would be an example. That's in fact the main reason why I chimed in again, I didn't really care if disko's internals are part of a stock module system before. |
|
My main reasoning for wanting this is that i try to use completely freeform options if possible, without any hardcoded or special cased options. If that is not possible, then i want to keep the option as similar to the upstream config file as possible and most upstreams use the |
wegank
added
the
2.status: stale
We can't make `port` and `uds` sum type, due to NixOS/nixpkgs#254790 not being merged.
| }; | ||
| someB = { | ||
| type = "b"; | ||
| foo = 456; |
There was a problem hiding this comment.
| foo = 456; | |
| bar = 456; |
Description of changes
This adds a new type: taggedSubmodules, which is similiar to oneOf but can be used in an attrsOf to have different submodules for each attr. A type like this is used heavily by disko and I briefly talked with @roberth about it and he said upstreaming it should be fine.
I'm not set on naming yet and haven't tested this fully. Just want to gather some ideas how to build this ideally. Happy to hear what you think! :)
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)