Fix/deprecate freeform type bad type#439189
Conversation
lib/types.nix
Outdated
There was a problem hiding this comment.
| "deprecation warning: while accessing option ${showOption loc}: (t.merge.v2 defs).value must only be accessed when `.headError == null`. This is a bug in code that consumes a module system type." | |
| "deprecation warning: while accessing option ${showOption loc}: (t.merge.v2 defs).value must only be accessed when `.headError == null`. This means the type is incorrect and will throw an error on the next release." |
Or something like this. "a bug in code that consumes a module system type" isn't really actionable.
As a user i would like to know that i need to fix my type. This gives the impression that there is nothing to be done from a users side.
There was a problem hiding this comment.
This is a niche warning that only triggers if users call it in other circumstances than freeformType.
The other message gets triggered for that.
See my commit messages.
hsjobeki
left a comment
There was a problem hiding this comment.
I'm not sure if we should print non-actionable error/warning messages
Please read my comment and/or commit messages
There was a problem hiding this comment.
I think i am in favor of this solution rather than #438558. The risk of changing merge further is currently to big, and feels a bit rushed, while this is the correct solution. Use mergeDefinitions fixing the merge behavior of freeform.
But also changing the freeform merge has the risk to reveal other regressions. That relied on freeform beeing unchecked.
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/breaking-changes-announcement-for-unstable/17574/97 |
83e749e to
8a99203
Compare
This helps users fix their code. Found in NixOS#438459 A nicer fix for the freeformType comes in the next commit.
…te bad types Allow bad types (not in attrsOf) to be used in freeformType again, but add a deprecation message. lib.either is special-cased, because it provides a less useful message for its own problem, which is best to ignore when that occurs in a freeformType.
8a99203 to
c58629c
Compare
This way we avoid blasting users with a ton of useless context many times in the same eval.
|
I think i am a bit afraid of this PR could change the semantics of freeformType Maybe we should reconsider a two step solution? enabling check for freeformType could introduce more hidden regressions. Step 1 could be to just remove the |
I believe this provides a clearer fix with fewer workarounds.
We may further simplify this by removing the first commit, which is a very niche corner case that might not even help anyone.
TODO
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.