lib.types: Add parentheses where description is ambiguous

2 files changed, 122 insertions, 13 deletions

diff --git a/lib/tests/misc.nix b/lib/tests/misc.nix
index 584a946e92c..9b1397a7915 100644
--- a/lib/tests/misc.nix
+++ b/lib/tests/misc.nix
@@ -1206,4 +1206,57 @@ runTests {
     expr = strings.levenshteinAtMost 3 "hello" "Holla";
     expected = true;
   };
+
+  testTypeDescriptionInt = {
+    expr = (with types; int).description;
+    expected = "signed integer";
+  };
+  testTypeDescriptionListOfInt = {
+    expr = (with types; listOf int).description;
+    expected = "list of signed integer";
+  };
+  testTypeDescriptionListOfListOfInt = {
+    expr = (with types; listOf (listOf int)).description;
+    expected = "list of list of signed integer";
+  };
+  testTypeDescriptionListOfEitherStrOrBool = {
+    expr = (with types; listOf (either str bool)).description;
+    expected = "list of (string or boolean)";
+  };
+  testTypeDescriptionEitherListOfStrOrBool = {
+    expr = (with types; either (listOf bool) str).description;
+    expected = "(list of boolean) or string";
+  };
+  testTypeDescriptionEitherStrOrListOfBool = {
+    expr = (with types; either str (listOf bool)).description;
+    expected = "string or list of boolean";
+  };
+  testTypeDescriptionOneOfListOfStrOrBool = {
+    expr = (with types; oneOf [ (listOf bool) str ]).description;
+    expected = "(list of boolean) or string";
+  };
+  testTypeDescriptionOneOfListOfStrOrBoolOrNumber = {
+    expr = (with types; oneOf [ (listOf bool) str number ]).description;
+    expected = "(list of boolean) or string or signed integer or floating point number";
+  };
+  testTypeDescriptionEitherListOfBoolOrEitherStringOrNumber = {
+    expr = (with types; either (listOf bool) (either str number)).description;
+    expected = "(list of boolean) or string or signed integer or floating point number";
+  };
+  testTypeDescriptionEitherEitherListOfBoolOrStringOrNumber = {
+    expr = (with types; either (either (listOf bool) str) number).description;
+    expected = "(list of boolean) or string or signed integer or floating point number";
+  };
+  testTypeDescriptionEitherNullOrBoolOrString = {
+    expr = (with types; either (nullOr bool) str).description;
+    expected = "null or boolean or string";
+  };
+  testTypeDescriptionEitherListOfEitherBoolOrStrOrInt = {
+    expr = (with types; either (listOf (either bool str)) int).description;
+    expected = "(list of (boolean or string)) or signed integer";
+  };
+  testTypeDescriptionEitherIntOrListOrEitherBoolOrStr = {
+    expr = (with types; either int (listOf (either bool str))).description;
+    expected = "signed integer or list of (boolean or string)";
+  };
 }
