Skip to content
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

[RFC 0110] Add "inherit-as-list" syntax construct to the Nix language #110

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from
Draft
135 changes: 135 additions & 0 deletions rfcs/0110-inherit-as-list.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
---
feature: inherit-as-list
start-date: 2021-10-17
author: Ryan Burns (@r-burns)
co-authors: (find a buddy later to help out with the RFC)
shepherd-team: @synthetica9, @infinisil, @kevincox, @bobvanderlinden
shepherd-leader: @kevincox
related-issues: (will contain links to implementation PRs)
---

# Summary
[summary]: #summary

This RFC proposes a new Nix syntax `<attrset>.[ <attrnames> ]`,
which constructs a list from the values of an attrset.

The goal is to provide a similarly-terse but more principled alternative
to the often-used `with <attrset>; [ <attrnames> ]`.

# Motivation
[motivation]: #motivation

It is currently cumbersome to create a list from the values of an attrset.
If one has an attrset `attrs` and wishes to create a list containing some of
its values, one could naively write:

```nix
[ attrs.a attrs.b attrs.c ]
```

To avoid typing `attrs` many times, one will typically use `with` instead:

```nix
with attrs; [ a b c ]
```

However, the `with` expression has many well-known drawbacks, such as
unintuitive shadowing behavior [1][2], prevention of static scope checking [3][4],
and reduced evaluation performance [3].

* [1] https://github.com/NixOS/nix/issues/490
* [2] https://github.com/NixOS/nix/issues/1361
* [3] https://github.com/NixOS/nixpkgs/pull/101139
* [4] https://nix.dev/anti-patterns/language#with-attrset-expression

Nonetheless, Nix expression authors are subtly guided toward the `with` form
because it is (or at least appears) simpler than any existing alternatives.
Some alternatives are suggested in
https://nix.dev/anti-patterns/language#with-attrset-expression, but as these
are more verbose and complex than `with`, they are rarely used.

The goal of this RFC is to provide a similarly-terse alternative which avoids
these drawbacks.

# Detailed design
[design]: #detailed-design

The proposed syntax is:

