From d030e2109fd491e32cb48df54d100aa608551298 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 24 Jan 2022 15:58:17 +0100 Subject: lib.modules: Let module declare options directly in bare submodule ... where a bare submodule is an option that has a type like `submoduleWith x`, as opposed to `attrsOf (submoduleWith x)`. This makes migration unnecessary when introducing a freeform type in an existing option tree. Closes #146882 --- lib/modules.nix | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) (limited to 'lib/modules.nix') diff --git a/lib/modules.nix b/lib/modules.nix index 79d54e4a538..bde89123cd9 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -474,6 +474,18 @@ 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; } ]; + }; + }; + }; + resultsByName = mapAttrs (name: decls: # We're descending into attribute ‘name’. let @@ -493,7 +505,15 @@ rec { 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}'." + if firstOption.options.type?isSubmodule && firstOption.options.type.isSubmodule + then + let opt = fixupOptionType loc (mergeOptionDecls loc (map optionTreeToOption decls)); + in { + matchedOptions = evalOptionValue loc opt defns'; + unmatchedDefns = []; + } + else + throw "The option `${showOption loc}' in `${firstOption._file}' is a prefix of options in `${firstNonOption._file}'." else mergeModules' loc decls defns) declsByName; -- cgit 1.4.1 From 58a8a48e9d14cf397181d1223eabeb001f499049 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Thu, 24 Feb 2022 14:11:53 +0100 Subject: lib.types.submodule: Remove redundant isSubmodule attr --- lib/modules.nix | 2 +- lib/types.nix | 5 ----- 2 files changed, 1 insertion(+), 6 deletions(-) (limited to 'lib/modules.nix') diff --git a/lib/modules.nix b/lib/modules.nix index bde89123cd9..540eba1dd3d 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -505,7 +505,7 @@ rec { firstOption = findFirst (m: isOption m.options) "" decls; firstNonOption = findFirst (m: !isOption m.options) "" decls; in - if firstOption.options.type?isSubmodule && firstOption.options.type.isSubmodule + if firstOption.options.type.name == "submodule" then let opt = fixupOptionType loc (mergeOptionDecls loc (map optionTreeToOption decls)); in { diff --git a/lib/types.nix b/lib/types.nix index 51046c2c31b..3fcac9c31b3 100644 --- a/lib/types.nix +++ b/lib/types.nix @@ -642,11 +642,6 @@ rec { else throw "A submoduleWith option is declared multiple times with conflicting shorthandOnlyDefinesConfig values"; }; }; - } // { - # Submodule-typed options get special treatment in order to facilitate - # certain migrations, such as the addition of freeformTypes onto - # existing option trees. - isSubmodule = true; }; # A value from a set of allowed ones. -- cgit 1.4.1 From 0c09eb343dfa186c0fbf2bb6cbc36e7a8f369ea5 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Thu, 24 Feb 2022 14:23:02 +0100 Subject: lib.modules: Refactor option scanning slightly This scans the options with fewer function calls, improving performance. It also removes a let Env from the happy flow of the new logic. --- lib/modules.nix | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) (limited to 'lib/modules.nix') diff --git a/lib/modules.nix b/lib/modules.nix index 540eba1dd3d..e6812625f98 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -9,7 +9,6 @@ let catAttrs concatLists concatMap - count elem filter findFirst @@ -492,20 +491,16 @@ rec { 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 - if firstOption.options.type.name == "submodule" + else if optionDecls != [] then + if (lib.head optionDecls).options.type.name == "submodule" then let opt = fixupOptionType loc (mergeOptionDecls loc (map optionTreeToOption decls)); in { @@ -513,7 +508,10 @@ rec { unmatchedDefns = []; } else - throw "The option `${showOption loc}' in `${firstOption._file}' is a prefix of options in `${firstNonOption._file}'." + let + firstNonOption = findFirst (m: !isOption m.options) "" decls; + in + throw "The option `${showOption loc}' in `${(lib.head optionDecls)._file}' is a prefix of options in `${firstNonOption._file}'." else mergeModules' loc decls defns) declsByName; -- cgit 1.4.1 From 81f342d1f3a6bab4a57e195ce99a153b82b1ef86 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Thu, 24 Feb 2022 14:50:40 +0100 Subject: lib.modules: Explain why options can only be merged into submodules --- lib/modules.nix | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'lib/modules.nix') diff --git a/lib/modules.nix b/lib/modules.nix index e6812625f98..62e9615d25b 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -501,6 +501,15 @@ rec { } else if optionDecls != [] then if (lib.head optionDecls).options.type.name == "submodule" + # 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 "" as a reminder that it behaves like (b). + # d. magically combine (a) and (c). + # All options are merely syntax sugar though. then let opt = fixupOptionType loc (mergeOptionDecls loc (map optionTreeToOption decls)); in { -- cgit 1.4.1 From 11537c9c0239dc4ae52477faa78a4a0a7bdf206c Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 28 Feb 2022 22:39:56 +0100 Subject: lib.modules: Improve option-is-prefix error message --- lib/modules.nix | 24 +++++++++++++++++++++--- lib/tests/modules.sh | 6 ++++++ lib/tests/modules/declare-set.nix | 12 ++++++++++++ 3 files changed, 39 insertions(+), 3 deletions(-) create mode 100644 lib/tests/modules/declare-set.nix (limited to 'lib/modules.nix') diff --git a/lib/modules.nix b/lib/modules.nix index 62e9615d25b..7d9c55f9a15 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -9,6 +9,7 @@ let catAttrs concatLists concatMap + concatStringsSep elem filter findFirst @@ -46,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 { @@ -500,7 +515,7 @@ rec { unmatchedDefns = []; } else if optionDecls != [] then - if (lib.head optionDecls).options.type.name == "submodule" + 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 @@ -509,7 +524,7 @@ rec { # b. option `foo.bar` is available in all `attrset.*` # c. reject and require "" as a reminder that it behaves like (b). # d. magically combine (a) and (c). - # All options are merely syntax sugar though. + # All of the above are merely syntax sugar though. then let opt = fixupOptionType loc (mergeOptionDecls loc (map optionTreeToOption decls)); in { @@ -519,8 +534,11 @@ rec { else let firstNonOption = findFirst (m: !isOption m.options) "" decls; + nonOptions = filter (m: !isOption m.options) decls; in - throw "The option `${showOption loc}' in `${(lib.head optionDecls)._file}' is a prefix of options in `${firstNonOption._file}'." + 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 ""}' does not support nested options.\n${ + showRawDecls loc nonOptions + }" else mergeModules' loc decls defns) declsByName; diff --git a/lib/tests/modules.sh b/lib/tests/modules.sh index 3591b8f1e6f..e903714a720 100755 --- a/lib/tests/modules.sh +++ b/lib/tests/modules.sh @@ -309,6 +309,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 diff --git a/lib/tests/modules/declare-set.nix b/lib/tests/modules/declare-set.nix new file mode 100644 index 00000000000..853418531a8 --- /dev/null +++ b/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 + ''; + }; +} -- cgit 1.4.1 From 8baea8b82cc80c6a2843045d5b554f7f65acbc4f Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 28 Feb 2022 22:57:03 +0100 Subject: lib.modules: Make option injection work when shorthandOnlyDefinesConfig --- lib/modules.nix | 1 + lib/tests/modules.sh | 1 + lib/tests/modules/declare-bare-submodule-nested-option.nix | 3 ++- lib/tests/modules/declare-bare-submodule.nix | 8 +++++++- lib/tests/modules/define-shorthandOnlyDefinesConfig-true.nix | 1 + lib/types.nix | 6 +++++- 6 files changed, 17 insertions(+), 3 deletions(-) create mode 100644 lib/tests/modules/define-shorthandOnlyDefinesConfig-true.nix (limited to 'lib/modules.nix') diff --git a/lib/modules.nix b/lib/modules.nix index 7d9c55f9a15..cc045391fcb 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -496,6 +496,7 @@ rec { options = mkOption { type = types.submoduleWith { modules = [ { options = decl.options; } ]; + shorthandOnlyDefinesConfig = null; }; }; }; diff --git a/lib/tests/modules.sh b/lib/tests/modules.sh index e903714a720..3950d83f584 100755 --- a/lib/tests/modules.sh +++ b/lib/tests/modules.sh @@ -66,6 +66,7 @@ checkConfigOutput '^1$' config.bare-submodule.nested ./declare-bare-submodule.ni 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 # Check integer types. # unsigned diff --git a/lib/tests/modules/declare-bare-submodule-nested-option.nix b/lib/tests/modules/declare-bare-submodule-nested-option.nix index 009dd4d6cb3..da125c84b25 100644 --- a/lib/tests/modules/declare-bare-submodule-nested-option.nix +++ b/lib/tests/modules/declare-bare-submodule-nested-option.nix @@ -1,10 +1,11 @@ -{ lib, ... }: +{ config, lib, ... }: let inherit (lib) mkOption types; in { options.bare-submodule = mkOption { type = types.submoduleWith { + shorthandOnlyDefinesConfig = config.shorthandOnlyDefinesConfig; modules = [ { options.nested = mkOption { diff --git a/lib/tests/modules/declare-bare-submodule.nix b/lib/tests/modules/declare-bare-submodule.nix index 298c71e3ca0..5402f4ff5a5 100644 --- a/lib/tests/modules/declare-bare-submodule.nix +++ b/lib/tests/modules/declare-bare-submodule.nix @@ -1,4 +1,4 @@ -{ lib, ... }: +{ config, lib, ... }: let inherit (lib) mkOption types; in @@ -6,7 +6,13 @@ 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; + }; } diff --git a/lib/tests/modules/define-shorthandOnlyDefinesConfig-true.nix b/lib/tests/modules/define-shorthandOnlyDefinesConfig-true.nix new file mode 100644 index 00000000000..bd3a73dce34 --- /dev/null +++ b/lib/tests/modules/define-shorthandOnlyDefinesConfig-true.nix @@ -0,0 +1 @@ +{ shorthandOnlyDefinesConfig = true; } diff --git a/lib/types.nix b/lib/types.nix index 3fcac9c31b3..bd4062d555a 100644 --- a/lib/types.nix +++ b/lib/types.nix @@ -637,7 +637,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"; }; -- cgit 1.4.1 From 6b077c47ff14cb9a4a8f5cb8986fa83ff626c732 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 28 Feb 2022 23:29:15 +0100 Subject: lib.modules: Remove redundant fixupOptionType in option injection --- lib/modules.nix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib/modules.nix') diff --git a/lib/modules.nix b/lib/modules.nix index cc045391fcb..470e3818820 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -527,7 +527,7 @@ rec { # 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)); + let opt = mergeOptionDecls loc (map optionTreeToOption decls); in { matchedOptions = evalOptionValue loc opt defns'; unmatchedDefns = []; -- cgit 1.4.1 From db08290453ae0eb7622648435bf7af187929b153 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 7 Mar 2022 10:59:03 +0100 Subject: Revert "lib.modules: Remove redundant fixupOptionType in option injection" This reverts commit 6b077c47ff14cb9a4a8f5cb8986fa83ff626c732. Thanks Infinisil for discovering this problem: > After a lot of trial and error, trying to prove why fixupOptionType should > be used here or not, I figured it out: It's needed for the sake of file > locations in error messages. --- lib/modules.nix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib/modules.nix') diff --git a/lib/modules.nix b/lib/modules.nix index 470e3818820..cc045391fcb 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -527,7 +527,7 @@ rec { # d. magically combine (a) and (c). # All of the above are merely syntax sugar though. then - let opt = mergeOptionDecls loc (map optionTreeToOption decls); + let opt = fixupOptionType loc (mergeOptionDecls loc (map optionTreeToOption decls)); in { matchedOptions = evalOptionValue loc opt defns'; unmatchedDefns = []; -- cgit 1.4.1 From e162ed8a142d6ff53b7d03018bfd95a3a044bd06 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 7 Mar 2022 11:02:02 +0100 Subject: lib/modules.nix: Move comment to the actual legacy code --- lib/modules.nix | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'lib/modules.nix') diff --git a/lib/modules.nix b/lib/modules.nix index cc045391fcb..5d1532af623 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -799,13 +799,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 -- cgit 1.4.1 From c4b38702e59ea156924d3297e3f7ec80f7f816cb Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 7 Mar 2022 11:23:24 +0100 Subject: lib/modules.nix: Add comment about internal shorthand null value --- lib/modules.nix | 3 +++ 1 file changed, 3 insertions(+) (limited to 'lib/modules.nix') diff --git a/lib/modules.nix b/lib/modules.nix index 5d1532af623..25cd5921dec 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -496,6 +496,9 @@ rec { 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; }; }; -- cgit 1.4.1