diff --git a/lib/types.nix b/lib/types.nix
index f235e341992..3750ba96555 100644
--- a/lib/types.nix
+++ b/lib/types.nix
@@ -113,6 +113,12 @@ rec {
       name
     , # Description of the type, defined recursively by embedding the wrapped type if any.
       description ? null
+      # A hint for whether or not this description needs parentheses. Possible values:
+      #  - "noun": a simple noun phrase such as "positive integer"
+      #  - "conjunction": a phrase with a potentially ambiguous "or" connective.
+      #  - "composite": a phrase with an "of" connective
+      # See the `optionDescriptionPhrase` function.
+    , descriptionClass ? null
     , # Function applied to each definition that should return true if
       # its type-correct, false otherwise.
       check ? (x: true)
@@ -158,10 +164,36 @@ rec {
       nestedTypes ? {}
     }:
     { _type = "option-type";
-      inherit name check merge emptyValue getSubOptions getSubModules substSubModules typeMerge functor deprecationMessage nestedTypes;
+      inherit
+        name check merge emptyValue getSubOptions getSubModules substSubModules
+        typeMerge functor deprecationMessage nestedTypes descriptionClass;
       description = if description == null then name else description;
     };
 
+  # optionDescriptionPhrase :: (str -> bool) -> optionType -> str
+  #
+  # Helper function for producing unambiguous but readable natural language
+  # descriptions of types.
+  #
+  # Parameters
+  #
+  #     optionDescriptionPhase unparenthesize optionType
+  #
+  # `unparenthesize`: A function from descriptionClass string to boolean.
+  #   It must return true when the class of phrase will fit unambiguously into
+  #   the description of the caller.
+  #
+  # `optionType`: The option type to parenthesize or not.
+  #   The option whose description we're returning.
+  #
+  # Return value
+  #
+  # The description of the `optionType`, with parentheses if there may be an
+  # ambiguity.
+  optionDescriptionPhrase = unparenthesize: t:
+    if unparenthesize (t.descriptionClass or null)
+    then t.description
+    else "(${t.description})";
 
   # When adding new types don't forget to document them in
   # nixos/doc/manual/development/option-types.xml!
@@ -170,6 +202,7 @@ rec {
     raw = mkOptionType rec {
       name = "raw";
       description = "raw value";
+      descriptionClass = "noun";
       check = value: true;
       merge = mergeOneOption;
     };
@@ -177,6 +210,7 @@ rec {
     anything = mkOptionType {
       name = "anything";
       description = "anything";
+      descriptionClass = "noun";
       check = value: true;
       merge = loc: defs:
         let
@@ -216,12 +250,14 @@ rec {
     };
 
     unspecified = mkOptionType {
-      name = "unspecified";
+      name = "unspecified value";
+      descriptionClass = "noun";
     };
 
     bool = mkOptionType {
       name = "bool";
       description = "boolean";
+      descriptionClass = "noun";
       check = isBool;
       merge = mergeEqualOption;
     };
@@ -229,6 +265,7 @@ rec {
     int = mkOptionType {
       name = "int";
       description = "signed integer";
+      descriptionClass = "noun";
       check = isInt;
       merge = mergeEqualOption;
     };
@@ -294,6 +331,7 @@ rec {
     float = mkOptionType {
       name = "float";
       description = "floating point number";
+      descriptionClass = "noun";
       check = isFloat;
       merge = mergeEqualOption;
     };
@@ -325,6 +363,7 @@ rec {
     str = mkOptionType {
       name = "str";
       description = "string";
+      descriptionClass = "noun";
       check = isString;
       merge = mergeEqualOption;
     };
@@ -332,6 +371,7 @@ rec {
     nonEmptyStr = mkOptionType {
       name = "nonEmptyStr";
       description = "non-empty string";
+      descriptionClass = "noun";
       check = x: str.check x && builtins.match "[ \t\n]*" x == null;
       inherit (str) merge;
     };
@@ -344,6 +384,7 @@ rec {
       mkOptionType {
         name = "singleLineStr";
         description = "(optionally newline-terminated) single-line string";
+        descriptionClass = "noun";
         inherit check;
         merge = loc: defs:
           lib.removeSuffix "\n" (merge loc defs);
@@ -352,6 +393,7 @@ rec {
     strMatching = pattern: mkOptionType {
       name = "strMatching ${escapeNixString pattern}";
       description = "string matching the pattern ${pattern}";
+      descriptionClass = "noun";
       check = x: str.check x && builtins.match pattern x != null;
       inherit (str) merge;
     };
@@ -364,6 +406,7 @@ rec {
         then "Concatenated string" # for types.string.
         else "strings concatenated with ${builtins.toJSON sep}"
       ;
+      descriptionClass = "noun";
       check = isString;
       merge = loc: defs: concatStringsSep sep (getValues defs);
       functor = (defaultFunctor name) // {
@@ -387,7 +430,7 @@ rec {
 
     passwdEntry = entryType: addCheck entryType (str: !(hasInfix ":" str || hasInfix "\n" str)) // {
       name = "passwdEntry ${entryType.name}";
-      description = "${entryType.description}, not containing newlines or colons";
+      description = "${optionDescriptionPhrase (class: class == "noun") entryType}, not containing newlines or colons";
     };
 
     attrs = mkOptionType {
@@ -407,6 +450,7 @@ rec {
     #   ("/nix/store/hash-foo"). These get a context added to them using builtins.storePath.
     package = mkOptionType {
       name = "package";
+      descriptionClass = "noun";
       check = x: isDerivation x || isStorePath x;
       merge = loc: defs:
         let res = mergeOneOption loc defs;
@@ -427,7 +471,8 @@ rec {
 
     listOf = elemType: mkOptionType rec {
       name = "listOf";
-      description = "list of ${elemType.description}";
+      description = "list of ${optionDescriptionPhrase (class: class == "noun" || class == "composite") elemType}";
+      descriptionClass = "composite";
       check = isList;
       merge = loc: defs:
         map (x: x.value) (filter (x: x ? value) (concatLists (imap1 (n: def:
@@ -450,13 +495,14 @@ rec {
     nonEmptyListOf = elemType:
       let list = addCheck (types.listOf elemType) (l: l != []);
       in list // {
-        description = "non-empty " + list.description;
+        description = "non-empty ${optionDescriptionPhrase (class: class == "noun") list}";
         emptyValue = { }; # no .value attr, meaning unset
       };
 
     attrsOf = elemType: mkOptionType rec {
       name = "attrsOf";
-      description = "attribute set of ${elemType.description}";
+      description = "attribute set of ${optionDescriptionPhrase (class: class == "noun" || class == "composite") elemType}";
+      descriptionClass = "composite";
       check = isAttrs;
       merge = loc: defs:
         mapAttrs (n: v: v.value) (filterAttrs (n: v: v ? value) (zipAttrsWith (name: defs:
@@ -479,7 +525,8 @@ rec {
     # 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}";
+      description = "lazy attribute set of ${optionDescriptionPhrase (class: class == "noun" || class == "composite") elemType}";
+      descriptionClass = "composite";
       check = isAttrs;
       merge = loc: defs:
         zipAttrsWith (name: defs:
@@ -509,7 +556,7 @@ rec {
     # 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 check;
+      inherit (elemType) description descriptionClass check;
       merge = mergeOneOption;
       emptyValue = elemType.emptyValue;
       getSubOptions = elemType.getSubOptions;
@@ -521,7 +568,7 @@ rec {
 
     unique = { message }: type: mkOptionType rec {
       name = "unique";
-      inherit (type) description check;
+      inherit (type) description descriptionClass check;
       merge = mergeUniqueOption { inherit message; };
       emptyValue = type.emptyValue;
       getSubOptions = type.getSubOptions;
@@ -534,7 +581,8 @@ rec {
     # Null or value of ...
     nullOr = elemType: mkOptionType rec {
       name = "nullOr";
-      description = "null or ${elemType.description}";
+      description = "null or ${optionDescriptionPhrase (class: class == "noun" || class == "conjunction") elemType}";
+      descriptionClass = "conjunction";
       check = x: x == null || elemType.check x;
       merge = loc: defs:
         let nrNulls = count (def: def.value == null) defs; in
@@ -552,7 +600,8 @@ rec {
 
     functionTo = elemType: mkOptionType {
       name = "functionTo";
-      description = "function that evaluates to a(n) ${elemType.description}";
+      description = "function that evaluates to a(n) ${optionDescriptionPhrase (class: class == "noun" || class == "composite") elemType}";
+      descriptionClass = "composite";
       check = isFunction;
       merge = loc: defs:
         fnArgs: (mergeDefinitions (loc ++ [ "[function body]" ]) elemType (map (fn: { inherit (fn) file; value = fn.value fnArgs; }) defs)).mergedValue;
@@ -578,6 +627,7 @@ rec {
     deferredModuleWith = attrs@{ staticModules ? [] }: mkOptionType {
       name = "deferredModule";
       description = "module";
+      descriptionClass = "noun";
       check = x: isAttrs x || isFunction x || path.check x;
       merge = loc: defs: {
         imports = staticModules ++ map (def: lib.setDefaultModuleLocation "${def.file}, via option ${showOption loc}" def.value) defs;
@@ -603,6 +653,7 @@ rec {
     optionType = mkOptionType {
       name = "optionType";
       description = "optionType";
+      descriptionClass = "noun";
       check = value: value._type or null == "option-type";
       merge = loc: defs:
         if length defs == 1
@@ -749,6 +800,10 @@ rec {
             "value ${show (builtins.head values)} (singular enum)"
           else
             "one of ${concatMapStringsSep ", " show values}";
+        descriptionClass =
+          if builtins.length values < 2
+          then "noun"
+          else "conjunction";
         check = flip elem values;
         merge = mergeEqualOption;
         functor = (defaultFunctor name) // { payload = values; binOp = a: b: unique (a ++ b); };
@@ -757,7 +812,8 @@ rec {
     # Either value of type `t1` or `t2`.
     either = t1: t2: mkOptionType rec {
       name = "either";
-      description = "${t1.description} or ${t2.description}";
+      description = "${optionDescriptionPhrase (class: class == "noun" || class == "conjunction") t1} or ${optionDescriptionPhrase (class: class == "noun" || class == "conjunction" || class == "composite") t2}";
+      descriptionClass = "conjunction";
       check = x: t1.check x || t2.check x;
       merge = loc: defs:
         let
@@ -795,7 +851,7 @@ rec {
           coercedType.description})";
       mkOptionType rec {
         name = "coercedTo";
-        description = "${finalType.description} or ${coercedType.description} convertible to it";
+        description = "${optionDescriptionPhrase (class: class == "noun") finalType} or ${optionDescriptionPhrase (class: class == "noun") coercedType} convertible to it";
         check = x: (coercedType.check x && finalType.check (coerceFunc x)) || finalType.check x;
         merge = loc: defs:
           let