Recursive type deprecation #99132
Merged
Recursive type deprecation #99132
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
infinisil
force-pushed
the
recursive-type-deprecation
branch
from
1eac859
to
258b3a6
Compare
|
I’m not enough of a type system wizard to review this, but maybe @nbp can take a look? |
Profpatsch
reviewed
Oct 16, 2020
roberth
reviewed
Oct 17, 2020
|
I marked this as stale due to inactivity. → More info |
stale
bot
added
the
2.status: stale
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
infinisil
force-pushed
the
recursive-type-deprecation
branch
from
258b3a6
to
8b957e3
Compare
stale
bot
removed
the
2.status: stale
roberth
approved these changes
May 4, 2021
Nice, it's actually working! |
ofborg
bot
added
10.rebuild-linux: 1-10
10.rebuild-linux: 1
and removed
10.rebuild-linux: 0
labels
|
OfBorg catches it, nice! |
|
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) |
|
In Nixpkgs, recursive types also set |
roberth
reviewed
May 5, 2021
| 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 |
There was a problem hiding this comment.
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.
This was referenced May 5, 2021
This was referenced Aug 10, 2023
infinisil
added a commit
to tweag/nixpkgs
that referenced
this pull request
Aug 10, 2023
…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
added a commit
to tweag/nixpkgs
that referenced
this pull request
Aug 11, 2023
…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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation for this change
In #97114 better type deprecation messages were introduced. However they didn't trigger for nested types. So for example
wouldn't warn about
stringbeing deprecated, which is no good! This PR fixes this, you will get the proper warning now:To be able to get this working properly, these steps were necessary:
options.foo.type.elemTypeas an option type and propagate its value as an attribute of thefoooption.Note: For some types,
.functor.wrappedcontains subtypes as well. This however can't be reused because that doesn't contain all the subtypes. And justifiably so, because.functoris only necessary for type merging.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