Documentation: style guide for use of `with`

[lolbinarycat]

Problem

i see a lot of license = with licenses; [ mit ] in PRs recently.

compared to the simpler license = licenses.mit, this is both longer and more syntactically complex, taking longer to read for humans, and possibly longer to evaluate too.

Proposal

this should be avoided for single-license packages (which is most of them).

this would not apply to meta.maintainers, for the following reason:

• it is more likely to have multiple values
• it is much more likely to have extra values added in the future.
• maintainers is plural, which implies a list

i would be willing to write this into pkgs/README.md but i'm not going to open a PR until i've gotten some feedback, it wouldn't make sense for me to single-handedly decide what the best practice is.

---

Add a :+1: reaction to issues you find important.

[eclairevoyant]

relevant: #208242

[lolbinarycat]

the nix best practices page seems to imply that with should basically never be used, this seems very extreme and would require rewriting basically every file in nixpkgs.

if we instead just recommend limiting its use to meta = with lib; { } and maintainers = with maintainers; [ ], that would allow most unproblematic usages of with to continue, while discouraging unusual and sometimes problematic usages.

[AndersonTorres]

i see a lot of license = with licenses; [ mit ] in PRs recently.

In my opinion, license should be a list, rather than "a list of licenses or a single license". The "singleton", even if being more typical, looks like an exception and not a rule.

So, as a personal preference, I am writing meta.license as a list unconditionally.

maintainers is plural, which implies a list

License should be called licenses, precisely because there is at least one multiple-licensed project. But now it is too late to change that historical inconsistency...

seems to imply that with should basically never be used, this seems very extreme and would require rewriting basically every file in nixpkgs.

There is more: we didn't find a decent replacement for with yet. The best we can do is to limit its use to predictable and easy-to-not-be-fooled examples.

To me, the exceptions are single-line statements, like:

1. Lists, with the restriction that the with applies to each and every element:

      with X; [ A B C D ];
      # is shorter than
      [ X.A X.B X.C X.D ];
   
      # however, if X.C and X.D does not exist, use:
      (with X; [ A B ]) ++ [ C D ];
     # equivalent to
      [ X.A X.B C D ];

2. the infamous toPythonApplication:

      aocd = with python3Packages; toPythonApplication aocd;
      # is shorter than
      aocd = python3Packages.toPythonApplication python3Packages.aocd;

meta = with lib; { }

This was discouraged too.

https://github.com/NixOS/nixpkgs/pull/293767

[lolbinarycat]

This was discouraged too.

correction: this is no longer encouraged.

as best i am able to tell, the only person who has a major problem with meta = with lib; is you. i am trying to document consensus, and one person's opinion does not consensus make.

i can drop meta = with lib from "acceptable" to "discouraged" if you find someone to second your opinion, i suppose.

[Aleksanaa]

correction: this is no longer encouraged.

as best i am able to tell, the only person who has a major problem with meta = with lib; is you. i am trying to document consensus, and one person's opinion does not consensus make.

I made a similar point here: https://github.com/NixOS/nixpkgs/pull/292120#discussion_r1514555270

To put it bluntly, this needs to be discussed. We cannot force change through partial adoption of previously accepted views without discussion. As far as the previous PRs and issues I have seen, other committers have expressed objections. Relevant changes should not be merged until controversy is settled.

[Atemu]

I've also yet to see a good reason why the 5 lines of the (usually trivial) meta attribute are somehow too large of a scope to sensibly apply with.

