Skip to content

lib.modules: init types checkAndMerge to allow adding 'valueMeta' #391544

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

Merged
roberth merged 14 commits into from
Aug 28, 2025
Merged

lib.modules: init types checkAndMerge to allow adding 'valueMeta' #391544

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
644527d
lib.modules: init types checkAndMerge to allow adding 'valueMeta' att…
roberth Mar 18, 2025
8fa3300
lib.modules: add tests for option valueMeta
hsjobeki Mar 20, 2025
648dbed
lib/types: add 'checkDefsForError' utility for checking defs with a g…
hsjobeki May 30, 2025
70ab11c
lib/modules: add new merge.v2 for 'types.{either,coercedTo}'
hsjobeki May 30, 2025
9f787b3
lib/modules: fix test by matching error message more generically
hsjobeki May 30, 2025
5d72133
lib/addCheck: add support for new merge
hsjobeki May 30, 2025
ebafc3e
lib/modules: optimize performance by inlining bindings
hsjobeki Jun 6, 2025
1765370
lib/modules: test revert unentional regression in check
hsjobeki Jun 6, 2025
50bef19
lib/modules: add nested 'headError.message'
hsjobeki Jun 6, 2025
cd2e5bd
types/merge: move 'configuration' of submodules into nested attribute…
hsjobeki Jun 20, 2025
45ed757
types/addCheck: add tests for merge v1 and v2
hsjobeki Jun 20, 2025
1fed602
lib/tests: introduce lib cross version checks
hsjobeki Aug 15, 2025
4f802d9
lib/modules: fix typo
hsjobeki Aug 18, 2025
bb0bd3d
lib/modules: add _internal to valueMeta of checkedAndMerged
hsjobeki Aug 20, 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
49 changes: 46 additions & 3 deletions lib/modules.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1121,6 +1121,7 @@ let
files = map (def: def.file) res.defsFinal;
definitionsWithLocations = res.defsFinal;
inherit (res) isDefined;
inherit (res.checkedAndMerged) valueMeta;
# This allows options to be correctly displayed using `${options.path.to.it}`
__toString = _: showOption loc;
};
Expand Down Expand Up @@ -1164,7 +1165,14 @@ let
# Type-check the remaining definitions, and merge them. Or throw if no definitions.
mergedValue =
if isDefined then
if all (def: type.check def.value) defsFinal then
if type.merge ? v2 then
# check and merge share the same closure
# .headError is either not-present, null, or a string describing the error
if checkedAndMerged.headError or null != null then
throw "A definition for option `${showOption loc}' is not of type `${type.description}'. TypeError: ${checkedAndMerged.headError.message}"
else
checkedAndMerged.value
else if all (def: type.check def.value) defsFinal then
type.merge loc defsFinal
else
let
Expand All @@ -1177,6 +1185,43 @@ let
throw
"The option `${showOption loc}' was accessed but has no value defined. Try setting the option.";

checkedAndMerged =
(
# This function (which is immediately applied) checks that type.merge
# returns the proper attrset.
# Once use of the merge.v2 feature has propagated, consider removing this
# for an estimated one thousandth performance improvement (NixOS by nr.thunks).
{
headError,
value,
valueMeta,
}@args:
args
)
(
if type.merge ? v2 then
let
r = type.merge.v2 {
inherit loc;
defs = defsFinal;
};
in
r
// {
valueMeta = r.valueMeta // {
_internal = {
inherit type;
};
};
}
else
{
headError = null;
value = mergedValue;
valueMeta = { };
}
Comment on lines 1216 to 1222
Copy link
Member

@roberth roberth Jun 7, 2025

Choose a reason for hiding this comment

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

Suggested change
}
else
{
headError = null;
value = mergedValue;
valueMeta = { };
}
}
// {
inherit type;
}
else
{
headError = null;
value = mergedValue;
valueMeta = {
inherit type;
};
}

Copy link
Member

@infinisil infinisil Jun 19, 2025

Choose a reason for hiding this comment

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

Motivation unclear, options already has the merged type 🤔

Copy link
Contributor Author

@hsjobeki hsjobeki Jun 19, 2025

Choose a reason for hiding this comment

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

I think we want to avoid double exposing the same data if there is not explicit need for it.
That would only create confusion about the option api. If we want to move type from options into valueMeta that should be a seperate migration, might involve double exposing, but unless there is an explicit benefit, right now i would not do it.

Copy link
Member

@roberth roberth Jun 19, 2025

Choose a reason for hiding this comment

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