```
attrs.[ a b c ]
Copy link
Contributor

Choose a reason for hiding this comment

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

One concern of this form is that foo.[bar] looks a lot like indexing operators in most languages. However I think this is a minor concern because the .[ makes it stand out and this would rarely be used for single items.

Copy link

Choose a reason for hiding this comment

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

since the | operator is not a valid attribute or variable name, perhaps we could make use of it do to something like [ with foo | a b c ] instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

This syntax looks more foreign to me. It may also cause confusion with other languages that use | for bitwise or. I think I prefer the proposed solution.

Copy link

Choose a reason for hiding this comment

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

I think a lot of people familiar with functional programming are familiar with list comprehensions, this isn't exactly a list comprehension but the idea was attempting to use that familiarity to make it obvious what's happening. I guess it is a bit awkward after all though 🤷

Choose a reason for hiding this comment

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

I'd also suggest it introduces a small inconsistency in the syntax where a beginner now expects

attrs.{ a b c } (or { attrs | a b c })

to work the same way for attributes, but instead they need to learn { inherit (attrs) a b c; } which appears arbitrarily different

Copy link
Contributor

Choose a reason for hiding this comment

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

One other concern is that this means that what [a b c] means depends on if it is after attrs.. I think this is going to cause very minimal confusion so I am ok with it. And it is far better than the unknowability of with it would replace.

Copy link
Member

Choose a reason for hiding this comment

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

What about:

pkgs.[ python pythonPackages.mypackage ]

Should that be possible?

Choose a reason for hiding this comment

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

It should, to cover previous behavior

with pkgs; [ python pythonPackages.mypackage ];

Copy link
Author

Choose a reason for hiding this comment

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

The current implementation only allows attrnames in the brackets, but adding support for this syntax seems logical. I think this behavior is pretty intuitive and does what one would expect, and covers the previous with pkgs use case well.

Copy link
Member

Choose a reason for hiding this comment

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

Trying to find how far it should go compared to with. Playing devil's advocate here:

pkgs.[ (python.override { ... }) ]

Not saying it should or shouldn't work, but either way, I can imagine there will be some confusion.

Are such examples also documented somewhere already?

Copy link
Member

Choose a reason for hiding this comment

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

I would say that «this is just sugar for a.[x y] becoming [a.x a.y]» justifies pkgs.[pythonPackages.mypy] but not really pkgs.[(python.override {})].

But yes, probably needs more explicit language.

Copy link

@pleshevskiy pleshevskiy Oct 21, 2022

Choose a reason for hiding this comment

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

But then it doesn't seem to make sense to replace with pkgs. Maybe it's better

We can make it simpler and forbid such overrides in this construct

pkgs.[stylua ormolu] ++ [inputs.agenix.packages.${system}.agenix];

Copy link

@xaverdh xaverdh Oct 21, 2022

Choose a reason for hiding this comment

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

pkgs.[(wrapCC gcc)]

PSEUDO CODE
[ ((wrapCC || pkgs.wrapCC) (gcc || pkgs.gcc)) ]

This will work exactly as it does now with with pkgs; [(wrapCC gcc) ], but not dump all variables to the internal scope.

What do you mean by not dump all variables to the internal scope? isn't that exactly what is happening with this approach?
It would mean that variables in the internal scope will not be shadowed by those from the pkgs set, but it will still bring them into scope.. For example in the presence of an undefined variable reference (due to e.g. a mistake by the programmer) the semantics change. What would previously cause a evaluation error might now evaluate (and to something the programmer did not intend).

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with 7c6f434c. We need to keep the rule simple. I am completely in favour of pkgs.[foo] == [pkgs.foo] and I think it is reasonable to extend to pkgs.[foo.bar] == [pkgs.foo.bar] but as soon as we pass a simple set of attributes I am definitely against. For example is pkgs.[(wrapCC gcc)] equivalent to pkgs.[(pkgs.wrapCC gcc)], pkgs.[(wrapCC pkgs.gcc)] or pkgs.[(pkgs.wrapCC pkgs.gcc)]? Sure, we can define a rule but it starts to get hard for a reader to remember.

In my opinion, it is logical to check variables for existence ... This will work exactly as it does now with with

I disagree. Now I need to know what variables exist to know how an expression will behave. And adding new packages can break expressions. There is a reason why a lot of people dislike with and want to create alternatives such as this very construct. I think that doing lookup like with basically defeats the point of this syntax in the first place. If you want that continue to use with pkgs; [ ... ].

In general I would recommend that we start with just the single-identifier case. We can always add pkgs.[foo.bar] support in a future RFC once we see more examples of real-world use.

Choose a reason for hiding this comment

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

I disagree. Now I need to know what variables exist to know how an expression will behave. And adding new packages can break expressions. There is a reason why a lot of people dislike with and want to create alternatives such as this very construct. I think that doing lookup like with basically defeats the point of this syntax in the first place. If you want that continue to use with pkgs; [ ... ].

I was just pondering this in the posts above and ended up coming to the same conclusion as you)

Copy link
Member

Choose a reason for hiding this comment

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

I cannot resolve conversations, but I think this one can be marked as such.

```

This expression is syntactic sugar for:

```nix
[ attrs.a attrs.b attrs.c ]
```

As the token `.` immediately preceding `[` is currently a syntax error,
a Nix interpreter which supports this new language feature will be compatible
with existing Nix code.

This RFC is implemented here: https://github.com/nixos/nix/pull/5402

For MVP functionality, only minor additions to `src/libexpr/parser.y` are
needed. If accepted, the changes to the Nix interpreter can be backported
to current releases if desired. Relevant language documentation and
third-party parsers/linters would also need to be updated.
Comment on lines +76 to +79
Copy link
Member

Choose a reason for hiding this comment

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

Should it be introduced behind an experimental flag?

Some features in the past were introduced this way. I am personally wondering what we would gain from placing it behind an experimental flag.


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

This would be useful for many languages and frameworks in Nixpkgs which
extract packages from a package set argument.

For example, `python3.withPackages (ps: ps.[ ... ])` will serve as a
more fine-grained alternative to `python3.withPackages (ps: with ps; [ ... ])`.
This would apply similarly to `vim.withPlugins`, `lua.withPackages`, etc.

Certain list-typed `meta` fields could also make use of this feature, e.g.:
```
meta.licenses = lib.licenses.[ bsd3 mit ];
meta.maintainers = lib.maintainers.[ johndoe janedoe ];
```

Copy link
Member

Choose a reason for hiding this comment

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