For entire package definitions or modules, that reason is easy to find as their code usually is not trivial where non-intuitive with; bugs can creep up comparatively easily. (I've been bitten by a file-global with; triggering infinite recursion a few weeks ago again.)
For rather small trivial scopes such as individual lists or a small attrset, this reason does not apply.

[AndersonTorres]

the 5 lines of the (usually trivial) meta attribute

1. Technically the current meta recognizes 10 sub-attributes;

2. "It just works" is not a serious reason to make an exception to nested with;

3. The argument can be reversed: if the scope is so small, then there is no tactile gain in squeezing some bytes in it;

4. Further, it brings consistency to the use of with, avoiding arguments of "small enough":

   • No nested with
   • One line is small, two or more is big.

5. Being blunt, I have seen no reason for the guidelines above, besides "saving 10 characters per line"

   • it would be no more than 100 bytes per file (per worst hypothesis usage on meta attrs)
   • or less than 8MB on the whole repo (worst hypothesis usage of each and every entry of find ./my-local-clone-of-nixpkgs).

[lolbinarycat]

@AndersonTorres trying to convince me is pointless, because it doesn't matter what i think. i'm trying to document consensus, not my own opinion.

(for the record, my only strong opinion is that with lib.maintainers should be allowed)

[AndersonTorres]

1. You opened a PR associated with this issue (https://github.com/NixOS/nixpkgs/pull/294383) - and in ready-for-review status, rather than as a draft.

   You even marked it as closing this issue. You are being inconsistent here.

2. It makes few to no sense to document consensus without polling opinions.

3. Many posts here - including those you are 👍-ing - express opinions and arguments, but only now that I am answering and arguing on them you suddenly remember you are merely documenting consensus?

[lolbinarycat]

You opened a PR associated with this issue (https://github.com/NixOS/nixpkgs/pull/294383) - and in ready-for-review status, rather than as a draft.

yes, because i want feedback on it (i.e. review)

It makes few to no sense to document consensus without polling opinions

correct! that's why i opened this issue before creating the PR! i also asked peoples' opinions in the matrix dev chatroom.

all of the actions i have taken i have done with the intent to get other people's opinions.

Many posts here - including those you are 👍-ing - express opinions and arguments, but only now that I am answering and arguing on them you suddenly remember you are merely documenting consensus?

i want to get several different perspectives on the issue.

they were a new voice, which is helpful

i am already well aware of your opinion, and you continuing to repeat it does nothing to establish consensus.

[eclairevoyant]

I've finally found a good example: version can come from both lib and the derivation itself.

This has now reared its head in an actual package (see https://github.com/NixOS/nixpkgs/pull/298316/files) where someone converted the mkDerivation call to use the fixed-point parameter (mkDerivation (final: { ... })), but missed changing one of the references to version within meta:

  meta = with lib; {
    changelog = "https://gitlab.archlinux.org/pacman/pacman/-/raw/v${version}/NEWS";
  };

which in turn broke the changelog. No eval error, because version easily came from lib.

For another example where this would cause further issues, version is an input for meta.platforms in citrix-workspace:

  meta = with lib; {
    platforms = [ "x86_64-linux" ] ++ optional (versionOlder version "24") "i686-linux";
  };

In other words, were this converted to use the fixed-point parameter, and someone missed a spot to use said param, this would break meta.platforms (!)

And to someone unaware of the scoping rules of with, they may easily miss that version is not meant to come from lib, nor will the LSP or even nix itself warn them of this.

Perhaps it's time to put the footgun down and get rid of meta = with lib; too.

[Aleksanaa]

Good catch!

And to someone unaware of the scoping rules of with, they may easily miss that version is not meant to come from lib, nor will the LSP or even nix itself warn them of this.

The rewritten nixd will probably be able to warn this!

[AndersonTorres]

I don't know if I am more impressed with such an awful example of unexpected with, or with Pacman in Nixpkgs.

[lolbinarycat]

well, i stand corrected!

guess it's time to discourage with lib in meta.

[RossComputerGuy]

guess it's time to discourage with lib in meta.

I'm still on the fence with the removal of with lib in meta. In the example which was given, couldn't you have version' and lib.version to explicitly state which ones? Or you could use let..in and inherit (lib) ...? Imo, the removal of with lib; in meta and leaving everything to have the "long form" of pulling data does not look as clean as the with lib; way.

[eclairevoyant]

I cannot get behind making packaging harder if the only potential benefit is it (sometimes) looks "cleaner".

As for using let exprs, that still entails removing with.

[RossComputerGuy]

I cannot get behind making packaging harder if the only potential benefit is it (sometimes) looks "cleaner".

Understandable, though do we have to remove all with lib;'s when setting meta? In the example you gave, there's two version variables. Not all packages will have duped variables. With the move to what #119942 implemented, maybe we could still use with lib; as there would be finalAttrs.version and so lib.version wouldn't collide. With smaller or simpler packages, maybe we could still use with lib; and if issues do arise then we could make the change?

[AndersonTorres]

does not look as clean as the with lib; way.

https://github.com/NixOS/rfcs/pull/110 is trying something to solve this

[RossComputerGuy]

https://github.com/NixOS/rfcs/pull/110 is trying something to solve this

This certainly looks like a better and more proper alternative.

[nbraud]

I've finally found a good example: version can come from both lib and the derivation itself.

Yes, with can easily shadow other implicit bindings (as from rec etc.) 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?

[eclairevoyant]

not really an option yet since most of the builders don't support finalAttrs. only mkDerivation and the builders for nim and php do iirc

[inclyc]

My suggestion is: just make sure the file is warning-free for escaping-with (lsp diagnostics). That is, you can use with ... in very deep level like with pkgs;. But not use it as a top-level with lib; because it may shadow things.

[AndersonTorres]

From that point of view, wouldn't it make more sense to deprecate the mkDerivation rec { ... } antipattern instead?

I like that idea! Nonetheless rec has a long way to become really deprecated, and a truckload of people will never change to it except if the manual says it should.

https://github.com/NixOS/nixpkgs/pulls?q=is%3Apr+is%3Aopen+makeoverridable

I love the solution from Best Practices guide, but no one is ready for the elegance of self-reference yet :(

(lsp diagnostics)

Too dependent on tooling.

A "true" solution would be a new keyword.

[inclyc]

Too dependent on tooling.

No. A styling guide may just suggest something neutral like: "don't make your variable look through with env and bind to static names", to avoid https://github.com/NixOS/nix/issues/490 .

[Atemu]

@inclyc could your escaping-with check be ran stand-alone?

That'd be great to have for CI in Nixpkgs.

[inclyc]

@inclyc could your escaping-with check be ran stand-alone?

That'd be great to have for CI in Nixpkgs.

Yes.

https://github.com/nix-community/nixd/blob/main/libnixf/tools/nixf-tidy.cpp

nixf-tidy, processes the file (stdin) and emit JSON diagnostics.

I have a draft action here: https://github.com/inclyc/nixf-tidy-action/blob/main/src/main.ts maybe someone would like to integrate it?

[nbraud]

From that point of view, wouldn't it make more sense to deprecate the mkDerivation rec { ... } antipattern instead?

I like that idea! Nonetheless rec has a long way to become really deprecated [...]

Sure, and as @eclairevoyant pointed out, it's also dependent on (many) builders being updated. Thankfully, helpers have been submitted to make this much easier for builders' authors.

and a truckload of people will never change to it except if the manual says it should.

The same is true of trying to remove with lib; etc. I think that makes it all the more important that we focus on the most approriate idioms to uniformise everything on.

I love the solution from Best Practices guide, but no one is ready for the elegance of self-reference yet :(

If you mean naming the attrset to refer back to its contents, this does not solve the override issue either AFAIK.

(lsp diagnostics)

Too dependent on tooling.

Isn't this kind of thing what we have CI for?

A "true" solution would be a new keyword.

@AndersonTorres, could you clarify why you think a new keyword & corresponding language feature would be needed?

[lolbinarycat]

@inclyc

Too dependent on tooling.

No. A styling guide may just suggest something neutral like: "don't make your variable look through with env and bind to static names", to avoid NixOS/nix#490 .

i'm not sure how you think with scoping works, but named varaiables will always be given priority over those in with:

$ nix-instantiate --eval --expr 'let a = "outer"; in with { a = "inner"; }; a'

[AndersonTorres]

Isn't this kind of thing what we have CI for?

This can be mildly acceptable for editorconfig, but LSP? It is many orders of magnitude bigger.

Being clearer: it is acceptable to install editorconfig locally, but LSP is more complicated. CI-side can possibly do more things - like building things on Darwin.

This is like a Rust novice struggling with the borrow checker, with the additional load of sending the code remotely to OfBorg.

could you clarify why you think a new keyword & corresponding language feature would be needed?

The drawbacks of with surpasses its slim benefits of squeezing some bytes. A new language construct, let it be a keyword, an operator or whatever, is what be need. Not a "let the CI do the checks" thing.

About this:

https://github.com/NixOS/rfcs/pull/110

[inclyc]

@inclyc

Too dependent on tooling.

No. A styling guide may just suggest something neutral like: "don't make your variable look through with env and bind to static names", to avoid NixOS/nix#490 .

i'm not sure how you think with scoping works, but named varaiables will always be given priority over those in with:

$ nix-instantiate --eval --expr 'let a = "outer"; in with { a = "inner"; }; a'

Yes. If you read the links to NixOS/nix issue, this scoping rule is in-fact a thing we are trying to avoid.

Link: https://github.com/NixOS/nix/issues/490 Link: https://github.com/NixOS/nix/pull/10620

[inclyc]

This can be mildly acceptable for editorconfig, but LSP? It is many orders of magnitude bigger.

If you read the comments above, you will know lsp diagnostics' could be run as a standalone tooling. A single exe with even smaller closure size than nix-instantiate.

[inclyc]

The drawbacks of with surpasses its slim benefits of squeezing some bytes. A new language construct, let it be a keyword, an operator or whatever, is what be need.

For now we cannot "remove" or even "deprecate" any language constructs by introducing a new one. Thus I think "introducing a new keyword or somehow" cannot resolve "overuse of with".

Think about it: if there are actually some RFCs accepted, then we must :

1. suggest contributors use the new syntax and
2. forbid the legacy with

how can we achieve this?

Since we cannot "remove" something from a language, there are merely two approaches to keep the code clean:

1. Review the code by maintainers
2. Automated tooling

Not a "let the CI do the checks" thing.

IMHO nixpkgs maintainers have more important things to do, e.g.

1. fixup a broken package,
2. testing the compabilites, ...
3. ...

So why not offload the "syntax checking" burden to automated tools?

[AndersonTorres]

After a month looking at the pile of notifications, I have read this only now.

1. I was talking about long term solution.
2. My concern was more about the burden of bootstrapping and running the CI locally. But this is not as bad as I thought initially.

[eclairevoyant]

It seems the check added in #330066 will get us closer to cleaning up meta = with lib; :tada:

[infinisil]

Unfortunately that specific error reporting is turned off again for now, see https://github.com/NixOS/nixpkgs/pull/330454 :sweat_smile: