From 130a0c987830c0a6f61b21765fcab27b883b2263 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Sat, 12 Oct 2019 19:32:59 +0200 Subject: [PATCH 1/6] lib/modules: Move the isDefined check into mergedValue Without this change, accessing `mergedValue` from `mergeDefinitions` in case there are no definitions will throw an error like error: evaluation aborted with the following error message: 'This case should never happen.' This change makes it throw the appropriate error error: The option `foo' is used but not defined. This is fully backwards compatible. --- lib/modules.nix | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/lib/modules.nix b/lib/modules.nix index 559697b3d57e..228cde002db9 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -365,16 +365,9 @@ rec { else mergeDefinitions loc opt.type defs'; - - # The value with a check that it is defined - valueDefined = if res.isDefined then res.mergedValue else - # (nixos-option detects this specific error message and gives it special - # handling. If changed here, please change it there too.) - throw "The option `${showOption loc}' is used but not defined."; - # Apply the 'apply' function to the merged value. This allows options to # yield a value computed from the definitions - value = if opt ? apply then opt.apply valueDefined else valueDefined; + value = if opt ? apply then opt.apply res.mergedValue else res.mergedValue; in opt // { value = builtins.addErrorContext "while evaluating the option `${showOption loc}':" value; @@ -408,11 +401,17 @@ rec { }; defsFinal = defsFinal'.values; - # Type-check the remaining definitions, and merge them. - mergedValue = foldl' (res: def: - if type.check def.value then res - else throw "The option value `${showOption loc}' in `${def.file}' is not of type `${type.description}'.") - (type.merge loc defsFinal) defsFinal; + # Type-check the remaining definitions, and merge them. Or throw if no definitions. + mergedValue = + if isDefined then + foldl' (res: def: + if type.check def.value then res + else throw "The option value `${showOption loc}' in `${def.file}' is not of type `${type.description}'." + ) (type.merge loc defsFinal) defsFinal + else + # (nixos-option detects this specific error message and gives it special + # handling. If changed here, please change it there too.) + throw "The option `${showOption loc}' is used but not defined."; isDefined = defsFinal != []; From d5a292264121e3a3e4015a1d7fb8ccb0bb985ab7 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Mon, 14 Oct 2019 17:42:13 +0200 Subject: [PATCH 2/6] nixos/doc: Note that attrsOf is strict in its values --- nixos/doc/manual/development/option-types.xml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/nixos/doc/manual/development/option-types.xml b/nixos/doc/manual/development/option-types.xml index 1ec7e3efad71..e4f8396a4bdc 100644 --- a/nixos/doc/manual/development/option-types.xml +++ b/nixos/doc/manual/development/option-types.xml @@ -352,6 +352,11 @@ An attribute set of where all the values are of t type. Multiple definitions result in the joined attribute set. + + This type is strict in its values, which in turn + means attributes cannot depend on other attributes. See + types.lazyAttrsOf for a lazy version. + From 4268b4f9cffc46482ece4a8e0128b8ef09ea6db2 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Fri, 13 Dec 2019 01:12:41 +0100 Subject: [PATCH 3/6] lib/types: Add emptyValue attribute to types Co-Authored-By: Robert Hensing --- lib/types.nix | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/lib/types.nix b/lib/types.nix index 4872a6766571..f406cb9204b7 100644 --- a/lib/types.nix +++ b/lib/types.nix @@ -65,6 +65,11 @@ rec { # definition values and locations (e.g. [ { file = "/foo.nix"; # value = 1; } { file = "/bar.nix"; value = 2 } ]). merge ? mergeDefaultOption + , # Whether this type has a value representing nothingness. If it does, + # this should be a value of the form { value = ; } + # If it doesn't, this should be {} + # This may be used when a value is required for `mkIf false`. This allows the extra laziness in e.g. `lazyAttrsOf`. + emptyValue ? {} , # Return a flat list of sub-options. Used to generate # documentation. getSubOptions ? prefix: {} @@ -88,7 +93,7 @@ rec { functor ? defaultFunctor name }: { _type = "option-type"; - inherit name check merge getSubOptions getSubModules substSubModules typeMerge functor; + inherit name check merge emptyValue getSubOptions getSubModules substSubModules typeMerge functor; description = if description == null then name else description; }; @@ -225,6 +230,7 @@ rec { description = "attribute set"; check = isAttrs; merge = loc: foldl' (res: def: mergeAttrs res def.value) {}; + emptyValue = { value = {}; }; }; # derivation is a reserved keyword. @@ -265,6 +271,7 @@ rec { ) def.value else throw "The option value `${showOption loc}` in `${def.file}` is not a list.") defs))); + emptyValue = { value = {}; }; getSubOptions = prefix: elemType.getSubOptions (prefix ++ ["*"]); getSubModules = elemType.getSubModules; substSubModules = m: listOf (elemType.substSubModules m); @@ -273,7 +280,10 @@ rec { nonEmptyListOf = elemType: let list = addCheck (types.listOf elemType) (l: l != []); - in list // { description = "non-empty " + list.description; }; + in list // { + description = "non-empty " + list.description; + # Note: emptyValue is left as is, because another module may define an element. + }; attrsOf = elemType: mkOptionType rec { name = "attrsOf"; @@ -285,6 +295,7 @@ rec { ) # Push down position info. (map (def: mapAttrs (n: v: { inherit (def) file; value = v; }) def.value) defs))); + emptyValue = { value = {}; }; getSubOptions = prefix: elemType.getSubOptions (prefix ++ [""]); getSubModules = elemType.getSubModules; substSubModules = m: attrsOf (elemType.substSubModules m); @@ -339,6 +350,7 @@ rec { description = "list or attribute set of ${elemType.description}s"; check = x: isList x || isAttrs x; merge = loc: defs: attrOnly.merge loc (convertAllLists loc defs); + emptyValue = { value = {}; }; getSubOptions = prefix: elemType.getSubOptions (prefix ++ [""]); getSubModules = elemType.getSubModules; substSubModules = m: loaOf (elemType.substSubModules m); @@ -350,6 +362,7 @@ rec { name = "uniq"; inherit (elemType) description check; merge = mergeOneOption; + emptyValue = elemType.emptyValue; getSubOptions = elemType.getSubOptions; getSubModules = elemType.getSubModules; substSubModules = m: uniq (elemType.substSubModules m); @@ -367,6 +380,7 @@ rec { else if nrNulls != 0 then throw "The option `${showOption loc}` is defined both null and not null, in ${showFiles (getFiles defs)}." else elemType.merge loc defs; + emptyValue = { value = null; }; getSubOptions = elemType.getSubOptions; getSubModules = elemType.getSubModules; substSubModules = m: nullOr (elemType.substSubModules m); @@ -407,6 +421,7 @@ rec { args.name = last loc; prefix = loc; }).config; + emptyValue = { value = {}; }; getSubOptions = prefix: (evalModules { inherit modules prefix specialArgs; # This is a work-around due to the fact that some sub-modules, @@ -515,6 +530,7 @@ rec { if finalType.check val then val else coerceFunc val; in finalType.merge loc (map (def: def // { value = coerceVal def.value; }) defs); + emptyValue = finalType.emptyValue; getSubOptions = finalType.getSubOptions; getSubModules = finalType.getSubModules; substSubModules = m: coercedTo coercedType coerceFunc (finalType.substSubModules m); From b48717d1eb9d4d9d60f3460274e7d9a961a402df Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Sat, 12 Oct 2019 21:18:53 +0200 Subject: [PATCH 4/6] lib/types: Introduce lazyAttrsOf The standard attrsOf is strict in its *values*, meaning it's impossible to access only one attribute value without evaluating all others as well. lazyAttrsOf is a version that doesn't have that problem, at the expense of conditional definitions not properly working anymore. --- lib/types.nix | 24 ++++++++++++++++++ nixos/doc/manual/development/option-types.xml | 25 +++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/lib/types.nix b/lib/types.nix index f406cb9204b7..e86f6d364761 100644 --- a/lib/types.nix +++ b/lib/types.nix @@ -302,6 +302,30 @@ rec { functor = (defaultFunctor name) // { wrapped = elemType; }; }; + # A version of attrsOf that's lazy in its values at the expense of + # conditional definitions not working properly. E.g. defining a value with + # `foo.attr = mkIf false 10`, then `foo ? attr == true`, whereas with + # attrsOf it would correctly be `false`. Accessing `foo.attr` would throw an + # error that it's not defined. Use only if conditional definitions don't make sense. + lazyAttrsOf = elemType: mkOptionType rec { + name = "lazyAttrsOf"; + description = "lazy attribute set of ${elemType.description}s"; + check = isAttrs; + merge = loc: defs: + zipAttrsWith (name: defs: + let merged = mergeDefinitions (loc ++ [name]) elemType defs; + # mergedValue will trigger an appropriate error when accessed + in merged.optionalValue.value or elemType.emptyValue.value or merged.mergedValue + ) + # Push down position info. + (map (def: mapAttrs (n: v: { inherit (def) file; value = v; }) def.value) defs); + emptyValue = { value = {}; }; + getSubOptions = prefix: elemType.getSubOptions (prefix ++ [""]); + getSubModules = elemType.getSubModules; + substSubModules = m: lazyAttrsOf (elemType.substSubModules m); + functor = (defaultFunctor name) // { wrapped = elemType; }; + }; + # List or attribute set of ... loaOf = elemType: let diff --git a/nixos/doc/manual/development/option-types.xml b/nixos/doc/manual/development/option-types.xml index e4f8396a4bdc..55d9c123e3f1 100644 --- a/nixos/doc/manual/development/option-types.xml +++ b/nixos/doc/manual/development/option-types.xml @@ -360,6 +360,31 @@ + + + types.lazyAttrsOf t + + + + An attribute set of where all the values are of + t type. Multiple definitions result in the + joined attribute set. This is the lazy version of types.attrsOf + , allowing attributes to depend on each other. + + This version does not fully support conditional definitions! With an + option foo of this type and a definition + foo.attr = lib.mkIf false 10, evaluating + foo ? attr will return true + even though it should be false. Accessing the value will then throw + an error. For types t that have an + emptyValue defined, that value will be returned + instead of throwing an error. So if the type of foo.attr + was lazyAttrsOf (nullOr int), null + would be returned instead for the same mkIf false definition. + + + + types.loaOf t From ab10e874141503cee57a4e4e92d4e3c31be2a2a3 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Sat, 12 Oct 2019 20:37:21 +0200 Subject: [PATCH 5/6] lib/tests: Add tests for attrsOf and lazyAttrsOf --- lib/tests/modules.sh | 9 +++++++++ lib/tests/modules/attrsOf-conditional-check.nix | 7 +++++++ lib/tests/modules/attrsOf-lazy-check.nix | 7 +++++++ lib/tests/modules/declare-attrsOf.nix | 6 ++++++ lib/tests/modules/declare-lazyAttrsOf.nix | 6 ++++++ 5 files changed, 35 insertions(+) create mode 100644 lib/tests/modules/attrsOf-conditional-check.nix create mode 100644 lib/tests/modules/attrsOf-lazy-check.nix create mode 100644 lib/tests/modules/declare-attrsOf.nix create mode 100644 lib/tests/modules/declare-lazyAttrsOf.nix diff --git a/lib/tests/modules.sh b/lib/tests/modules.sh index 79d90670fb53..c8340ff7f157 100755 --- a/lib/tests/modules.sh +++ b/lib/tests/modules.sh @@ -186,6 +186,15 @@ checkConfigError 'The option .* defined in .* does not exist' config.enable ./di # Check that imports can depend on derivations checkConfigOutput "true" config.enable ./import-from-store.nix +# Check attrsOf and lazyAttrsOf. Only lazyAttrsOf should be lazy, and only +# attrsOf should work with conditional definitions +# In addition, lazyAttrsOf should honor an options emptyValue +checkConfigError "is not lazy" config.isLazy ./declare-attrsOf.nix ./attrsOf-lazy-check.nix +checkConfigOutput "true" config.isLazy ./declare-lazyAttrsOf.nix ./attrsOf-lazy-check.nix +checkConfigOutput "true" config.conditionalWorks ./declare-attrsOf.nix ./attrsOf-conditional-check.nix +checkConfigOutput "false" config.conditionalWorks ./declare-lazyAttrsOf.nix ./attrsOf-conditional-check.nix +checkConfigOutput "empty" config.value.foo ./declare-lazyAttrsOf.nix ./attrsOf-conditional-check.nix + cat < Date: Thu, 3 Oct 2019 00:12:47 +0200 Subject: [PATCH 6/6] lib/modules: Switch _module.args from attrsOf to lazyAttrsOf --- lib/modules.nix | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/modules.nix b/lib/modules.nix index 228cde002db9..e2315290ff0d 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -41,7 +41,13 @@ rec { options = { _module.args = mkOption { - type = types.attrsOf types.unspecified; + # Because things like `mkIf` are entirely useless for + # `_module.args` (because there's no way modules can check which + # arguments were passed), we'll use `lazyAttrsOf` which drops + # support for that, in turn it's lazy in its values. This means e.g. + # a `_module.args.pkgs = import (fetchTarball { ... }) {}` won't + # start a download when `pkgs` wasn't evaluated. + type = types.lazyAttrsOf types.unspecified; internal = true; description = "Arguments passed to each module."; };