Skip to content

lib/types.nix: implement getSubOptions for either #422272

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.

Sign up for GitHub

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

Open
lilyball wants to merge 7 commits into
base: master
Choose a base branch
from
Open

lib/types.nix: implement getSubOptions for either #422272

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8895863
lib/types.nix: implement getSubOptions for either
lilyball Jul 3, 2025
135273f
nixos/dconf: fix option examples
lilyball Jul 3, 2025
1f90ec3
nixos/pay-respects: fix option documentation
lilyball Jul 3, 2025
1a61a83
nixos/rspamd: fix option documentation
lilyball Jul 3, 2025
4edc3d7
nixos/movim: add missing option documentation
lilyball Jul 3, 2025
67da488
nixos/libvirtd-autosnapshot: fix option documentation
lilyball Nov 21, 2025
b18d7a3
nixos/tor: add missing option documentation
lilyball Jul 3, 2025
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
29 changes: 29 additions & 0 deletions lib/types.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1439,6 +1439,9 @@ let
# Either value of type `t1` or `t2`.
either =
t1: t2:
let
isEitherOrSubmodule = t: t.name == "either" || t.name == "submodule";
Copy link
Contributor

@MattSturgeon MattSturgeon Oct 24, 2025

Choose a reason for hiding this comment

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

A cleaner way to write this would be:

Suggested change
isEitherOrSubmodule = t: t.name == "either" || t.name == "submodule";
isEitherOrSubmodule = t: elem t.name [ "either" "submodule" ];

However, as per my comment on the comment below, I don't think these special cases should exist at all.

in
mkOptionType rec {
name = "either";
description =
Expand Down Expand Up @@ -1520,6 +1523,32 @@ let
t2
];
};
# We have to be really careful of recursively-defined types here. either is the type used
# to allow finitely-sized values for recursively-defined types, so we have to put limits
# on how we recurse when looking for submodules. To that end, we'll look for submodule
# children, and only recurse into either itself. This means `either str submodule` will
# work, and `either str (attrsOf submodule)` won't, but that's better than nothing.
Comment on lines +1526 to +1530
Copy link
Contributor

@MattSturgeon MattSturgeon Oct 24, 2025

Choose a reason for hiding this comment

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

Personally I disagree with this comment.

I do think either should try to detect new cases of unhandled recursion, so that it can warn/throw a helpful error message.

But I don't think there should be fundamental limits put in place for which sub types should be recursed into. And I don't think either should be considered special when it comes to recursion.

Copy link
Member Author

@lilyball lilyball Oct 26, 2025

Choose a reason for hiding this comment

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

If we don't treat either specially and put limits on how it can recurse then we break the moment we hit a recursive type, and that's a pretty significant breaking change.

Copy link
Contributor

@MattSturgeon MattSturgeon Oct 26, 2025

Choose a reason for hiding this comment

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

I think the logical difference I was missing between current recursive structures and a recursive either structure, is that there's often an option boundary where we can check (isBool opt.visible && opt.visible) || opt.visible != "shallow" before recursing into its sub-options. Whereas with plain types, that usually isn't possible.

However I still feel like having arbitrary restrictions on which types we support recursing into is a flawed approach. I much prefer @hsjobeki and @infinisil 's suggestions of counting the recursion depth and/or checking for repeated/duplicate nodes, as that feels like a more general solution. Or perhaps there's another approach?

Copy link
Member Author

@lilyball lilyball Oct 27, 2025

Choose a reason for hiding this comment

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

How do you propose to count recursion depth, without completely replacing getSubOptions with something that externally walks the type tree? As for checking for repeated/duplicate nodes, how are you doing that? And how are you going to defend against an infinite type that calls a function at each level of recursion in order to generate a new instance of the type?

I do agree that having restrictions on what types we support isn't great. But it's not arbitrary, it's better than what we have today, and I don't actually see any practical alternative right now.

Copy link
Contributor

@hsjobeki hsjobeki Oct 27, 2025

Choose a reason for hiding this comment

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

As i have written down in my other comment

A possible solution: deprecate/change getSubOptions or maybe introduce a more powerful alternative to it and build that with recursive types and extensibility in mind.

Giving types more control over docs generation is probably generally a good idea. And can be done in a non-breaking way.

For example we could introduce a new getDocOptions that takes more arguments than getSubOptions given it exists it is called for docs generation instead.

getSubOptions =
prefix:
lib.recursiveUpdateUntil
Copy link
Contributor

