[Reverted] Module-builtin assertions, disabling assertions and submodule assertions

lib/modules.nix

@@ -46,6 +46,7 @@ let
     showFiles
     showOption
     unknownModule
+    literalExample
     ;
 in
 
@@ -72,14 +73,20 @@ rec {
                   check ? true
                 }:
     let
-      # This internal module declare internal options under the `_module'
-      # attribute.  These options are fragile, as they are used by the
-      # module system to change the interpretation of modules.
+      # An internal module that's always added, defining special options which
+      # change the behavior of the module evaluation itself. This is under a
+      # `_`-prefixed namespace in order to prevent name clashes with
+      # user-defined options
       internalModule = rec {
-        _file = ./modules.nix;
+        # FIXME: Using ./modules.nix directly breaks the doc for some reason
+        _file = "lib/modules.nix";
 
         key = _file;
 
+        # Most of these options are set to be internal only for prefix != [],
+        # aka it's a submodule evaluation. This way their docs are displayed
+        # only once as a top-level NixOS option, but will be hidden for all
+        # submodules, even though they are available there too
         options = {
           _module.args = mkOption {
             # Because things like `mkIf` are entirely useless for
@@ -89,21 +96,27 @@ rec {
             # a `_module.args.pkgs = import (fetchTarball { ... }) {}` won't
             # start a download when `pkgs` wasn't evaluated.
             type = types.lazyAttrsOf types.unspecified;
-            internal = true;
+            internal = prefix != [];
             description = "Arguments passed to each module.";
           };
 
           _module.check = mkOption {
             type = types.bool;
             internal = true;
             default = check;
-            description = "Whether to check whether all option definitions have matching declarations.";
+            description = ''
+              Whether to check whether all option definitions have matching
+              declarations.
+
+              Note that this has nothing to do with the similarly named
+              <option>_module.checks</option> option
+            '';
           };
 
           _module.freeformType = mkOption {
             # Disallow merging for now, but could be implemented nicely with a `types.optionType`
             type = types.nullOr (types.uniq types.attrs);
-            internal = true;
+            internal = prefix != [];
             default = null;
             description = ''
               If set, merge all definitions that don't have an associated option
@@ -116,6 +129,75 @@ rec {
               turned off.
             '';
           };
+
+          _module.checks = mkOption {
+            description = ''
+              Evaluation checks to trigger during module evaluation. The
+              attribute name will be displayed when it is triggered, allowing
+              users to disable/change these checks if necessary. See
+              the section on Warnings and Assertions in the manual for more
+              information.
+            '';
+            example = literalExample ''
+              {
+                gpgSshAgent = {
+                  enable = config.programs.gnupg.agent.enableSSHSupport && config.programs.ssh.startAgent;
+                  message = "You can't use ssh-agent and GnuPG agent with SSH support enabled at the same time!";
+                };
+
+                grafanaPassword = {
+                  enable = config.services.grafana.database.password != "";
+                  message = "Grafana passwords will be stored as plaintext in the Nix store!";
+                  type = "warning";
+                };
+              }
+            '';
+            default = {};
+            internal = prefix != [];
+            type = types.attrsOf (types.submodule {
+              options.enable = mkOption {
+                description = ''
+                  Whether to enable this check. Set this to false to not trigger
+                  any errors or warning messages. This is useful for ignoring a
+                  check in case it doesn't make sense in certain scenarios.
+                '';
+                default = true;
+                type = types.bool;
+              };
+
+              options.check = mkOption {
+                description = ''
+                  The condition that must succeed in order for this check to be
+                  successful and not trigger a warning or error.
+                '';
+                readOnly = true;
+                type = types.bool;
+              };
+
+              options.type = mkOption {
+                description = ''
+                  The type of the check. The default
+                  <literal>"error"</literal> type will cause evaluation to fail,
+                  while the <literal>"warning"</literal> type will only show a
+                  warning.
+                '';
+                type = types.enum [ "error" "warning" ];
+                default = "error";
+                example = "warning";
+              };
+
+              options.message = mkOption {
+                description = ''
+                  The message to display if this check triggers.
+                  To display option names in the message, add
+                  <literal>options</literal> to the module function arguments
+                  and use <literal>''${options.path.to.option}</literal>.
+                '';
+                type = types.str;
+                example = "Enabling both \${options.services.foo.enable} and \${options.services.bar.enable} is not possible.";
+              };
+            });
+          };
         };
 
         config = {
@@ -154,6 +236,35 @@ rec {
           # paths, meaning recursiveUpdate will never override any value
           else recursiveUpdate freeformConfig declaredConfig;
 
+      # Triggers all checks defined by _module.checks before returning its argument
+      triggerChecks = let
+
+        handleCheck = errors: name:
+          let
+            value = config._module.checks.${name};
+            show =
+              # Assertions with a _ prefix aren't meant to be configurable
+              if lib.hasPrefix "_" name then value.message
+              else "[${showOption prefix}${optionalString (prefix != []) "/"}${name}] ${value.message}";
+          in
+            if value.enable -> value.check then errors
+            else if value.type == "warning" then lib.warn show errors
+            else if value.type == "error" then errors ++ [ show ]
+            else abort "Unknown check type ${value.type}";
+
+        errors = lib.foldl' handleCheck [] (lib.attrNames config._module.checks);
+
+        errorMessage = ''
+          Failed checks:
+          ${lib.concatMapStringsSep "\n" (a: "- ${a}") errors}
+        '';
+
+        trigger = if errors == [] then null else throw errorMessage;
+
+      in builtins.seq trigger;
+
+      finalConfig = triggerChecks (removeAttrs config [ "_module" ]);
+
       checkUnmatched =
         if config._module.check && config._module.freeformType == null && merged.unmatchedDefns != [] then
           let
@@ -173,7 +284,7 @@ rec {
 
       result = builtins.seq checkUnmatched {
         inherit options;
-        config = removeAttrs config [ "_module" ];
+        config = finalConfig;
         inherit (config) _module;
       };
     in result;
@@ -514,6 +625,8 @@ rec {
         definitions = map (def: def.value) res.defsFinal;
         files = map (def: def.file) res.defsFinal;
         inherit (res) isDefined;
+        # This allows options to be correctly displayed using `${options.path.to.it}`
+        __toString = _: showOption loc;
       };
 
   # Merge definitions of a value of a given type.
@@ -773,14 +886,15 @@ rec {
         visible = false;
         apply = x: throw "The option `${showOption optionName}' can no longer be used since it's been removed. ${replacementInstructions}";
       });
-      config.assertions =
-        let opt = getAttrFromPath optionName options; in [{
-          assertion = !opt.isDefined;
+      config._module.checks =
+        let opt = getAttrFromPath optionName options; in {
+        ${"removed-" + showOption optionName} = lib.mkIf opt.isDefined {
           message = ''
             The option definition `${showOption optionName}' in ${showFiles opt.files} no longer has any effect; please remove it.
             ${replacementInstructions}
           '';
-        }];
+        };
+      };
     };
 
   /* Return a module that causes a warning to be shown if the
@@ -841,14 +955,18 @@ rec {
       })) from);
 
       config = {
-        warnings = filter (x: x != "") (map (f:
-          let val = getAttrFromPath f config;
-              opt = getAttrFromPath f options;
-          in
-          optionalString
-            (val != "_mkMergedOptionModule")
-            "The option `${showOption f}' defined in ${showFiles opt.files} has been changed to `${showOption to}' that has a different type. Please read `${showOption to}' documentation and update your configuration accordingly."
-        ) from);
+        _module.checks =
+          let warningMessages = map (f:
+            let val = getAttrFromPath f config;
+                opt = getAttrFromPath f options;
+            in {
+              ${"merged" + showOption f} = lib.mkIf (val != "_mkMergedOptionModule") {
+                type = "warning";
+                message = "The option `${showOption f}' defined in ${showFiles opt.files} has been changed to `${showOption to}' that has a different type. Please read `${showOption to}' documentation and update your configuration accordingly.";
+              };
+            }
+            ) from;
+          in mkMerge warningMessages;
       } // setAttrByPath to (mkMerge
              (optional
                (any (f: (getAttrFromPath f config) != "_mkMergedOptionModule") from)
@@ -907,8 +1025,10 @@ rec {
       });
       config = mkMerge [
         {
-          warnings = optional (warn && fromOpt.isDefined)
-            "The option `${showOption from}' defined in ${showFiles fromOpt.files} has been renamed to `${showOption to}'.";
+          _module.checks.${"renamed-" + showOption from} = mkIf (warn && fromOpt.isDefined) {
+            type = "warning";
+            message = "The option `${showOption from}' defined in ${showFiles fromOpt.files} has been renamed to `${showOption to}'.";
+          };
         }
         (if withPriority
           then mkAliasAndWrapDefsWithPriority (setAttrByPath to) fromOpt

lib/options.nix

@@ -192,7 +192,7 @@ rec {
           let ss = opt.type.getSubOptions opt.loc;
           in if ss != {} then optionAttrSetToDocList' opt.loc ss else [];
       in
-        [ docOption ] ++ optionals docOption.visible subOptions) (collect isOption options);
+        [ docOption ] ++ optionals (docOption.visible && ! docOption.internal) subOptions) (collect isOption options);
 
 
   /* This function recursively removes all derivation attributes from

lib/tests/misc.nix

@@ -655,7 +655,7 @@ runTests {
           modules = [ module ];
         }).options;
 
-        locs = filter (o: ! o.internal) (optionAttrSetToDocList options);
+        locs = filter (o: ! o.internal) (optionAttrSetToDocList (removeAttrs options [ "_module" ]));
       in map (o: o.loc) locs;
     expected = [ [ "foo" ] [ "foo" "<name>" "bar" ] [ "foo" "bar" ] ];
   };

lib/tests/modules.sh

@@ -27,37 +27,50 @@ reportFailure() {
     fail=$((fail + 1))
 }
 
-checkConfigOutput() {
+checkConfigCodeOutErr() {
+    local expectedExit=$1
+    shift;
     local outputContains=$1
     shift;
-    if evalConfig "$@" 2>/dev/null | grep --silent "$outputContains" ; then
-        pass=$((pass + 1))
-        return 0;
-    else
-        echo 2>&1 "error: Expected result matching '$outputContains', while evaluating"
+    local errorContains=$1
+    shift;
+    out=$(mktemp)
+    err=$(mktemp)
+    evalConfig "$@" 1>"$out" 2>"$err"
+    if [[ "$?" -ne "$expectedExit" ]]; then
+        echo 2>&1 "error: Expected exit code $expectedExit while evaluating"
+        reportFailure "$@"
+        return 1
+    fi
+
+    if [[ -n "$outputContains" ]] && ! grep -zP --silent "$outputContains" "$out"; then
+        echo 2>&1 "error: Expected output matching '$outputContains', while evaluating"
         reportFailure "$@"
         return 1
     fi
+
+    if [[ -n "$errorContains" ]] && ! grep -zP --silent "$errorContains" "$err"; then
+        echo 2>&1 "error: Expected error matching '$errorContains', while evaluating"
+        reportFailure "$@"
+        return 1
+    fi
+
+    pass=$((pass + 1))
+
+    # Clean up temp files
+    rm "$out" "$err"
+}
+
+checkConfigOutput() {
+    local outputContains=$1
+    shift;
+    checkConfigCodeOutErr 0 "$outputContains" "" "$@"
 }
 
 checkConfigError() {
     local errorContains=$1
-    local err=""
     shift;
-    if err==$(evalConfig "$@" 2>&1 >/dev/null); then
-        echo 2>&1 "error: Expected error code, got exit code 0, while evaluating"
-        reportFailure "$@"
-        return 1
-    else
-        if echo "$err" | grep -zP --silent "$errorContains" ; then
-            pass=$((pass + 1))
-            return 0;
-        else
-            echo 2>&1 "error: Expected error matching '$errorContains', while evaluating"
-            reportFailure "$@"
-            return 1
-        fi
-    fi
+    checkConfigCodeOutErr 1 "" "$errorContains" "$@"
 }
 
 # Check boolean option.
@@ -262,6 +275,29 @@ checkConfigOutput true config.value.mkbefore ./types-anything/mk-mods.nix
 checkConfigOutput 1 config.value.nested.foo ./types-anything/mk-mods.nix
 checkConfigOutput baz config.value.nested.bar.baz ./types-anything/mk-mods.nix
 
+## Module assertions
+# Check that assertions are triggered by default for just evaluating config
+checkConfigError 'Failed checks:\n- \[test\] Assertion failed' config ./assertions/simple.nix
+
+# Assertion is not triggered when enable is false or condition is true
+checkConfigOutput '{ }' config ./assertions/condition-true.nix
+checkConfigOutput '{ }' config ./assertions/enable-false.nix
+
+# Warnings should be displayed on standard error
+checkConfigCodeOutErr 0 '{ }' 'warning: \[test\] Warning message' config ./assertions/warning.nix
+
+# Check that multiple assertions and warnings can be triggered at once
+checkConfigError 'Failed checks:\n- \[test1\] Assertion 1 failed\n- \[test2\] Assertion 2 failed' config ./assertions/multi.nix
+checkConfigError 'trace: warning: \[test3\] Warning 3 failed\ntrace: warning: \[test4\] Warning 4 failed' config ./assertions/multi.nix
+
+# Submodules should be able to trigger assertions and display the submodule prefix in their error
+checkConfigError 'Failed checks:\n- \[foo/test\] Assertion failed' config.foo ./assertions/submodule.nix
+checkConfigError 'Failed checks:\n- \[foo.bar/test\] Assertion failed' config.foo.bar ./assertions/submodule-attrsOf.nix
+checkConfigError 'Failed checks:\n- \[foo.bar.baz/test\] Assertion failed' config.foo.bar.baz ./assertions/submodule-attrsOf-attrsOf.nix
+
+# Assertions with an attribute starting with _ shouldn't have their name displayed
+checkConfigError 'Failed checks:\n- Assertion failed' config ./assertions/underscore-attributes.nix
+
 cat <<EOF
 ====== module tests ======
 $pass Pass

lib/tests/modules/assertions/condition-true.nix

@@ -0,0 +1,8 @@
+{
+
+  _module.checks.test = {
+    check = true;
+    message = "Assertion failed";
+  };
+
+}

lib/tests/modules/assertions/enable-false.nix

@@ -0,0 +1,9 @@
+{
+
+  _module.checks.test = {
+    enable = false;
+    check = false;
+    message = "Assertion failed";
+  };
+
+}