Skip to content

Commit 5869611

Browse files
roberthroberth
authored
lib/modules: Report error for unsupported t // { check = ...; } (NixOS#454964)
2 parents 81f2a11 + 93ea59f commit 5869611

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

@@ -694,7 +717,10 @@ let
694717
optionDescriptionPhrase (class: class == "noun" || class == "composite") elemType
695718
}";
696719
descriptionClass = "composite";
697-
check = isList;
720+
check = {
721+
__functor = _self: isList;
722+
isV2MergeCoherent = true;
723+
};
698724
merge = {
699725
__functor =
700726
self: loc: defs:
@@ -807,7 +833,10 @@ let
807833
(if lazy then "lazy attribute set" else "attribute set")
808834
+ " of ${optionDescriptionPhrase (class: class == "noun" || class == "composite") elemType}";
809835
descriptionClass = "composite";
810-
check = isAttrs;
836+
check = {
837+
__functor = _self: isAttrs;
838+
isV2MergeCoherent = true;
839+
};
811840
merge = {
812841
__functor =
813842
self: loc: defs:
@@ -1219,7 +1248,10 @@ let
12191248

12201249
name = "submodule";
12211250

1222-
check = x: isAttrs x || isFunction x || path.check x;
1251+
check = {
1252+
__functor = _self: x: isAttrs x || isFunction x || path.check x;
1253+
isV2MergeCoherent = true;
1254+
};
12231255
in
12241256
mkOptionType {
12251257
inherit name;
@@ -1403,7 +1435,10 @@ let
14031435
) t2
14041436
}";
14051437
descriptionClass = "conjunction";
1406-
check = x: t1.check x || t2.check x;
1438+
check = {
1439+
__functor = _self: x: t1.check x || t2.check x;
1440+
isV2MergeCoherent = true;
1441+
};
14071442
merge = {
14081443
__functor =
14091444
self: loc: defs:
@@ -1413,7 +1448,7 @@ let
14131448
let
14141449
t1CheckedAndMerged =
14151450
if t1.merge ? v2 then
1416-
t1.merge.v2 { inherit loc defs; }
1451+
checkV2MergeCoherence loc t1 (t1.merge.v2 { inherit loc defs; })
14171452
else
14181453
{
14191454
value = t1.merge loc defs;
@@ -1422,7 +1457,7 @@ let
14221457
};
14231458
t2CheckedAndMerged =
14241459
if t2.merge ? v2 then
1425-
t2.merge.v2 { inherit loc defs; }
1460+
checkV2MergeCoherence loc t2 (t2.merge.v2 { inherit loc defs; })
14261461
else
14271462
{
14281463
value = t2.merge loc defs;
@@ -1492,7 +1527,10 @@ let
14921527
description = "${optionDescriptionPhrase (class: class == "noun") finalType} or ${
14931528
optionDescriptionPhrase (class: class == "noun") coercedType
14941529
} convertible to it";
1495-
check = x: (coercedType.check x && finalType.check (coerceFunc x)) || finalType.check x;
1530+
check = {
1531+
__functor = _self: x: (coercedType.check x && finalType.check (coerceFunc x)) || finalType.check x;
1532+
isV2MergeCoherent = true;
1533+
};
14961534
merge = {
14971535
__functor =
14981536
self: loc: defs:
@@ -1507,10 +1545,16 @@ let
15071545
// {
15081546
value =
15091547
let
1510-
merged = coercedType.merge.v2 {
1511-
inherit loc;
1512-
defs = [ def ];
1513-
};
1548+
merged =
1549+
if coercedType.merge ? v2 then
1550+
checkV2MergeCoherence loc coercedType (
1551+
coercedType.merge.v2 {
1552+
inherit loc;
1553+
defs = [ def ];
1554+
}
1555+
)
1556+
else
1557+
null;
15141558
in
15151559
if coercedType.merge ? v2 then
15161560
if merged.headError == null then coerceFunc def.value else def.value
@@ -1523,10 +1567,12 @@ let
15231567
);
15241568
in
15251569
if finalType.merge ? v2 then
1526-
finalType.merge.v2 {
1527-
inherit loc;
1528-
defs = finalDefs;
1529-
}
1570+
checkV2MergeCoherence loc finalType (
1571+
finalType.merge.v2 {
1572+
inherit loc;
1573+
defs = finalDefs;
1574+
}
1575+
)
15301576
else
15311577
{
15321578
value = finalType.merge loc finalDefs;
@@ -1559,15 +1605,18 @@ let
15591605
if elemType.merge ? v2 then
15601606
elemType
15611607
// {
1562-
check = x: elemType.check x && check x;
1608+
check = {
1609+
__functor = _self: x: elemType.check x && check x;
1610+
isV2MergeCoherent = true;
1611+
};
15631612
merge = {
15641613
__functor =
15651614
self: loc: defs:
15661615
(self.v2 { inherit loc defs; }).value;
15671616
v2 =
15681617
{ loc, defs }:
15691618
let
1570-
orig = elemType.merge.v2 { inherit loc defs; };
1619+
orig = checkV2MergeCoherence loc elemType (elemType.merge.v2 { inherit loc defs; });
15711620
headError' = if orig.headError != null then orig.headError else checkDefsForError check loc defs;
15721621
in
15731622
orig

0 commit comments

Comments
 (0)