From b78ba9bc68b003288d56bab62693ea28e2cdfd76 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sun, 28 Jan 2024 14:09:27 +0100 Subject: [PATCH 1/4] lib.types.unique: Check inner type deeply This doesn't change uniq. Why not? - In NixOS it seems that uniq is only used with simple types that are fully checked by t.check. - It exists for much longer and is used more widely. - I believe we should deprecate it, because unique was already better. - unique can be a proving ground. --- lib/options.nix | 33 +++++++++++++++++++++++++----- lib/tests/modules.sh | 10 +++++++++ lib/tests/modules/types-unique.nix | 27 ++++++++++++++++++++++++ lib/types.nix | 2 +- 4 files changed, 66 insertions(+), 6 deletions(-) create mode 100644 lib/tests/modules/types-unique.nix diff --git a/lib/options.nix b/lib/options.nix index 9c10dfc8b36a..03ae32d22916 100644 --- a/lib/options.nix +++ b/lib/options.nix @@ -254,13 +254,36 @@ rec { else if all isInt list && all (x: x == head list) list then head list else throw "Cannot merge definitions of `${showOption loc}'. Definition values:${showDefs defs}"; + /* + Require a single definition. + + WARNING: Does not perform nested checks, as this does not run the merge function! + */ mergeOneOption = mergeUniqueOption { message = ""; }; - mergeUniqueOption = { message }: loc: defs: - if length defs == 1 - then (head defs).value - else assert length defs > 1; - throw "The option `${showOption loc}' is defined multiple times while it's expected to be unique.\n${message}\nDefinition values:${showDefs defs}\n${prioritySuggestion}"; + /* + Require a single definition. + + NOTE: When the type is not checked completely by check, pass a merge function for further checking (of sub-attributes, etc). + */ + mergeUniqueOption = args@{ message, merge ? null }: + let + notUnique = loc: defs: + assert length defs > 1; + throw "The option `${showOption loc}' is defined multiple times while it's expected to be unique.\n${message}\nDefinition values:${showDefs defs}\n${prioritySuggestion}"; + in + if merge == null + # The inner conditional could be factored out, but this way we take advantage of partial application. + then + loc: defs: + if length defs == 1 + then (head defs).value + else notUnique loc defs + else + loc: defs: + if length defs == 1 + then merge loc defs + else notUnique loc defs; /* "Merge" option definitions by checking that they all have the same value. */ mergeEqualOption = loc: defs: diff --git a/lib/tests/modules.sh b/lib/tests/modules.sh index a90ff4ad9a2f..1221ba7143f6 100755 --- a/lib/tests/modules.sh +++ b/lib/tests/modules.sh @@ -406,6 +406,16 @@ checkConfigOutput "{}" config.submodule.a ./emptyValues.nix checkConfigError 'The option .int.a. is used but not defined' config.int.a ./emptyValues.nix checkConfigError 'The option .nonEmptyList.a. is used but not defined' config.nonEmptyList.a ./emptyValues.nix +# types.unique +# requires a single definition +checkConfigError 'The option .examples\.merged. is defined multiple times while it.s expected to be unique' config.examples.merged.a ./types-unique.nix +# user message is printed +checkConfigError 'We require a single definition, because seeing the whole value at once helps us maintain critical invariants of our system.' config.examples.merged.a ./types-unique.nix +# let the inner merge function check the values (on demand) +checkConfigError 'A definition for option .examples\.badLazyType\.a. is not of type .string.' config.examples.badLazyType.a ./types-unique.nix +# overriding still works (unlike option uniqueness) +checkConfigOutput '^"bee"$' config.examples.override.b ./types-unique.nix + ## types.raw checkConfigOutput '^true$' config.unprocessedNestingEvaluates.success ./raw.nix checkConfigOutput "10" config.processedToplevel ./raw.nix diff --git a/lib/tests/modules/types-unique.nix b/lib/tests/modules/types-unique.nix new file mode 100644 index 000000000000..115be0126975 --- /dev/null +++ b/lib/tests/modules/types-unique.nix @@ -0,0 +1,27 @@ +{ lib, ... }: +let + inherit (lib) mkOption types; +in +{ + options.examples = mkOption { + type = types.lazyAttrsOf + (types.unique + { message = "We require a single definition, because seeing the whole value at once helps us maintain critical invariants of our system."; } + (types.attrsOf types.str)); + }; + imports = [ + { examples.merged = { b = "bee"; }; } + { examples.override = lib.mkForce { b = "bee"; }; } + ]; + config.examples = { + merged = { + a = "aye"; + }; + override = { + a = "aye"; + }; + badLazyType = { + a = true; + }; + }; +} diff --git a/lib/types.nix b/lib/types.nix index cea63c598321..284c8df24f67 100644 --- a/lib/types.nix +++ b/lib/types.nix @@ -629,7 +629,7 @@ rec { unique = { message }: type: mkOptionType rec { name = "unique"; inherit (type) description descriptionClass check; - merge = mergeUniqueOption { inherit message; }; + merge = mergeUniqueOption { inherit message; inherit (type) merge; }; emptyValue = type.emptyValue; getSubOptions = type.getSubOptions; getSubModules = type.getSubModules; From 2b4a1a1d4f9db1b7007966d6205645acc5f7e57b Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Mon, 29 Jan 2024 19:13:37 +0100 Subject: [PATCH 2/4] doc/option-types: Definitions are not declared --- nixos/doc/manual/development/option-types.section.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nixos/doc/manual/development/option-types.section.md b/nixos/doc/manual/development/option-types.section.md index f9c7ac80018e..04edf99e70b0 100644 --- a/nixos/doc/manual/development/option-types.section.md +++ b/nixos/doc/manual/development/option-types.section.md @@ -326,7 +326,7 @@ Composed types are types that take a type as parameter. `listOf `types.uniq` *`t`* : Ensures that type *`t`* cannot be merged. It is used to ensure option - definitions are declared only once. + definitions are provided only once. `types.unique` `{ message = m }` *`t`* From bd285d2c11a4d906ba5a15ee2472f33dfa113546 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sun, 4 Feb 2024 15:48:27 +0100 Subject: [PATCH 3/4] lib.types.uniq: Check inner type We now reuse the `unique` type, which implements this. Keeping the duplication around would be bad at this point. --- lib/types.nix | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/lib/types.nix b/lib/types.nix index 284c8df24f67..e6353e3f43c7 100644 --- a/lib/types.nix +++ b/lib/types.nix @@ -613,18 +613,7 @@ rec { nestedTypes.elemType = elemType; }; - # Value of given type but with no merging (i.e. `uniq list`s are not concatenated). - uniq = elemType: mkOptionType rec { - name = "uniq"; - inherit (elemType) description descriptionClass check; - merge = mergeOneOption; - emptyValue = elemType.emptyValue; - getSubOptions = elemType.getSubOptions; - getSubModules = elemType.getSubModules; - substSubModules = m: uniq (elemType.substSubModules m); - functor = (defaultFunctor name) // { wrapped = elemType; }; - nestedTypes.elemType = elemType; - }; + uniq = unique { message = ""; }; unique = { message }: type: mkOptionType rec { name = "unique"; From 542f5d4f4d80a35d8f03aa5cf2a2a0b1a0345c41 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Sun, 4 Feb 2024 16:02:13 +0100 Subject: [PATCH 4/4] lib.option.mergeUniqueOption: Simplify and add warning about merge function The previous code was optimized for the old uniq behavior, which did not call merge. That's changed, so the legacy path is not a hot path anymore, and is not worth any tech debt. --- lib/options.nix | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/lib/options.nix b/lib/options.nix index 03ae32d22916..eea5a091b408 100644 --- a/lib/options.nix +++ b/lib/options.nix @@ -266,24 +266,19 @@ rec { NOTE: When the type is not checked completely by check, pass a merge function for further checking (of sub-attributes, etc). */ - mergeUniqueOption = args@{ message, merge ? null }: - let - notUnique = loc: defs: + mergeUniqueOption = args@{ + message, + # WARNING: the default merge function assumes that the definition is a valid (option) value. You MUST pass a merge function if the return value needs to be + # - type checked beyond what .check does (which should be very litte; only on the value head; not attribute values, etc) + # - if you want attribute values to be checked, or list items + # - if you want coercedTo-like behavior to work + merge ? loc: defs: (head defs).value }: + loc: defs: + if length defs == 1 + then merge loc defs + else assert length defs > 1; throw "The option `${showOption loc}' is defined multiple times while it's expected to be unique.\n${message}\nDefinition values:${showDefs defs}\n${prioritySuggestion}"; - in - if merge == null - # The inner conditional could be factored out, but this way we take advantage of partial application. - then - loc: defs: - if length defs == 1 - then (head defs).value - else notUnique loc defs - else - loc: defs: - if length defs == 1 - then merge loc defs - else notUnique loc defs; /* "Merge" option definitions by checking that they all have the same value. */ mergeEqualOption = loc: defs: