[Discussion] How to use linter (nixf) to standardize the usage of with?

[Aleksanaa]

Currently nixf-tidy (and nixd, of course) has some limitations for with:

1. Cannot use variables outside the with expression (let foo = "foo" in with bar; [ foo sthInsideBar ]).
2. Cannot make the with expression unused (with bar; [] with bar; [ sthOutsideBar ]).

The first one seemed a bit strict, and we already have a lot of "anti-patterns" in Nixpkgs, so I disabled it for now. @inclyc also wants to remove this check from nixf itself. The second one is also a bit problematic, because when people abandon packages, they write maintainers = with lib.maintainers; [ ]; which won't go through nixf-tidy, but since this is not so obvious, I choose to leave it alone and wait for others' feedback.

The question now is: can we come up with some better rules?

Some ideas, after discussing with @inclyc

1. Ban meta = with lib;, so if the linter sees meta, it forbids using with lib to enclose the following expression. This is effective but a bit too non-universal
2. Ban with expression on "top level", e.g. at the top of expressions in a file, or directly behind function arguments. This is pretty good, but may not be very friendly to small files.
3. Ban nested with expression at all, like with a; with b;, but it's also not very friendly.
4. Ban nested with, where the second argument comes from the first, like with a; with bFromA, this can also be controversial and requires a bit of evaluation involved, not just parsing

This is what we have come up with so far, hope you can provide more ideas!

Related: #292468

CC @inclyc @AndersonTorres @lolbinarycat @eclairevoyant @infinisil @roberth

[eclairevoyant]

I am still leaning towards banning escaping-with. Not only is the version in meta case specifically relevant, but also if you have the same name in two package sets, it may not be clear to the users that in the below code:

{ foo, bar, someBuilder, pythonPackages }:
someBuilder {
  # ...
  someList = with pythonPackages; [ bar baz ];
}

bar will never come from pythonPackages even if pythonPackages.bar exists, because there is some binding bar already in scope that takes precedence.

If we still want with, it's better to explicitly write:

{ foo, bar, someBuilder, pythonPackages }:
someBuilder {
  # ...
  someList = (with pythonPackages; [ baz ]) ++ [ bar ];
}

And of course if we instead want to get bar from pythonPackages, we wouldn't use with.

IMO we should not permit constructs that invite confusion and make the code harder to read (since the human reading it has to do the "eval", not just "parsing"). I'd rather make the computers do that work where we can.

[Aleksanaa]

I saw this example yesterday and it's kinda reasonable:

nixpkgs/pkgs/os-specific/linux/kernel/linux-rt-5.15.nix

Lines 33 to 41 in b73c222

	structuredExtraConfig = with lib.kernel; {
	PREEMPT_RT = yes;
	# Fix error: unused option: PREEMPT_RT.
	EXPERT = yes; # PREEMPT_RT depends on it (in kernel/Kconfig.preempt)
	# Fix error: option not set correctly: PREEMPT_VOLUNTARY (wanted 'y', got 'n').
	PREEMPT_VOLUNTARY = lib.mkForce no; # PREEMPT_RT deselects it.
	# Fix error: unused option: RT_GROUP_SCHED.
	RT_GROUP_SCHED = lib.mkForce (option no); # Removed by sched-disable-rt-group-sched-on-rt.patch.
	} // structuredExtraConfig;

But as long as lib.mkForce is used inside, it will be escaping with, so not agreed by nixf.

[eclairevoyant]

I think this specific example happens to be fine from a human point of view because most people won't expect lib.kernel.lib.mkForce to be a thing.

However, if we talk about a generalisable rule, what would be the negative to disallowing escaping-with? (Especially if the checks only affect files that passed in the past or new files?)

[emilazy]

let inherit (lib.kernel) option yes no; seems okay‐ish in that case. Although I’d rather just ban with entirely with no exceptions, so my opinions can’t be trusted here.

[roberth]

