From 4a014ed92e468cce5ba375d086f61fa31512682b Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 3 Jul 2024 10:41:01 +0200 Subject: [PATCH 1/6] lib/modules: Use fixed lib instead of args.lib The practical use for this should be very limited because I don't think anyone should change `lib`, let alone change `lib.functionArgs`, but, but it would be even stranger to rely on `args.lib` (or really `specialArgs.lib` for what's clearly a behavior of the current `evalModules`, which uses its own ambient lib for basically everything. The shadowing of `lib` by `args.lib` here seems to be a small mistake, which is easy to make. --- lib/modules.nix | 4 ++-- lib/tests/modules.sh | 2 ++ lib/tests/modules/specialArgs-lib.nix | 28 +++++++++++++++++++++++++++ 3 files changed, 32 insertions(+), 2 deletions(-) create mode 100644 lib/tests/modules/specialArgs-lib.nix diff --git a/lib/modules.nix b/lib/modules.nix index 79892f50c4fe..44de50769633 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -485,10 +485,10 @@ let config = addFreeformType (removeAttrs m ["_class" "_file" "key" "disabledModules" "require" "imports" "freeformType"]); }; - applyModuleArgsIfFunction = key: f: args@{ config, options, lib, ... }: + applyModuleArgsIfFunction = key: f: args@{ config, ... }: if isFunction f then applyModuleArgs key f args else f; - applyModuleArgs = key: f: args@{ config, options, lib, ... }: + applyModuleArgs = key: f: args@{ config, ... }: let # Module arguments are resolved in a strict manner when attribute set # deconstruction is used. As the arguments are now defined with the diff --git a/lib/tests/modules.sh b/lib/tests/modules.sh index 750b1d025e02..280d0b47d574 100755 --- a/lib/tests/modules.sh +++ b/lib/tests/modules.sh @@ -94,6 +94,8 @@ checkConfigOutput '^true$' config.result ./module-argument-default.nix # gvariant checkConfigOutput '^true$' config.assertion ./gvariant.nix +checkConfigOutput '"ok"' config.result ./specialArgs-lib.nix + # https://github.com/NixOS/nixpkgs/pull/131205 # We currently throw this error already in `config`, but throwing in `config.wrong1` would be acceptable. checkConfigError 'It seems as if you.re trying to declare an option by placing it into .config. rather than .options.' config.wrong1 ./error-mkOption-in-config.nix diff --git a/lib/tests/modules/specialArgs-lib.nix b/lib/tests/modules/specialArgs-lib.nix new file mode 100644 index 000000000000..8c9d2103862a --- /dev/null +++ b/lib/tests/modules/specialArgs-lib.nix @@ -0,0 +1,28 @@ +{ config, lib, ... }: + +{ + options = { + result = lib.mkOption { }; + weird = lib.mkOption { + type = lib.types.submoduleWith { + # I generally recommend against overriding lib, because that leads to + # slightly incompatible dialects of the module system. + # Nonetheless, it's worth guarding the property that the module system + # evaluates with a completely custom lib, as a matter of separation of + # concerns. + specialArgs.lib = { }; + modules = [ ]; + }; + }; + }; + config.weird = args@{ ... /* note the lack of a `lib` argument */ }: + assert args.lib == { }; + assert args.specialArgs == { lib = { }; }; + { + options.foo = lib.mkOption { }; + config.foo = lib.mkIf true "alright"; + }; + config.result = + assert config.weird.foo == "alright"; + "ok"; +} From 0a0e37c217f40ea7134059680569b526b0ea805e Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 3 Jul 2024 10:47:23 +0200 Subject: [PATCH 2/6] lib/modules: Memoize addErrorContext lookup --- lib/modules.nix | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/modules.nix b/lib/modules.nix index 44de50769633..04207203f8f2 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -2,6 +2,7 @@ let inherit (lib) + addErrorContext all any attrByPath @@ -265,9 +266,9 @@ let let optText = showOption (prefix ++ firstDef.prefix); defText = - builtins.addErrorContext + addErrorContext "while evaluating the error message for definitions for `${optText}', which is an option that does not exist" - (builtins.addErrorContext + (addErrorContext "while evaluating a definition from `${firstDef.file}'" ( showDefs [ firstDef ]) ); @@ -503,8 +504,8 @@ let # not their values. The values are forwarding the result of the # evaluation of the option. context = name: ''while evaluating the module argument `${name}' in "${key}":''; - extraArgs = builtins.mapAttrs (name: _: - builtins.addErrorContext (context name) + extraArgs = mapAttrs (name: _: + addErrorContext (context name) (args.${name} or config._module.args.${name}) ) (lib.functionArgs f); @@ -806,7 +807,7 @@ let "The type `types.${opt.type.name}' of option `${showOption loc}' defined in ${showFiles opt.declarations} is deprecated. ${opt.type.deprecationMessage}"; in warnDeprecation opt // - { value = builtins.addErrorContext "while evaluating the option `${showOption loc}':" value; + { value = addErrorContext "while evaluating the option `${showOption loc}':" value; inherit (res.defsFinal') highestPrio; definitions = map (def: def.value) res.defsFinal; files = map (def: def.file) res.defsFinal; @@ -822,7 +823,7 @@ let let # Process mkMerge and mkIf properties. defs' = concatMap (m: - map (value: { inherit (m) file; inherit value; }) (builtins.addErrorContext "while evaluating definitions from `${m.file}':" (dischargeProperties m.value)) + map (value: { inherit (m) file; inherit value; }) (addErrorContext "while evaluating definitions from `${m.file}':" (dischargeProperties m.value)) ) defs; # Process mkOverride properties. From 88a9a933c455d5e87d2ce88d108f93ae32826394 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 3 Jul 2024 10:53:06 +0200 Subject: [PATCH 3/6] lib/modules: Memoize functionArgs lookup This would also make specialArgs-lib.nix pass. --- lib/modules.nix | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/modules.nix b/lib/modules.nix index 04207203f8f2..cd7e34ceb2f1 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -14,6 +14,7 @@ let elem filter foldl' + functionArgs getAttrFromPath head id @@ -507,7 +508,7 @@ let extraArgs = mapAttrs (name: _: addErrorContext (context name) (args.${name} or config._module.args.${name}) - ) (lib.functionArgs f); + ) (functionArgs f); # Note: we append in the opposite order such that we can add an error # context on the explicit arguments of "args" too. This update From d0438fb00f90b3641a16fa4561e99f8fa1f93cde Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 3 Jul 2024 11:01:12 +0200 Subject: [PATCH 4/6] lib/modules: Memoize remaining lookups into lib Benefits: - some lookups happened in the hot path, and will now be slightly faster, with only a variable lookup and no attribute selection - it's now harder to accidentally use args.lib aka specialArgs.lib, which has happened - shorter --- lib/modules.nix | 46 +++++++++++++++++++++++++--------------------- 1 file changed, 25 insertions(+), 21 deletions(-) diff --git a/lib/modules.nix b/lib/modules.nix index cd7e34ceb2f1..b15919ba3865 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -22,6 +22,7 @@ let isAttrs isBool isFunction + isInOldestRelease isList isString length @@ -35,8 +36,11 @@ let recursiveUpdate reverseList sort setAttrByPath + throwIfNot types + warn warnIf + zipAttrs zipAttrsWith ; inherit (lib.options) @@ -91,8 +95,8 @@ let }: let withWarnings = x: - lib.warnIf (evalModulesArgs?args) "The args argument to evalModules is deprecated. Please set config._module.args instead." - lib.warnIf (evalModulesArgs?check) "The check argument to evalModules is deprecated. Please set config._module.check instead." + warnIf (evalModulesArgs?args) "The args argument to evalModules is deprecated. Please set config._module.args instead." + warnIf (evalModulesArgs?check) "The check argument to evalModules is deprecated. Please set config._module.check instead." x; legacyModules = @@ -314,7 +318,7 @@ let prefix = extendArgs.prefix or evalModulesArgs.prefix or []; }); - type = lib.types.submoduleWith { + type = types.submoduleWith { inherit modules specialArgs class; }; @@ -346,8 +350,8 @@ let else throw ( "Could not load a value as a module, because it is of type ${lib.strings.escapeNixString m._type}" - + lib.optionalString (fallbackFile != unknownModule) ", in file ${toString fallbackFile}." - + lib.optionalString (m._type == "configuration") " If you do intend to import this configuration, please only import the modules that make up the configuration. You may have to create a `let` binding, file or attribute to give yourself access to the relevant modules.\nWhile loading a configuration into the module system is a very sensible idea, it can not be done cleanly in practice." + + optionalString (fallbackFile != unknownModule) ", in file ${toString fallbackFile}." + + optionalString (m._type == "configuration") " If you do intend to import this configuration, please only import the modules that make up the configuration. You may have to create a `let` binding, file or attribute to give yourself access to the relevant modules.\nWhile loading a configuration into the module system is a very sensible idea, it can not be done cleanly in practice." # Extended explanation: That's because a finalized configuration is more than just a set of modules. For instance, it has its own `specialArgs` that, by the nature of `specialArgs` can't be loaded through `imports` or the the `modules` argument. So instead, we have to ask you to extract the relevant modules and use those instead. This way, we keep the module system comparatively simple, and hopefully avoid a bad surprise down the line. ) else if isList m then @@ -477,7 +481,7 @@ let } else # shorthand syntax - lib.throwIfNot (isAttrs m) "module ${file} (${key}) does not look like a module." + throwIfNot (isAttrs m) "module ${file} (${key}) does not look like a module." { _file = toString m._file or file; _class = m._class or null; key = toString m.key or key; @@ -549,7 +553,7 @@ let (n: concatLists) (map (module: let subtree = module.options; in - if !(builtins.isAttrs subtree) then + if !(isAttrs subtree) then throw '' An option declaration for `${builtins.concatStringsSep "." prefix}' has type `${builtins.typeOf subtree}' rather than an attribute set. @@ -567,7 +571,7 @@ let # The root of any module definition must be an attrset. checkedConfigs = assert - lib.all + all (c: # TODO: I have my doubts that this error would occur when option definitions are not matched. # The implementation of this check used to be tied to a superficially similar check for @@ -669,7 +673,7 @@ let let nonOptions = filter (m: !isOption m.options) decls; in - throw "The option `${showOption loc}' in module `${(lib.head optionDecls)._file}' would be a parent of the following options, but its type `${(lib.head optionDecls).options.type.description or ""}' does not support nested options.\n${ + throw "The option `${showOption loc}' in module `${(head optionDecls)._file}' would be a parent of the following options, but its type `${(head optionDecls).options.type.description or ""}' does not support nested options.\n${ showRawDecls loc nonOptions }" else @@ -974,12 +978,12 @@ let mergeAttrDefinitionsWithPrio = opt: let defsByAttr = - lib.zipAttrs ( - lib.concatLists ( - lib.concatMap + zipAttrs ( + concatLists ( + concatMap ({ value, ... }@def: map - (lib.mapAttrsToList (k: value: { ${k} = def // { inherit value; }; })) + (mapAttrsToList (k: value: { ${k} = def // { inherit value; }; })) (pushDownProperties value) ) opt.definitionsWithLocations @@ -987,9 +991,9 @@ let ); in assert opt.type.name == "attrsOf" || opt.type.name == "lazyAttrsOf"; - lib.mapAttrs + mapAttrs (k: v: - let merging = lib.mergeDefinitions (opt.loc ++ [k]) opt.type.nestedTypes.elemType v; + let merging = mergeDefinitions (opt.loc ++ [k]) opt.type.nestedTypes.elemType v; in { value = merging.mergedValue; inherit (merging.defsFinal') highestPrio; @@ -1025,9 +1029,9 @@ let mkForce = mkOverride 50; mkVMOverride = mkOverride 10; # used by ‘nixos-rebuild build-vm’ - defaultPriority = lib.warnIf (lib.isInOldestRelease 2305) "lib.modules.defaultPriority is deprecated, please use lib.modules.defaultOverridePriority instead." defaultOverridePriority; + defaultPriority = warnIf (isInOldestRelease 2305) "lib.modules.defaultPriority is deprecated, please use lib.modules.defaultOverridePriority instead." defaultOverridePriority; - mkFixStrictness = lib.warn "lib.mkFixStrictness has no effect and will be removed. It returns its argument unmodified, so you can just remove any calls." id; + mkFixStrictness = warn "lib.mkFixStrictness has no effect and will be removed. It returns its argument unmodified, so you can just remove any calls." id; mkOrder = priority: content: { _type = "order"; @@ -1141,8 +1145,8 @@ let }: doRename { inherit from to; visible = false; - warn = lib.isInOldestRelease sinceRelease; - use = lib.warnIf (lib.isInOldestRelease sinceRelease) + warn = isInOldestRelease sinceRelease; + use = warnIf (isInOldestRelease sinceRelease) "Obsolete option `${showOption from}' is used. It was renamed to `${showOption to}'."; }; @@ -1374,8 +1378,8 @@ let config = lib.importTOML file; }; - private = lib.mapAttrs - (k: lib.warn "External use of `lib.modules.${k}` is deprecated. If your use case isn't covered by non-deprecated functions, we'd like to know more and perhaps support your use case well, instead of providing access to these low level functions. In this case please open an issue in https://github.com/nixos/nixpkgs/issues/.") + private = mapAttrs + (k: warn "External use of `lib.modules.${k}` is deprecated. If your use case isn't covered by non-deprecated functions, we'd like to know more and perhaps support your use case well, instead of providing access to these low level functions. In this case please open an issue in https://github.com/nixos/nixpkgs/issues/.") { inherit applyModuleArgsIfFunction From fed26baf1e0ab95efb32a88d427b92e400298af7 Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 3 Jul 2024 11:07:12 +0200 Subject: [PATCH 5/6] lib: Expose typeOf, unsafeGetAttrPos All builtins should be in mirrored in lib, for consistency, as well as control to let the Nixpkgs pin effect *subtle* improvements in behavior (such as the foldl' accumulator strictness). --- lib/default.nix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/default.nix b/lib/default.nix index 9c6f886c9ee4..ef0bb60ab8c5 100644 --- a/lib/default.nix +++ b/lib/default.nix @@ -66,7 +66,7 @@ let # TODO: For consistency, all builtins should also be available from a sub-library; # these are the only ones that are currently not - inherit (builtins) addErrorContext isPath trace; + inherit (builtins) addErrorContext isPath trace typeOf unsafeGetAttrPos; inherit (self.trivial) id const pipe concat or and xor bitAnd bitOr bitXor bitNot boolToString mergeAttrs flip mapNullable inNixShell isFloat min max importJSON importTOML warn warnIf warnIfNot throwIf throwIfNot checkListOfEnum From 4eb6883b860e99ecb6cd17eba63fb5a806f5f18a Mon Sep 17 00:00:00 2001 From: Robert Hensing Date: Wed, 3 Jul 2024 11:09:53 +0200 Subject: [PATCH 6/6] lib/modules: Memoize remaining lookups into builtins Similar to the previous commit about lookups into lib Main benefits - consistent - faster - shorter --- lib/modules.nix | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/lib/modules.nix b/lib/modules.nix index b15919ba3865..b9e9ca1e5d78 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -16,6 +16,7 @@ let foldl' functionArgs getAttrFromPath + genericClosure head id imap1 @@ -35,9 +36,14 @@ let optionalString recursiveUpdate reverseList sort + seq setAttrByPath + substring throwIfNot + trace + typeOf types + unsafeGetAttrPos warn warnIf zipAttrs @@ -304,7 +310,7 @@ let else throw baseMsg else null; - checked = builtins.seq checkUnmatched; + checked = seq checkUnmatched; extendModules = extendArgs@{ modules ? [], @@ -421,7 +427,7 @@ let moduleKey = file: m: if isString m then - if builtins.substring 0 1 m == "/" + if substring 0 1 m == "/" then m else toString modulesPath + "/" + m @@ -439,11 +445,11 @@ let else if isAttrs m then throw "Module `${file}` contains a disabledModules item that is an attribute set, presumably a module, that does not have a `key` attribute. This means that the module system doesn't have any means to identify the module that should be disabled. Make sure that you've put the correct value in disabledModules: a string path relative to modulesPath, a path value, or an attribute set with a `key` attribute." - else throw "Each disabledModules item must be a path, string, or a attribute set with a key attribute, or a value supported by toString. However, one of the disabledModules items in `${toString file}` is none of that, but is of type ${builtins.typeOf m}."; + else throw "Each disabledModules item must be a path, string, or a attribute set with a key attribute, or a value supported by toString. However, one of the disabledModules items in `${toString file}` is none of that, but is of type ${typeOf m}."; disabledKeys = concatMap ({ file, disabled }: map (moduleKey file) disabled) disabled; keyFilter = filter (attrs: ! elem attrs.key disabledKeys); - in map (attrs: attrs.module) (builtins.genericClosure { + in map (attrs: attrs.module) (genericClosure { startSet = keyFilter modules; operator = attrs: keyFilter attrs.modules; }); @@ -555,14 +561,14 @@ let (module: let subtree = module.options; in if !(isAttrs subtree) then throw '' - An option declaration for `${builtins.concatStringsSep "." prefix}' has type - `${builtins.typeOf subtree}' rather than an attribute set. + An option declaration for `${concatStringsSep "." prefix}' has type + `${typeOf subtree}' rather than an attribute set. Did you mean to define this outside of `options'? '' else mapAttrs (n: option: - [{ inherit (module) _file; pos = builtins.unsafeGetAttrPos n subtree; options = option; }] + [{ inherit (module) _file; pos = unsafeGetAttrPos n subtree; options = option; }] ) subtree ) @@ -577,11 +583,11 @@ let # The implementation of this check used to be tied to a superficially similar check for # options, so maybe that's why this is here. isAttrs c.config || throw '' - In module `${c.file}', you're trying to define a value of type `${builtins.typeOf c.config}' + In module `${c.file}', you're trying to define a value of type `${typeOf c.config}' rather than an attribute set for the option - `${builtins.concatStringsSep "." prefix}'! + `${concatStringsSep "." prefix}'! - This usually happens if `${builtins.concatStringsSep "." prefix}' has option + This usually happens if `${concatStringsSep "." prefix}' has option definitions inside that are not matched. Please check how to properly define this option by e.g. referring to `man 5 configuration.nix'! '' @@ -1127,7 +1133,7 @@ let inherit from to; visible = false; warn = true; - use = builtins.trace "Obsolete option `${showOption from}' is used. It was renamed to `${showOption to}'."; + use = trace "Obsolete option `${showOption from}' is used. It was renamed to `${showOption to}'."; }; mkRenamedOptionModuleWith = {