Skip to content

Commit 93ea59f

Browse files
roberthroberth
committed
lib/modules: Report error for unsupported // { check }
`check` can have a new place since the introduction of merge.v2. This makes the // { check = ... } idiom unreliable. In this PR we add checks to detect and report this. merge.v2 introduced in: NixOS#391544 Real world case: https://hercules-ci.com/github/hercules-ci/hercules-ci-effects/jobs/875
1 parent ddd7747 commit 93ea59f

File tree

4 files changed

+232
-21
lines changed

4 files changed

+232
-21
lines changed

‎lib/modules.nix‎

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1126,6 +1126,29 @@ let
11261126
__toString = _: showOption loc;
11271127
};
11281128

1129+
# Check that a type with v2 merge has a coherent check attribute.
1130+
# Throws an error if the type uses an ad-hoc `type // { check }` override.
1131+
# Returns the last argument like `seq`, allowing usage: checkV2MergeCoherence loc type expr
1132+
checkV2MergeCoherence =
1133+
loc: type: result:
1134+
if type.check.isV2MergeCoherent or false then
1135+
result
1136+
else
1137+
throw ''
1138+
The option `${showOption loc}' has a type `${type.description}' that uses
1139+
an ad-hoc `type // { check = ...; }' override, which is incompatible with
1140+
the v2 merge mechanism.
1141+
1142+
Please use `lib.types.addCheck` instead of `type // { check }' to add
1143+
custom validation. For example:
1144+
1145+
lib.types.addCheck baseType (value: /* your check */)
1146+
1147+
instead of:
1148+
1149+
baseType // { check = value: /* your check */; }
1150+
'';
1151+
11291152
# Merge definitions of a value of a given type.
11301153
mergeDefinitions = loc: type: defs: rec {
11311154
defsFinal' =
@@ -1201,10 +1224,13 @@ let
12011224
(
12021225
if type.merge ? v2 then
12031226
let
1204-
r = type.merge.v2 {
1205-
inherit loc;
1206-
defs = defsFinal;
1207-
};
1227+
# Check for v2 merge coherence
1228+
r = checkV2MergeCoherence loc type (
1229+
type.merge.v2 {
1230+
inherit loc;
1231+
defs = defsFinal;
1232+
}
1233+
);
12081234
in
12091235
r
12101236
// {

‎lib/tests/modules.sh‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -806,6 +806,25 @@ checkConfigError 'A definition for option .* is not of type .signed integer.*' c
806806
checkConfigOutput '^true$' config.v2checkedPass ./add-check.nix
807807
checkConfigError 'A definition for option .* is not of type .attribute set of signed integer.*' config.v2checkedFail ./add-check.nix
808808

809+
# v2 merge check coherence
810+
# Tests checkV2MergeCoherence call in modules.nix (mergeDefinitions for lazyAttrsOf)
811+
checkConfigError 'ad-hoc.*override.*incompatible' config.adhocFail.foo ./v2-check-coherence.nix
812+
# Tests checkV2MergeCoherence call in modules.nix (mergeDefinitions for lazyAttrsOf)
813+
checkConfigError 'ad-hoc.*override.*incompatible' config.adhocOuterFail.bar ./v2-check-coherence.nix
814+
# Tests checkV2MergeCoherence call in types.nix (either t1)
815+
checkConfigError 'ad-hoc.*override.*incompatible' config.eitherLeftFail ./v2-check-coherence.nix
816+
# Tests checkV2MergeCoherence call in types.nix (either t2)
817+
checkConfigError 'ad-hoc.*override.*incompatible' config.eitherRightFail ./v2-check-coherence.nix
818+
# Tests checkV2MergeCoherence call in types.nix (coercedTo coercedType)
819+
checkConfigError 'ad-hoc.*override.*incompatible' config.coercedFromFail.bar ./v2-check-coherence.nix
820+
# Tests checkV2MergeCoherence call in types.nix (coercedTo finalType)
821+
checkConfigError 'ad-hoc.*override.*incompatible' config.coercedToFail.foo ./v2-check-coherence.nix
822+
# Tests checkV2MergeCoherence call in types.nix (addCheck elemType)
823+
checkConfigError 'ad-hoc.*override.*incompatible' config.addCheckNested.foo ./v2-check-coherence.nix
824+
checkConfigError 'Please use.*lib.types.addCheck.*instead' config.adhocFail.foo ./v2-check-coherence.nix
825+
checkConfigError 'A definition for option .* is not of type .*' config.addCheckFail.bar.baz ./v2-check-coherence.nix
826+
checkConfigOutput '^true$' config.result ./v2-check-coherence.nix
827+
809828

810829
cat <<EOF
811830
====== module tests ======

‎lib/tests/modules/v2-check-coherence.nix‎

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,117 @@
1+
# Tests for v2 merge check coherence
2+
{ lib, ... }:
3+
let
4+
inherit (lib) types mkOption;
5+
6+
# The problematic pattern: overriding check with //
7+
# This inner type should reject everything (check always returns false)
8+
adhocOverrideType = (types.lazyAttrsOf types.raw) // {
9+
check = _: false;
10+
};
11+
12+
# Using addCheck is the correct way to add custom checks
13+
properlyCheckedType = types.addCheck (types.lazyAttrsOf types.raw) (v: v ? foo);
14+
15+
# Using addCheck with a check that will fail
16+
failingCheckedType = types.addCheck (types.lazyAttrsOf types.raw) (v: v ? foo);
17+
18+
# Ad-hoc override on outer type
19+
adhocOuterType = types.lazyAttrsOf types.int // {
20+
check = _: false;
21+
};
22+
23+
# Ad-hoc override on left side of either
24+
adhocEitherLeft = types.lazyAttrsOf types.raw // {
25+
check = _: false;
26+
};
27+
28+
# Ad-hoc override on coercedType in coercedTo
29+
adhocCoercedFrom = types.lazyAttrsOf types.raw // {
30+
check = _: false;
31+
};
32+
33+
# Ad-hoc override on finalType in coercedTo
34+
adhocCoercedTo = types.lazyAttrsOf types.raw // {
35+
check = _: false;
36+
};
37+
38+
# Ad-hoc override wrapped in addCheck
39+
adhocAddCheck = types.addCheck (types.lazyAttrsOf types.raw // { check = _: false; }) (v: true);
40+
in
41+
{
42+
# Test 1: Ad-hoc check override in nested type should be detected
43+
options.adhocFail = mkOption {
44+
type = types.lazyAttrsOf adhocOverrideType;
45+
default = { };
46+
};
47+
config.adhocFail = {
48+
foo = { };
49+
};
50+
51+
# Test 1b: Ad-hoc check override in outer type should be detected
52+
options.adhocOuterFail = mkOption {
53+
type = adhocOuterType;
54+
default = { };
55+
};
56+
config.adhocOuterFail.bar = 42;
57+
58+
# Test 1c: Ad-hoc check override on left side of either type
59+
options.eitherLeftFail = mkOption {
60+
type = types.either adhocEitherLeft types.int;
61+
};
62+
config.eitherLeftFail.foo = { };
63+
64+
# Test 1d: Ad-hoc check override on right side of either type
65+
options.eitherRightFail = mkOption {
66+
type = types.either types.int (types.lazyAttrsOf types.raw // { check = _: false; });
67+
};
68+
config.eitherRightFail.foo = { };
69+
70+
# Test 1e: Ad-hoc check override on coercedType in coercedTo
71+
options.coercedFromFail = mkOption {
72+
type = types.coercedTo adhocCoercedFrom (x: { bar = 1; }) (types.lazyAttrsOf types.int);
73+
};
74+
config.coercedFromFail = {
75+
foo = { };
76+
};
77+
78+
# Test 1f: Ad-hoc check override on finalType in coercedTo
79+
options.coercedToFail = mkOption {
80+
type = types.coercedTo types.str (x: { }) adhocCoercedTo;
81+
};
82+
config.coercedToFail.foo = { };
83+
84+
# Test 1g: Ad-hoc check override wrapped in addCheck
85+
options.addCheckNested = mkOption {
86+
type = adhocAddCheck;
87+
};
88+
config.addCheckNested.foo = { };
89+
90+
# Test 2: Using addCheck should work correctly
91+
options.addCheckPass = mkOption {
92+
type = types.lazyAttrsOf properlyCheckedType;
93+
default = { };
94+
};
95+
config.addCheckPass.bar.foo = "value";
96+
97+
# Test 3: addCheck should validate values properly
98+
options.addCheckFail = mkOption {
99+
type = types.lazyAttrsOf failingCheckedType;
100+
default = { };
101+
};
102+
config.addCheckFail.bar.baz = "value"; # Missing required 'foo' attribute
103+
104+
# Test 4: Normal v2 types should work without coherence errors
105+
options.normalPass = mkOption {
106+
type = types.lazyAttrsOf (types.attrsOf types.int);
107+
default = { };
108+
};
109+
config.normalPass.foo.bar = 42;
110+
111+
# Success assertion - only checks things that should succeed
112+
options.result = mkOption {
113+
type = types.bool;
114+
default = false;
115+
};
116+
config.result = true;
117+
}

‎lib/types.nix‎

Lines changed: 66 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,29 @@ let
106106
in
107107
if invalidDefs != [ ] then { message = "Definition values: ${showDefs invalidDefs}"; } else null;
108108

109+
# Check that a type with v2 merge has a coherent check attribute.
110+
# Throws an error if the type uses an ad-hoc `type // { check }` override.
111+
# Returns the last argument like `seq`, allowing usage: checkV2MergeCoherence loc type expr
112+
checkV2MergeCoherence =
113+
loc: type: result:
114+
if type.check.isV2MergeCoherent or false then
115+
result
116+
else
117+
throw ''
118+
The option `${showOption loc}' has a type `${type.description}' that uses
119+
an ad-hoc `type // { check = ...; }' override, which is incompatible with
120+
the v2 merge mechanism.
121+
122+
Please use `lib.types.addCheck` instead of `type // { check }' to add
123+
custom validation. For example:
124+
125+
lib.types.addCheck baseType (value: /* your check */)
126+
127+
instead of:
128+
129+
baseType // { check = value: /* your check */; }
130+
'';
131+
109132
outer_types = rec {
110133
isType = type: x: (x._type or "") == type;
111134

@@ -711,7 +734,10 @@ let
711734
optionDescriptionPhrase (class: class == "noun" || class == "composite") elemType
712735
}";
713736
descriptionClass = "composite";
714-
check = isList;
737+
check = {
738+
__functor = _self: isList;
739+
isV2MergeCoherent = true;
740+
};
715741
merge = {
716742
__functor =
717743
self: loc: defs:
@@ -824,7 +850,10 @@ let
824850
(if lazy then "lazy attribute set" else "attribute set")
825851
+ " of ${optionDescriptionPhrase (class: class == "noun" || class == "composite") elemType}";
826852
descriptionClass = "composite";
827-
check = isAttrs;
853+
check = {
854+
__functor = _self: isAttrs;
855+
isV2MergeCoherent = true;
856+
};
828857
merge = {
829858
__functor =
830859
self: loc: defs:
@@ -1236,7 +1265,10 @@ let
12361265

12371266
name = "submodule";
12381267

1239-
check = x: isAttrs x || isFunction x || path.check x;
1268+
check = {
1269+
__functor = _self: x: isAttrs x || isFunction x || path.check x;
1270+
isV2MergeCoherent = true;
1271+
};
12401272
in
12411273
mkOptionType {
12421274
inherit name;
@@ -1420,7 +1452,10 @@ let
14201452
) t2
14211453
}";
14221454
descriptionClass = "conjunction";
1423-
check = x: t1.check x || t2.check x;
1455+
check = {
1456+
__functor = _self: x: t1.check x || t2.check x;
1457+
isV2MergeCoherent = true;
1458+
};
14241459
merge = {
14251460
__functor =
14261461
self: loc: defs:
@@ -1430,7 +1465,7 @@ let
14301465
let
14311466
t1CheckedAndMerged =
14321467
if t1.merge ? v2 then
1433-
t1.merge.v2 { inherit loc defs; }
1468+
checkV2MergeCoherence loc t1 (t1.merge.v2 { inherit loc defs; })
14341469
else
14351470
{
14361471
value = t1.merge loc defs;
@@ -1439,7 +1474,7 @@ let
14391474
};
14401475
t2CheckedAndMerged =
14411476
if t2.merge ? v2 then
1442-
t2.merge.v2 { inherit loc defs; }
1477+
checkV2MergeCoherence loc t2 (t2.merge.v2 { inherit loc defs; })
14431478
else
14441479
{
14451480
value = t2.merge loc defs;
@@ -1509,7 +1544,10 @@ let
15091544
description = "${optionDescriptionPhrase (class: class == "noun") finalType} or ${
15101545
optionDescriptionPhrase (class: class == "noun") coercedType
15111546
} convertible to it";
1512-
check = x: (coercedType.check x && finalType.check (coerceFunc x)) || finalType.check x;
1547+
check = {
1548+
__functor = _self: x: (coercedType.check x && finalType.check (coerceFunc x)) || finalType.check x;
1549+
isV2MergeCoherent = true;
1550+
};
15131551
merge = {
15141552
__functor =
15151553
self: loc: defs:
@@ -1524,10 +1562,16 @@ let
15241562
// {
15251563
value =
15261564
let
1527-
merged = coercedType.merge.v2 {
1528-
inherit loc;
1529-
defs = [ def ];
1530-
};
1565+
merged =
1566+
if coercedType.merge ? v2 then
1567+
checkV2MergeCoherence loc coercedType (
1568+
coercedType.merge.v2 {
1569+
inherit loc;
1570+
defs = [ def ];
1571+
}
1572+
)
1573+
else
1574+
null;
15311575
in
15321576
if coercedType.merge ? v2 then
15331577
if merged.headError == null then coerceFunc def.value else def.value
@@ -1540,10 +1584,12 @@ let
15401584
);
15411585
in
15421586
if finalType.merge ? v2 then
1543-
finalType.merge.v2 {
1544-
inherit loc;
1545-
defs = finalDefs;
1546-
}
1587+
checkV2MergeCoherence loc finalType (
1588+
finalType.merge.v2 {
1589+
inherit loc;
1590+
defs = finalDefs;
1591+
}
1592+
)
15471593
else
15481594
{
15491595
value = finalType.merge loc finalDefs;
@@ -1576,15 +1622,18 @@ let
15761622
if elemType.merge ? v2 then
15771623
elemType
15781624
// {
1579-
check = x: elemType.check x && check x;
1625+
check = {
1626+
__functor = _self: x: elemType.check x && check x;
1627+
isV2MergeCoherent = true;
1628+
};
15801629
merge = {
15811630
__functor =
15821631
self: loc: defs:
15831632
(self.v2 { inherit loc defs; }).value;
15841633
v2 =
15851634
{ loc, defs }:
15861635
let
1587-
orig = elemType.merge.v2 { inherit loc defs; };
1636+
orig = checkV2MergeCoherence loc elemType (elemType.merge.v2 { inherit loc defs; });
15881637
headError' = if orig.headError != null then orig.headError else checkDefsForError check loc defs;
15891638
in
15901639
orig

0 commit comments

Comments
 (0)