AndersonTorres
commented
1 year ago Open AndersonTorres opened 1 year ago
AndersonTorres
commented
1 year ago 
SuperSandro2000
commented
1 year ago A prime example of over used with is https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/services/x11/desktop-managers/plasma5.nix#L282-L285
It is unclear what package is using which with and most packages could come from multiple and some even from all.
Those kinds of usages must be cleared up.
The problem is not when with is used in meta where it is very local and only covering a few lines. I am completely against changing those treewide.
The examples given on nixos.dev are also not great to say it diplomatic.
For example the following code snippet is backwards and is making the code longer and harder to understand especially for beginners which only start to learn the language. I know that I can explain the first line to a beginner and they will understand it without much trouble. The suggested alternative I would need to lookup first and then I would expect a beginner to have some trouble and a harder time to understand what is happening. If I show them both I would expect them to ask why not the first one is used and my answer would be "I don't get it either why this was changed, makes no sense to me 🤷🏼"
# instead of:
buildInputs = with pkgs; [ curl jq ];
# try this instead:
buildInputs = builtins.attrValues {
inherit (pkgs) curl jq;
};with lib in modules is a bit harder. Having it span over the entire file is not ideal but I understand why people are doing it because lib is used a lot in modules. Keeping it for options or functions locally which use a lot of lib is totally fine to me especially if it is more than 2 or 3 occurrences.
Inheriting function from lib near the top of a file is more busy work for me and not improving much. Why do I need to keep track which functions from lib are used? Why do I need to scroll up and manually update the list? Why not just use with lib or write lib. before the function?
Those lines are often not updated when the usage of a function is removed and it is also hard to review by eye.
You could argue that is the same for packages but packages have an actual build and/or runtime cost in comparison to lib functions which are, except evaluation time, free. Also mixing different package sets from the same ecosystem can be dangerous like multiple python versions. Mixing functions from lib worst case increases evaluation time and if code logic is not changed, can't break a package.
Shawn8901
commented
1 year ago Hi!
i am also removing a ton of global with lib; from pkgs the initial pr was https://github.com/NixOS/nixpkgs/pull/211241 and i am currently splitting it down and most changes already have been merged.
On my current pattern i add a with lib; in the meta attr in case that there is not yet any other usage inside meta (if so i do the same), i am now a bit wondering if i should continue on that pattern for meta or switch to something else?
edit: i should read Sandros comment more carefully as it already asks that question.
AndersonTorres
commented
1 year ago The problem is not when with is used in meta where it is very local and only covering a few lines. I am completely against changing those treewide.
I am not happy with such uses because it is easy to extrapolate them. with should be regarded as an exception and not a rule.
For example the following code snippet is backwards and is making the code longer and harder to understand especially for beginners which only start to learn the language.
Day after day you youself justify many seemingly equivalent pieces of code as "do this the other way because it will not break splicing", or any other obscure reference to Nixpkgs internals.
Do you think a beginner that is trying to package a Python library will understand why there are seemingly equivalent appearances of python3.pkgs and python3Packages around the codebase and be OK with a mere "splicing"?
I could buy the "shorter and easier to understand" idea right before the "beginner" part. Hell, day after day I pick beginners failing to understanding the CONTRIBUTING guide! Definitely we have way more problems with beginners than longer codes.
Just to be sure: I am not being elitistic about the beginners above (sometimes I am, but not here and now). It is completely the reverse - our documentation is still a bit far from beginner-friendliness.
If I show them both I would expect them to ask why not the first one is used and my answer would be "I don't get it either why this was changed, makes no sense to me 🤷🏼"
I always link the nix.dev anti patterns guide - a NixOS-backed reference.
About the example below
# instead of: buildInputs = with pkgs; [ curl jq ]; # try this instead: buildInputs = builtins.attrValues { inherit (pkgs) curl jq; };
The aforementioned "beginners" you cited write, without hesitation, code like
{pkgs}:
with pkgs;
stdenv.mkDerivation rec {
. . .
buildInputs = [ jq curl ];
. . .
}This is a typical example of a beginner code trying to be merged into Nixpkgs.
Certainly, the Nixpkgs way below:
{ lib
, stdenv
, curl
, jq
}:
stdenv.mkDerivation rec {
. . .
buildInputs = [ jq curl ];
. . .
}is what I would call longer and harder to understand especially for beginners which only start to learn the language.
Inheriting function from lib near the top of a file is more busy work for me and not improving much. Why do I need to keep track which functions from lib are used? Why do I need to scroll up and manually update the list? Why not just use with lib or write lib. before the function?
I explained this a hundred times in a row: that was a draft - a draft, subject to incremental, future polishing.
eclairevoyant
commented
1 year ago I am not happy with such uses because it is easy to extrapolate them. with should be regarded as an exception and not a rule.
Then the nixpkgs manual itself should be changed; as it is, it liberally uses meta = with lib; - see https://nixos.org/manual/nixpkgs/unstable/#chap-meta.
The nix.dev antipatterns guide warns against using with at the top of a Nix file - a reasonable suggestion, but does not mention anything about using with in the very predictable/standardised spot of only in meta.
AndersonTorres
commented
1 year ago It is not as if I disagree with this. Indeed I planned to touch on documentation after the code, but I changed my mind. I will modify the docs asap
philiptaron
commented
7 months ago I'm in the process of making a PR to refactor away most uses of with lib; at the first column (aka rg "^with lib;") and replace them with
let
inherit (lib) <... uses of things from lib in the file ...>;
in <... rest of the file>This lets the code stay compact (no adding lib. where it was short before) but clarifying where the names are coming from and avoiding the with-at-top-scope antipattern.
PRs:
AndersonTorres
commented
7 months ago @philiptaron many thanks! Certainly there are a truckload of them inside nixos dir, it will be a huge work!
philiptaron
commented
6 months ago I have a branch with all the top-level with lib; instances removed: https://github.com/philiptaron/nixpkgs/tree/remove-toplevel-with-lib
The only question now is how to get it merged. I'm thinking PR by PR currently.
nixos-discourse
commented
6 months ago This issue has been mentioned on NixOS Discourse. There might be relevant details there:
https://discourse.nixos.org/t/removing-most-uses-of-top-level-with/41233/1
AndersonTorres
commented
6 months ago Hall of Fame:
philiptaron
commented
6 months ago Hall of Fame:
295975
You might like https://github.com/NixOS/nixpkgs/pull/299245/commits/2c47e6068cb6b59fe3d2d6aad05d1974d988ccec too.
inclyc
commented
5 months ago Since this syntax is much unintuitive, I just implemented a diagnostic for this:
inclyc
commented
5 months ago Would you like to have a "code action" based on AST, instead of having pattern matching stuff?
philiptaron
commented
5 months ago Would you like to have a "code action" based on AST, instead of having pattern matching stuff?
Absolutely. I'd love a codemod style thing.
nbraud
commented
5 months ago Would you like to have a "code action" based on AST, instead of having pattern matching stuff?
A parser-based approach would be incredibly useful, if only because it would eliminate many potential false positives. As I commented on the documentation issue:
I think it's still fine (and quite ergonomic) to use with... outside of implicitly recursive attrsets, which have other issues anyways when it comes to derivations, like overrides not behaving as one could hope.
From that point of view, wouldn't it make more sense to deprecate the mkDerivation rec { ... } antipattern instead?
jtojnar
commented
3 months ago
with libin modules is a bit harder. Having it span over the entire file is not ideal but I understand why people are doing it because lib is used a lot in modules. Keeping it for options or functions locally which use a lot of lib is totally fine to me especially if it is more than 2 or 3 occurrences.Inheriting function from lib near the top of a file is more busy work for me and not improving much. Why do I need to keep track which functions from lib are used? Why do I need to scroll up and manually update the list? Why not just use
with libor writelib.before the function?
By necessity, with essentially turns symbol resolution within lazy. As a result, it masks errors that would normally occur when referencing a non-existent symbol in unevaluated branches of code. This undermines our trust in correctness of any non-trivial Nix expression such as a NixOS module, since those will likely contain extensive branching.
For instance, the following expression containing with will evaluate just fine in the default non-borked branch:
{ borked ? false }:
let
a = 1;
in
with { };
if !borked then
a
else
aa + 1But if we evaluate the borked branch, it reveals “undefined variable 'b'” error.
Getting rid of the with expression will make the error obvious in either branch, thus making it much more likely to be caught.
Or, if you want a real world example, see https://github.com/NixOS/nixpkgs/pull/308033.
mibmo
commented
1 month ago SuperSandro2000: The problem is not when with is used in meta where it is very local and only covering a few lines. I am completely against changing those treewide.
AndersonTorres: I am not happy with such uses because it is easy to extrapolate them.
withshould be regarded as an exception and not a rule.
Usage inside of meta is exactly where with shines.
{
...
meta = with lib; {
maintainers = with maintainers; [ many different people ];
license = with licenses; [ mpl20 lgpl21Plus lgpl3Plus free ];
...
};
}also types
let inherit (lib) mkOption types; in
mkOption {
# a proper example with submodules would demonstrate this **much** better, but _i am lazy._
type = with types; listOf (coercedTo int toString str);
description = ''My complicated submodule option'';
}The meta section as well as mkOption types should be clear exceptions to the "rule".
emilazy
commented
1 month ago In my opinion, complex submodule types are exactly a case where with is tempting but harmful due to the lack of scope checking and potential name confusion. meta is whatever by comparison.
AndersonTorres
commented
1 month ago Usage in meta shadows free variables. It was discussed before:
https://github.com/nix-community/nix-init/pull/292
https://github.com/NixOS/nixpkgs/issues/292468#issuecomment-2016454242
emilazy
commented
1 month ago Okay, version is pretty bad. Though with lib.maintainers and with lib.licenses are still probably fine. (I don’t prefer to use them myself, but I think it’d be pointlessly antisocial to nitpick others doing so.)
mibmo
commented
1 month ago In my opinion, complex submodule types are exactly a case where
withis tempting but harmful due to the lack of scope checking and potential name confusion.metais whatever by comparison.
It's only an issue with submodules if you don't "split" them, right? i guess that's not a requirement, but it's a very common pattern afaict, i.e.
let
inherit (lib) mkOption types;
somethingModule = {};
otherModule = {};
in
{
options.parent = mkOption {
# dunno if you even _can_ use `either` with submodules, but this is just an example to show the pattern :) just see, idunno, the nginx module.
type = with types; either (submodule somethingModule) (submodule otherModule);
};
}I don’t prefer to use them myself, but I think it’d be pointlessly antisocial to nitpick others doing so.
I agree. Honestly, I just don't want to lose with types; (for cases where scoping is a non-issue) and with lib.{maintainers,license};. That's essentially the only place I see with being useful anyway (other than actual nixosConfigurations where e.g. making package lists is a very normal ask1, but even then it can sometimes come back to bite you...).
1. I wouldn't really consider lib.attrValues { inherit (pkgs) ... ;} a nice nor beginner-friendly pattern (and it seems this whole with debate mostly concerns not overcomplicating it for newcomers)
mibmo
commented
1 month ago Okay,
versionis pretty bad. Thoughwith lib.maintainersandwith lib.licensesare still probably fine. (I don’t prefer to use them myself, but I think it’d be pointlessly antisocial to nitpick others doing so.)
Yeah, that is... not good. nixd would've complained a lot as mentioned in the thread (in favor of using lib.version though), so it's possible it could have been caught like that, but regardless this is very not ideal...
inclyc
commented
1 month ago Okay,
versionis pretty bad. Thoughwith lib.maintainersandwith lib.licensesare still probably fine. (I don’t prefer to use them myself, but I think it’d be pointlessly antisocial to nitpick others doing so.)Yeah, that is... not good.
nixdwould've complained a lot as mentioned in the thread (in favor of usinglib.versionthough), so it's possible it could have been caught like that, but regardless this is very not ideal...
That diagnostic was removed in https://github.com/nix-community/nixd/pull/582. I'm currently awaiting a consensus on this issue to proceed with reimplementation.
eadwu
commented
2 weeks ago Is there any performance benefit from this?
I don't really like this change, you're basically asking people to explicitly either enumerate all the functions they use from a set or always add lib. to it. I agree that it is harder for someone who is not involved or just starting out to figure out the ancestry, but this should be a maintainer's decision, rather than an enforcement?
The only way this probably should be enforced if you had something like Go which "auto-imports" the module you use so it is transparent to the editor if there are no performance implications.
It's easy to do after writing the module, but when initially writing the library it's just a pain in the ass without with lib; and becomes annoying rather quickly.
Unrelated to the with lib; but the documentation changes have also been rather been annoying, if I recall correctly the enforcement of lib.mdDoc broke all my local modules back when it was introduced and now it's going away which also decided to spam a bunch of warnings, to the point I don't even write descriptions for custom modules anymore, or do it in comments.
SuperSandro2000
commented
2 weeks ago Is there any performance benefit from this?
Depends. Inheriting things from lib inside of lib yields small performance improvements which stacks up when a function is called 10 000 times.
but this should be a maintainer's decision, rather than an enforcement?
If you are working in a shared repo where you often read other peoples code, we need enforcements to write similar quality code, otherwise every module would look different and it would be way harder to understand things.
The only way this probably should be enforced if you had something like Go which "auto-imports" the module you use so it is transparent to the editor if there are no performance implications.
That is done by your editor or go mod tidy. When running go build it will complain about missing things. Also it only does that when the module is already imported by go.mod and not ambiguous.
when initially writing the library it's just a pain in the ass without with lib; and becomes annoying rather quickly.
Prefixing library functions with lib. is not much more work and compared to ambiguities which nested with's can cause that is worth it.
the enforcement of lib.mdDoc broke all my local modules back when it was introduced and now it's going away which also decided to spam a bunch of warnings
We changed the format of the descriptions from docbook to markdown. Unless you write plain text, the formatting would be wrong. If you converted the format already, removing lib.mdDoc is just one sed call away sed -i -e 's/lib.mdDoc //g' -e 's/mdDoc //g'.
eadwu
commented
2 weeks ago Nothing is just 1 sed call away, that's how you get random errors you don't know the reason of. I don't like having to check a large number of diffs to make sure I haven't use mdDoc anywhere it wasn't suppose to be used no matter how unlikely it is, especially for something that hasn't changed since it was committed (description).
Prefixing 1 with lib. isn't much work, but if you have a large number of options adding lib. it adds up (I'm looking at lib.types), and it isn't just a 1 sed away this time.
If inheriting provides the improvements I don't think the majority of the PRs even fulfill that bar. Adding enforcements that are not automatically handled raises the bar for contribution, which is not something you'd want to increase even more for Nix, keep it to things that are automatically handled, like nixpkgs-fmt.
SuperSandro2000
commented
2 weeks ago Nothing is just 1 sed call away, that's how you get random errors you don't know the reason of. I don't like having to check a large number of diffs to make sure I haven't use mdDoc anywhere it wasn't suppose to be used no matter how unlikely it is, especially for something that hasn't changed since it was committed (description).
I encourage you to look at https://github.com/NixOS/nixpkgs/pull/303841 to see how we did it.
Also I would like to keep this issue somewhat on topic and about with lib and not past treewide changes.
Prefixing 1 with
lib.isn't much work, but if you have a large number of options addinglib.it adds up (I'm looking atlib.types), and it isn't just a 1 sed away this time.
For that reason it is also acceptable to use inherit and currently what to use is a maintainer decision.
If inheriting provides the improvements I don't think the majority of the PRs even fulfill that bar.
It is very small and for all places which are not called a thousand times (so basically everything outside lib places) it is negligible. The lib areas have special rules anyway because of their special purpose.
Adding enforcements that are not automatically handled raises the bar for contribution
We have tooling like nixf-tidy https://github.com/nix-community/nixd/tree/main/libnixf#nixf-tidy that can provide that for common patterns but someone needs to code a high quality linter that detects those things and reports them. I personally lack completely the knowledge to write something like that but maybe you can help here out?
AndersonTorres
commented
2 weeks ago Is there any performance benefit from this?
Definitely yes: with requires a round of evaluation, while the explicit inherit does not.
I don't really like this change, you're basically asking people to explicitly either enumerate all the functions they use from a set or always add
lib.to it.
with is all-or-all, it does not provide the granularity of inherit.
The enumeration of all used functions is at least justified, because many languages like Python and C do this in some degree.
I agree that it is harder for someone who is not involved or just starting out to figure out the ancestry, but this should be a maintainer's decision, rather than an enforcement?
This is not a simple matter of taste, it can change the evaluation result, as eclairevoyant (In Memorian) has shown a long time ago. Summarizing their discovery, version can come from package and from lib too.
Issue description
There are many overused
with lib;expressions on Nixpkgs, especially on NixOS modules. As explained in this link, it is a problematic anti-pattern.Steps to reproduce
rg "with lib;"or anything like that.Technical details
"x86_64-linux"Linux 5.15.83, NixOS, 23.05 (Stoat), 23.05.20221222.7ba7b45yesyesnix-env (Nix) 2.12.0/nix/var/nix/profiles/per-user/root/channels/nixosReferences
https://nix.dev/anti-patterns/language#with-attrset-expression
https://github.com/NixOS/nix/issues/490
https://github.com/NixOS/nixpkgs/pull/305642