Change `types.string` error to a warning instead by infinisil · Pull Request #248278 · NixOS/nixpkgs

infinisil · 2023-08-10T04:39:14Z

Description of changes

For one, reverts #247848.

This was a bit too ambitious, as reported in #247937 and other issues, because no warnings were previously triggered when string was nested e.g. attrsOf string, nullOr string, etc.

Support for nested type deprecation warnings was introduced in #99132, but had to be reverted in #121816 because it caused infinite recursion for some users, and I couldn't remember that it was reverted.

Instead this PR switches strings to emit a warning using lib.warn instead. This will cause a poorer error message without option location information, but it will catch all uses of its use, hopefully allowing an actual error within a few years, any day now.

This reverts commit c59c6b1. This was a bit too ambitious, because no warnings were previously triggered when `string` was nested e.g. `attrsOf string`, `nullOr string`, etc. Support for nested type deprecation warnings was introduced in 4b54aed, but had to be reverted in a36e676 because it caused infinite recursion for some users, and I couldn't remember that it was reverted.

This will cause a poorer error message without option location information, but it will catch all uses of its use.

emilazy

This works for me, although the warning message is of course not very helpful; I guess it is too technically difficult to get option name information for this?

Maybe it'd be a good idea to leave a comment with the release it can be turned into a hard throw by so that it doesn't take another 4 years to get rid of?

roberth · 2023-08-10T09:22:48Z

You can get a trace for a warning using NIX_ABORT_ON_WARN=1, --impure, see https://nixos.org/manual/nixpkgs/stable/#sec-function-library-lib.trivial.warn
I presume we have an informative added error context when we access the type? Or it might just print the location of the types.string?

We should explain how to solve the problem instead of referring to a discussion following a post claiming that we should get rid of it (instead of why and how!).

infinisil · 2023-08-10T15:38:29Z

This works for me, although the warning message is of course not very helpful; I guess it is too technically difficult to get option name information for this?

Yeah that was tried in the PR that had to be reverted, mentioned in the PR description

I presume we have an informative added error context when we access the type? Or it might just print the location of the types.string?
Nope, it's really just the warning that's printed, no locations. But we can't do any better afaik unless we make #99132 work again (which I think could be doable if types explicitly specify their recursion), but I don't have the time for that now

Michael-J-Ward · 2023-08-10T17:31:48Z

Nope, it's really just the warning that's printed, no locations. But we can't do any better afaik unless we make #99132 work again (which I think could be doable if types explicitly specify their recursion), but I don't have the time for that now

Is there a recommendation for manually debugging this?

--show-trace does not appear to include a single line from my flake.

infinisil · 2023-08-10T17:39:52Z

@Michael-J-Ward It should at least be only a warning now, which as @roberth showed can be turned into an error, then also supporting --show-trace

If you're having trouble with the backtrace, could you share it here?

emilazy · 2023-08-10T19:05:16Z

I presume we have an informative added error context when we access the type? Or it might just print the location of the types.string?