@MattSturgeon MattSturgeon Oct 24, 2025

Choose a reason for hiding this comment

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

getSubOptions shouldn't need to do a recursive update.

The returned subopts attrset doesn't have to match the "real" attrnames, because attrnames are discarded when collecting options for docs.

Elsewhere, we avoid name conflicts and the need to do updates by doing things like:

getSubOptions = loc: {
  left = t1.getSubOptions loc;
  right = t2.getSubOptions loc;
}

Copy link
Member Author

@lilyball lilyball Oct 26, 2025

Choose a reason for hiding this comment

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

Where do we do this? I would expect that writing code like that would trip up nixos-option's ability to dig into the nested options.

Copy link
Contributor

@MattSturgeon MattSturgeon Oct 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

E.g. for freeformType's sub-options, we nest them in _freeformOptions:

nixpkgs/lib/types.nix

Lines 1276 to 1282 in 35e52de

docsEval.options
// optionalAttrs (freeformType != null) {
# Expose the sub options of the freeform type. Note that the option
# discovery doesn't care about the attribute name used here, so this
# is just to avoid conflicts with potential options from the submodule
_freeformOptions = freeformType.getSubOptions prefix;
};

The relevant docs code in lib.options.optionAttrSetToDocList uses lib.collect lib.isOption to collect all options, then it does this recursively on all options' type.getSubOptions unless the option is marked visible = false or visible = "shallow".

nixpkgs/lib/options.nix

Line 607 in a033f62

) (collect isOption options);

Copy link
Member Author

@lilyball lilyball Oct 27, 2025

Choose a reason for hiding this comment

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

That code isn't what you described before. What you described before was a type that inserted keys for its nested types. We can't do that because it breaks nixos-option's ability to recurse into options, since it promotes types into path elements. What you quoted about freeformType, the _freeformOptions is just effectively an option name. And I'd expect that for nixos-option that just makes it appear as yet another sub-option.

Copy link
Contributor

@MattSturgeon MattSturgeon Oct 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The point I'm trying to make is that the option's loc is the canonical place where the option's attrpath is defined, not the "actual" attrnames returned by getSubOptions. Therefore it is often better to choose arbitrary attrnames that avoid potential name conflicts, rather than attempting to match the attrnames with the option locs.

The loc is what is used by showOption and/or when you stringify an option with "${opt}" string interpolation. It's also what gets used by the docs in lib.options.optionAttrSetToDocList. In all these cases, the "actual" attrnames are discarded, and are only really useful when inspecting via the repl.

For example, with the freeformType sub-options above, the loc will be something like foo.<name>.bar while the attrpath will be something like _freeformOptions.bar.

Even if we look at a simpler example, like attrsOf (submodule { options.bar = ... }), we'll see a similar inconsistency: loc <name>.bar vs simply bar (without its <name> prefix). Or if we have nested attrsOf, like attrsOf (attrsOf (submodule { options.bar = ... })) then the loc would be <name>.<name>.bar.

One scenario where this may be relevant for an either type, is either (listOf module) (attrsOf module), which will produce both foo.<name>.bar and foo.*.bar; it is important that both show up in the docs. In more complex examples the attrsOf and listOf modules may be different but with some overlap.

