Skip to content

[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.

Sign up for GitHub

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

Draft
6543 wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

[RFC 0190] Ban with expression in nixpkgs #190

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
123 changes: 123 additions & 0 deletions rfcs/0190-forbid-with.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
---
feature: ban-with-expressions
start-date: 2025-10-07
author: 6543
co-authors:
shepherd-team:
shepherd-leader:
related-issues:
- https://github.com/NixOS/rfcs/pull/120
- https://github.com/NixOS/nixpkgs/pull/413393
---

# Summary
[summary]: #summary

Completely prohibit the usage of `with` expressions in nixpkgs to improve code clarity, maintainability, and enable better static analysis. Instead using explicit `inherit` statements should be used if bringing attributes into scope is desired.

# Motivation
[motivation]: #motivation

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
Copy link
Contributor

@kevincox kevincox Sep 7, 2025 •
edited
Loading

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.

Copy link
Member Author

@6543 6543 Sep 7, 2025

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 ... :/

Copy link
Contributor

@kevincox kevincox Sep 7, 2025

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".

Copy link

@oxalica oxalica Sep 7, 2025

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.
image
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.

Copy link

@NovaViper NovaViper Sep 8, 2025

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 👀

- **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
Copy link

@KiaraGrouwstra KiaraGrouwstra Sep 7, 2025

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

Copy link
Member Author

@6543 6543 Sep 7, 2025

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


The `inherit` statement provides a better alternative that is:

- **Explicit**: Clearly shows which attributes are being brought into scope
- **Traceable**: Easy to see where each variable comes from
- **Analyzable**: Static analysis tools can easily understand the code structure
- **Debuggable**: Error messages can point to specific inherit statements

# Detailed design
[design]: #detailed-design

1. **Immediate prohibition**: New code in Nixpkgs must not use `with` expressions
2. **Gradual migration**: Existing `with` expressions should be migrated to `inherit` over time
3. **CI enforcement**: Add automated checks to prevent new `with` expressions from being merged
4. **Documentation updates**: Update Nixpkgs contributor guidelines to reflect this policy

# Examples and Interactions
[examples-and-interactions]: #examples-and-interactions

## Simple attribute access

```nix
# Before
with pkgs; [
git
vim
curl
]

# After
let
inherit (pkgs) git vim curl;
Copy link

@Tom-Hubrecht Tom-Hubrecht Sep 7, 2025

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
]

Copy link
Member Author

@6543 6543 Sep 7, 2025

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 😅

Copy link
Contributor

@kevincox kevincox Sep 7, 2025

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.

Copy link
Member Author

@6543 6543 Sep 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

about lists: #190 (comment)

in [
git
vim
curl
]
```

## Function arguments

```nix
# Before
{ pkgs, ... }:
with pkgs; {
buildInputs = [ git vim ];
}

# After
{ pkgs, ... }:
let
inherit (pkgs) git vim;
in {
buildInputs = [ git vim ];
Copy link
Member

@Ma27 Ma27 Sep 7, 2025

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 🫠

Copy link
Contributor

@kevincox kevincox Sep 7, 2025 •
edited
Loading

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.

Copy link

@khaneliman khaneliman Sep 7, 2025

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.

Copy link
Member Author

@6543 6543 Sep 7, 2025

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?

}
```

## Nested scopes

```nix
# Before
with pkgs; {
meta = with lib; {
license = with licenses; [ mit ];
};
}

# After
let
inherit (pkgs.lib.licenses) mit;
in {
meta = {
license = [ mit ];
Copy link
Member

@Ma27 Ma27 Sep 7, 2025

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?

Copy link
Member Author

@6543 6543 Sep 7, 2025

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

Copy link
Member

@Ma27 Ma27 Sep 7, 2025

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.

Copy link
Member

@GetPsyched GetPsyched Sep 7, 2025

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.

Copy link

@philipwilk philipwilk Sep 7, 2025

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

Copy link
Member Author

@6543 6543 Sep 7, 2025

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.

-> #190 (comment)

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

Copy link
Member

@piegamesde piegamesde Sep 8, 2025

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

};
}
```

# Drawbacks
[drawbacks]: #drawbacks

- **Increased verbosity**: Code may become slightly longer due to explicit prefixes or inherit statements
- **Migration effort**: Existing codebase requires changes
- **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.
Copy link
Member

@Ma27 Ma27 Sep 7, 2025

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 withs).

Copy link
Contributor

@kevincox kevincox Sep 7, 2025

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.

Copy link

@khaneliman khaneliman Sep 7, 2025

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.

Copy link
Member Author

@6543 6543 Sep 7, 2025

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!

Copy link
Member Author

@6543 6543 Sep 7, 2025

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 ]


# Alternatives
[alternatives]: #alternatives

Just discourage usage of `with`, but in this case most things still can not be statically checked.
Copy link
Member

@piegamesde piegamesde Sep 7, 2025

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;


# Unresolved questions
[unresolved]: #unresolved-questions

# Future work
[future]: #future-work