Show correct position info for errors in submodules

E.g.

  The unique option `fileSystems./.device' is defined multiple times, in `/etc/nixos/configuration.nix' and `/etc/nixos/foo.nix'.

This requires passing file/value tuples to the merge functions.

3 files changed, 72 insertions, 79 deletions

diff --git a/lib/modules.nix b/lib/modules.nix
index 11a2f64799f6c..040ccf682140f 100644
--- a/lib/modules.nix
+++ b/lib/modules.nix
@@ -17,7 +17,7 @@ rec {
       # Traverse options and extract the option values into the final
       # config set.  At the same time, check whether all option
       # definitions have matching declarations.
-      config = yieldConfig [] options;
+      config = yieldConfig prefix options;
       yieldConfig = prefix: set:
         let res = removeAttrs (mapAttrs (n: v:
           if isOption v then v.value
@@ -52,22 +52,22 @@ rec {
      of ‘options’, ‘config’ and ‘imports’ attributes. */
   unifyModuleSyntax = file: key: m:
     if m ? config || m ? options then
-      let badAttrs = removeAttrs m ["imports" "options" "config" "key"]; in
+      let badAttrs = removeAttrs m ["imports" "options" "config" "key" "_file"]; in
       if badAttrs != {} then
         throw "Module `${key}' has an unsupported attribute `${head (attrNames badAttrs)}'."
       else
-        { inherit file;
+        { file = m._file or file;
           key = toString m.key or key;
           imports = m.imports or [];
           options = m.options or {};
           config = m.config or {};
         }
     else
-      { inherit file;
+      { file = m._file or file;
         key = toString m.key or key;
         imports = m.require or [] ++ m.imports or [];
         options = {};
-        config = removeAttrs m ["key" "require" "imports"];
+        config = removeAttrs m ["key" "_file" "require" "imports"];
       };
 
   applyIfFunction = f: arg: if builtins.isFunction f then f arg else f;
@@ -151,7 +151,7 @@ rec {
     let
       # Process mkOverride properties, adding in the default
       # value specified in the option declaration (if any).
-      defsFinal = filterOverrides'
+      defsFinal = filterOverrides
         ((if opt ? default then [{ file = head opt.declarations; value = mkOptionDefault opt.default; }] else []) ++ defs);
       files = map (def: def.file) defsFinal;
       # Type-check the remaining definitions, and merge them if
@@ -163,7 +163,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; inherit files; } (map (m: m.value) defsFinal)) defsFinal;
+            (opt.type.merge loc defsFinal) defsFinal;
       # Finally, apply the ‘apply’ function to the merged
       # value.  This allows options to yield a value computed
       # from the definitions.
@@ -240,7 +240,7 @@ rec {
 
      Note that "z" has the default priority 100.
   */
-  filterOverrides' = defs:
+  filterOverrides = defs:
     let
       defaultPrio = 100;
       getPrio = def: if def.value._type or "" == "override" then def.value.priority else defaultPrio;
@@ -249,9 +249,6 @@ rec {
       strip = def: if def.value._type or "" == "override" then def // { value = def.value.content; } else def;
     in concatMap (def: if getPrio def == highestPrio then [(strip def)] else []) defs;
 
-  /* For use in options like environment.variables. */
-  filterOverrides = defs: map (def: def.value) (filterOverrides' (map (def: { value = def; }) defs));
-
   /* Hack for backward compatibility: convert options of type
      optionSet to configOf.  FIXME: remove eventually. */
   fixupOptionType = loc: opt:
diff --git a/lib/options.nix b/lib/options.nix
index 2b211478765de..5a05775e8c252 100644
--- a/lib/options.nix
+++ b/lib/options.nix
@@ -30,36 +30,39 @@ rec {
     type = lib.types.bool;
   };
 
-  mergeDefaultOption = args: list:
+  mergeDefaultOption = loc: defs:
+    let list = getValues defs; in
     if length list == 1 then head list
-    else if all builtins.isFunction list then x: mergeDefaultOption args (map (f: f x) list)
+    else if all builtins.isFunction list then x: mergeDefaultOption loc (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 then head list
-    else throw "Cannot merge definitions of `${showOption args.prefix}' given in ${showFiles args.files}.";
+    else throw "Cannot merge definitions of `${showOption loc}' given in ${showFiles (getFiles defs)}.";
 
   /* Obsolete, will remove soon.  Specify an option type or apply
      function instead.  */
-  mergeTypedOption = typeName: predicate: merge: args: list:
-    if all predicate list then merge list
-    else throw "Expect a ${typeName}.";
+  mergeTypedOption = typeName: predicate: merge: loc: list:
+    let list' = map (x: x.value) list; in
+    if all predicate list then merge list'
+    else throw "Expected a ${typeName}.";
 
   mergeEnableOption = mergeTypedOption "boolean"
     (x: true == x || false == x) (fold lib.or false);
 
   mergeListOption = mergeTypedOption "list" isList concatLists;
 
-  mergeStringOption = mergeTypedOption "string"
-    (x: if builtins ? isString then builtins.isString x else x + "")
-    lib.concatStrings;
+  mergeStringOption = mergeTypedOption "string" builtins.isString lib.concatStrings;
 
-  mergeOneOption = args: list:
-    if list == [] then abort "This case should never happen."
-    else if length list != 1 then
-      throw "The unique option `${showOption args.prefix}' is defined multiple times, in ${showFiles args.files}."
-    else head list;
+  mergeOneOption = loc: defs:
+    if defs == [] then abort "This case should never happen."
+    else if length defs != 1 then
+      throw "The unique option `${showOption loc}' is defined multiple times, in ${showFiles (getFiles defs)}."
+    else (head defs).value;
+
+  getValues = map (x: x.value);
+  getFiles = map (x: x.file);
 
 
   # Generate documentation template from the list of option declaration like
diff --git a/lib/types.nix b/lib/types.nix
index 8deb379c25a90..cf8eef0083375 100644
--- a/lib/types.nix
+++ b/lib/types.nix
@@ -27,6 +27,11 @@ rec {
       # its type-correct, false otherwise.
       check ? (x: true)
     , # Merge a list of definitions together into a single value.
+      # This function is called with two arguments: the location of
+      # the option in the configuration as a list of strings
+      # (e.g. ["boot" "loader "grub" "enable"]), and a list of
+      # definition values and locations (e.g. [ { file = "/foo.nix";
+      # value = 1; } { file = "/bar.nix"; value = 2 } ]).
       merge ? mergeDefaultOption
     , # Return a flat list of sub-options.  Used to generate
       # documentation.
@@ -46,12 +51,13 @@ rec {
     bool = mkOptionType {
       name = "boolean";
       check = builtins.isBool;
-      merge = args: fold lib.or false;
+      merge = loc: fold (x: lib.or x.value) false;
     };
 
     int = mkOptionType {
       name = "integer";
       check = builtins.isInt;
+      merge = mergeOneOption;
     };
 
     str = mkOptionType {
@@ -60,38 +66,26 @@ rec {
       merge = mergeOneOption;
     };
 
-    # Deprecated; should not be used because it quietly concatenates
-    # strings, which is usually not what you want.
-    string = mkOptionType {
-      name = "string";
-      check = builtins.isString;
-      merge = args: lib.concatStrings;
-    };
-
-    # Like ‘string’, but add newlines between every value.  Useful for
-    # configuration file contents.
-    lines = mkOptionType {
+    # Merge multiple definitions by concatenating them (with the given
+    # separator between the values).
+    separatedString = sep: mkOptionType {
       name = "string";
       check = builtins.isString;
-      merge = args: lib.concatStringsSep "\n";
+      merge = loc: defs: lib.concatStringsSep sep (getValues defs);
     };
 
-    commas = mkOptionType {
-      name = "string";
-      check = builtins.isString;
-      merge = args: lib.concatStringsSep ",";
-    };
+    lines = separatedString "\n";
+    commas = separatedString ",";
+    envVar = separatedString ":";
 
-    envVar = mkOptionType {
-      name = "environment variable";
-      inherit (string) check;
-      merge = args: lib.concatStringsSep ":";
-    };
+    # Deprecated; should not be used because it quietly concatenates
+    # strings, which is usually not what you want.
+    string = separatedString "";
 
     attrs = mkOptionType {
       name = "attribute set";
       check = isAttrs;
-      merge = args: fold lib.mergeAttrs {};
+      merge = loc: fold (def: lib.mergeAttrs def.value) {};
     };
 
     # derivation is a reserved keyword.
@@ -114,15 +108,21 @@ rec {
     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 = loc: defs:
+        concatLists (imap (n: def: imap (m: def':
+          elemType.merge (loc ++ ["[${toString n}-${toString m}]"])
+            [{ inherit (def) file; value = def'; }]) def.value) 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:
-        elemType.merge (addToPrefix (args // { inherit name; }) name));
+      merge = loc: defs:
+        zipAttrsWith (name: elemType.merge (loc ++ [name]))
+          # Push down position info.
+          (map (def: listToAttrs (mapAttrsToList (n: def':
+            { name = n; value = { inherit (def) file; value = def'; }; }) def.value)) defs);
       getSubOptions = prefix: elemType.getSubOptions (prefix ++ ["<name>"]);
     };
 
@@ -130,22 +130,25 @@ rec {
     loaOf = elemType:
       let
         convertIfList = defIdx: def:
-          if isList def then
-            listToAttrs (
-              flip imap def (elemIdx: elem:
-                { name = "unnamed-${toString defIdx}.${toString elemIdx}"; value = elem; }))
+          if isList def.value then
+            { inherit (def) file;
+              value = listToAttrs (
+                imap (elemIdx: elem:
+                  { name = "unnamed-${toString defIdx}.${toString elemIdx}";
+                    value = elem;
+                  }) def.value);
+            }
           else
             def;
         listOnly = listOf elemType;
         attrOnly = attrsOf elemType;
-
       in mkOptionType {
         name = "list or attribute set of ${elemType.name}s";
         check = x:
           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 = loc: defs: attrOnly.merge loc (imap convertIfList defs);
         getSubOptions = prefix: elemType.getSubOptions (prefix ++ ["<name?>"]);
       };
 
@@ -155,29 +158,23 @@ rec {
       getSubOptions = elemType.getSubOptions;
     };
 
-    none = elemType: mkOptionType {
-      inherit (elemType) name check;
-      merge = args: list:
-        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:
-        if all isNull defs then null
-        else if any isNull defs then
-          throw "The option `${showOption args.prefix}' is defined both null and not null, in ${showFiles args.files}."
-        else elemType.merge args defs;
+      merge = loc: defs:
+        let nrNulls = count (def: isNull def.value) defs; in
+        if nrNulls == length defs then null
+        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;
       getSubOptions = elemType.getSubOptions;
     };
 
     functionTo = elemType: mkOptionType {
       name = "function that evaluates to a(n) ${elemType.name}";
       check = builtins.isFunction;
-      merge = args: fns:
-        fnArgs: elemType.merge args (map (fn: fn fnArgs) fns);
+      merge = loc: defs:
+        fnArgs: elemType.merge loc (map (fn: { inherit (fn) file; value = fn.value fnArgs; }) defs);
       getSubOptions = elemType.getSubOptions;
     };
 
@@ -186,11 +183,11 @@ rec {
       mkOptionType rec {
         name = "submodule";
         check = x: isAttrs x || builtins.isFunction x;
-        merge = args: defs:
+        merge = loc: defs:
           let
             coerce = def: if builtins.isFunction def then def else { config = def; };
-            modules = opts' ++ map coerce defs;
-          in (evalModules { inherit modules args; prefix = args.prefix; }).config;
+            modules = opts' ++ map (def: { _file = def.file; imports = [(coerce def.value)]; }) defs;
+          in (evalModules { inherit modules; args.name = last loc; prefix = loc; }).config;
         getSubOptions = prefix: (evalModules
           { modules = opts'; inherit prefix;
             # FIXME: hack to get shit to evaluate.
@@ -206,8 +203,4 @@ rec {
 
   };
 
-
-  /* Helper function. */
-  addToPrefix = args: name: args // { prefix = args.prefix ++ [name]; };
-
 }