| 29 May 2026 |
Matt Sturgeon | Are you happy for me to squash your changes into a "co-authored-by" commit, or would you prefer I merge your actual commits as-is? Typically we avoid having commits in a PR that fix earlier commits in the same PR, unless there's a specific story we want to tell in the git history. | 19:21:47 |
Matt Sturgeon | - throwIf (headError.causedByMixedNulls or false) headError.message value;
+ if (headError.causedByMixedNulls or false) then throw headError.message else value;
Does this actually have a significant performance impact? I strongly prefer idiomatic usage of helpers like throwIf to keep the code more readable. Seems like we'd be saving at most O(n) function applications? This codepath is also only evaluated when nullOr is wrapped by a non-v2-aware wrapper type.
| 19:29:14 |
llakala | co-authored is fine | 19:45:08 |
llakala | yeah sure | 19:45:11 |
llakala | i have an issue on helpers like throwIf and assertMsg - they're actually very evil for hot code. see https://github.com/NixOS/nixpkgs/issues/523851. this one will likely make a very small difference, so you can keep it if you want - but they are something to watch out for | 19:46:45 |
llakala | also note that PR is giving me an error for evaluating my personal config, because of this bit of code in display-managers/default.nix:
defaultSession = lib.mkOption {
type = lib.types.nullOr lib.types.str // {
description = "session name";
check =
d:
lib.assertMsg (d != null -> (lib.types.str.check d && lib.elem d cfg.sessionData.sessionNames)) ''
Default graphical session, '${d}', not found.
Valid names for 'services.displayManager.defaultSession' are:
${lib.concatStringsSep "\n " cfg.sessionData.sessionNames}
'';
};
default = null;
example = "gnome";
description = ''
Graphical session to pre-select in the session chooser (only effective for GDM, LightDM and SDDM).
On GDM, LightDM and SDDM, it will also be used as a session for auto-login.
Set this option to empty string to get an error with a list of currently available sessions.
'';
};
so we may want to fix instances of nullOr being merged before merging this.
| 19:52:13 |
llakala | * also was just about to do a real-time test on my own config, but found that this PR is giving me an error, because of this bit of code in display-managers/default.nix:
defaultSession = lib.mkOption {
type = lib.types.nullOr lib.types.str // {
description = "session name";
check =
d:
lib.assertMsg (d != null -> (lib.types.str.check d && lib.elem d cfg.sessionData.sessionNames)) ''
Default graphical session, '${d}', not found.
Valid names for 'services.displayManager.defaultSession' are:
${lib.concatStringsSep "\n " cfg.sessionData.sessionNames}
'';
};
default = null;
example = "gnome";
description = ''
Graphical session to pre-select in the session chooser (only effective for GDM, LightDM and SDDM).
On GDM, LightDM and SDDM, it will also be used as a session for auto-login.
Set this option to empty string to get an error with a list of currently available sessions.
'';
};
so we may want to fix instances of nullOr being merged before merging this.
| 19:52:46 |
Matt Sturgeon |
so we may want to fix instances of nullOr being merged before merging this.
Interesting. Is there a test I can run that covers that?
By "merged" do you mean the // { description, check } update? Feels like that's related to the wider "addCheck is broken" issues exposed by v2 check+merge. IIRC there is a check somewhere that tries to warn when check is shadowed on a type that supports v2 check+merge, though. Did the warning not trigger on your example? (Report error for unsupported t // { check = ...; })
| 20:19:25 |
Matt Sturgeon | *
so we may want to fix instances of nullOr being merged before merging this.
Interesting. Is there a test I can run that covers that? By "merged" do you mean the // { description, check } update?
Feels like that's related to the wider "addCheck is broken" issues exposed by v2 check+merge. IIRC there is a check somewhere that tries to warn when check is shadowed on a type that supports v2 check+merge, though. Did the warning not trigger on your example? (Report error for unsupported t // { check = ...; })
| 20:19:40 |
Matt Sturgeon | *
so we may want to fix instances of nullOr being merged before merging this.
Interesting. Is there a test I can run that covers that? By "merged" do you mean the // { description, check } update?
Feels like that's related to the wider "addCheck is broken" issues exposed by v2 check+merge. There is (Report error for unsupported t // { check = ...; }) that tries to warn when check is shadowed on a type that supports v2 check+merge, though. Did the warning not trigger on your example?
| 20:20:11 |
Matt Sturgeon | *
so we may want to fix instances of nullOr being merged before merging this.
Interesting. Is there a test I can run that covers that? By "merged" do you mean the // { description, check } update?
Feels like that's related to the wider "addCheck is broken" issues exposed by v2 check+merge. There is report error for unsupported t // { check = ...; } that tries to warn when check is shadowed on a type that supports v2 check+merge, though. Did the warning not trigger on your example?
| 20:20:22 |
Matt Sturgeon | Yeah, I agree on hot-code. But on "normal" code, I tend to favor legibility over mirco-optimisation.
Based on roberth's comment on your linked issue I'll keep your version, though:
I support removing these calls from lib and other hot paths.
| 20:24:11 |
llakala | oh, i should've sent the error - yeah that's the error that triggered
error: The option `services.displayManager.defaultSession' has a type `session name' that uses
an ad-hoc `type // { check = ...; }' override, which is incompatible with
the v2 merge mechanism.
Please use `lib.types.addCheck` instead of `type // { check }' to add
custom validation. For example:
lib.types.addCheck baseType (value: /* your check */)
instead of:
baseType // { check = value: /* your check */; }
Alternatively, this message may also occur as false positive when mixing Nixpkgs
versions, if one Nixpkgs is between 83fed2e6..58696117 (Aug 28 - Oct 28 2025)
| 20:25:10 |
| 30 May 2026 |
| aktaboot joined the room. | 09:08:13 |
| Alexandru Caraus joined the room. | 14:42:56 |
| 31 May 2026 |
| @613fd0ba9f744876:matrix.org joined the room. | 15:42:01 |
| @613fd0ba9f744876:matrix.org left the room. | 15:45:02 |
| vandycarlos changed their display name from vandycarlos to Vandy arlos. | 22:00:31 |
| vandycarlos changed their display name from Vandy arlos to Vandy Carlos. | 22:00:39 |
| 5 Jun 2026 |
| zoë (@blokyk) joined the room. | 14:41:30 |
zoë (@blokyk) | is there any reason why this check would pass?
nix-repl> (lib.types.listOf (lib.types.enum [])).check [ "hello" ]
true
(this is a minimal example but the original type is also just an enum, and the merging doesn't throw any errors at all)
i found this issue but i don't think that's related since it seems to be submodule- and oneOf-specific?
| 14:46:14 |
zoë (@blokyk) | * is there any reason why this check would pass?
nix-repl> (lib.types.listOf (lib.types.enum [])).check [ "hello" ]
true
(this is a minimal example but the original type is also just an enum, and the merging doesn't throw any errors at all)
(also sorry if this isn't the place to ask noobish question like this, it seems like mostly technical discussion ;-;) i found this issue but i don't think that's related since it seems to be submodule- and oneOf-specific?
| 14:53:12 |
zoë (@blokyk) | * is there any reason why this check would pass?
nix-repl> (lib.types.listOf (lib.types.enum [])).check [ "hello" ]
true
(this is a minimal example but the original type is also just an enum, and the merging doesn't throw any errors at all)
i found this issue but i don't think that's related since it seems to be submodule- and oneOf-specific?
(also sorry if this isn't the place to ask noobish question like this, it seems like mostly technical discussion ;-;)
| 14:54:10 |
Matt Sturgeon | types.enum's check is literally x: elem x values, so (lib.types.enum []).check "hello" should fail.
types.listOf's check just uses isList, without also checking all elemType.check. This laziness was introduced in https://github.com/NixOS/nixpkgs/pull/6794
| 14:57:57 |
Matt Sturgeon | * types.enum's check is literally x: elem x values, so (lib.types.enum []).check "hello" should fail.
types.listOf's check just uses isList, without also checking all elemType.check. This laziness was introduced in https://github.com/NixOS/nixpkgs/pull/6794 (specifically https://github.com/NixOS/nixpkgs/commit/e4bc2592f3c5fa2f05484e7258f99ebb0507d304)
| 14:58:49 |
Matt Sturgeon | * types.enum's check is literally x: elem x values, so (lib.types.enum []).check "hello" should fail.
types.listOf's check just uses isList, without also checking all elemType.check. This laziness was introduced in https://github.com/NixOS/nixpkgs/pull/6794 (specifically https://github.com/NixOS/nixpkgs/commit/e4bc2592f3c5fa2f05484e7258f99ebb0507d304)
| 14:59:07 |
zoë (@blokyk) | is it also supposed to do that during merging? this really feels like a bug, or at least a gigantic footgun if listOf throws away any further typecheck ^^; | 14:59:41 |
toonn | It delays it until use, rather than throw it away, no? | 15:00:07 |
zoë (@blokyk) | personally in my case it just ignores it if i'm not mistaken. (i'll go check with my non-reduced example but i'd be surprised if it's different) | 15:00:49 |
Matt Sturgeon | Hard to tell from skim-reading the merge function, but it looks like it is delegating checking & merging to the elemType and should surface type errors. It's possible that there's a v1/v2 checkAndMerge interface bug though, e.g. if the outer or inner type is using the v1 interface and the other part is using v2? | 15:02:11 |