This might need some more examples to show what is possible and what isn't.

It allows referring to deeper attributes:

buildInputs = pkgs.[ python310 python310Packages.pytorch ];

It has lower precedence than ++, so it can be combined without brackets:

buildInputs = pkgs.[ zlib openssl ] ++ mypkgs.[ mypackage ];

The left-hand operand may be any expression:

environment.systemPackages = (import nixpkgs-unstable {
  inherit system;
}).[ vim ];

Copy link
Member

@bobvanderlinden bobvanderlinden Nov 3, 2022

Choose a reason for hiding this comment

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

Is using it inside lists allowed?

[ [ 1 2 ] a.[ b c ] ]

Being equal to:

[ [ 1 2 ] [ a.b a.c ] ]

👍 or 👎?

Copy link
Member

@bobvanderlinden bobvanderlinden Nov 3, 2022

Choose a reason for hiding this comment

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

Is using it inside of a inherit-as-list allowed?

a.[ b c d.[ e f ] ]

Being equal to:

[ a.b a.c [ a.d.e a.d.f ] ]

👍 or 👎?

Copy link
Member

@bobvanderlinden bobvanderlinden Nov 3, 2022

Choose a reason for hiding this comment

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

Is list inside of inherit-as-list allowed?

a.[ [ b c ] [ e f ] ]

Being equal to:

[ [ a.b a.c ] [ a.e a.f ] ]

👍 or 👎?

Copy link
Member

Choose a reason for hiding this comment

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

Any more examples of practical cases that should be allowed?

Copy link
Contributor

Choose a reason for hiding this comment

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

It isn't clear to me what that would mean so I'm voting no.

a.[ [ b c ] [ e f ] ]

This seems the clearest and my best guess would be [ [ a.b a.c ] [ a.e a.f ] ]. But I still feel unsure about it. I would definitely not include this for the first version and we can always consider adding it later once we have more experience with the feature.

Copy link
Member

@bobvanderlinden bobvanderlinden Nov 4, 2022

Choose a reason for hiding this comment

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

I indeed agree on that one.

I've expanded the examples a bit with what they represent without 'inherit as list' syntax.

The other cases are a bit more tricky I think. You'd expect that you can add any arbitrary expression to lists. A list in a list is possible (flatten [ [ pkgs.python pkgs.jq ] ]), so why shouldn't flatten [ pkgs.[ python jq ] ] work? It could be surprising for people that this won't work.

With that in mind, if we do allow the syntax inside lists, because they are arbitrary expressions , then it would only make sense to allow it inside inherit-as-list as well. It would be surprising to not allow this.

Then again, I can't think of common practical use-cases to allow these cases 🤷

For the implementation, I can imagine that the parsing will be a bit more annoying when disallowing the above examples, because it cannot be implemented as an arbitrary expression anymore. It needs explicit checks whether it is used inside lists or inherit-as-lists.

Copy link
Member

Choose a reason for hiding this comment

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

I would support inherit-as-list as expression, while its content must be expressions but with restrictions (well, restrictions are needed in any case there…)

Note that only simple textual attrnames are allowed in the square brackets.
For example, `pkgs.[ (openssl.overrideAttrs { patches = [ ... ]; }) ]`
is currently a syntax error, as is `pkgs.[ "${some_expression}" ]`,
`a.[ b.[ c d ] ]`, and `a.[ [ b c ] [ d e ] ]`.
Future RFCs may add additional support for useful idioms such as
`pkgs.[ python310 python310Packages.pytorch ]` on a case-by-case basis,
but that is not planned for this RFC.

Other forms of syntax which were considered but are not proposed
in this RFC include:
* `[ inherit (attrs) a b c; ]`
* `[ inherit a b c; ]`
* `inherit (attrs) [ a b c ]`
* `pkgs.{ python = python310; openssl = openssl_3; }`

Copy link
Member

Choose a reason for hiding this comment

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

It also needs some examples of what will not work (compared to with):

Its elements cannot be call expressions, only attribute names (and deeper attributes) are allowed:

buildInputs = pkgs.[ (openssl.overrideAttrs { patches = [ ... ]; }) ];

It only works for lists, so attributes are not supported:

pkgs.{ python = python310; openssl = openssl_3; }

Copy link
Member

@bobvanderlinden bobvanderlinden Nov 3, 2022