(
_: a: b:
lib.isOption a || lib.isOption b
)
(optionalAttrs (isEitherOrSubmodule t2) (t2.getSubOptions prefix))
(optionalAttrs (isEitherOrSubmodule t1) (t1.getSubOptions prefix));
getSubModules =
let
t1sm = if isEitherOrSubmodule t1 then t1.getSubModules else null;
t2sm = if isEitherOrSubmodule t2 then t2.getSubModules else null;
in
if t1sm == null then t2sm else t1sm;
substSubModules =
m:
let
t1sm = if isEitherOrSubmodule t1 then t1.getSubModules else null;
in
if t1sm == null then either t1 (t2.substSubModules m) else either (t1.substSubModules m) t2;
nestedTypes.left = t1;
nestedTypes.right = t2;
};
Expand Down
4 changes: 2 additions & 2 deletions nixos/modules/programs/dconf.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ let
type = attrs;
default = { };
description = "An attrset used to generate dconf keyfile.";
example = literalExpression ''
example = lib.literalExpression ''
with lib.gvariant;
{
"com/raggesilver/BlackBox" = {
Expand All @@ -139,7 +139,7 @@ let
A list of dconf keys to be lockdown. This doesn't take effect if `lockAll`
is set.
'';
example = literalExpression ''
example = lib.literalExpression ''
[ "/org/gnome/desktop/background/picture-uri" ]
'';
};
Expand Down
1 change: 1 addition & 0 deletions nixos/modules/programs/pay-respects.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ in
};
locale = mkOption {
default = toLower (replaceStrings [ "_" ] [ "-" ] (substring 0 5 config.i18n.defaultLocale));
defaultText = literalExpression ''toLower (replaceStrings [ "_" ] [ "-" ] (substring 0 5 config.i18n.defaultLocale))'';
example = "nl-be";
type = str;
description = ''
Expand Down
4 changes: 2 additions & 2 deletions nixos/modules/services/backup/libvirtd-autosnapshot.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,15 +185,15 @@ in
default = null;
description = ''
Type of snapshot to create (internal or external).
If not specified, uses global snapshotType (${toString cfg.snapshotType}).
If not specified, uses global snapshotType.
'';
};
keep = lib.mkOption {
type = lib.types.nullOr lib.types.int;
default = null;
description = ''
Number of snapshots to keep for this VM.
If not specified, uses global keep (${toString cfg.keep}).
If not specified, uses global keep.
'';
};
};
Expand Down
2 changes: 2 additions & 0 deletions nixos/modules/services/mail/rspamd.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,13 @@ let
owner = mkOption {
type = types.str;
default = "${cfg.user}";
defaultText = literalExpression "config.services.rspamd.user";
description = "Owner to set on unix socket";
};
group = mkOption {
type = types.str;
default = "${cfg.group}";
defaultText = literalExpression "config.services.rspamd.group";
description = "Group to set on unix socket";
};
rawEntry = mkOption {
Expand Down
19 changes: 17 additions & 2 deletions nixos/modules/services/security/tor.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,9 @@ let
(enum [ "auto" ])
]);
default = null;
description = ''
Port number or "auto".
'';
};
optionPorts =
optionName:
Expand All @@ -114,13 +117,15 @@ let
SessionGroup = lib.mkOption {
type = nullOr int;
default = null;
description = descriptionGeneric "SocksPort";
};
}
// lib.genAttrs isolateFlags (
name:
lib.mkOption {
type = types.bool;
default = false;
description = descriptionGeneric "SocksPort";
}
);
config = {
Expand Down Expand Up @@ -183,13 +188,15 @@ let
SessionGroup = lib.mkOption {
type = nullOr int;
default = null;
description = descriptionGeneric "SocksPort";
};
}
// lib.genAttrs flags (
name:
lib.mkOption {
type = types.bool;
default = false;
description = descriptionGeneric "SocksPort";
}
);
config = lib.mkIf doConfig {
Expand All @@ -204,6 +211,7 @@ let
optionFlags = lib.mkOption {
type = with lib.types; listOf str;
default = [ ];
internal = true;
};
optionORPort =
optionName:
Expand Down Expand Up @@ -239,6 +247,7 @@ let
lib.mkOption {
type = types.bool;
default = false;
description = descriptionGeneric "ORPort";
}
);
config = {
Expand Down Expand Up @@ -727,7 +736,9 @@ in
{ ... }:
{
options = {
port = optionPort;
port = optionPort // {
description = "Virtual port number.";
};
target = lib.mkOption {
default = null;
type = nullOr (
Expand All @@ -737,11 +748,14 @@ in
options = {
unix = optionUnix;
addr = optionAddress;
port = optionPort;
port = optionPort // {
description = "Port number."; # we shouldn't accept auto here
};
};
}
)
);
description = descriptionGeneric "HiddenServicePort";
};
};
}
Expand Down Expand Up @@ -924,6 +938,7 @@ in
lib.mkOption {
type = types.bool;
default = false;
description = descriptionGeneric "ControlPort";
}
);
config = {
Expand Down
5 changes: 4 additions & 1 deletion nixos/modules/services/web-apps/movim.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,11 +295,12 @@ in
};
};
};
description = "Script minification options";
};
style = mkOption {
type = types.submodule {
options = {
enable = mkEnableOption "Script minification via Lightning CSS";
enable = mkEnableOption "Style minification via Lightning CSS";
target = mkOption {
type = types.nullOr types.nonEmptyStr;
default = null;
Expand All @@ -311,13 +312,15 @@ in
};
};
};
description = "Style minification options";
};
svg = mkOption {
type = types.submodule {
options = {
enable = mkEnableOption "SVG minification via Scour";
};
};
description = "SVG minification options";
};
};
}
Expand Down
Loading