I don't feel strongly about how we reduce the use of with, and I'd be happy to see any rule implemented.
You could even start with banning triple with, or immediate nestings of it, ie matching with a; with b; c, but not with a; x: with b; x foo bar.
Those are simple rules that I think would be uncontroversial, and gives you a starting point for more effective rules, and you might find something along the way.

[Atemu]

Anecdotally, I let nixf-tidy loose on my nixos-config yesterday and refactored away all instances of ambiguous with references. Subjectively, I think my code is a lot better for it.

Compare this example before:

https://github.com/Atemu/nixos-config/blob/48a40cecdf2c1212a0e59384ae5bf4b1f6903e97/modules/btrfs/module.nix#L123-L127

vs. after:

https://github.com/Atemu/nixos-config/blob/3ef3385eebbf02b65fce1f3f3d68a1ed52ccf3e9/modules/btrfs/module.nix#L121-L127

Not relying on with forced me to write better code here and I think that's a good thing.

I don't think strict with linters should be enabled in Nixpkgs by default though as there is wayy to much precedent for it and I don't see a pressing need to have people be forced to change this.
What we could do however is enforce it for new files and files that passed the check before, similar to the recently introduced formatting check.

Enabling strict linter rules generally feels like the sort of thing that would require an RFC to me but OTOH I also feel like, if there are very good reasons for the rules, we should just try them out. If a significant amount of people take significant issue with that and/or have strong opinions on it, we can always remove the check again and only then bother with an RFC.

[Aleksanaa]

What we could do however is enforce it for new files and files that passed the check before, similar to the recently introduced formatting check.

Yes, I'm refactoring the checker, and it should filter new type of messages and messages popping up on changed lines. Currently, if you turn it on, more files will skip checking, which is not conducive to our goal of removing unused variables.

[inclyc]

Overall, I believe that the escaping-with diagnostic can sometimes be overly pedantic. However, in other cases, it does help us detect errors in new pull requests.

Therefore, it might be beneficial to establish a set of rules that permit certain "acceptable" patterns. For instance, using lib and pkgs in with xxx; is widely accepted by the Nix community. We can almost always assume these bindings refer directly to nixpkgs/lib and nixpkgs itself.

[eclairevoyant]

with pkgs; maybe, but the consensus about with lib; at the top-level is definitely to remove it.

(Even meta = with lib; doesn't have a serious technical argument against it yet, that I've seen in the past year...)

[inclyc]

with pkgs; maybe, but the consensus about with lib; at the top-level is definitely to remove it.

(Even meta = with lib; doesn't have a serious technical argument against it yet, that I've seen in the past year...)

I mean lib as a variable used in with xxx; , e.g. with pkgs; lib.mkHiPrio foo.

[AndersonTorres]

As a with-hater myself, my opinions are a bit radical...

My rule of thumb is to confine with to a single line.
E.G. maintainers = with lib.maintainers; [ A B C D ]; as equivalent to maintainers = [ lib.maintainers.A lib.maintainers.B lib.maintainers.C lib.maintainers.D ];, implying that maintainers = [ lib.maintainers.A lib.maintainers.B C D ]; should throw an "error".

1. Ban meta = with lib;, so if the linter sees meta, it forbids using with lib to enclose the following expression. This is effective but a bit too non-universal

Well, @\eclairevoyant found a wild example...

2. Ban with expression on "top level", e.g. at the top of expressions in a file, or directly behind function arguments. This is pretty good, but may not be very friendly to small files.

Arguments of "small files exception" are not compelling.
A small file will not suffer too much of banning an anti-pattern, since they are small, so the corrections are easier to do.

Further, I believe we do not have "small files" in Nixpkgs. In a preliminary scripting, I found an average of more than 80 lines in Nix files at pkgs/.

3. Ban nested with expression at all, like with a; with b;, but it's also not very friendly.

It is not easy to reason when the code has a nested with.