From 93ea59f66d5e33c0734f2c8f27d7b3c0b4207ab3 Mon Sep 17 00:00:00 2001
From: Robert Hensing <robert@roberthensing.nl>
Date: Thu, 23 Oct 2025 18:31:35 +0200
Subject: [PATCH] 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:
https://github.com/NixOS/nixpkgs/pull/391544

Real world case:
https://hercules-ci.com/github/hercules-ci/hercules-ci-effects/jobs/875
---
 lib/modules.nix                          |  34 ++++++-
 lib/tests/modules.sh                     |  19 ++++
 lib/tests/modules/v2-check-coherence.nix | 117 +++++++++++++++++++++++
 lib/types.nix                            |  83 ++++++++++++----
 4 files changed, 232 insertions(+), 21 deletions(-)
 create mode 100644 lib/tests/modules/v2-check-coherence.nix

diff --git a/lib/modules.nix b/lib/modules.nix
index 42e88dbbf625..09f0908a69da 100644
--- a/lib/modules.nix
+++ b/lib/modules.nix
@@ -1126,6 +1126,29 @@ let
       __toString = _: showOption loc;
     };
 
+  # Check that a type with v2 merge has a coherent check attribute.
+  # Throws an error if the type uses an ad-hoc `type // { check }` override.
+  # Returns the last argument like `seq`, allowing usage: checkV2MergeCoherence loc type expr
+  checkV2MergeCoherence =
+    loc: type: result:
+    if type.check.isV2MergeCoherent or false then
+      result
+    else
+      throw ''
+        The option `${showOption loc}' has a type `${type.description}' 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 */; }
+      '';
+
   # Merge definitions of a value of a given type.
   mergeDefinitions = loc: type: defs: rec {
     defsFinal' =
@@ -1201,10 +1224,13 @@ let
         (
           if type.merge ? v2 then
             let
-              r = type.merge.v2 {
-                inherit loc;
-                defs = defsFinal;
-              };
+              # Check for v2 merge coherence
+              r = checkV2MergeCoherence loc type (
+                type.merge.v2 {
+                  inherit loc;
+                  defs = defsFinal;
+                }
+              );
             in
             r
             // {
diff --git a/lib/tests/modules.sh b/lib/tests/modules.sh
index 5cd781752317..02f73310dd9b 100755
--- a/lib/tests/modules.sh
+++ b/lib/tests/modules.sh
@@ -806,6 +806,25 @@ checkConfigError 'A definition for option .* is not of type .signed integer.*' c
 checkConfigOutput '^true$' config.v2checkedPass ./add-check.nix
 checkConfigError 'A definition for option .* is not of type .attribute set of signed integer.*' config.v2checkedFail ./add-check.nix
 
+# v2 merge check coherence
+# Tests checkV2MergeCoherence call in modules.nix (mergeDefinitions for lazyAttrsOf)
+checkConfigError 'ad-hoc.*override.*incompatible' config.adhocFail.foo ./v2-check-coherence.nix
+# Tests checkV2MergeCoherence call in modules.nix (mergeDefinitions for lazyAttrsOf)
+checkConfigError 'ad-hoc.*override.*incompatible' config.adhocOuterFail.bar ./v2-check-coherence.nix
+# Tests checkV2MergeCoherence call in types.nix (either t1)
+checkConfigError 'ad-hoc.*override.*incompatible' config.eitherLeftFail ./v2-check-coherence.nix
+# Tests checkV2MergeCoherence call in types.nix (either t2)
+checkConfigError 'ad-hoc.*override.*incompatible' config.eitherRightFail ./v2-check-coherence.nix
+# Tests checkV2MergeCoherence call in types.nix (coercedTo coercedType)
+checkConfigError 'ad-hoc.*override.*incompatible' config.coercedFromFail.bar ./v2-check-coherence.nix
+# Tests checkV2MergeCoherence call in types.nix (coercedTo finalType)
+checkConfigError 'ad-hoc.*override.*incompatible' config.coercedToFail.foo ./v2-check-coherence.nix
+# Tests checkV2MergeCoherence call in types.nix (addCheck elemType)
+checkConfigError 'ad-hoc.*override.*incompatible' config.addCheckNested.foo ./v2-check-coherence.nix
+checkConfigError 'Please use.*lib.types.addCheck.*instead' config.adhocFail.foo ./v2-check-coherence.nix
+checkConfigError 'A definition for option .* is not of type .*' config.addCheckFail.bar.baz ./v2-check-coherence.nix
+checkConfigOutput '^true$' config.result ./v2-check-coherence.nix
+
 
 cat <<EOF
 ====== module tests ======
diff --git a/lib/tests/modules/v2-check-coherence.nix b/lib/tests/modules/v2-check-coherence.nix
new file mode 100644
index 000000000000..60e134ff483e
--- /dev/null
+++ b/lib/tests/modules/v2-check-coherence.nix
@@ -0,0 +1,117 @@
+# Tests for v2 merge check coherence
+{ lib, ... }:
+let
+  inherit (lib) types mkOption;
+
+  # The problematic pattern: overriding check with //
+  # This inner type should reject everything (check always returns false)
+  adhocOverrideType = (types.lazyAttrsOf types.raw) // {
+    check = _: false;
+  };
+
+  # Using addCheck is the correct way to add custom checks
+  properlyCheckedType = types.addCheck (types.lazyAttrsOf types.raw) (v: v ? foo);
+
+  # Using addCheck with a check that will fail
+  failingCheckedType = types.addCheck (types.lazyAttrsOf types.raw) (v: v ? foo);
+
+  # Ad-hoc override on outer type
+  adhocOuterType = types.lazyAttrsOf types.int // {
+    check = _: false;
+  };
+
+  # Ad-hoc override on left side of either
+  adhocEitherLeft = types.lazyAttrsOf types.raw // {
+    check = _: false;
+  };
+
+  # Ad-hoc override on coercedType in coercedTo
+  adhocCoercedFrom = types.lazyAttrsOf types.raw // {
+    check = _: false;
+  };
+
+  # Ad-hoc override on finalType in coercedTo
+  adhocCoercedTo = types.lazyAttrsOf types.raw // {
+    check = _: false;
+  };
+
+  # Ad-hoc override wrapped in addCheck
+  adhocAddCheck = types.addCheck (types.lazyAttrsOf types.raw // { check = _: false; }) (v: true);
+in
+{
+  # Test 1: Ad-hoc check override in nested type should be detected
+  options.adhocFail = mkOption {
+    type = types.lazyAttrsOf adhocOverrideType;
+    default = { };
+  };
+  config.adhocFail = {
+    foo = { };
+  };
+
+  # Test 1b: Ad-hoc check override in outer type should be detected
+  options.adhocOuterFail = mkOption {
+    type = adhocOuterType;
+    default = { };
+  };
+  config.adhocOuterFail.bar = 42;
+
+  # Test 1c: Ad-hoc check override on left side of either type
+  options.eitherLeftFail = mkOption {
+    type = types.either adhocEitherLeft types.int;
+  };
+  config.eitherLeftFail.foo = { };
+
+  # Test 1d: Ad-hoc check override on right side of either type
+  options.eitherRightFail = mkOption {
+    type = types.either types.int (types.lazyAttrsOf types.raw // { check = _: false; });
+  };
+  config.eitherRightFail.foo = { };
+
+  # Test 1e: Ad-hoc check override on coercedType in coercedTo
+  options.coercedFromFail = mkOption {
+    type = types.coercedTo adhocCoercedFrom (x: { bar = 1; }) (types.lazyAttrsOf types.int);
+  };
+  config.coercedFromFail = {
+    foo = { };
+  };
+
+  # Test 1f: Ad-hoc check override on finalType in coercedTo
+  options.coercedToFail = mkOption {
+    type = types.coercedTo types.str (x: { }) adhocCoercedTo;
+  };
+  config.coercedToFail.foo = { };
+
+  # Test 1g: Ad-hoc check override wrapped in addCheck
+  options.addCheckNested = mkOption {
+    type = adhocAddCheck;
+  };
+  config.addCheckNested.foo = { };
+
+  # Test 2: Using addCheck should work correctly
+  options.addCheckPass = mkOption {
+    type = types.lazyAttrsOf properlyCheckedType;
+    default = { };
+  };
+  config.addCheckPass.bar.foo = "value";
+
+  # Test 3: addCheck should validate values properly
+  options.addCheckFail = mkOption {
+    type = types.lazyAttrsOf failingCheckedType;
+    default = { };
+  };
+  config.addCheckFail.bar.baz = "value"; # Missing required 'foo' attribute
+
+  # Test 4: Normal v2 types should work without coherence errors
+  options.normalPass = mkOption {
+    type = types.lazyAttrsOf (types.attrsOf types.int);
+    default = { };
+  };
+  config.normalPass.foo.bar = 42;
+
+  # Success assertion - only checks things that should succeed
+  options.result = mkOption {
+    type = types.bool;
+    default = false;
+  };
+  config.result = true;
+}
diff --git a/lib/types.nix b/lib/types.nix
index ae77c9fcf8cb..db4c2ddb97f3 100644
--- a/lib/types.nix
+++ b/lib/types.nix
@@ -106,6 +106,29 @@ let
     in
     if invalidDefs != [ ] then { message = "Definition values: ${showDefs invalidDefs}"; } else null;
 
+  # Check that a type with v2 merge has a coherent check attribute.
+  # Throws an error if the type uses an ad-hoc `type // { check }` override.
+  # Returns the last argument like `seq`, allowing usage: checkV2MergeCoherence loc type expr
+  checkV2MergeCoherence =
+    loc: type: result:
+    if type.check.isV2MergeCoherent or false then
+      result
+    else
+      throw ''
+        The option `${showOption loc}' has a type `${type.description}' 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 */; }
+      '';
+
   outer_types = rec {
     isType = type: x: (x._type or "") == type;
 
@@ -711,7 +734,10 @@ let
             optionDescriptionPhrase (class: class == "noun" || class == "composite") elemType
           }";
           descriptionClass = "composite";
-          check = isList;
+          check = {
+            __functor = _self: isList;
+            isV2MergeCoherent = true;
+          };
           merge = {
             __functor =
               self: loc: defs:
@@ -824,7 +850,10 @@ let
             (if lazy then "lazy attribute set" else "attribute set")
             + " of ${optionDescriptionPhrase (class: class == "noun" || class == "composite") elemType}";
           descriptionClass = "composite";
-          check = isAttrs;
+          check = {
+            __functor = _self: isAttrs;
+            isV2MergeCoherent = true;
+          };
           merge = {
             __functor =
               self: loc: defs:
@@ -1236,7 +1265,10 @@ let
 
           name = "submodule";
 
-          check = x: isAttrs x || isFunction x || path.check x;
+          check = {
+            __functor = _self: x: isAttrs x || isFunction x || path.check x;
+            isV2MergeCoherent = true;
+          };
         in
         mkOptionType {
           inherit name;
@@ -1420,7 +1452,10 @@ let
                 ) t2
               }";
           descriptionClass = "conjunction";
-          check = x: t1.check x || t2.check x;
+          check = {
+            __functor = _self: x: t1.check x || t2.check x;
+            isV2MergeCoherent = true;
+          };
           merge = {
             __functor =
               self: loc: defs:
@@ -1430,7 +1465,7 @@ let
               let
                 t1CheckedAndMerged =
                   if t1.merge ? v2 then
-                    t1.merge.v2 { inherit loc defs; }
+                    checkV2MergeCoherence loc t1 (t1.merge.v2 { inherit loc defs; })
                   else
                     {
                       value = t1.merge loc defs;
@@ -1439,7 +1474,7 @@ let
                     };
                 t2CheckedAndMerged =
                   if t2.merge ? v2 then
-                    t2.merge.v2 { inherit loc defs; }
+                    checkV2MergeCoherence loc t2 (t2.merge.v2 { inherit loc defs; })
                   else
                     {
                       value = t2.merge loc defs;
@@ -1509,7 +1544,10 @@ let
           description = "${optionDescriptionPhrase (class: class == "noun") finalType} or ${
             optionDescriptionPhrase (class: class == "noun") coercedType
           } convertible to it";
-          check = x: (coercedType.check x && finalType.check (coerceFunc x)) || finalType.check x;
+          check = {
+            __functor = _self: x: (coercedType.check x && finalType.check (coerceFunc x)) || finalType.check x;
+            isV2MergeCoherent = true;
+          };
           merge = {
             __functor =
               self: loc: defs:
@@ -1524,10 +1562,16 @@ let
                     // {
                       value =
                         let
-                          merged = coercedType.merge.v2 {
-                            inherit loc;
-                            defs = [ def ];
-                          };
+                          merged =
+                            if coercedType.merge ? v2 then
+                              checkV2MergeCoherence loc coercedType (
+                                coercedType.merge.v2 {
+                                  inherit loc;
+                                  defs = [ def ];
+                                }
+                              )
+                            else
+                              null;
                         in
                         if coercedType.merge ? v2 then
                           if merged.headError == null then coerceFunc def.value else def.value
@@ -1540,10 +1584,12 @@ let
                 );
               in
               if finalType.merge ? v2 then
-                finalType.merge.v2 {
-                  inherit loc;
-                  defs = finalDefs;
-                }
+                checkV2MergeCoherence loc finalType (
+                  finalType.merge.v2 {
+                    inherit loc;
+                    defs = finalDefs;
+                  }
+                )
               else
                 {
                   value = finalType.merge loc finalDefs;
@@ -1576,7 +1622,10 @@ let
         if elemType.merge ? v2 then
           elemType
           // {
-            check = x: elemType.check x && check x;
+            check = {
+              __functor = _self: x: elemType.check x && check x;
+              isV2MergeCoherent = true;
+            };
             merge = {
               __functor =
                 self: loc: defs:
@@ -1584,7 +1633,7 @@ let
               v2 =
                 { loc, defs }:
                 let
-                  orig = elemType.merge.v2 { inherit loc defs; };
+                  orig = checkV2MergeCoherence loc elemType (elemType.merge.v2 { inherit loc defs; });
                   headError' = if orig.headError != null then orig.headError else checkDefsForError check loc defs;
                 in
                 orig