lib.modules: Let module declare options directly in bare submodule

lib/modules.nix

@@ -9,7 +9,7 @@ let
     catAttrs
     concatLists
     concatMap
-    count
+    concatStringsSep
     elem
     filter
     findFirst
@@ -47,6 +47,20 @@ let
     showOption
     unknownModule
     ;
+
+  showDeclPrefix = loc: decl: prefix:
+    " - option(s) with prefix `${showOption (loc ++ [prefix])}' in module `${decl._file}'";
+  showRawDecls = loc: decls:
+    concatStringsSep "\n"
+      (sort (a: b: a < b)
+        (concatMap
+          (decl: map
+            (showDeclPrefix loc decl)
+            (attrNames decl.options)
+          )
+          decls
+      ));
+
 in
 
 rec {
@@ -474,26 +488,61 @@ rec {
           [{ inherit (module) file; inherit value; }]
         ) configs;
 
+      # Convert an option tree decl to a submodule option decl
+      optionTreeToOption = decl:
+        if isOption decl.options
+        then decl
+        else decl // {
+            options = mkOption {
+              type = types.submoduleWith {
+                modules = [ { options = decl.options; } ];
+                # `null` is not intended for use by modules. It is an internal
+                # value that means "whatever the user has declared elsewhere".
+                # This might become obsolete with https://github.com/NixOS/nixpkgs/issues/162398
+                shorthandOnlyDefinesConfig = null;
+              };
+            };
+          };
+
       resultsByName = mapAttrs (name: decls:
         # We're descending into attribute ‘name’.
         let
           loc = prefix ++ [name];
           defns = defnsByName.${name} or [];
           defns' = defnsByName'.${name} or [];
-          nrOptions = count (m: isOption m.options) decls;
+          optionDecls = filter (m: isOption m.options) decls;
         in
-          if nrOptions == length decls then
+          if length optionDecls == length decls then
             let opt = fixupOptionType loc (mergeOptionDecls loc decls);
             in {
               matchedOptions = evalOptionValue loc opt defns';
               unmatchedDefns = [];
             }
-          else if nrOptions != 0 then
-            let
-              firstOption = findFirst (m: isOption m.options) "" decls;
-              firstNonOption = findFirst (m: !isOption m.options) "" decls;
-            in
-              throw "The option `${showOption loc}' in `${firstOption._file}' is a prefix of options in `${firstNonOption._file}'."
+          else if optionDecls != [] then
+              if all (x: x.options.type.name == "submodule") optionDecls
+              # Raw options can only be merged into submodules. Merging into
+              # attrsets might be nice, but ambiguous. Suppose we have
+              # attrset as a `attrsOf submodule`. User declares option
+              # attrset.foo.bar, this could mean:
+              #  a. option `bar` is only available in `attrset.foo`
+              #  b. option `foo.bar` is available in all `attrset.*`
+              #  c. reject and require "<name>" as a reminder that it behaves like (b).
+              #  d. magically combine (a) and (c).
+              # All of the above are merely syntax sugar though.
+              then
+                let opt = fixupOptionType loc (mergeOptionDecls loc (map optionTreeToOption decls));
+                in {
+                  matchedOptions = evalOptionValue loc opt defns';
+                  unmatchedDefns = [];
+                }
+              else
+                let
+                  firstNonOption = findFirst (m: !isOption m.options) "" decls;
+                  nonOptions = filter (m: !isOption m.options) decls;
+                in
+                throw "The option `${showOption loc}' in module `${(lib.head optionDecls)._file}' would be a parent of the following options, but its type `${(lib.head optionDecls).options.type.description or "<no description>"}' does not support nested options.\n${
+                  showRawDecls loc nonOptions
+                }"
           else
             mergeModules' loc decls defns) declsByName;
 
