lib/types.nix: implement getSubOptions for either#422272
lib/types.nix: implement getSubOptions for either#422272lilyball wants to merge 7 commits intoNixOS:masterfrom
Conversation
There was a problem hiding this comment.
I'm not entirely sure about this one, from my reading of the module it looks like flags isn't supposed to be set by users directly, it's calculated from the per-flag attributes.
nixpkgs-ci
bot
added
6.topic: nixos
|
A few thoughts on this PR:
|
|
I tested out the "make Alternatively this also suggests we could just update the 2 affected types in Edit: actually, the fully recursive change hurts some of the Also we can't just trivially update those two types in |
|
Another thing to consider here is With that context, we could consider changing |
lilyball
force-pushed
the
push-sxtxlvwouuno
branch
from
6fe4329 to
21241df
Compare
|
The changes to
|
nixpkgs-ci
bot
added
the
2.status: merge conflict
lilyball
force-pushed
the
push-sxtxlvwouuno
branch
from
21241df to
8726a0e
Compare
|
Rebased to fix merge conflict (conflict was due to the tor module being formatted) |
nixpkgs-ci
bot
removed
the
2.status: merge conflict
There was a problem hiding this comment.
Could you split the changes in types.nix and non-lib related things into a speerate PR? I.e. the fixes on some modules should be seperated. This is a common practise for us to avoid mixing lib changes with nixos fixes. I'll spend a bit more time these days looking at this. I fear that the changes in nixos might be due to a breaking change in the either type. Which me must preserve for downstream users. Moving the lib change out of this PR would make it more clear to us.
|
Mixing isn't so bad by itself. |
|
I certainly can split this up, though the non- If I split this up, would it be better to do one PR for all the non- |
To clarify on this point, the changes in nixos are to fix code that is currently broken but that brokenness wasn't noticed because the options aren't rendered into documentation until the All of this stuff could be detected by using the repl to poke around at the module structure, but it went unnoticed because none of these options ended up in the documentation manual before. So technically this |
hsjobeki
left a comment
hsjobeki
left a comment
There was a problem hiding this comment.
After looking into this for a short bit i am not sure if implementing getSubOptions for either is the right approach to solve the documentation problem. getSubOptions is only meant to documentation to automatically traverse options and types via `optionAttrSetToDocList´ with either (and derived types such as oneOf) it is possible to create recursive types.
In case of those reursive types there is no formally correct way to stop / break on a certain point. Thats why we didn't implement getSubOptions historically i think.
Even before this PR either was already broken on recursive types - take this simple recursive type for example:
simpleJson = types.oneOf [ # renders into a tree of either
types.str
(types.attrsOf simpleJson)
(types.listOf simpleJson)
];
#... a module
{
options.foo = mkOption {
type = simpleJson;
};
}This would fail to produce docs, because either has a recursive description, which would yield an infinitely long string in this case.
The actual JSON type solves this by overriding the description (this is also not ideal):
valueType =
nullOr (oneOf [
#...
])
// {
description = "JSON value";
};We could maybe think about other solutions such as providing documentation hints, instead of trying to smartly recurse an infinite structure.
Some ideas how to solve this problem: Add something like a docsModel / docsType which requires to be finite and is provided through the interface while creating the type?
For the description, yeah, which is why any recursive types have to override that. nixpkgs already has a bunch of definitions of recursive types and they all override the description. I don't really see any alternative to this for recursive types, you must provide a description no matter how you construct it because any automatically-constructed description will be infinite. And I'm not sure why you said "Even before this PR", as this PR doesn't introduce any new brokenness. It restricts the
Can you please explain what you mean by this? Please bear in mind the actual thing this PR aims to solve is submodules that are currently being omitted from documentation because they're part of finite types that use type =
let
valueType =
nullOr (oneOf [
bool
int
float
str
path
(attrsOf valueType)
(listOf valueType)
])
// {
description = "JSON value";
+ getSubOptions = prefix: {};
+ getSubModules = null;
+ substSubModules = m: null;
};
in
valueType;That's a lot of recursive types to update just to make The other option here is to say that any type that uses I have considered introducing an unconditionally-recursive |
Hm i think i phrased this wrong. Given that For documentation generation in nixpkgs I'll try to start with some reasoning on Given the traversal function: getSubOptions =
prefix:
lib.recursiveUpdateUntil
(
_: a: b:
lib.isOption a || lib.isOption b
)
(optionalAttrs (isEitherOrSubmodule t2) (t2.getSubOptions prefix))
(optionalAttrs (isEitherOrSubmodule t1) (t1.getSubOptions prefix));I have some questions:
For example This would not fail to generate docs prior to this change.
For example types.either (submodule {
options.bar = mkOption {
type = types.str;
description = "A string option inside the submodule.";
};
options.conflict = mkOption {
type = types.str;
description = "A string option inside the submodule.";
};
}) (submodule {
options.foo = mkOption {
type = types.str;
description = "A string option inside the submodule.";
};
options.conflict = mkOption {
type = types.int;
description = "An integer option inside the submodule.";
};
});It would probably show slightly wrong docs (as figured from testing), which is maybe better than currently - no docs. On the counterside merging options using |
|
What i meant with Something along these lines: # New type, or other solution that allows for user defined and formally correct abort criteria
recursiveJson = types.namedUnion "json" [
types.str
types.number
(types.attrsOf (types.typePlug recursiveJson)) # Explicit recursion marker; docs generation returns the name, instead of the type
(types.listOf (types.typePlug recursiveJson)) # Explicit recursion marker
];This is just an idea; and is probably the other extreme, forcing Users to control their own recursive types, because the cannot safely be traversed. For this PR, i would probably expect to add some tests, to ensure we don't cause downstream breakages. |
The logic here only controls one step of the traversal. So the answer is that it will stop traversing as soon as it hits a child of In your example {
options.someOpt = mkOption {
type = moduleOrSelf;
};
config.someOpt = [ "foo" ];
}This will end up recursing infinitely as the
This argument here is precisely why I went with the limited approach of only recursing when it's provably safe to do so, instead of unconditionally recursing and simply fixing all 26 infinite types in nixpkgs. Limiting the recursion to nested
I believe the answer is that the option definition from
What sort of tests are you envisioning? Something I did talk about in an earlier comment is that if you have a type that looks like types.either (submodule {
options.bar = mkOption {
type = types.str;
description = "A string option in the submodule.";
};
}) (listOf (submodule {
options.bar = mkOption {
type = types.str;
description = "A string option in the listOf submodule.";
};
}))then we should be able to generate docs for both of these, but we'll actually only generate them for one of them, and that's because the getSubOptions = prefix: elemType.getSubOptions (prefix ++ [ "*" ]);If instead we do listOf = {
…
getSubOptions = prefix: { "*" = elemType.getSubOptions (prefix ++ [ "*" ]); };
};
attrsWith = {
…
getSubOptions = prefix: { "<${placeholder}>" = elemType.getSubOptions (prefix ++ [ "<${placeholder}>" ]); };
};Then doing |
What would you actually expect for generated documentation for a type like this? Because it looks to me like generating documentation for this type should produce an infinite amount of docs, as it needs to document not just This type is not the sort of infinite type we've been talking about so far. This type is not going to hit a recursion loop when evaluating recursivePayload =
types.submodule {
options.child = mkOption {
type = recursivePayload;
description = "An option inside the submodule.";
};
};This type here is like the previous one in that The closest thing I've seen to this in practice instead looks like options.someOpt = mkOption {
type = let
submoduleType = submodule {
options.child = mkOption {
type = types.str;
description = "A string option inside the submodule.";
};
};
in types.either submoduleType (attrsOf submoduleType);
description = "Option that's a submodule or attrsOf that submodule.";
};And this type isn't recursie. Also today this type won't generate any docs for |
I can see how you'd think this when dealing with fairly primitive types. With submodules you can have fairly useful recursive types, where a sub-option or freeformType re-uses an outer type recursively. E.g.: https://github.com/nix-community/nixvim/blob/ecb75f49d10fe2823b0822e4e95e53f80e426742/docs/modules/page.nix#L12-L23 I imagine
Correct. That example would need the sub-options to be declared with
Given
Maybe we do need a new type to have general recursion support. Incidentally, nixvim has been using an overlay that replaces I don't think Footnotes
|
This is an interesting type. But it's not the type of recursive type I was talking about because it doesn't infinitely recurse in
If you write something that produces an infinite tree of options, then it's not unreasonable to expect you to break that infinite series of options with something like It's certainly possible that someone has, out of tree, written something that would produce an infinite sea of options except it uses
What exactly do you think the purpose here is? Because the options that aren't being documented today, that this PR aims to produce documentation for, they aren't recursive. That's the thing, For the options that this PR has to fix because their documentation doesn't evaluate correctly, I'd wager the module authors didn't even realize those options weren't being documented. If we move ahead with a plan that requires users to switch to some alternative |
|
After thinking about it a bit more, I've realized that a recursive submodule that doesn't set Which is to say, this type childModule = types.submodule {
options.foo = mkOption {
type = types.str;
description = "A string option";
};
options.child = mkOption {
type = types.either types.str childModule;
description = "A string or child submodule";
};
};is just as broken as this type: someOption = mkOption {
type = types.either types.str (types.submodule {
options.foo = mkOption {
type = types.str;
description = functionThatDoesntExist "A string option";
};
};
description = "A string or submodule";
};Both of them get away with it today, both of them will have an issue with this PR, both of them are buggy and should be fixed regardless of this PR. |
|
I don't think these two cases are equivalent:
description = functionThatDoesntExist "...";This is objectively broken code - a bug in users code.
childModule = types.submodule {
options.child = mkOption {
type = types.either types.str childModule;
};
};valid, correct code
The only "problem" is that Proposed solutions: For case 2, we have several options as we pointed out earlier:
We shouldn't call correct types "broken" just because the documentation system doesn't handle them yet. If we merge this PR right now it is a breaking change, which was promised to be avoided if possible.
EDIT: I think this is probably a bad idea. It should be explored how a non-breaking way is feasible |
|
Yeah unless we have good reason to believe nobody actually uses such recursive types that would be broken, we really should avoid such potentially breaking changes. It's really discouraging to be met with random infinite recursions for a NixOS update. Here's a quick draft of a recursion-limit based, backwards/forwards-compatible diff --git a/lib/types.nix b/lib/types.nix
index 4e20492909c7..f6c3cd11c799 100644
--- a/lib/types.nix
+++ b/lib/types.nix
@@ -99,6 +99,12 @@ let
}is accessed, use `${lib.optionalString (loc != null) "type."}nestedTypes.elemType` instead.
'' payload.elemType;
+ # Before getSubOptions.v2, no recursion limit could be specified, so we need to assume one
+ subOptionsv1Limit = 5;
+ subOptions =
+ type: args:
+ if type.getSubOptions ? v2 then type.getSubOptions.v2 args else type.getSubOptions args.prefix;
+
checkDefsForError =
check: loc: defs:
let
@@ -728,7 +734,20 @@ let
emptyValue = {
value = [ ];
};
- getSubOptions = prefix: elemType.getSubOptions (prefix ++ [ "*" ]);
+ getSubOptions = {
+ __functor =
+ self: prefix:
+ self.v2 {
+ inherit prefix;
+ limit = subOptionsv1Limit;
+ };
+ v2 =
+ { prefix, limit }:
+ subOptions elemType {
+ prefix = prefix ++ [ "*" ];
+ limit = limit - 1;
+ };
+ };
getSubModules = elemType.getSubModules;
substSubModules = m: listOf (elemType.substSubModules m);
functor = (elemTypeFunctor name { inherit elemType; }) // {Note that But yeah some explicit recursion type would probably be even better, though harder to get right. |
This is unfortunately not a useful type. tl;dr it always picks |
|
I like @infinisil s approach, had something like this in mind This goes in hand with changing It should allow types to return named That would solve type recursion problems in general, |
But it can traverse it. If you don't try to build documentation, then this type will work perfectly fine as well. There's no infinite recursion when poking at the type. It's only infinite recursion when trying to build documentation, as it gathers the options and finds an infinite amount of them.
This doesn't fix anything, because The infinite recursion problem with If your worry is an infinitely recursive submodule that generates an infinite amount of options without using
A recursive submodule that doesn't set I think I have a better example for the equivalence in broken code here. A recursive submodule without options.someOpt = mkOption {
type = types.either types.str (types.submodule {
options.child = mkOption {
type = types.str;
};
});
}This type is missing the If we say "we cannot risk breaking any users" then that means we simply cannot fix |
|
I think to move forward with this PR we need to acknowledge the stability guarantees of Before:
After
To respect
Or/and:
You wrote:
But they will be surprised:
As said: at minimum, this PR should include:
Without these safeguards, this PR violates This is a blocker for merging Can you add a recursion depth check to |
|
I'm just returning to this now. @infinisil What infinite recursion are you trying to solve here? This PR doesn't introduce infinite recursion in This PR does have the potential issue of a recursive submodule that doesn't declare What's more, recursive submodules are pretty rare (or at least, recursive submodules that use Footnotes
|
Also implement getSubModules and substSubModules. Because either is key
to any recursively-defined type (such as `(pkgs.formats.json {}).type`),
we can't just blindly recurse into both sides or we'll hit infinite
recursion on a recursive type. To that end, this implementation only
recurses into a child type if that type is submodule or another either.
This means we'll generate documentation for `either str submodule` but
not for `either str (attrsOf submodule)`.
This change allows us to generate documentation for a bunch of options
that are currently being missed.
The `example` key of a couple of options didn't evaluate, and this went unnoticed as this option wasn't part of generated documentation before.
The `default` references a config value from another module, so it needs `defaultText` as well.
Option descriptions for a submodule were referencing config values from the parent module. This isn't allowed when building the manual, so just delete those references.
lilyball
force-pushed
the
push-sxtxlvwouuno
branch
from
8726a0e to
b18d7a3
Compare
|
Rebased, and added a new commit to account for a new option that broke the manual rules (referencing a config value from outside the module). |
nixpkgs-ci
bot
removed
the
2.status: merge conflict
That was not my original idea. I wanted to abort with a clear error message, that explains how to fix the problem. Rather than having the native "infinite recursion" error. Thats the least we should do. Because docs will break downstream. Not sure if should do more, as you pointed out it might be "rare" breakages. |
|
I do have a local change I used while diagnosing the breakages from an experiment with doing unrestricted recursion that looks like: Commit ID: cc6dd6b7351e15a8f5632a8b0e866442a5e6bcf9
Change ID: lvoxlxrlvmlutzrrmwznsooxxlnnsmpy
Author : Lily Ballard <lily@ballards.net> (2025-07-07 00:57:49)
Committer: Lily Ballard <lily@ballards.net> (2025-11-20 20:22:51)
WIP: add error contexts
diff --git a/lib/modules.nix b/lib/modules.nix
index e545f9c970..72637d45ec 100644
--- a/lib/modules.nix
+++ b/lib/modules.nix
@@ -1395,7 +1395,7 @@
# TODO: Merge this into mergeOptionDecls
fixupOptionType =
loc: opt:
- if opt.type.getSubModules or null == null then
+ if builtins.addErrorContext "while evaluating the option at ${showOption loc}" (opt.type.getSubModules or null) == null then
opt // { type = opt.type or types.unspecified; }
else
opt
diff --git a/lib/options.nix b/lib/options.nix
index b1db6ff76e..89a8643a45 100644
--- a/lib/options.nix
+++ b/lib/options.nix
@@ -598,7 +598,7 @@
let
ss = opt.type.getSubOptions opt.loc;
in
- if ss != { } then optionAttrSetToDocList' opt.loc ss else [ ];
+ if ss != { } then builtins.addErrorContext "while evaluating the option at ${showOption opt.loc}" (optionAttrSetToDocList' opt.loc ss) else [ ];
subOptionsVisible = if isBool visible then visible else visible == "transparent";
in
# To find infinite recursion in NixOS option docs:I could just pull that into this PR. I don't know how cheap |
|
If you could make it an explicit throw, you could catch that with tryEval. And then it could be made non-breaking. So i think you should change 'optionAttrsetToDocList' as well to make it non-breaking. It could print a warning about omited options from docs, rather than failing completely. |
nixpkgs-ci
bot
added
the
2.status: merge conflict
5 participants
This implements
getSubOptions,getSubModules, andsubstSubModulesfor theeithertype. This allows us to generate documentation for a bunch of options that were getting missed before.Because
eitheris the key to making recursive types, I had to be careful about the implementation here to avoid breaking on a type like(pkgs.formats.json {}).type. To that end, this implementation only looks at child types if they'resubmoduleor anothereither. This does mean types likeeither str (listOf submodule)will still be missed, but I'm not sure how else to handle this besides recursively searching all nested types to see if any are equal (==) to the current type, and I don't like that solution. I also considered simply stopping onattrsOfandlistOfand allowing other children (this would allow e.g.either str (nullOr submodule)) but I thought that was sufficiently fragile. And I considered looking for any recursively-defined type in nixpkgs and overridinggetSubOptions/getSubModulesexplicitly, but that would mean types defined outside of nixpkgs could still be broken by this.For the case of
either submodule submoduleI decided thatgetSubOptionsshould merge all returned options, butgetSubModulesandsubstSubModulesshould ensure only one child actually has submodules. I don't expect this to matter because a type like that isn't really going to work, this logic makes more sense for something likeeither submodule (attrsOf submodule)except for the fact that we won't recurse into theattrsOf(to avoid problems on recursive types).With this change, I found a number of options whose documentation were missing or broken. Fixes for each of those modules are included as separate commits. Most of them were pretty straightforward, but the
tormodule had a lot of generated options and it wasn't always clear what the best way to handle it was.I tested this with
nix-build nixos/release.nix -A manual.aarch64-linux, I'm not sure if there's any other documentation set that I should have tested too. I also didn't touch the lib tests, I'm not sure if this change should have an explicit test written for it, but I did make surelib/tests/modules.shpasses. I'm also not sure if this change warrants a release note.Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.