-
-
Notifications
You must be signed in to change notification settings - Fork 159
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
base: master
Are you sure you want to change the base?
Changes from 10 commits
1cbffdb
7886f94
de1c4d2
1d1ffeb
2a6bbb4
ad09702
75cd80e
63ea2a1
f7730ef
35da03c
1676024
d67d442
d869514
7dbdd8e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
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 ] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One other concern is that this means that what There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about:
Should that be possible? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should, to cover previous behavior
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Trying to find how far it should go compared to
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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would say that «this is just sugar for But yes, probably needs more explicit language. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But then it doesn't seem to make sense to replace
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What do you mean by There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
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 In general I would recommend that we start with just the single-identifier case. We can always add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I was just pondering this in the posts above and ended up coming to the same conclusion as you) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ]; | ||
``` | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
It has lower precedence than
The left-hand operand may be any expression:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is using it inside lists allowed?
Being equal to:
👍 or 👎? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is using it inside of a inherit-as-list allowed?
Being equal to:
👍 or 👎? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is list inside of inherit-as-list allowed?
Being equal to:
👍 or 👎? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any more examples of practical cases that should be allowed? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
This seems the clearest and my best guess would be There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ( 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; }` | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It also needs some examples of what will not work (compared to Its elements cannot be call expressions, only attribute names (and deeper attributes) are allowed:
It only works for lists, so attributes are not supported:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any more practical cases of |
||
# 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; }` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This alternative has one downside, since
but There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
|
||
|
||
# Unresolved questions | ||
[unresolved]: #unresolved-questions | ||
|
||
How would this feature be adopted, if accepted? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
# Future work | ||
[future]: #future-work | ||
|
||
Determine best practices regarding when this language construct should be used | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 When the new feature is adopted in nixpkgs, a linter can suggest the new feature as an alternative to usage of There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Do you come up with the pattern of "confusing with"? I could also help with my language server. |
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 🤷
There was a problem hiding this comment.
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