From d1e18a369a6c89d150a503c599d11674fa18a581 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 11 Jul 2023 11:37:56 +0200 Subject: [PATCH 01/11] lib/modules.nix: Inline byName byName is not an abstraction. This is the first commit in a series that refactors it away. --- lib/modules.nix | 70 ++++++++++++++++++++++++++++++------------------- 1 file changed, 43 insertions(+), 27 deletions(-) diff --git a/lib/modules.nix b/lib/modules.nix index 0bedd28e877e..730d30576f1d 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -539,28 +539,8 @@ let mergeModules' = prefix: options: configs: let - /* byName is like foldAttrs, but will look for attributes to merge in the - specified attribute name. - - byName "foo" (module: value: ["module.hidden=${module.hidden},value=${value}"]) - [ - { - hidden="baz"; - foo={qux="bar"; gla="flop";}; - } - { - hidden="fli"; - foo={qux="gne"; gli="flip";}; - } - ] - ===> - { - gla = [ "module.hidden=baz,value=flop" ]; - gli = [ "module.hidden=fli,value=flip" ]; - qux = [ "module.hidden=baz,value=bar" "module.hidden=fli,value=gne" ]; - } - */ - byName = attr: f: modules: + # an attrset 'name' => list of submodules that declare ‘name’. + declsByName = (attr: f: modules: zipAttrsWith (n: concatLists) (map (module: let subtree = module.${attr}; in if !(builtins.isAttrs subtree) then @@ -579,17 +559,53 @@ let '') else mapAttrs (n: f module) subtree - ) modules); - # an attrset 'name' => list of submodules that declare ‘name’. - declsByName = byName "options" (module: option: + ) modules)) "options" (module: option: [{ inherit (module) _file; options = option; }] ) options; # an attrset 'name' => list of submodules that define ‘name’. - defnsByName = byName "config" (module: value: + defnsByName = (attr: f: modules: + zipAttrsWith (n: concatLists) + (map (module: let subtree = module.${attr}; in + if !(builtins.isAttrs subtree) then + throw (if attr == "config" then '' + You're trying to define a value of type `${builtins.typeOf subtree}' + rather than an attribute set for the option + `${builtins.concatStringsSep "." prefix}'! + + This usually happens if `${builtins.concatStringsSep "." prefix}' has option + definitions inside that are not matched. Please check how to properly define + this option by e.g. referring to `man 5 configuration.nix'! + '' else '' + An option declaration for `${builtins.concatStringsSep "." prefix}' has type + `${builtins.typeOf subtree}' rather than an attribute set. + Did you mean to define this outside of `options'? + '') + else + mapAttrs (n: f module) subtree + ) modules)) "config" (module: value: map (config: { inherit (module) file; inherit config; }) (pushDownProperties value) ) configs; # extract the definitions for each loc - defnsByName' = byName "config" (module: value: + defnsByName' = (attr: f: modules: + zipAttrsWith (n: concatLists) + (map (module: let subtree = module.${attr}; in + if !(builtins.isAttrs subtree) then + throw (if attr == "config" then '' + You're trying to define a value of type `${builtins.typeOf subtree}' + rather than an attribute set for the option + `${builtins.concatStringsSep "." prefix}'! + + This usually happens if `${builtins.concatStringsSep "." prefix}' has option + definitions inside that are not matched. Please check how to properly define + this option by e.g. referring to `man 5 configuration.nix'! + '' else '' + An option declaration for `${builtins.concatStringsSep "." prefix}' has type + `${builtins.typeOf subtree}' rather than an attribute set. + Did you mean to define this outside of `options'? + '') + else + mapAttrs (n: f module) subtree + ) modules)) "config" (module: value: [{ inherit (module) file; inherit value; }] ) configs; From c70a5e922322f081c6cfb249a6cf4fbf291e53ee Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 11 Jul 2023 11:43:16 +0200 Subject: [PATCH 02/11] lib/modules.nix: Apply argument `attr` of old byName --- lib/modules.nix | 46 +++++++++++++++------------------------------- 1 file changed, 15 insertions(+), 31 deletions(-) diff --git a/lib/modules.nix b/lib/modules.nix index 730d30576f1d..84715a5b1bb8 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -540,34 +540,26 @@ let mergeModules' = prefix: options: configs: let # an attrset 'name' => list of submodules that declare ‘name’. - declsByName = (attr: f: modules: + declsByName = (f: modules: zipAttrsWith (n: concatLists) - (map (module: let subtree = module.${attr}; in + (map (module: let subtree = module.options; in if !(builtins.isAttrs subtree) then - throw (if attr == "config" then '' - You're trying to define a value of type `${builtins.typeOf subtree}' - rather than an attribute set for the option - `${builtins.concatStringsSep "." prefix}'! - - This usually happens if `${builtins.concatStringsSep "." prefix}' has option - definitions inside that are not matched. Please check how to properly define - this option by e.g. referring to `man 5 configuration.nix'! - '' else '' + throw '' An option declaration for `${builtins.concatStringsSep "." prefix}' has type `${builtins.typeOf subtree}' rather than an attribute set. Did you mean to define this outside of `options'? - '') + '' else mapAttrs (n: f module) subtree - ) modules)) "options" (module: option: + ) modules)) (module: option: [{ inherit (module) _file; options = option; }] ) options; # an attrset 'name' => list of submodules that define ‘name’. - defnsByName = (attr: f: modules: + defnsByName = (f: modules: zipAttrsWith (n: concatLists) - (map (module: let subtree = module.${attr}; in + (map (module: let subtree = module.config; in if !(builtins.isAttrs subtree) then - throw (if attr == "config" then '' + throw '' You're trying to define a value of type `${builtins.typeOf subtree}' rather than an attribute set for the option `${builtins.concatStringsSep "." prefix}'! @@ -575,22 +567,18 @@ let This usually happens if `${builtins.concatStringsSep "." prefix}' has option definitions inside that are not matched. Please check how to properly define this option by e.g. referring to `man 5 configuration.nix'! - '' else '' - An option declaration for `${builtins.concatStringsSep "." prefix}' has type - `${builtins.typeOf subtree}' rather than an attribute set. - Did you mean to define this outside of `options'? - '') + '' else mapAttrs (n: f module) subtree - ) modules)) "config" (module: value: + ) modules)) (module: value: map (config: { inherit (module) file; inherit config; }) (pushDownProperties value) ) configs; # extract the definitions for each loc - defnsByName' = (attr: f: modules: + defnsByName' = (f: modules: zipAttrsWith (n: concatLists) - (map (module: let subtree = module.${attr}; in + (map (module: let subtree = module.config; in if !(builtins.isAttrs subtree) then - throw (if attr == "config" then '' + throw '' You're trying to define a value of type `${builtins.typeOf subtree}' rather than an attribute set for the option `${builtins.concatStringsSep "." prefix}'! @@ -598,14 +586,10 @@ let This usually happens if `${builtins.concatStringsSep "." prefix}' has option definitions inside that are not matched. Please check how to properly define this option by e.g. referring to `man 5 configuration.nix'! - '' else '' - An option declaration for `${builtins.concatStringsSep "." prefix}' has type - `${builtins.typeOf subtree}' rather than an attribute set. - Did you mean to define this outside of `options'? - '') + '' else mapAttrs (n: f module) subtree - ) modules)) "config" (module: value: + ) modules)) (module: value: [{ inherit (module) file; inherit value; }] ) configs; From 65de18210d67a513ffd90d17b91738b597476f7a Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 11 Jul 2023 11:46:12 +0200 Subject: [PATCH 03/11] lib/modules.nix: Apply argument `f` of old old byName --- lib/modules.nix | 36 +++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/lib/modules.nix b/lib/modules.nix index 84715a5b1bb8..b97d288228a8 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -540,7 +540,7 @@ let mergeModules' = prefix: options: configs: let # an attrset 'name' => list of submodules that declare ‘name’. - declsByName = (f: modules: + declsByName = (modules: zipAttrsWith (n: concatLists) (map (module: let subtree = module.options; in if !(builtins.isAttrs subtree) then @@ -550,12 +550,14 @@ let Did you mean to define this outside of `options'? '' else - mapAttrs (n: f module) subtree - ) modules)) (module: option: - [{ inherit (module) _file; options = option; }] - ) options; + mapAttrs + (n: (module: option: + [{ inherit (module) _file; options = option; }] + ) module) + subtree + ) modules)) options; # an attrset 'name' => list of submodules that define ‘name’. - defnsByName = (f: modules: + defnsByName = (modules: zipAttrsWith (n: concatLists) (map (module: let subtree = module.config; in if !(builtins.isAttrs subtree) then @@ -569,12 +571,14 @@ let this option by e.g. referring to `man 5 configuration.nix'! '' else - mapAttrs (n: f module) subtree - ) modules)) (module: value: - map (config: { inherit (module) file; inherit config; }) (pushDownProperties value) - ) configs; + mapAttrs + (n: (module: value: + map (config: { inherit (module) file; inherit config; }) (pushDownProperties value) + ) module) + subtree + ) modules)) configs; # extract the definitions for each loc - defnsByName' = (f: modules: + defnsByName' = (modules: zipAttrsWith (n: concatLists) (map (module: let subtree = module.config; in if !(builtins.isAttrs subtree) then @@ -588,10 +592,12 @@ let this option by e.g. referring to `man 5 configuration.nix'! '' else - mapAttrs (n: f module) subtree - ) modules)) (module: value: - [{ inherit (module) file; inherit value; }] - ) configs; + mapAttrs + (n: (module: value: + [{ inherit (module) file; inherit value; }] + ) module) + subtree + ) modules)) configs; # Convert an option tree decl to a submodule option decl optionTreeToOption = decl: From eb410eab821b013017196cc39a401c030051937c Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 11 Jul 2023 11:48:24 +0200 Subject: [PATCH 04/11] lib/modules.nix: Apply argument `modules` of old old old byName --- lib/modules.nix | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/modules.nix b/lib/modules.nix index b97d288228a8..2b05e8726255 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -540,7 +540,7 @@ let mergeModules' = prefix: options: configs: let # an attrset 'name' => list of submodules that declare ‘name’. - declsByName = (modules: + declsByName = zipAttrsWith (n: concatLists) (map (module: let subtree = module.options; in if !(builtins.isAttrs subtree) then @@ -555,9 +555,9 @@ let [{ inherit (module) _file; options = option; }] ) module) subtree - ) modules)) options; + ) options); # an attrset 'name' => list of submodules that define ‘name’. - defnsByName = (modules: + defnsByName = zipAttrsWith (n: concatLists) (map (module: let subtree = module.config; in if !(builtins.isAttrs subtree) then @@ -576,9 +576,9 @@ let map (config: { inherit (module) file; inherit config; }) (pushDownProperties value) ) module) subtree - ) modules)) configs; + ) configs); # extract the definitions for each loc - defnsByName' = (modules: + defnsByName' = zipAttrsWith (n: concatLists) (map (module: let subtree = module.config; in if !(builtins.isAttrs subtree) then @@ -597,7 +597,7 @@ let [{ inherit (module) file; inherit value; }] ) module) subtree - ) modules)) configs; + ) configs); # Convert an option tree decl to a submodule option decl optionTreeToOption = decl: From fb988c6193788f57ab4d53d8cbc8c014e88952ae Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 11 Jul 2023 11:50:03 +0200 Subject: [PATCH 05/11] lib/modules.nix: Apply argument `module` of old f --- lib/modules.nix | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/modules.nix b/lib/modules.nix index 2b05e8726255..0f416f099b8f 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -551,9 +551,9 @@ let '' else mapAttrs - (n: (module: option: - [{ inherit (module) _file; options = option; }] - ) module) + (n: option: + [{ inherit (module) _file; options = option; }] + ) subtree ) options); # an attrset 'name' => list of submodules that define ‘name’. @@ -572,9 +572,9 @@ let '' else mapAttrs - (n: (module: value: - map (config: { inherit (module) file; inherit config; }) (pushDownProperties value) - ) module) + (n: value: + map (config: { inherit (module) file; inherit config; }) (pushDownProperties value) + ) subtree ) configs); # extract the definitions for each loc @@ -593,9 +593,9 @@ let '' else mapAttrs - (n: (module: value: - [{ inherit (module) file; inherit value; }] - ) module) + (n: value: + [{ inherit (module) file; inherit value; }] + ) subtree ) configs); From 448b153f819c1fc43db5954ee9788f6f51b93b27 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 11 Jul 2023 11:57:39 +0200 Subject: [PATCH 06/11] lib/modules.nix: Rename defnsByName' -> rawDefinitionsByName --- lib/modules.nix | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/modules.nix b/lib/modules.nix index 0f416f099b8f..14efb39b8a00 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -578,7 +578,7 @@ let subtree ) configs); # extract the definitions for each loc - defnsByName' = + rawDefinitionsByName = zipAttrsWith (n: concatLists) (map (module: let subtree = module.config; in if !(builtins.isAttrs subtree) then @@ -620,7 +620,7 @@ let let loc = prefix ++ [name]; defns = defnsByName.${name} or []; - defns' = defnsByName'.${name} or []; + defns' = rawDefinitionsByName.${name} or []; optionDecls = filter (m: isOption m.options) decls; in if length optionDecls == length decls then @@ -663,7 +663,7 @@ let # Propagate all unmatched definitions from nested option sets mapAttrs (n: v: v.unmatchedDefns) resultsByName # Plus the definitions for the current prefix that don't have a matching option - // removeAttrs defnsByName' (attrNames matchedOptions); + // removeAttrs rawDefinitionsByName (attrNames matchedOptions); in { inherit matchedOptions; From c28dd7d9210731257775e8bd8722e27eb260c00e Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 11 Jul 2023 12:04:50 +0200 Subject: [PATCH 07/11] lib/modules.nix: Rename defnsByName -> pushedDownDefinitionsByName --- lib/modules.nix | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/modules.nix b/lib/modules.nix index 14efb39b8a00..18d3f23b1c19 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -557,7 +557,7 @@ let subtree ) options); # an attrset 'name' => list of submodules that define ‘name’. - defnsByName = + pushedDownDefinitionsByName = zipAttrsWith (n: concatLists) (map (module: let subtree = module.config; in if !(builtins.isAttrs subtree) then @@ -619,7 +619,7 @@ let # We're descending into attribute ‘name’. let loc = prefix ++ [name]; - defns = defnsByName.${name} or []; + defns = pushedDownDefinitionsByName.${name} or []; defns' = rawDefinitionsByName.${name} or []; optionDecls = filter (m: isOption m.options) decls; in From 6acc3114c30564d112c2c83836c9eb0685165d9a Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 11 Jul 2023 12:18:41 +0200 Subject: [PATCH 08/11] lib/modules.nix: Make entire definition list strict in config check This is a non-trivial refactor that slightly changes the semantics of the internal definition lists. Whereas previously only individual list items would trigger the exception, now the error is promoted to the whole list. This is mostly ok, because we compute the value, it is wrong to ignore a definition. However, we don't always compute the value. For instance `readOnly` only needs to count definitions. That won't be possible anymore when the error is raised for one of the items. As a consequence, an error will be raised for the errant definition instead of the number of definitions. --- lib/modules.nix | 45 +++++++++++++++++++++------------------------ 1 file changed, 21 insertions(+), 24 deletions(-) diff --git a/lib/modules.nix b/lib/modules.nix index 18d3f23b1c19..5ae8bd1a4f72 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -556,48 +556,45 @@ let ) subtree ) options); + + # The root of any module definition must be an attrset. + checkedConfigs = + assert + lib.all + (c: + isAttrs c.config || throw '' + You're trying to define a value of type `${builtins.typeOf c.config}' + rather than an attribute set for the option + `${builtins.concatStringsSep "." prefix}'! + + This usually happens if `${builtins.concatStringsSep "." prefix}' has option + definitions inside that are not matched. Please check how to properly define + this option by e.g. referring to `man 5 configuration.nix'! + '' + ) + configs; + configs; + # an attrset 'name' => list of submodules that define ‘name’. pushedDownDefinitionsByName = zipAttrsWith (n: concatLists) (map (module: let subtree = module.config; in - if !(builtins.isAttrs subtree) then - throw '' - You're trying to define a value of type `${builtins.typeOf subtree}' - rather than an attribute set for the option - `${builtins.concatStringsSep "." prefix}'! - - This usually happens if `${builtins.concatStringsSep "." prefix}' has option - definitions inside that are not matched. Please check how to properly define - this option by e.g. referring to `man 5 configuration.nix'! - '' - else mapAttrs (n: value: map (config: { inherit (module) file; inherit config; }) (pushDownProperties value) ) subtree - ) configs); + ) checkedConfigs); # extract the definitions for each loc rawDefinitionsByName = zipAttrsWith (n: concatLists) (map (module: let subtree = module.config; in - if !(builtins.isAttrs subtree) then - throw '' - You're trying to define a value of type `${builtins.typeOf subtree}' - rather than an attribute set for the option - `${builtins.concatStringsSep "." prefix}'! - - This usually happens if `${builtins.concatStringsSep "." prefix}' has option - definitions inside that are not matched. Please check how to properly define - this option by e.g. referring to `man 5 configuration.nix'! - '' - else mapAttrs (n: value: [{ inherit (module) file; inherit value; }] ) subtree - ) configs); + ) checkedConfigs); # Convert an option tree decl to a submodule option decl optionTreeToOption = decl: From 4dd51a9acec772931976d325c7021b7156c13335 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 11 Jul 2023 12:20:25 +0200 Subject: [PATCH 09/11] lib/modules.nix: Inline single-use `subtree` bindings --- lib/modules.nix | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/modules.nix b/lib/modules.nix index 5ae8bd1a4f72..0320a5d02c10 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -578,22 +578,22 @@ let # an attrset 'name' => list of submodules that define ‘name’. pushedDownDefinitionsByName = zipAttrsWith (n: concatLists) - (map (module: let subtree = module.config; in + (map (module: mapAttrs (n: value: map (config: { inherit (module) file; inherit config; }) (pushDownProperties value) ) - subtree + module.config ) checkedConfigs); # extract the definitions for each loc rawDefinitionsByName = zipAttrsWith (n: concatLists) - (map (module: let subtree = module.config; in + (map (module: mapAttrs (n: value: [{ inherit (module) file; inherit value; }] ) - subtree + module.config ) checkedConfigs); # Convert an option tree decl to a submodule option decl From 8f700580b984290adefa4b32ce1abafda04af6cf Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 11 Jul 2023 12:38:49 +0200 Subject: [PATCH 10/11] lib/modules.nix: Format --- lib/modules.nix | 47 ++++++++++++++++++++++++++++------------------- 1 file changed, 28 insertions(+), 19 deletions(-) diff --git a/lib/modules.nix b/lib/modules.nix index 0320a5d02c10..de0fcce6ef42 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -541,8 +541,10 @@ let let # an attrset 'name' => list of submodules that declare ‘name’. declsByName = - zipAttrsWith (n: concatLists) - (map (module: let subtree = module.options; in + zipAttrsWith + (n: concatLists) + (map + (module: let subtree = module.options; in if !(builtins.isAttrs subtree) then throw '' An option declaration for `${builtins.concatStringsSep "." prefix}' has type @@ -555,7 +557,8 @@ let [{ inherit (module) _file; options = option; }] ) subtree - ) options); + ) + options); # The root of any module definition must be an attrset. checkedConfigs = @@ -577,24 +580,30 @@ let # an attrset 'name' => list of submodules that define ‘name’. pushedDownDefinitionsByName = - zipAttrsWith (n: concatLists) - (map (module: - mapAttrs - (n: value: - map (config: { inherit (module) file; inherit config; }) (pushDownProperties value) - ) - module.config - ) checkedConfigs); + zipAttrsWith + (n: concatLists) + (map + (module: + mapAttrs + (n: value: + map (config: { inherit (module) file; inherit config; }) (pushDownProperties value) + ) + module.config + ) + checkedConfigs); # extract the definitions for each loc rawDefinitionsByName = - zipAttrsWith (n: concatLists) - (map (module: - mapAttrs - (n: value: - [{ inherit (module) file; inherit value; }] - ) - module.config - ) checkedConfigs); + zipAttrsWith + (n: concatLists) + (map + (module: + mapAttrs + (n: value: + [{ inherit (module) file; inherit value; }] + ) + module.config + ) + checkedConfigs); # Convert an option tree decl to a submodule option decl optionTreeToOption = decl: From 8014460c4dbf26d6f962c01920b2c25fc00e0d03 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Tue, 11 Jul 2023 12:48:16 +0200 Subject: [PATCH 11/11] lib.mergeModules: Add context to error message --- lib/modules.nix | 5 ++++- lib/tests/modules.sh | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/modules.nix b/lib/modules.nix index de0fcce6ef42..f16df20425ef 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -565,8 +565,11 @@ let assert lib.all (c: + # TODO: I have my doubts that this error would occur when option definitions are not matched. + # The implementation of this check used to be tied to a superficially similar check for + # options, so maybe that's why this is here. isAttrs c.config || throw '' - You're trying to define a value of type `${builtins.typeOf c.config}' + In module `${c.file}', you're trying to define a value of type `${builtins.typeOf c.config}' rather than an attribute set for the option `${builtins.concatStringsSep "." prefix}'! diff --git a/lib/tests/modules.sh b/lib/tests/modules.sh index f5f0701c63a4..50f24c09ca40 100755 --- a/lib/tests/modules.sh +++ b/lib/tests/modules.sh @@ -207,7 +207,7 @@ checkConfigOutput '^"foo"$' config.submodule.foo ./declare-submoduleWith-special ## shorthandOnlyDefines config behaves as expected checkConfigOutput '^true$' config.submodule.config ./declare-submoduleWith-shorthand.nix ./define-submoduleWith-shorthand.nix checkConfigError 'is not of type `boolean' config.submodule.config ./declare-submoduleWith-shorthand.nix ./define-submoduleWith-noshorthand.nix -checkConfigError "You're trying to define a value of type \`bool'\n\s*rather than an attribute set for the option" config.submodule.config ./declare-submoduleWith-noshorthand.nix ./define-submoduleWith-shorthand.nix +checkConfigError "In module ..*define-submoduleWith-shorthand.nix., you're trying to define a value of type \`bool'\n\s*rather than an attribute set for the option" config.submodule.config ./declare-submoduleWith-noshorthand.nix ./define-submoduleWith-shorthand.nix checkConfigOutput '^true$' config.submodule.config ./declare-submoduleWith-noshorthand.nix ./define-submoduleWith-noshorthand.nix ## submoduleWith should merge all modules in one swoop