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
153 changes: 153 additions & 0 deletions rfcs/0110-inherit-as-list.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,153 @@
---
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: (names, to be nominated and accepted by RFC steering committee)
shepherd-leader: (name to be appointed by RFC steering committee)
edolstra marked this conversation as resolved.
Show resolved Hide resolved
related-issues: (will contain links to implementation PRs)
---

# Summary
[summary]: #summary

This RFC proposes a new Nix syntax `[ inherit (<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:

```
[ inherit (attrs) a b c; ]
[ inherit a b c; ]
r-burns marked this conversation as resolved.
Show resolved Hide resolved
```

These expressions are (respectively) syntactic sugar for:

```nix
builtins.attrValues { inherit (attrs) a b c; }
builtins.attrValues { inherit a b c; }
r-burns marked this conversation as resolved.
Show resolved Hide resolved
```

The `inherit` keyword is intentionally reused so as to not add any new
r-burns marked this conversation as resolved.
Show resolved Hide resolved
keywords to the Nix grammar, and to evoke the similarities with the
existing attribute-based inherit syntax.

As the `inherit` keyword is currently a syntax error inside a list expression,
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: [ inherit (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 = [ inherit (lib.licenses) bsd3 mit; ];
meta.maintainers = [ inherit (lib.maintainers) johndoe janedoe; ];
```

List-inherits can be used multiple times in a list-expression, and intermingled
with ordinary list elements without needing to concatenate:

```
buildInputs = [
a b c /* ordinary list elements */
inherit (pkgsFoo) d e f;
g h i /* ordinary list elements */
inherit (pkgsBar) j k l;
];
```

Sinc attributes inherit from the current scope with `{ inherit a b c; }`,
this RFC proposes a corresponding list-inherit syntax which also inherits
from the current scope. So if the order of the ordinary elements in the
previous list is not significant, one could rewrite the list as:

```
buildInputs = [
inherit a b c;
inherit (pkgsFoo) d e f;
inherit g h i;
inherit (pkgsBar) j k l;
];
```

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…)


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.