The throw does not seem to give anything useful, at least :( Not sure if the warning will be different.

emilazy · 2023-08-10T19:07:26Z

An example backtrace with the previous throw from a nix-darwin configuration:

Details

error:
       … while calling 'g'
     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/attrsets.nix:599:19:

      598|           g =
      599|             name: value:
         |                   ^
      600|             if isAttrs value && cond value

   … from call site

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/attrsets.nix:602:20:

      601|               then recurse (path ++ [name]) value
      602|               else f (path ++ [name]) value;
         |                    ^
      603|         in mapAttrs g;

   … while calling anonymous lambda

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/modules.nix:242:72:

      241|           # For definitions that have an associated option
      242|           declaredConfig = mapAttrsRecursiveCond (v: ! isOption v) (_: v: v.value) options;
         |                                                                        ^
      243|

   … while evaluating the option `system.build':

   … from call site

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/modules.nix:805:59:

      804|       if isDefined then
      805|         if all (def: type.check def.value) defsFinal then type.merge loc defsFinal
         |                                                           ^
      806|         else let allInvalid = filter (def: ! type.check def.value) defsFinal;

   … while calling 'merge'

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/types.nix:535:20:

      534|       check = isAttrs;
      535|       merge = loc: defs:
         |                    ^
      536|         mapAttrs (n: v: v.value) (filterAttrs (n: v: v ? value) (zipAttrsWith (name: defs:

   … from call site

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/types.nix:536:35:

      535|       merge = loc: defs:
      536|         mapAttrs (n: v: v.value) (filterAttrs (n: v: v ? value) (zipAttrsWith (name: defs:
         |                                   ^
      537|             (mergeDefinitions (loc ++ [name]) elemType defs).optionalValue

   … while calling 'filterAttrs'

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/attrsets.nix:309:5:

      308|     # The attribute set to filter
      309|     set:
         |     ^
      310|     listToAttrs (concatMap (name: let v = set.${name}; in if pred name v then [(nameValuePair name v)] else []) (attrNames set));

   … while calling anonymous lambda

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/attrsets.nix:310:29:

      309|     set:
      310|     listToAttrs (concatMap (name: let v = set.${name}; in if pred name v then [(nameValuePair name v)] else []) (attrNames set));
         |                             ^
      311|

   … from call site

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/attrsets.nix:310:62:

      309|     set:
      310|     listToAttrs (concatMap (name: let v = set.${name}; in if pred name v then [(nameValuePair name v)] else []) (attrNames set));
         |                                                              ^
      311|

   … while calling anonymous lambda

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/types.nix:536:51:

      535|       merge = loc: defs:
      536|         mapAttrs (n: v: v.value) (filterAttrs (n: v: v ? value) (zipAttrsWith (name: defs:
         |                                                   ^
      537|             (mergeDefinitions (loc ++ [name]) elemType defs).optionalValue

   … while calling anonymous lambda

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/types.nix:536:86:

      535|       merge = loc: defs:
      536|         mapAttrs (n: v: v.value) (filterAttrs (n: v: v ? value) (zipAttrsWith (name: defs:
         |                                                                                      ^
      537|             (mergeDefinitions (loc ++ [name]) elemType defs).optionalValue

   … while calling anonymous lambda

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/modules.nix:783:28:

      782|         # Process mkMerge and mkIf properties.
      783|         defs' = concatMap (m:
         |                            ^
      784|           map (value: { inherit (m) file; inherit value; }) (builtins.addErrorContext "while evaluating definitions from `${m.file}':" (dischargeProperties m.value))

   … while evaluating definitions from `/nix/store/lng0rb29r0m088w82rgp4ksp8j6xclk9-source/modules/system':

   … from call site

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/modules.nix:784:137:

      783|         defs' = concatMap (m:
      784|           map (value: { inherit (m) file; inherit value; }) (builtins.addErrorContext "while evaluating definitions from `${m.file}':" (dischargeProperties m.value))
         |                                                                                                                                         ^
      785|         ) defs;

   … while calling 'dischargeProperties'

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/modules.nix:855:25:

      854|   */
      855|   dischargeProperties = def:
         |                         ^
      856|     if def._type or "" == "merge" then

   … from call site

     at /nix/store/lng0rb29r0m088w82rgp4ksp8j6xclk9-source/modules/system/default.nix:91:29:

       90|
       91|     system.build.toplevel = throwAssertions (showWarnings (stdenvNoCC.mkDerivation ({
         |                             ^
       92|       name = "darwin-system-${cfg.darwinLabel}";

   … while calling 'throwAssertions'

     at /nix/store/lng0rb29r0m088w82rgp4ksp8j6xclk9-source/modules/system/default.nix:13:21:

       12|
       13|   throwAssertions = res: if (failedAssertions != []) then throw "\nFailed assertions:\n${concatStringsSep "\n" (map (x: "- ${x}") failedAssertions)}" else res;
         |                     ^
       14|   showWarnings = res: fold (w: x: builtins.trace "warning: ${w}" x) res config.warnings;

   … while calling 'g'

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/attrsets.nix:599:19:

      598|           g =
      599|             name: value:
         |                   ^
      600|             if isAttrs value && cond value

   … from call site

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/attrsets.nix:602:20:

      601|               then recurse (path ++ [name]) value
      602|               else f (path ++ [name]) value;
         |                    ^
      603|         in mapAttrs g;

   … while calling anonymous lambda

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/modules.nix:242:72:

      241|           # For definitions that have an associated option
      242|           declaredConfig = mapAttrsRecursiveCond (v: ! isOption v) (_: v: v.value) options;
         |                                                                        ^
      243|

   … while evaluating the option `assertions':

   … while calling anonymous lambda

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/modules.nix:783:28:

      782|         # Process mkMerge and mkIf properties.
      783|         defs' = concatMap (m:
         |                            ^
      784|           map (value: { inherit (m) file; inherit value; }) (builtins.addErrorContext "while evaluating definitions from `${m.file}':" (dischargeProperties m.value))

   … while evaluating definitions from `/nix/store/4dknln6rsv5kbyiiymk5v1pvans0bf7k-source/nixos/common.nix':

   … from call site

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/modules.nix:784:137:

      783|         defs' = concatMap (m:
      784|           map (value: { inherit (m) file; inherit value; }) (builtins.addErrorContext "while evaluating definitions from `${m.file}':" (dischargeProperties m.value))
         |                                                                                                                                         ^
      785|         ) defs;

   … while calling 'dischargeProperties'

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/modules.nix:855:25:

      854|   */
      855|   dischargeProperties = def:
         |                         ^
      856|     if def._type or "" == "merge" then

   … while calling 'g'

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/attrsets.nix:599:19:

      598|           g =
      599|             name: value:
         |                   ^
      600|             if isAttrs value && cond value

   … from call site

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/attrsets.nix:602:20:

      601|               then recurse (path ++ [name]) value
      602|               else f (path ++ [name]) value;
         |                    ^
      603|         in mapAttrs g;

   … while calling anonymous lambda

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/modules.nix:242:72:

      241|           # For definitions that have an associated option
      242|           declaredConfig = mapAttrsRecursiveCond (v: ! isOption v) (_: v: v.value) options;
         |                                                                        ^
      243|

   … while evaluating the option `home-manager.users':

   … while calling anonymous lambda

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/modules.nix:783:28:

      782|         # Process mkMerge and mkIf properties.
      783|         defs' = concatMap (m:
         |                            ^
      784|           map (value: { inherit (m) file; inherit value; }) (builtins.addErrorContext "while evaluating definitions from `${m.file}':" (dischargeProperties m.value))

   … while evaluating definitions from `<unknown-file>':

   … from call site

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/modules.nix:784:137:

      783|         defs' = concatMap (m:
      784|           map (value: { inherit (m) file; inherit value; }) (builtins.addErrorContext "while evaluating definitions from `${m.file}':" (dischargeProperties m.value))
         |                                                                                                                                         ^
      785|         ) defs;

   … while calling 'dischargeProperties'

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/modules.nix:855:25:

      854|   */
      855|   dischargeProperties = def:
         |                         ^
      856|     if def._type or "" == "merge" then

   … while calling 'g'

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/attrsets.nix:599:19:

      598|           g =
      599|             name: value:
         |                   ^
      600|             if isAttrs value && cond value

   … while calling anonymous lambda

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/modules.nix:668:37:

      667|
      668|       matchedOptions = mapAttrs (n: v: v.matchedOptions) resultsByName;
         |                                     ^
      669|

   … from call site

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/modules.nix:638:32:

      637|             in {
      638|               matchedOptions = evalOptionValue loc opt defns';
         |                                ^
      639|               unmatchedDefns = [];

   … while calling 'evalOptionValue'

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/modules.nix:739:31:

      738|      config value. */
      739|   evalOptionValue = loc: opt: defs:
         |                               ^
      740|     let

   … from call site

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/modules.nix:764:9:

      763|       warnDeprecation =
      764|         warnIf (opt.type.deprecationMessage != null)
         |         ^
      765|           "The type `types.${opt.type.name}' of option `${showOption loc}' defined in ${showFiles opt.declarations} is deprecated. ${opt.type.deprecationMessage}";

   … while calling 'warnIf'

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/trivial.nix:357:18:

      356|   */
      357|   warnIf = cond: msg: if cond then warn msg else x: x;
         |                  ^
      358|

   … from call site

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/modules.nix:636:23:

      635|           if length optionDecls == length decls then
      636|             let opt = fixupOptionType loc (mergeOptionDecls loc decls);
         |                       ^
      637|             in {

   … while calling 'fixupOptionType'

     at /nix/store/sralzj4w34v4mwr0sxq417b4ys202in3-source/lib/modules.nix:916:26:

      915|   # TODO: Merge this into mergeOptionDecls
      916|   fixupOptionType = loc: opt:
         |                          ^
      917|     if opt.type.getSubModules or null == null

   error: The type `types.string` is deprecated. See https://github.com/NixOS/nixpkgs/pull/66346 for better alternative types.

roberth · 2023-08-10T21:55:33Z

If you get the error through docs generation, you'll want to use Nix master (or 2.18 when released) in which I've added the relevant attribute path to the --show-trace.
Otherwise the trace does not seem to contain sufficient context.
Examples

emilazy · 2023-08-10T22:29:30Z

I'm not sure if this would be related to docs generation; the nix-darwin and Home Manager manuals don't include user-imported modules. In this case it was the configuration of a user who was having unrelated issues I was debugging that happened to have a local module with a nullOr string type. It seems like it's going through assertions machinery, though, so maybe there's something nix-darwin is or isn't doing here that produces worse results than NixOS.

infinisil · 2023-08-11T00:57:56Z

I hacked on improved recursive type deprecation today, which does indeed work correctly and shouldn't infinite rec: #248444

Michael-J-Ward · 2023-08-11T11:30:50Z

If it's useful, here's the backtrace that (I think) originates from home-manager.

https://gist.github.com/Michael-J-Ward/b9a7e4a5053481cd97a225d23340a777

And I'd like to verify an assumption of mine: should I expect --show-backtrace to include some line from my own *.nix files?

roberth · 2023-08-11T19:13:33Z

If it's useful, here's the backtrace that (I think) originates from home-manager.

Correct. Specifically it is triggered via the manual which is generated from the options.

Nix master may help get the relevant attr reflecting the option path, inside the intermediate json file Change types.string error to a warning instead #248278 (comment)

should I expect --show-trace to include some line from my own *.nix files?

It depends. You'd have to declare an option in such a file instead of just defining values.

NixOS/nixpkgs#248278

infinisil added 2 commits August 10, 2023 06:25

lib.types.string: Use lib.warn instead of deprecationMessage

5075a51

This will cause a poorer error message without option location information, but it will catch all uses of its use.

infinisil requested review from emilazy, jian-lin and roberth August 10, 2023 04:39

infinisil requested a review from edolstra as a code owner August 10, 2023 04:39

github-actions bot added 6.topic: module system About "NixOS" module system internals 6.topic: lib The Nixpkgs function library labels Aug 10, 2023

This was referenced Aug 10, 2023

lib.types.string: Deprecation error instead of warning #247848

Merged

treewide: stop using types.string #247937

Merged

emilazy approved these changes Aug 10, 2023

View reviewed changes

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Aug 10, 2023

jian-lin approved these changes Aug 10, 2023

View reviewed changes

infinisil merged commit 6cd54de into NixOS:master Aug 10, 2023

infinisil deleted the revert-strings-error branch August 10, 2023 17:35

rubas mentioned this pull request Aug 13, 2023

Replace types.string with str malob/nix-config#17

Merged

malob pushed a commit to malob/nix-config that referenced this pull request Sep 11, 2023

Update users.nix

9fb2533

NixOS/nixpkgs#248278

infinisil mentioned this pull request Sep 19, 2023

Cannot override options with type of attrs #85508

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Change `types.string` error to a warning instead#248278

Change `types.string` error to a warning instead#248278
infinisil merged 2 commits intoNixOS:masterfrom
infinisil:revert-strings-error

infinisil commented Aug 10, 2023

Uh oh!

emilazy left a comment

Uh oh!

roberth commented Aug 10, 2023

Uh oh!

infinisil commented Aug 10, 2023

Uh oh!

Michael-J-Ward commented Aug 10, 2023

Uh oh!

infinisil commented Aug 10, 2023

Uh oh!

emilazy commented Aug 10, 2023

Uh oh!

emilazy commented Aug 10, 2023

Uh oh!

roberth commented Aug 10, 2023

Uh oh!

emilazy commented Aug 10, 2023

Uh oh!

infinisil commented Aug 11, 2023

Uh oh!

Michael-J-Ward commented Aug 11, 2023 •

edited

Loading

Uh oh!

roberth commented Aug 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

infinisil commented Aug 10, 2023

Description of changes

Uh oh!

emilazy left a comment

Choose a reason for hiding this comment

Uh oh!

roberth commented Aug 10, 2023

Uh oh!

infinisil commented Aug 10, 2023

Uh oh!

Michael-J-Ward commented Aug 10, 2023

Uh oh!

infinisil commented Aug 10, 2023

Uh oh!

emilazy commented Aug 10, 2023

Uh oh!

emilazy commented Aug 10, 2023

Uh oh!

roberth commented Aug 10, 2023

Uh oh!

emilazy commented Aug 10, 2023

Uh oh!

infinisil commented Aug 11, 2023

Uh oh!

Michael-J-Ward commented Aug 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roberth commented Aug 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Michael-J-Ward commented Aug 11, 2023 •

edited

Loading