@@ -753,13 +802,14 @@ rec {
       compare = a: b: (a.priority or 1000) < (b.priority or 1000);
     in sort compare defs';
 
-  /* Hack for backward compatibility: convert options of type
-     optionSet to options of type submodule.  FIXME: remove
-     eventually. */
   fixupOptionType = loc: opt:
     let
       options = opt.options or
         (throw "Option `${showOption loc}' has type optionSet but has no option attribute, in ${showFiles opt.declarations}.");
+
+      # Hack for backward compatibility: convert options of type
+      # optionSet to options of type submodule.  FIXME: remove
+      # eventually.
       f = tp:
         let optionSetIn = type: (tp.name == type) && (tp.functor.wrapped.name == "optionSet");
         in

lib/tests/modules.sh

@@ -62,6 +62,13 @@ checkConfigError() {
 checkConfigOutput '^false$' config.enable ./declare-enable.nix
 checkConfigError 'The option .* does not exist. Definition values:\n\s*- In .*: true' config.enable ./define-enable.nix
 
+checkConfigOutput '^1$' config.bare-submodule.nested ./declare-bare-submodule.nix ./declare-bare-submodule-nested-option.nix
+checkConfigOutput '^2$' config.bare-submodule.deep ./declare-bare-submodule.nix ./declare-bare-submodule-deep-option.nix
+checkConfigOutput '^42$' config.bare-submodule.nested ./declare-bare-submodule.nix ./declare-bare-submodule-nested-option.nix ./declare-bare-submodule-deep-option.nix ./define-bare-submodule-values.nix
+checkConfigOutput '^420$' config.bare-submodule.deep ./declare-bare-submodule.nix ./declare-bare-submodule-nested-option.nix ./declare-bare-submodule-deep-option.nix ./define-bare-submodule-values.nix
+checkConfigOutput '^2$' config.bare-submodule.deep ./declare-bare-submodule.nix ./declare-bare-submodule-deep-option.nix ./define-shorthandOnlyDefinesConfig-true.nix
+checkConfigError 'The option .bare-submodule.deep. in .*/declare-bare-submodule-deep-option.nix. is already declared in .*/declare-bare-submodule-deep-option-duplicate.nix' config.bare-submodule.deep ./declare-bare-submodule.nix ./declare-bare-submodule-deep-option.nix  ./declare-bare-submodule-deep-option-duplicate.nix
+
 # Check integer types.
 # unsigned
 checkConfigOutput '^42$' config.value ./declare-int-unsigned-value.nix ./define-value-int-positive.nix
@@ -304,6 +311,12 @@ checkConfigOutput "10" config.processedToplevel ./raw.nix
 checkConfigError "The option .multiple. is defined multiple times" config.multiple ./raw.nix
 checkConfigOutput "bar" config.priorities ./raw.nix
 
+## Option collision
+checkConfigError \
+  'The option .set. in module .*/declare-set.nix. would be a parent of the following options, but its type .attribute set of signed integers. does not support nested options.\n\s*- option[(]s[)] with prefix .set.enable. in module .*/declare-enable-nested.nix.' \
+  config.set \
+  ./declare-set.nix ./declare-enable-nested.nix
+
 # Test that types.optionType merges types correctly
 checkConfigOutput '^10$' config.theOption.int ./optionTypeMerging.nix
 checkConfigOutput '^"hello"$' config.theOption.str ./optionTypeMerging.nix

lib/tests/modules/declare-bare-submodule-deep-option-duplicate.nix

@@ -0,0 +1,10 @@
+{ lib, ... }:
+let
+  inherit (lib) mkOption types;
+in
+{
+  options.bare-submodule.deep = mkOption {
+    type = types.int;
+    default = 2;
+  };
+}

lib/tests/modules/declare-bare-submodule-deep-option.nix

@@ -0,0 +1,10 @@
+{ lib, ... }:
+let
+  inherit (lib) mkOption types;
+in
+{
+  options.bare-submodule.deep = mkOption {
+    type = types.int;
+    default = 2;
+  };
+}

lib/tests/modules/declare-bare-submodule-nested-option.nix

@@ -0,0 +1,19 @@
+{ config, lib, ... }:
+let
+  inherit (lib) mkOption types;
+in
+{
+  options.bare-submodule = mkOption {
+    type = types.submoduleWith {
+      shorthandOnlyDefinesConfig = config.shorthandOnlyDefinesConfig;
+      modules = [
+        {
+          options.nested = mkOption {
+            type = types.int;
+            default = 1;
+          };
+        }
+      ];
+    };
+  };
+}

lib/tests/modules/declare-bare-submodule.nix

@@ -0,0 +1,18 @@
+{ config, lib, ... }:
+let
+  inherit (lib) mkOption types;
+in
+{
+  options.bare-submodule = mkOption {
+    type = types.submoduleWith {
+      modules = [ ];
+      shorthandOnlyDefinesConfig = config.shorthandOnlyDefinesConfig;
+    };
+    default = {};
+  };
+
+  # config-dependent options: won't recommend, but useful for making this test parameterized
+  options.shorthandOnlyDefinesConfig = mkOption {
+    default = false;
+  };
+}

lib/tests/modules/declare-set.nix

@@ -0,0 +1,12 @@
+{ lib, ... }:
+
+{
+  options.set = lib.mkOption {
+    default = { };
+    example = { a = 1; };
+    type = lib.types.attrsOf lib.types.int;
+    description = ''
+      Some descriptive text
+    '';
+  };
+}

lib/tests/modules/define-bare-submodule-values.nix

@@ -0,0 +1,4 @@
+{
+  bare-submodule.nested = 42;
+  bare-submodule.deep = 420;
+}

lib/tests/modules/define-shorthandOnlyDefinesConfig-true.nix

@@ -0,0 +1 @@
+{ shorthandOnlyDefinesConfig = true; }

lib/types.nix

@@ -562,14 +562,18 @@ rec {
       let
         inherit (lib.modules) evalModules;
 
-        coerce = unify: value: if isFunction value
-          then setFunctionArgs (args: unify (value args)) (functionArgs value)
-          else unify (if shorthandOnlyDefinesConfig then { config = value; } else value);
+        shorthandToModule = if shorthandOnlyDefinesConfig == false
+          then value: value
+          else value: { config = value; };
 
         allModules = defs: imap1 (n: { value, file }:
-          if isAttrs value || isFunction value then
-            # Annotate the value with the location of its definition for better error messages
-            coerce (lib.modules.unifyModuleSyntax file "${toString file}-${toString n}") value
+          if isFunction value
+          then setFunctionArgs
+                (args: lib.modules.unifyModuleSyntax file "${toString file}-${toString n}" (value args))
+                (functionArgs value)
+          else if isAttrs value
+          then
+            lib.modules.unifyModuleSyntax file "${toString file}-${toString n}" (shorthandToModule value)
           else value
         ) defs;
 
@@ -637,7 +641,11 @@ rec {
               then lhs.specialArgs // rhs.specialArgs
               else throw "A submoduleWith option is declared multiple times with the same specialArgs \"${toString (attrNames intersecting)}\"";
             shorthandOnlyDefinesConfig =
-              if lhs.shorthandOnlyDefinesConfig == rhs.shorthandOnlyDefinesConfig
+              if lhs.shorthandOnlyDefinesConfig == null
+              then rhs.shorthandOnlyDefinesConfig
+              else if rhs.shorthandOnlyDefinesConfig == null
+              then lhs.shorthandOnlyDefinesConfig
+              else if lhs.shorthandOnlyDefinesConfig == rhs.shorthandOnlyDefinesConfig
               then lhs.shorthandOnlyDefinesConfig
               else throw "A submoduleWith option is declared multiple times with conflicting shorthandOnlyDefinesConfig values";
           };