Choose a reason for hiding this comment

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

Any more practical cases of with that shouldn't be allowed with inherit-as-list?

# Drawbacks
[drawbacks]: #drawbacks

* This will add complexity to the Nix grammar and any third-party tools which
operate on Nix expressions.
* Expressions reliant on the new syntax will be incompatible with
Nix versions prior to the introduction of this feature.

# Alternatives
Copy link
Member

Choose a reason for hiding this comment

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

Was a new keyword suggested?

[alternatives]: #alternatives

* Skillful use of `with` to avoid its drawbacks
* Fine-grained attribute selection via existing (more verbose) language
features, such as `builtins.attrValues { inherit (attrs) a b c; }`
Copy link

Choose a reason for hiding this comment

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

This alternative has one downside, since attrValues returns a alphabetically sorted list. Repl example:

> attrs = { a=1; b=2; }
> builtins.attrValues { inherit (attrs) b a; }
[1 2]

but [2 1] was specified.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, definitely unintuitive. I think the worst aspect is that renaming an attr may reorder the resulting list despite not changing any values:

nix-repl> builtins.attrValues { a = 1; b = 2; }        
[ 1 2 ]

nix-repl> builtins.attrValues { c = 1; b = 2; } 
[ 2 1 ]


# Unresolved questions
[unresolved]: #unresolved-questions

How would this feature be adopted, if accepted?
Copy link
Member

Choose a reason for hiding this comment

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

Is the question whether we should as a follow-up for instance replace all with licenses; [ ... ] with licenses.[ ... ] in nixpkgs?


# Future work
[future]: #future-work

Determine best practices regarding when this language construct should be used
Copy link
Member

Choose a reason for hiding this comment

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

Should we say something about adoption in nixpkgs? Probably disallow its usage for the foreseeable future until a majority is using a version of Nix with support for inherit-as-list.

Copy link
Member

Choose a reason for hiding this comment

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

Should the feature be back ported to (for instance) 2.4.1, so that adoption will cost less time?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think backporting decisions should be made depending on how invasive the change is.

Copy link
Member

Choose a reason for hiding this comment

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

Should these kinds of notes be part of the future work section of the RFC?

Copy link
Author

Choose a reason for hiding this comment

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

I am not sure whether a new feature like this qualifies for backporting, but the currently proposed implementation is very simple, so it would be easy to backport to any versions we desire.

Copy link
Member

Choose a reason for hiding this comment

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

When this is implemented in Nix, it's easy for this feature to be used in Nixpkgs and slip through the review process.

It might be useful to describe a systematic way in this RFC to avoid this feature in nixpkgs until the other RFC is picked up.

Not doing anything with this in this RFC seems like it will cause problems.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, good point. It seems like the experimental-features flag would be good for this, but I think there was some opposition to gating this syntax behind an experimental feature. Maybe we should revisit that option?

Copy link
Member

Choose a reason for hiding this comment

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

What people want to do in their own repositories is something they may decide for themselves. The experimental flag will cause problems when sharing such repositories. Anyone contributing to the repo needs to enable the flag: the flag will then also be enabled when contributing to nixpkgs. It's messy and confusing especially for new people. The flag is forces upon you and eventually the older community members will converge to having it enabled, just like what happened to flakes. Newcomers will get surprised.

Personally I think nixpkgs lacks a linter. There it is possible to already support the new feature, but show a linting error with proper description why it is not allowed. I'm guessing this requires a separate RFC 😅

With a linter it will also be possible to guard against confusing usage of with.

When the new feature is adopted in nixpkgs, a linter can suggest the new feature as an alternative to usage of with.

Copy link
Author

Choose a reason for hiding this comment

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

Right, I guess unlike most experimental features, this could be something we'd want to have enabled by default, but I think it would still be useful to have a way to disable it for CI purposes. Or lacking a way to disable it in Nix itself, it seems straightforward to add a check to https://github.com/nix-community/nixpkgs-lint and/or https://github.com/jtojnar/nixpkgs-hammering.

Copy link

Choose a reason for hiding this comment

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

@bobvanderlinden

With a linter it will also be possible to guard against confusing usage of with.

Do you come up with the pattern of "confusing with"? I could also help with my language server. let foo = ...; in with ...; [ foo ] would make too many false-positives, because that's the only way to access variables outside with.