Show precise error messages in option merge failures

For instance, if time.timeZone is defined multiple times, you now get
the error message:

  error: user-thrown exception: The unique option `time.timeZone' is defined multiple times, in `/etc/nixos/configurations/misc/eelco/x11vnc.nix' and `/etc/nixos/configuration.nix'.

while previously you got:

  error: user-thrown exception: Multiple definitions of string. Only one is allowed for this option.

and only an inspection of the stack trace gave a clue as to what
option caused the problem.

lib/modules.nix

@@ -128,7 +128,7 @@ rec {
          opt.options ? apply && res ? apply ||
          opt.options ? type && res ? type
       then
-        throw "The option `${showOption loc}' in `${opt.file}' is already declared in ${concatStringsSep " and " (map (d: "`${d}'") res.declarations)}."
+        throw "The option `${showOption loc}' in `${opt.file}' is already declared in ${showFiles res.declarations}."
       else
         opt.options // res //
           { declarations = [opt.file] ++ res.declarations;
@@ -153,7 +153,7 @@ rec {
           fold (def: res:
             if opt.type.check def.value then res
             else throw "The option value `${showOption loc}' in `${def.file}' is not a ${opt.type.name}.")
-            (opt.type.merge' { prefix = loc; } (map (m: m.value) defsFinal)) defsFinal;
+            (opt.type.merge { prefix = loc; files = map (m: m.file) defsFinal; } (map (m: m.value) defsFinal)) defsFinal;
       # Finally, apply the ‘apply’ function to the merged
       # value.  This allows options to yield a value computed
       # from the definitions.

lib/options.nix

@@ -35,7 +35,7 @@ rec {
   addDefaultOptionValues = defs: opts: opts //
     builtins.listToAttrs (map (defName:
       { name = defName;
-        value = 
+        value =
           let
             defValue = builtins.getAttr defName defs;
             optValue = builtins.getAttr defName opts;
@@ -49,27 +49,26 @@ rec {
           else
             # `defValue' is an attribute set containing options.
             # So recurse.
-            if hasAttr defName opts && isAttrs optValue 
+            if hasAttr defName opts && isAttrs optValue
             then addDefaultOptionValues defValue optValue
             else addDefaultOptionValues defValue {};
       }
     ) (attrNames defs));
 
-  mergeDefaultOption = list:
+  mergeDefaultOption = args: list:
     if length list == 1 then head list
-    else if all builtins.isFunction list then x: mergeDefaultOption (map (f: f x) list)
+    else if all builtins.isFunction list then x: mergeDefaultOption args (map (f: f x) list)
     else if all isList list then concatLists list
     else if all isAttrs list then fold lib.mergeAttrs {} list
     else if all builtins.isBool list then fold lib.or false list
     else if all builtins.isString list then lib.concatStrings list
-    else if all builtins.isInt list && all (x: x == head list) list
+    else if all builtins.isInt list && all (x: x == head list) list then head list
-         then head list
+    else throw "Cannot merge definitions of `${showOption args.prefix}' given in ${showFiles args.files}.";
-    else throw "Cannot merge values.";
 
 
   /* Obsolete, will remove soon.  Specify an option type or apply
      function instead.  */
-  mergeTypedOption = typeName: predicate: merge: list:
+  mergeTypedOption = typeName: predicate: merge: args: list:
     if all predicate list then merge list
     else throw "Expect a ${typeName}.";
 
@@ -82,9 +81,10 @@ rec {
     (x: if builtins ? isString then builtins.isString x else x + "")
     lib.concatStrings;
 
-  mergeOneOption = list:
+  mergeOneOption = args: list:
     if list == [] then abort "This case should never happen."
-    else if length list != 1 then throw "Multiple definitions. Only one is allowed for this option."
+    else if length list != 1 then
+      throw "The unique option `${showOption args.prefix}' is defined multiple times, in ${showFiles args.files}."
     else head list;
 
 
@@ -135,6 +135,7 @@ rec {
 
   /* Helper functions. */
   showOption = concatStringsSep ".";
+  showFiles = files: concatStringsSep " and " (map (f: "`${f}'") files);
   unknownModule = "<unknown-file>";
 
 }

lib/types.nix

@@ -19,27 +19,24 @@ rec {
   };
 
 
-  # name (name of the type)
-  # check (check the config value)
-  # merge (default merge function)
-  # getSubOptions (returns sub-options for manual generation)
   isOptionType = isType "option-type";
   mkOptionType =
-    { name
+    { # Human-readable representation of the type.
-    , check ? (x: true)
+      name
-    , merge ? mergeDefaultOption
+    , # Function applied to each definition that should return true if
-    , merge' ? args: merge
+      # its type-correct, false otherwise.
-    , getSubOptions ? prefix: {}
+      check ? (x: true)
+    , # Merge a list of definitions together into a single value.
+      merge ? mergeDefaultOption
+    , # Return a flat list of sub-options.  Used to generate
+      # documentation.
+      getSubOptions ? prefix: {}
     }:
-
     { _type = "option-type";
-      inherit name check merge merge' getSubOptions;
+      inherit name check merge getSubOptions;
     };
 
 
-  addToPrefix = args: name: args // { prefix = args.prefix ++ [name]; };
-
-
   types = rec {
 
     unspecified = mkOptionType {
@@ -49,7 +46,7 @@ rec {
     bool = mkOptionType {
       name = "boolean";
       check = builtins.isBool;
-      merge = fold lib.or false;
+      merge = args: fold lib.or false;
     };
 
     int = mkOptionType {
@@ -60,7 +57,7 @@ rec {
     string = mkOptionType {
       name = "string";
       check = builtins.isString;
-      merge = lib.concatStrings;
+      merge = args: lib.concatStrings;
     };
 
     # Like ‘string’, but add newlines between every value.  Useful for
@@ -68,54 +65,56 @@ rec {
     lines = mkOptionType {
       name = "string";
       check = builtins.isString;
-      merge = lib.concatStringsSep "\n";
+      merge = args: lib.concatStringsSep "\n";
     };
 
     commas = mkOptionType {
       name = "string";
       check = builtins.isString;
-      merge = lib.concatStringsSep ",";
+      merge = args: lib.concatStringsSep ",";
     };
 
     envVar = mkOptionType {
       name = "environment variable";
       inherit (string) check;
-      merge = lib.concatStringsSep ":";
+      merge = args: lib.concatStringsSep ":";
     };
 
     attrs = mkOptionType {
       name = "attribute set";
       check = isAttrs;
-      merge = fold lib.mergeAttrs {};
+      merge = args: fold lib.mergeAttrs {};
     };
 
     # derivation is a reserved keyword.
     package = mkOptionType {
       name = "derivation";
       check = isDerivation;
+      merge = mergeOneOption;
     };
 
     path = mkOptionType {
       name = "path";
       # Hacky: there is no ‘isPath’ primop.
       check = x: builtins.unsafeDiscardStringContext (builtins.substring 0 1 (toString x)) == "/";
+      merge = mergeOneOption;
     };
 
     # drop this in the future:
-    list = builtins.trace "types.list is deprecated; use types.listOf instead" types.listOf;
+    list = builtins.trace "`types.list' is deprecated; use `types.listOf' instead" types.listOf;
 
     listOf = elemType: mkOptionType {
       name = "list of ${elemType.name}s";
       check = value: isList value && all elemType.check value;
-      merge' = args: defs: imap (n: def: elemType.merge' (addToPrefix args (toString n)) [def]) (concatLists defs);
+      merge = args: defs: imap (n: def: elemType.merge (addToPrefix args (toString n)) [def]) (concatLists defs);
       getSubOptions = prefix: elemType.getSubOptions (prefix ++ ["*"]);
     };
 
     attrsOf = elemType: mkOptionType {
       name = "attribute set of ${elemType.name}s";
       check = x: isAttrs x && all elemType.check (lib.attrValues x);
-      merge' = args: lib.zipAttrsWith (name:
+      merge = args: lib.zipAttrsWith (name:
-        elemType.merge' (addToPrefix (args // { inherit name; }) name));
+        elemType.merge (addToPrefix (args // { inherit name; }) name));
       getSubOptions = prefix: elemType.getSubOptions (prefix ++ ["<name>"]);
     };
 
@@ -138,43 +137,39 @@ rec {
           if isList x       then listOnly.check x
           else if isAttrs x then attrOnly.check x
           else false;
-        merge' = args: defs: attrOnly.merge' args (imap convertIfList defs);
+        merge = args: defs: attrOnly.merge args (imap convertIfList defs);
         getSubOptions = prefix: elemType.getSubOptions (prefix ++ ["<name?>"]);
       };
 
     uniq = elemType: mkOptionType {
       inherit (elemType) name check;
-      merge = list:
+      merge = mergeOneOption;
-        if length list == 1 then
-          head list
-        else
-          throw "Multiple definitions of ${elemType.name}. Only one is allowed for this option.";
       getSubOptions = elemType.getSubOptions;
     };
 
     none = elemType: mkOptionType {
       inherit (elemType) name check;
-      merge = list:
+      merge = args: list:
-        throw "No definitions are allowed for this option.";
+        throw "No definitions are allowed for the option `${showOption args.prefix}'.";
       getSubOptions = elemType.getSubOptions;
     };
 
     nullOr = elemType: mkOptionType {
       name = "null or ${elemType.name}";
       check = x: builtins.isNull x || elemType.check x;
-      merge' = args: defs:
+      merge = args: defs:
         if all isNull defs then null
         else if any isNull defs then
-          throw "Some but not all values are null."
+          throw "The option `${showOption args.prefix}' is defined both null and not null, in ${showFiles args.files}."
-        else elemType.merge' args defs;
+        else elemType.merge args defs;
       getSubOptions = elemType.getSubOptions;
     };
 
     functionTo = elemType: mkOptionType {
       name = "function that evaluates to a(n) ${elemType.name}";
       check = builtins.isFunction;
-      merge' = args: fns:
+      merge = args: fns:
-        fnArgs: elemType.merge' args (map (fn: fn fnArgs) fns);
+        fnArgs: elemType.merge args (map (fn: fn fnArgs) fns);
       getSubOptions = elemType.getSubOptions;
     };
 
@@ -183,8 +178,7 @@ rec {
       mkOptionType rec {
         name = "submodule";
         check = x: isAttrs x || builtins.isFunction x;
-        merge = merge' {};
+        merge = args: defs:
-        merge' = args: defs:
           let
             coerce = def: if builtins.isFunction def then def else { config = def; };
             modules = opts' ++ map coerce defs;
@@ -204,4 +198,8 @@ rec {
 
   };
 
+
+  /* Helper function. */
+  addToPrefix = args: name: args // { prefix = args.prefix ++ [name]; };
+
 }

nixos/modules/config/shells-environment.nix

@@ -25,11 +25,14 @@ in
       '';
       type = types.attrsOf (mkOptionType {
         name = "a string or a list of strings";
-        merge = xs:
+        merge = args: xs:
           let xs' = filterOverrides xs; in
           if isList (head xs') then concatLists xs'
-          else if builtins.lessThan 1 (length xs') then abort "variable in ‘environment.variables’ has multiple values"
+          else if builtins.lessThan 1 (length xs') then
-          else if !builtins.isString (head xs') then abort "variable in ‘environment.variables’ does not have a string value"
+            # Don't show location info here, since it's too general.
+            throw "The option `${showOption args.prefix}' is defined multiple times."
+          else if !builtins.isString (head xs') then
+            throw "The option `${showOption args.prefix}' does not have a string value."
           else head xs';
       });
       apply = mapAttrs (n: v: if isList v then concatStringsSep ":" v else v);

nixos/modules/config/sysctl.nix

@@ -7,7 +7,7 @@ let
   sysctlOption = mkOptionType {
     name = "sysctl option value";
     check = x: builtins.isBool x || builtins.isString x || builtins.isInt x;
-    merge = xs: last xs; # FIXME: hacky way to allow overriding in configuration.nix.
+    merge = args: xs: last xs; # FIXME: hacky way to allow overriding in configuration.nix.
   };
 
 in

nixos/modules/misc/nixpkgs.nix

@@ -26,7 +26,7 @@ let
   configType = mkOptionType {
     name = "nixpkgs config";
     check = traceValIfNot isConfig;
-    merge = fold mergeConfig {};
+    merge = args: fold mergeConfig {};
   };
 
 in