-
-
Notifications
You must be signed in to change notification settings - Fork 159
[RFC 0110] Add "inherit-as-list" syntax construct to the Nix language #110
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
base: master
Are you sure you want to change the base?
Changes from 2 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,125 @@ | ||
| --- | ||
| 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) | ||
| 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 expression syntax is: | ||
|
|
||
| ```nix | ||
| inherit (attrs) [ a b c ] | ||
| ``` | ||
|
|
||
| The `inherit` keyword is intentionally reused so as to not add any new | ||
r-burns marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| keywords to the Nix grammar. | ||
|
|
||
| As this expression is currently a syntax error, a Nix interpreter which | ||
| supports this language feature will be compatible with existing Nix code. | ||
|
|
||
| An implementation PR is pending. | ||
|
|
||
| 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: 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.: | ||
| ```nix | ||
| meta.licenses = inherit (lib.licenses) [ bsd3 mit ]; | ||
| meta.maintainers = inherit (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…) |
||
| And build inputs which are commonly extracted from attrsets: | ||
| ```nix | ||
| buildInputs = inherit (darwin.apple_sdk.frameworks) [ | ||
| IOKit | ||
| ] ++ inherit (llvmPackages) [ | ||
| llvm | ||
| ]; | ||
| ``` | ||
|
|
||
|
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;)` | ||
|
|
||
| # 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. |
||
Uh oh!
There was an error while loading. Please reload this page.