From 55ea29fd8c7a4b130a114b0f06b3fadfaf028356 Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Fri, 23 Jul 2021 10:43:44 +0200 Subject: [PATCH 1/6] lib/generators/toPretty: add evaluation-limit When having e.g. recursive attr-set, it cannot be printed which is solved by Nix itself like this: $ nix-instantiate --eval -E 'let a.b = 1; a.c = a; in builtins.trace a 1' trace: { b = 1; c = ; } 1 However, `generators.toPretty` tries to evaluate something until it's done which can result in a spurious `stack-overflow`-error: $ nix-instantiate --eval -E 'with import ; generators.toPretty { } (mkOption { type = types.str; })' error: stack overflow (possible infinite recursion) Those attr-sets are in fact rather common, one example is shown above, a `types.`-declaration is such an example. By adding an optional `depthLimit`-argument, `toPretty` will stop evaluating as soon as the limit is reached: $ nix-instantiate --eval -E 'with import ./Projects/nixpkgs-update-int/lib; generators.toPretty { depthLimit = 2; } (mkOption { type = types.str; })' |xargs -0 echo -e "{ _type = \"option\"; type = { _type = \"option-type\"; check = ; deprecationMessage = null; description = \"string\"; emptyValue = { }; functor = { binOp = ; name = ; payload = ; type = ; wrapped = ; }; getSubModules = null; getSubOptions = ; merge = ; name = \"str\"; nestedTypes = { }; substSubModules = ; typeMerge = ; }; }" Optionally, it's also possible to let `toPretty` throw an error if the limit is exceeded. --- lib/generators.nix | 24 +++++++++++++++++------- lib/tests/misc.nix | 18 ++++++++++++++++++ 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/lib/generators.nix b/lib/generators.nix index bcb0f371a9b5..b1639b677f65 100644 --- a/lib/generators.nix +++ b/lib/generators.nix @@ -205,13 +205,23 @@ rec { (This means fn is type Val -> String.) */ allowPrettyValues ? false, /* If this option is true, the output is indented with newlines for attribute sets and lists */ - multiline ? true - }@args: let - go = indent: v: with builtins; + multiline ? true, + /* If this option is not null, `toPretty` will stop evaluating at a certain depth */ + depthLimit ? null, + /* If this option is true, an error will be thrown, if a certain given depth is exceeded */ + throwOnDepthLimit ? false + }@args: + assert depthLimit != null -> builtins.isInt depthLimit; + assert throwOnDepthLimit -> depthLimit != null; + let + go = depth: indent: v: with builtins; let isPath = v: typeOf v == "path"; introSpace = if multiline then "\n${indent} " else " "; outroSpace = if multiline then "\n${indent}" else " "; - in if isInt v then toString v + in if depthLimit != null && depth > depthLimit then + if throwOnDepthLimit then throw "Exceeded maximum eval-depth limit of ${toString depthLimit} while trying to pretty-print with `generators.toPretty'!" + else "" + else if isInt v then toString v else if isFloat v then "~${toString v}" else if isString v then let @@ -233,7 +243,7 @@ rec { else if isList v then if v == [] then "[ ]" else "[" + introSpace - + libStr.concatMapStringsSep introSpace (go (indent + " ")) v + + libStr.concatMapStringsSep introSpace (go (depth + 1) (indent + " ")) v + outroSpace + "]" else if isFunction v then let fna = lib.functionArgs v; @@ -252,10 +262,10 @@ rec { else "{" + introSpace + libStr.concatStringsSep introSpace (libAttr.mapAttrsToList (name: value: - "${libStr.escapeNixIdentifier name} = ${go (indent + " ") value};") v) + "${libStr.escapeNixIdentifier name} = ${go (depth + 1) (indent + " ") value};") v) + outroSpace + "}" else abort "generators.toPretty: should never happen (v = ${v})"; - in go ""; + in go 0 ""; # PLIST handling toPlist = {}: v: let diff --git a/lib/tests/misc.nix b/lib/tests/misc.nix index 4b2e5afc1d60..110716ca6913 100644 --- a/lib/tests/misc.nix +++ b/lib/tests/misc.nix @@ -529,6 +529,24 @@ runTests { }; }; + testToPrettyLimit = + let + a.b = 1; + a.c = a; + in { + expr = generators.toPretty { depthLimit = 2; } a; + expected = "{\n b = 1;\n c = {\n b = 1;\n c = {\n b = ;\n c = ;\n };\n };\n}"; + }; + + testToPrettyLimitThrow = + let + a.b = 1; + a.c = a; + in { + expr = (builtins.tryEval (generators.toPretty { depthLimit = 2; throwOnDepthLimit = true; } a)).success; + expected = false; + }; + testToPrettyMultiline = { expr = mapAttrs (const (generators.toPretty { })) rec { list = [ 3 4 [ false ] ]; From fbc9084c39297f6a8ca618b0f6a3ccdd4489ab6a Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Fri, 23 Jul 2021 10:49:51 +0200 Subject: [PATCH 2/6] lib/options: use `depthLimit` for `toPretty` when showing a definition When having a bogus declaration such as { lib, ... }: { foo.bar = mkOption { type = types.str; }; } the evaluation will terminate with a not-so helpful error: stack overflow (possible infinite recursion) To make sure a useful error is still provided, I added a `depthLimit` of `10` which should be perfectly sufficient to `toPretty` when it's used in an error-case for `showDefs`. --- lib/options.nix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/options.nix b/lib/options.nix index 204c86df9f51..4b39ce824ea0 100644 --- a/lib/options.nix +++ b/lib/options.nix @@ -247,7 +247,7 @@ rec { showDefs = defs: concatMapStrings (def: let # Pretty print the value for display, if successful - prettyEval = builtins.tryEval (lib.generators.toPretty {} def.value); + prettyEval = builtins.tryEval (lib.generators.toPretty { depthLimit = 10; } def.value); # Split it into its lines lines = filter (v: ! isList v) (builtins.split "\n" prettyEval.value); # Only display the first 5 lines, and indent them for better visibility From b6d3c9f821b704fbfb68d20a76520fa9e160df36 Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Fri, 23 Jul 2021 10:56:24 +0200 Subject: [PATCH 3/6] lib/modules: fix error-message when declaring an option inside `config' The message I originally implemented here was to catch a mixup of `config' and `options' in a `types.submodule'[1]. However it looks rather weird for a wrongly declared top-level option. So I decided to throw error: The option `foo' does not exist. Definition values: - In `': { bar = { _type = "option"; type = { _type = "option-type"; ... It seems as you're trying to declare an option by placing it into `config' rather than `options'! for an expression like with import ./lib; evalModules { modules = [ { foo.bar = mkOption { type = types.str; }; } ]; } [1] fa30c9abed61f30218a211842204705986d486f9 --- lib/modules.nix | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/lib/modules.nix b/lib/modules.nix index b124ea000a2e..51e12f0659c9 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -162,13 +162,24 @@ rec { baseMsg = "The option `${showOption (prefix ++ firstDef.prefix)}' does not exist. Definition values:${showDefs [ firstDef ]}"; in if attrNames options == [ "_module" ] - then throw '' - ${baseMsg} + then + let + optionName = showOption prefix; + in + if optionName == "" + then throw '' + ${baseMsg} - However there are no options defined in `${showOption prefix}'. Are you sure you've - declared your options properly? This can happen if you e.g. declared your options in `types.submodule' - under `config' rather than `options'. - '' + It seems as you're trying to declare an option by placing it into `config' rather than `options'! + '' + else + throw '' + ${baseMsg} + + However there are no options defined in `${showOption prefix}'. Are you sure you've + declared your options properly? This can happen if you e.g. declared your options in `types.submodule' + under `config' rather than `options'. + '' else throw baseMsg else null; From 5773ae93f75cfd504d7971c1b26955fa50a00744 Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Thu, 26 Aug 2021 00:28:49 +0200 Subject: [PATCH 4/6] lib/generators: move limit detection into `withRecursion` As suggested in #131205. Now it's possible to pretty-print a value with `lib.generators` like this: with lib.generators; toPretty { } (withRecursion { depthLimit = 10; } /* arbitrarily complex value */) Also, this can be used for any other pretty-printer now if needed. --- lib/generators.nix | 45 ++++++++++++++++++++++++++++++--------------- lib/options.nix | 4 +++- lib/tests/misc.nix | 7 ++++--- 3 files changed, 37 insertions(+), 19 deletions(-) diff --git a/lib/generators.nix b/lib/generators.nix index b1639b677f65..9271b9746aee 100644 --- a/lib/generators.nix +++ b/lib/generators.nix @@ -195,6 +195,30 @@ rec { */ toYAML = {}@args: toJSON args; + withRecursion = + args@{ + /* If this option is not null, `toPretty` will stop evaluating at a certain depth */ + depthLimit + /* If this option is true, an error will be thrown, if a certain given depth is exceeded */ + , throwOnDepthLimit ? true + }: + assert builtins.isInt depthLimit; + let + transform = depth: + if depthLimit != null && depth > depthLimit then + if throwOnDepthLimit + then throw "Exceeded maximum eval-depth limit of ${toString depthLimit} while trying to pretty-print with `generators.withRecursion'!" + else const "" + else id; + mapAny = with builtins; depth: v: + let + evalNext = x: mapAny (depth + 1) (transform (depth + 1) x); + in + if isAttrs v then mapAttrs (const evalNext) v + else if isList v then map evalNext v + else transform (depth + 1) v; + in + mapAny 0; /* Pretty print a value, akin to `builtins.trace`. * Should probably be a builtin as well. @@ -205,23 +229,14 @@ rec { (This means fn is type Val -> String.) */ allowPrettyValues ? false, /* If this option is true, the output is indented with newlines for attribute sets and lists */ - multiline ? true, - /* If this option is not null, `toPretty` will stop evaluating at a certain depth */ - depthLimit ? null, - /* If this option is true, an error will be thrown, if a certain given depth is exceeded */ - throwOnDepthLimit ? false + multiline ? true }@args: - assert depthLimit != null -> builtins.isInt depthLimit; - assert throwOnDepthLimit -> depthLimit != null; let - go = depth: indent: v: with builtins; + go = indent: v: with builtins; let isPath = v: typeOf v == "path"; introSpace = if multiline then "\n${indent} " else " "; outroSpace = if multiline then "\n${indent}" else " "; - in if depthLimit != null && depth > depthLimit then - if throwOnDepthLimit then throw "Exceeded maximum eval-depth limit of ${toString depthLimit} while trying to pretty-print with `generators.toPretty'!" - else "" - else if isInt v then toString v + in if isInt v then toString v else if isFloat v then "~${toString v}" else if isString v then let @@ -243,7 +258,7 @@ rec { else if isList v then if v == [] then "[ ]" else "[" + introSpace - + libStr.concatMapStringsSep introSpace (go (depth + 1) (indent + " ")) v + + libStr.concatMapStringsSep introSpace (go (indent + " ")) v + outroSpace + "]" else if isFunction v then let fna = lib.functionArgs v; @@ -262,10 +277,10 @@ rec { else "{" + introSpace + libStr.concatStringsSep introSpace (libAttr.mapAttrsToList (name: value: - "${libStr.escapeNixIdentifier name} = ${go (depth + 1) (indent + " ") value};") v) + "${libStr.escapeNixIdentifier name} = ${go (indent + " ") value};") v) + outroSpace + "}" else abort "generators.toPretty: should never happen (v = ${v})"; - in go 0 ""; + in go ""; # PLIST handling toPlist = {}: v: let diff --git a/lib/options.nix b/lib/options.nix index 4b39ce824ea0..119a67fb7d82 100644 --- a/lib/options.nix +++ b/lib/options.nix @@ -247,7 +247,9 @@ rec { showDefs = defs: concatMapStrings (def: let # Pretty print the value for display, if successful - prettyEval = builtins.tryEval (lib.generators.toPretty { depthLimit = 10; } def.value); + prettyEval = builtins.tryEval + (lib.generators.toPretty { } + (lib.generators.withRecursion { depthLimit = 10; throwOnDepthLimit = false; } def.value)); # Split it into its lines lines = filter (v: ! isList v) (builtins.split "\n" prettyEval.value); # Only display the first 5 lines, and indent them for better visibility diff --git a/lib/tests/misc.nix b/lib/tests/misc.nix index 110716ca6913..00eeaa2a77d7 100644 --- a/lib/tests/misc.nix +++ b/lib/tests/misc.nix @@ -534,8 +534,8 @@ runTests { a.b = 1; a.c = a; in { - expr = generators.toPretty { depthLimit = 2; } a; - expected = "{\n b = 1;\n c = {\n b = 1;\n c = {\n b = ;\n c = ;\n };\n };\n}"; + expr = generators.toPretty { } (generators.withRecursion { throwOnDepthLimit = false; depthLimit = 2; } a); + expected = "{\n b = 1;\n c = {\n b = \"\";\n c = {\n b = \"\";\n c = \"\";\n };\n };\n}"; }; testToPrettyLimitThrow = @@ -543,7 +543,8 @@ runTests { a.b = 1; a.c = a; in { - expr = (builtins.tryEval (generators.toPretty { depthLimit = 2; throwOnDepthLimit = true; } a)).success; + expr = (builtins.tryEval + (generators.toPretty { } (generators.withRecursion { depthLimit = 2; } a))).success; expected = false; }; From b96101a35f5cd0bd348449b245244732df3ee545 Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Thu, 26 Aug 2021 00:37:33 +0200 Subject: [PATCH 5/6] lib/modules: grammar fix in error msg --- lib/modules.nix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/modules.nix b/lib/modules.nix index 51e12f0659c9..46ae3f136310 100644 --- a/lib/modules.nix +++ b/lib/modules.nix @@ -170,7 +170,7 @@ rec { then throw '' ${baseMsg} - It seems as you're trying to declare an option by placing it into `config' rather than `options'! + It seems as if you're trying to declare an option by placing it into `config' rather than `options'! '' else throw '' From 681758d41588c27aaa6f3c3a72184dcc4d0aeba8 Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Tue, 28 Sep 2021 22:26:51 +0200 Subject: [PATCH 6/6] lib/generators: fix error message --- lib/generators.nix | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/generators.nix b/lib/generators.nix index 9271b9746aee..bd82f0a19af1 100644 --- a/lib/generators.nix +++ b/lib/generators.nix @@ -197,7 +197,7 @@ rec { withRecursion = args@{ - /* If this option is not null, `toPretty` will stop evaluating at a certain depth */ + /* If this option is not null, the given value will stop evaluating at a certain depth */ depthLimit /* If this option is true, an error will be thrown, if a certain given depth is exceeded */ , throwOnDepthLimit ? true @@ -207,7 +207,7 @@ rec { transform = depth: if depthLimit != null && depth > depthLimit then if throwOnDepthLimit - then throw "Exceeded maximum eval-depth limit of ${toString depthLimit} while trying to pretty-print with `generators.withRecursion'!" + then throw "Exceeded maximum eval-depth limit of ${toString depthLimit} while trying to evaluate with `generators.withRecursion'!" else const "" else id; mapAny = with builtins; depth: v: