Recursive type deprecation #99132

infinisil · 2020-09-30T00:41:52Z

Motivation for this change

In #97114 better type deprecation messages were introduced. However they didn't trigger for nested types. So for example

attrsOf string

wouldn't warn about string being deprecated, which is no good! This PR fixes this, you will get the proper warning now:

trace: warning: The type `types.string' of option `value' defined in `/home/infinisil/src/nixpkgs/config.nix'
  is deprecated. See https://github.com/NixOS/nixpkgs/pull/66346 for better alternative types.

To be able to get this working properly, these steps were necessary:

The option types newly expose their subtypes directly. In addition to making this PR possible, this now also allows easy aliasing of partial options. E.g. you can use options.foo.type.elemType as an option type and propagate its value as an attribute of the foo option.
Note: For some types, .functor.wrapped contains subtypes as well. This however can't be reused because that doesn't contain all the subtypes. And justifiably so, because .functor is only necessary for type merging.
The type deprecation messages are issued recursively for all subtypes of a type as well. To break infinite recursion, we essentially do a breadth-first-search over the subtype graph, with type names being the node identifiers. We track the visited graph nodes with a set of type names, and don't recurse if we've already visited a node.

This does make option evaluation a bit more strict, because to be able to issue deprecation warnings, you need to evaluate all subtypes. This shouldn't be a problem in practice though. If anything, it exposes some errors. Performance will get a little worse, almost not significantly so in my testing though.

Note that 0bf115e doesn't really have anything to do with this PR, other than this PR being a trigger for it.

Ping @roberth @rnhmjoj @rycee @Profpatsch

Things done

Added tests for deprecation message functionality
Do a quick performance measurement -> Doesn't change it much.

lib/tests/modules/type-deprecation.nix

Profpatsch · 2020-10-09T13:17:15Z

I’m not enough of a type system wizard to review this, but maybe @nbp can take a look?

lib/modules.nix

lib/types.nix

stale · 2021-04-18T23:12:52Z

I marked this as stale due to inactivity. → More info

This will be used to issue deprecation warnings recursively in the next commit In addition, this allows easily getting nested types of other options, which is useful when you want to create an option that aliases a part of another one.

In 2d45a62, the submodule type description was amended with the freeformType description. This causes all the modules passed to the submodule to be evaluated once on their own, without any extra definitions from the config section. This means that the specified modules need to be valid on their own, without any undeclared options. This commit adds a test that evaluates a submodules option description, which would trigger the above problem for one of the tests, if it were not fixed by this commit as well. This is done because the next commit makes option evaluation a bit more strict, which would also trigger this test failure, even though it's not related to the change at all.

Previously, an option of type attrsOf string wouldn't throw a deprecation warning, even though the string type is deprecated. This was because the deprecation warning trigger only looked at the type of the option itself, not any of its subtypes. This commit fixes this, causing each of the types deprecationMessages to trigger for the option. This relies on the subtypes mkOptionType attribute introduced in 26607a5a2e06653fec453c83d063cdfc4b59185f

roberth

lgtm. OfBorg failed to eval though, so please look into that.

infinisil · 2021-05-05T01:29:24Z

["trace: warning: The type `types.string\' of option `services.disnix.profiles\' defined in `/var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/ofborg-evaluator-7/nixos/modules/services/misc/disnix.nix\' is deprecated. See https://github.com/NixOS/nixpkgs/pull/66346 for better alternative types."
 "trace: warning: The type `types.string\' of option `services.prometheus.remoteRead.*.name\' defined in `/var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/ofborg-evaluator-7/nixos/modules/services/monitoring/prometheus/default.nix\' is deprecated. See https://github.com/NixOS/nixpkgs/pull/66346 for better alternative types."
 "trace: warning: The type `types.string\' of option `services.prometheus.remoteWrite.*.name\' defined in `/var/lib/ofborg/checkout/repo/38dca4e3aa6bca43ea96d2fcc04e8229/mr-est/ofborg-evaluator-7/nixos/modules/services/monitoring/prometheus/default.nix\' is deprecated. See https://github.com/NixOS/nixpkgs/pull/66346 for better alternative types."]%

Nice, it's actually working!

roberth · 2021-05-05T09:13:20Z

OfBorg catches it, nice!

arcnmx · 2021-05-05T16:35:20Z

Hm this broke my config with an infinite recursion error in like 5 different places and debugging it seems too hard :<

(I'll try to investigate when I can but just reverting locally for now)

roberth · 2021-05-05T16:44:09Z

In Nixpkgs, recursive types also set name in order to terminate. This used to only be required for documentation, but with this PR, users always need to do it.
This is an awful failure mode that does not compare favorably to use of deprecated types. It's best to revert for now.

roberth · 2021-05-05T16:50:26Z

lib/modules.nix

+          maybeWarn = warnIf (type.deprecationMessage != null)
+            "The type `types.${type.name}' of option `${showOption loc}' defined in ${showFiles opt.declarations} is deprecated. ${type.deprecationMessage}";
+        in
+          if visited ? ${type.name} then visited


Being strict in type.name turns out to be a significant problem, leading to incomprehensible errors when users define their own recursive types.
While this is technically a limitation of name, it manifests because of this use of it.

…ely"" This reverts commit a36e676. There was one report saying that this broke a configuration, but I cannot reproduce this: NixOS#99132 (comment) We should give this another go

infinisil requested review from edolstra and nbp as code owners September 30, 2020 00:41

infinisil added the 6.topic: module system About "NixOS" module system internals label Sep 30, 2020

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Sep 30, 2020

worldofpeace reviewed Sep 30, 2020

View reviewed changes

lib/tests/modules/type-deprecation.nix Outdated Show resolved Hide resolved

infinisil force-pushed the recursive-type-deprecation branch from 1eac859 to 258b3a6 Compare September 30, 2020 20:44

Profpatsch reviewed Oct 16, 2020

View reviewed changes

lib/modules.nix Show resolved Hide resolved

roberth reviewed Oct 17, 2020

View reviewed changes

lib/types.nix Outdated Show resolved Hide resolved

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 18, 2021

infinisil added 4 commits May 3, 2021 22:15

lib/tests: Add type deprecation tests

8b957e3

infinisil force-pushed the recursive-type-deprecation branch from 258b3a6 to 8b957e3 Compare May 3, 2021 20:16

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label May 3, 2021

infinisil requested review from roberth and Profpatsch May 3, 2021 20:17

roberth approved these changes May 4, 2021

View reviewed changes

nixos/treewide: Remove usages of deprecated types.string

0a377f1

github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels May 5, 2021

ofborg bot added 10.rebuild-linux: 1-10 10.rebuild-linux: 1 and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels May 5, 2021

roberth merged commit ce93c98 into NixOS:master May 5, 2021

infinisil deleted the recursive-type-deprecation branch May 5, 2021 10:55

roberth reviewed May 5, 2021

View reviewed changes

This was referenced May 5, 2021

Revert 99132 recursive type deprecation #121816

Merged

nixos/unbound: allow list of strings in top-level settings option type #121927

Merged

lapp0 mentioned this pull request Nov 18, 2021

Get Types from lib.nix nix-gui/nix-gui#180

Open

This was referenced Aug 10, 2023

treewide: stop using types.string #247937

Merged

Change types.string error to a warning instead #248278

Merged

infinisil mentioned this pull request Aug 11, 2023

Recursive type deprecation, again #248444

Draft

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

Recursive type deprecation #99132

Recursive type deprecation #99132

infinisil commented Sep 30, 2020 •

edited

Loading

Profpatsch commented Oct 9, 2020

stale bot commented Apr 18, 2021

roberth left a comment

infinisil commented May 5, 2021

roberth commented May 5, 2021

arcnmx commented May 5, 2021

roberth commented May 5, 2021

roberth May 5, 2021

Recursive type deprecation #99132

Recursive type deprecation #99132

Conversation

infinisil commented Sep 30, 2020 • edited Loading

Motivation for this change

Things done

Profpatsch commented Oct 9, 2020

stale bot commented Apr 18, 2021

roberth left a comment

Choose a reason for hiding this comment

infinisil commented May 5, 2021

roberth commented May 5, 2021

arcnmx commented May 5, 2021

roberth commented May 5, 2021

roberth May 5, 2021

Choose a reason for hiding this comment

infinisil commented Sep 30, 2020 •

edited

Loading