Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
[RFC 0110] Add "inherit-as-list" syntax construct to the Nix language #110
Changes from 6 commits
1cbffdb
7886f94
de1c4d2
1d1ffeb
2a6bbb4
ad09702
75cd80e
63ea2a1
f7730ef
35da03c
1676024
d67d442
d869514
7dbdd8e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
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 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
++
, so it can be combined without brackets:The left-hand operand may be any expression:
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.
Is using it inside lists allowed?
Being equal to:
👍 or 👎?
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.
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 comment
The 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 comment
The 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 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.
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.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 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'tflatten [ 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.
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 would support inherit-as-list as expression, while its content must be expressions but with restrictions (well, restrictions are needed in any case there…)
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.
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:
It only works for lists, so attributes are not supported:
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.
Any more practical cases of
with
that shouldn't be allowed with inherit-as-list?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.
Was a new keyword suggested?
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 alternative has one downside, since
attrValues
returns a alphabetically sorted list. Repl example:but
[2 1]
was specified.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.
Yeah, definitely unintuitive. I think the worst aspect is that renaming an attr may reorder the resulting list despite not changing any values:
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.
Is the question whether we should as a follow-up for instance replace all
with licenses; [ ... ]
withlicenses.[ ... ]
innixpkgs
?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.
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 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?
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 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 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?
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 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 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.
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.
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 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
.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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
@bobvanderlinden
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 outsidewith
.