-
-
Notifications
You must be signed in to change notification settings - Fork 159
[RFC 0190] Ban with
expression in nixpkgs
#190
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
ef44661
to
b4711e6
Compare
PS: this rfc is probably still improvable - it's my first one :D and i want to mention RFC 0120 as it goes into the same direction. I think we should just be more strict to have better code linting opportunities |
PS: PS: I'll misspell things a lot etc ... so disclousure claude.ai did helped me a lot with creating the TEXT, the intention&idea is mine and i reviewed and adjusted it myselfe ... so if it looks like AI, it's probably relicts I did let stay because i dont know how to formulate it better |
inherit (pkgs.lib.licenses) mit; | ||
in { | ||
meta = { | ||
license = [ mit ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
until now, with lib.licenses
was OK on the last "level". What did change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nothing jet, just proposing to not use with
in nixpkgs
and this are examples, i would jsut use the full prefix lib.licenses.mit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nothing jet, just proposing to not use with in nixpkgs
What I want to get at is, the benefits/downsides you mentioned apply to with
at high levels (or nested withs), but having it on the last level was the "sweet-spot" a lot of people seemed to have agreed on. So I'd expect thsi RFC to also give good reasons why this must also go because that's effectively the last remaining use with
in new code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would jsut use the full prefix
lib.licenses.mit
This would get really annoying when a project has multiple licenses. What do you propose for those instances? I think the following looks really ugly.
license = [
lib.licenses.mit
lib.licenses.some-custom-license
];
As compared to:
license = with lib.licenses; [ mit some-custom-license ];
All in all, I don't see an issue with using with
at the last level of scopes. It doesn't hinder readability to warrant "banning" it, if at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps a better approach would be some sort of linting action that shows a warn or even errors on newly changed paths if it detects with
on attributes that are not at the last level? Might be hard to implement as it would completely break if it actually eval'd the items
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I'd expect thsi RFC to also give good reasons why this must also go because that's effectively the last remaining use with in new code.
i change the rfc to propose an helper to not get ugly code ;)
but as of right now with:
license = with lib.licenses; [ mit some-custom-license ];
you expect all items to come from lib.licenses right?
but technikally that has to be not the case!
some-custom-license can be defined elsewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, then please be selective about it. Unless there is a second, non-top-level with
(which is fine to forbid), some-custom-license
must either be defined in lib.licenses
or within the current file. The latter is trivially detectable with some static analysis, and thus there is no need to indiscriminately forbid the general case altogether
let | ||
inherit (pkgs) git vim; | ||
in { | ||
buildInputs = [ git vim ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be super annoying to type 🫠
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In cases like this I would much rather:
{
buildInputs = [
pkgs.git
pkgs.vim
];
}
If I am going to repeat something I may as well repeat the path and get the benefit of being able to see a more qualified source locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the examples could be improved. Because I'd use more inline usages > jumping to an inherit variable unless the usage becomes great enough to look cleaner from it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well if it's a package that should be in the inputs ... so you have them standing twice anyway
else just use pkgs.
prefix if you dont wat to write them twice?
- **Learning curve**: Contributors familiar with `with` need to adapt to new patterns | ||
- **Backward incompatible**: requires changing Nix code used "in the wild" | ||
|
||
However, these drawbacks are outweighed by the benefits of clearer, more maintainable code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is true for a top-level with lib;
, but I don't really see the problem with the current de-facto rule, only allowing with
on the last level of an expression. In that case a symbol comes either from the with
or its parent scope (which doesn't have any further with
s).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think a carve-out for small local scopes even as small as with x; [ a b c ]
would be appropriate. This is commonly seen in licences, buildInputs, maintainers.
I am generally a fan of being explicit but in these case it just seems worth it and the risk of confusion is low.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I have tried to migrate everything I touch to an inherit > with style. But, have no problems with last level with
and use it often myself because there's no obfuscation of where it's coming from, at that point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well tjats the problem you expect implizite that the elements in the list come from within the scope colapsed by with but it does not have to!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but i get why its very handy and i think if it's your own code and you dont colab its fine as you know what you do?
but for nixpkgs we should be explizite to get better code quality. why not have a simple helper to still get handy simple lists?
lib.elements = stringList: map:
builtins.map (key: map.${key}) stringList;
this func will fail if you specify an item that is not comming from the map -> explizite & matches the expectations we use the with to list e.g. packages ...
let
myMap = { a = 1; b = 2; c = 3; d = 4; };
keys = [ "a" "c" "d" ]; # all keys exist
in
lib.elements keys myMap
# Result: [ 1 3 4 ]
|
||
# After | ||
let | ||
inherit (pkgs) git vim curl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget to show an example with > 4 items, where nixfmt will set one item per line, e.g.
let
inherit (pkgs)
git
vim
curl
htop
;
in [
git
vim
curl
htop
]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha yes 🫠
in that case
[
pkgs.git
pkgs.vim
pkgs.curl
pkgs.htop
]
is still more text but not as bad as your ting 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I prefer this. But this is also rare in nixpkgs anyways due to the way callPackage
works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
about lists: #190 (comment)
The `with` expression in Nix creates several problems that impact code quality and tooling: | ||
|
||
- **Unclear variable origins**: When reading code with `with`, it's unclear where variables come from without evaluating the expression | ||
- **Difficult static analysis**: Tools cannot determine variables without running full nix evaluation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is true but barely. There is only ambiguity when there are nested with
expressions. For example:
let
x = 1;
in
with scope;
x
In this case x
is always the 1 from the above binding.
let
x = 1;
in
with scope;
y
In this case y
is always scope.y
.
The only trouble is:
with scope-a;
with scope-b;
x
Now you don't know if this is scope-a.x
or scope-b.x
.
For this point we could just ban nested with
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my main problem is with reviewing stuff that come out of nowhere ... yes nix repl will tell me but ... :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what you mean by "come out of nowhere".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two "problems" mention in the text is inaccurate and only happen on nested with
. It seems the author is criticizing JavaScript's with
while willing to ban Nix's with
. There is a comparison, and the most notable difference is that single-level with
in Nix can always be syntactically (statically) desugared without the need to evaluate what's inside the attrset of with
.
If you are using nil language server, variables from and not from with
are also highlighted differently.
I have many similar lists with tens of elements. Manually prefixing pkgs.
or using inherit
would at least double the code size and hurt readability more than with
by introducing 50%+ syntax noise, not mentioning keystrokes and writability.
It is already a anti-pattern to use nested with
or top-level omnivorous with
that covers too many code. Only banning these complex cases makes sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oxalica I agree! There should be some exceptions, like in the example you pointed out. I use with
for declaring lists of packages pretty frequently and couldn't imagine having to prefix pkgs
to every single package in the list, especially messy when you're dealing with lists that can have over 30 packages 👀
I think there is no real upside anymore. The pitfalls with using I would disagree that With non nested Additionally, in Nixpkgs you rarely have cases where static analysis would help since you will use
In the last case you're maybe less likely to mistype twice, but that doesn't seem like a very convincing argument. |
- **Unclear variable origins**: When reading code with `with`, it's unclear where variables come from without evaluating the expression | ||
- **Difficult static analysis**: Tools cannot determine variables without running full nix evaluation | ||
- **Debugging difficulties**: Error messages become less helpful when variable sources are ambiguous | ||
- **Code review challenges**: Reviewers can not simply relay on a diff view and must read the full code and not miss any scope |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
technically, the proposed inherit
similarly would not allow relying just on a diff view
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true ... will remove that line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am strongly against this. Indiscriminately forbidding with
without understanding where it is actually harmful is unfortunately a Nixpkgs review pattern already. And let's be real, the provided alternatives here are verbose and not pretty.
At the very least, I'd expect this RFC to be very precise about which exact with
patterns are problematic, and then only forbid those through a dedicated linter. But ideally, I think that we should strive to make usage of with
obsolete by providing some true and good alternatives instead of mere workarounds.
# Alternatives | ||
[alternatives]: #alternatives | ||
|
||
Just discourage usage of `with`, but in this case most things still can not be statically checked. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is worthy to mention https://gerrit.lix.systems/c/lix/+/1987/11 (and the following commits as well) as an attempt to provide a safer alternative to the most prominent use cases of with;
I agree with the sentiment that top level TL;DR: I strongly disagree with this RFC being adopted. |
Rendered