Add Module System error contexts for imports and module argument infinite recursions

lib/modules.nix

@@ -404,7 +404,10 @@ let
           };
         in parentFile: parentKey: initialModules: args: collectResults (imap1 (n: x:
           let
-            module = checkModule (loadModule args parentFile "${parentKey}:anon-${toString n}" x);
+            module =
+              builtins.addErrorContext
+                "while loading imports of module `${parentFile}`"
+                (checkModule (loadModule args parentFile "${parentKey}:anon-${toString n}" x));
             collectedImports = collectStructuredModules module._file module.key module.imports args;
           in {
             key = module.key;
@@ -497,6 +500,30 @@ let
 
   applyModuleArgs = key: f: args@{ config, ... }:
     let
+      # Should we add this context to the selection of `_module.args` too?
+      # Triggering at that point seems much less common, and accomodating that
+      # scenario complicates the already long error message. That said, I wonder
+      # if `config` isn't already strict in `_module` defs and decls...
+      # Let's keep it simple for now.
+      config' =
+        name:
+          builtins.addErrorContext
+            ''
+              *risky* while evaluating the module fixpoint, `config`
+
+              Note that the module structure and `imports` fundamentally can not depend on the module fixpoint!
+              An argument `${name}` was not provided externally through .specialArgs., so we try to load it from the module fixpoint...
+              If the following message is "infinite recursion", this was the cause, and possible solutions include:
+              - Perhaps ${name /* unquoted for simplicity */} was misspelled.
+              - Make sure that the module was loaded into the right context.
+              - Provide the argument externally through `specialArgs`.
+              - Structure the module such that the top level attribute set and special attributes like `config`, `imports` and `config._module.args` do not refer to `${name}`.
+                (Except perhaps in a deeper structure, like a more deeply nested attribute, or a `mkIf` condition.)${
+                  # Remove after 25.11.
+                  optionalString (! lib.oldestSupportedReleaseIsAtLeast 2505) "\n\nThe above explanation is new. If it is inaccurate or out of place, please report a minimal example in a Nixpkgs issue and ping the Module System maintainers." }
+            ''
+            config;
+
       # Module arguments are resolved in a strict manner when attribute set
       # deconstruction is used.  As the arguments are now defined with the
       # config._module.args option, the strictness used on the attribute
@@ -512,7 +539,7 @@ let
       context = name: ''while evaluating the module argument `${name}' in "${key}":'';
       extraArgs = mapAttrs (name: _:
         addErrorContext (context name)
-          (args.${name} or config._module.args.${name})
+          (args.${name} or (config' name)._module.args.${name})
       ) (functionArgs f);
 
       # Note: we append in the opposite order such that we can add an error

lib/tests/modules.sh

@@ -8,6 +8,9 @@
 # [nixpkgs]$ lib/tests/modules.sh
 # or:
 # [nixpkgs]$ nix-build lib/tests/release.nix
+#
+# You may show all error traces, even for successful tests, by setting DUMP_TRACES=1
+# This can answer some questions about the occurrence of certain addErrorContext traces.
 
 set -o errexit -o noclobber -o nounset -o pipefail
 shopt -s failglob inherit_errexit
@@ -20,6 +23,14 @@ cd "$DIR"/modules
 pass=0
 fail=0
 
+if [[ -z "${DUMP_TRACES:-}" ]]; then
+    dump_traces() { :; }
+else
+    dump_traces() {
+        echo "$@"
+    }
+fi
+
 # loc
 #   prints the location of the call of to the function that calls it
 # loc n
@@ -96,6 +107,7 @@ checkConfigError() {
     else
         if echo "$err" | grep -zP --silent "$errorContains" ; then
             ((++pass))
+            dump_traces "$err"
         else
             logStartFailure
             echo "ACTUAL:"
@@ -107,6 +119,32 @@ checkConfigError() {
     fi
 }
 
+checkConfigNotError() {
+    local errorContains=$1
+    local err=""
+    shift
+    if err="$(evalConfig "$@" 2>&1 >/dev/null)"; then
+        logStartFailure
+        echo "ACTUAL: exit code 0, output:"
+        reportFailure "$@"
+        echo "EXPECTED: non-zero exit code"
+        logFailure
+        logEndFailure
+    else
+        if echo "$err" | grep -zP --silent "$errorContains" ; then
+            logStartFailure
+            echo "ACTUAL:"
+            reportFailure "$@"
+            echo "EXPECTED: log does not contain '$errorContains'"
+            logFailure
+            logEndFailure
+        else
+            ((++pass))
+            dump_traces "$err"
+        fi
+    fi
+}
+
 # Shorthand meta attribute does not duplicate the config
 checkConfigOutput '^"one two"$' config.result ./shorthand-meta.nix
 
@@ -285,6 +323,9 @@ checkConfigOutput '^true$' "$@" ./define-_module-args-custom.nix
 set -- "$@" ./define-_module-args-custom.nix ./import-custom-arg.nix
 checkConfigError 'while evaluating the module argument .*custom.* in .*import-custom-arg.nix.*:' "$@"
 checkConfigError 'infinite recursion encountered' "$@"
+# Make sure the error context for the `config` fixpoint is not triggered
+checkConfigNotError '\*risky\* while evaluating the module fixpoint.*custom. was not provided externally through .specialArgs., so I will now attempt to load it from the module fixpoint' config.result ./error-context-module-argument-failing.nix
+checkConfigError 'while evaluating the module argument .custom.' config.result ./error-context-module-argument-failing.nix
 
 # Check _module.check.
 set -- config.enable ./declare-enable.nix ./define-enable.nix ./define-attrsOfSub-foo.nix
@@ -535,6 +576,15 @@ checkConfigError 'Type foo defines both `functor.payload` and `functor.wrapped`
 # because of an `extendModules` bug, issue 168767.
 checkConfigOutput '^1$' config.sub.specialisation.value ./extendModules-168767-imports.nix
 
+# Imports error context
+checkConfigError 'while loading imports of module .a.' config.result ./error-context-imports.nix
+checkConfigError 'while loading imports of module .a-parent.' config.result ./error-context-imports-anonymous.nix
+# Full ancestry is generally not relevant, so we don't want to pollute the trace with it. Note that addErrorContext is printed regardless of --no-show-trace.
+checkConfigNotError 'while loading imports of module .not-this-file.' config.result ./error-context-imports.nix
+checkConfigError 'while loading imports of module .a.' config.result ./error-context-imports-fail-in-body.nix
+checkConfigError 'while loading imports of module .a.' config.result ./error-context-imports-fail-in-body-structure.nix
+checkConfigError 'while loading imports of module .a.' config.result ./error-context-imports-fail-in-body-infinite-recursion.nix
+
 # Class checks, evalModules
 checkConfigOutput '^{}$' config.ok.config ./class-check.nix
 checkConfigOutput '"nixos"' config.ok.class ./class-check.nix

lib/tests/modules/error-context-imports-anonymous.nix

@@ -0,0 +1,13 @@
+let
+  b =
+    abort "bad file or whatever";
+  a =
+    {
+      # anonymous module
+      imports = [ b ];
+    };
+in
+{
+  _file = "a-parent";
+  imports = [ a ];
+}

lib/tests/modules/error-context-imports-fail-in-body-infinite-recursion.nix

@@ -0,0 +1,8 @@
+let
+  b =
+    { someUnknownArg, ... }: someUnknownArg;
+in
+{
+  _file = "a";
+  imports = [ b ];
+}

lib/tests/modules/error-context-imports-fail-in-body-structure.nix

@@ -0,0 +1,15 @@
+let
+  b =
+    { ... }: {
+      _file = "b";
+      options = { };
+      config = { };
+
+      # This will be rejected, because this is not a shorthand module syntax.
+      dummyBadAttr = { };
+    };
+in
+{
+  _file = "a";
+  imports = [ b ];
+}

lib/tests/modules/error-context-imports-fail-in-body.nix

@@ -0,0 +1,8 @@
+let
+  b =
+    { ... }: abort "bad module";
+in
+{
+  _file = "a";
+  imports = [ b ];
+}

lib/tests/modules/error-context-imports.nix

@@ -0,0 +1,14 @@
+let
+  b =
+    abort "bad file or whatever";
+  a =
+    {
+      _file = "a";
+      imports = [ b ];
+    };
+in
+{
+  # This _file will not be reported.
+  _file = "not-this-file";
+  imports = [ a ];
+}

lib/tests/modules/error-context-module-argument-failing.nix

@@ -0,0 +1,5 @@
+{ lib, custom, ... }: {
+  options.result = lib.mkOption {};
+  config._module.args.custom = throw "gotcha123";
+  config.result = custom;
+}