Commit 93ea59f6 authored by Robert Hensing's avatar Robert Hensing
Browse files

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
parent ddd7747c

lib/modules.nix

+30 −4
Original line number Diff line number Diff line
@@ -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 {

              # Check for v2 merge coherence

              r = checkV2MergeCoherence loc type (

                type.merge.v2 {

                  inherit loc;

                  defs = defsFinal;

              };

                }

              );

            in

            r

            // {

lib/tests/modules.sh

+19 −0
Original line number Diff line number Diff line
@@ -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 ======

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

0 → 100644
+117 −0
Original line number Diff line number Diff line 
# 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;

}

lib/types.nix

+66 −17
Original line number Diff line number Diff line
@@ -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 {

                          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

                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