It was supposed to make recursion into the combined type + meta structure easier, and it claims the name type, so that it isn't used inconsistently. For instance, if attrsOf were to add a type to its valueMeta, it has a choice whether to put attrsOf t (itself) or just t (its element type)
By forcing it to be attrsOf t, we ensure that this attribute is consistent regardless of what the type does.
In fact, attrsOf t // { description = "ha"; } can't even put its type anywhere, because its functions are unaware of the //.
The type may become particularly relevant if we implement attrsWith { type = name: ...; }, in which case the type may be name-dependent. My suggestion adds valueMeta.attrs.<name>.type (among other .types), so that users who read valueMeta don't have to reconstruct each item's type again.
It's not the only solution to that problem, as valueMeta.attrTypes could be added, but then users need to traverse both attrsets simultaneously, and make the assumption that the attribute name sets are equal.
So in conclusion I believe that by deciding this attribute here, we make valueMeta more consistent and easier to use.

Copy link
Contributor Author

@hsjobeki hsjobeki Aug 13, 2025

Choose a reason for hiding this comment

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

Would you be okay with delaying this into a follow-up PR? I'm not sure if its required right now. It might be okay to make this assumption based on the current types that implement merge.v2

Copy link
Member

@infinisil infinisil Aug 14, 2025

Choose a reason for hiding this comment

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

@roberth I think valueMeta shouldn't be set by the module system internally, but rather be entirely left up to the types to define, with the module system just exposing it.

To expose the effective type of an option, the module system always has the option to set other attributes in the options structure, like options.foo.effectiveType.

(discussed with @hsjobeki in a call as well)

Copy link
Member

@roberth roberth Aug 14, 2025

Choose a reason for hiding this comment

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

Would you be okay with delaying this into a follow-up PR?

I'd prefer not to because soon after the merge, adding type will have to be considered a breaking change.

@infinisil I see the problem now. My suggestion isn't just the effective type of an option, because yes, we have a perfectly fine options for that.
Where it matters is when the types are nested and dependent, for example as in this commit.
In such a case, valueMeta.attrs.<name>.type can only be provided by attrsWith, where it would interfere with the valueMeta.attrs.<name>, which is an open attrset where a user-provided type may exist.
The straightforward solution to this is to change the valueMeta of attrsWith to have
valueMeta.attrs.<name>.{valueMeta, type} instead of valueMeta.attrs.<name> = valueMeta // { type }, or valueMeta.types.<name> in addition to valueMeta.attrMetas.<name>, or something like that.
But is that really nicer than always having type in valueMeta?
User code that traverses more than a single submodule will have to deal in values of { type; value; valueMeta; } instead of { value; valueMeta; }.

I think I've made my case. I'll let you decide and support that.

Copy link
Contributor Author

@hsjobeki hsjobeki Aug 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@roberth @infinisil I tested both approaches now:

  1. Adding type via modules.nix

Requires using another let binding and one update operator in the hot path (merging).
The second one should be lazy i think

        (
          let
            r =
                type.merge.v2 {
                  inherit loc;
                  defs = defsFinal;
                };
          in
          r
          // {
            valueMeta = r.valueMeta // {
              inherit type;
            };
          }
        );

Did nixos eval, 20 runs with hyperfine, 1 warmup

CPU Time increased, but i also got another run, where noise was bigger than the change.

Key Before (μ ± SEM) After (μ ± SEM) Δ Abs Δ %
cpuTime 2.2103133428664434 ± 0.015082956966886411 2.2564680462791804 ± 0.01613245960140519 0.046155 2.09%
nrOpUpdates 306096 ± 0.0 311462 ± 0.0 5366.000000 1.75%

... other stats omited

This means we need to update every checkedAndMerged which holds value which explains the stats.

  1. adding valueMeta.type in types, instead of updating afterwards.
    Most notably (nixos eval)
    0% change, since updating valueMeta is not needed to access the value.
    Obvious drawback. Every type is responsible to properly set it, which may make traversal later on brittle, especially on custom / downstream types.

I am not sure if we should add another update operation into the hot path?
On the other hand if we really want to support attrsWith { getElemType ... } like in the commit you showed i cannot think of a different reasonable way.

We could add checking for valueMeta, so every type that defines valueMeta also needs to set type. This would defer the problem into the time when somebody tries to access malformed valueMeta, which is probably also kind of bad.
We cannot really control peoples custom types because we wanted valueMeta to be freeform. It would be a pitty if custom types siltently don't work with this, because we have to many unbespoke conventions.

For making this tradeof more future extensible i wouldn't add type directly to valueMeta but maybe reserve _internal = { inherit type; } or similar that we can extend in the future without creating collisions in valueMeta

);

isDefined = defsFinal != [ ];

optionalValue = if isDefined then { value = mergedValue; } else { };
Expand Down Expand Up @@ -1586,13 +1631,11 @@ let
New option path as list of strings.
*/
to,

/**
Release number of the first release that contains the rename, ignoring backports.
Set it to the upcoming release, matching the nixpkgs/.version file.
*/
sinceRelease,

}:
doRename {
inherit from to;
Expand Down
Loading
Loading