-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
lib: Add encapsulate, attrsets that have overlay-based private attrs in their closure #158781
base: master
Are you sure you want to change the base?
Conversation
Creates objects that have overlay-based private attrs in their closure.
I think this needs more explanation. What are "objects" here? Is the goal to turn Nix into an object-oriented language? In that case, it sounds like a classic case of the Lisp Curse, namely implementing language features in library code. IMHO such features tend to make it harder to understand Nixpkgs. Maybe it's better to discuss the big goal in an RFC? |
I'm not trying to do OOP as that would imply creating messages or methods. That'd be a bad idea. I'll avoid that term because its common association only applies partially. |
|
||
Synopsis: | ||
|
||
r = encapsulate (final@{extend, ...}: { |
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.
Putting extend
in final
is conceptually not valid. It should be a separate parameter.
(It's the kind of code you end up with when trying to stick too close to past patterns, as in #119942)
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.
Putting
extend
infinal
is conceptually not valid. It should be a separate parameter.
I believe this comes from the discussion in #157056 (comment)? If so, I think the concerns there have been cleared
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.
Indeed your comment seems to be about the same kind of problem.
I think I was setting the bar too high when I started this thread. What's the alternative? Add an extra parameter?
Reserving a name in the attrset of protected attributes is not pretty, but gets the job done quite well, without burdening the user with extra syntax.
The extra parameter will cause it to look like this:
lib.encapsulate (extend: this: {
# ...
public.withSomething = s: extend (extend: this: super: {
something = s;
});
});
Doesn't seem like a usability improvement, especially if you're not actually going to use extend
.
this.extend
seems better now, even if it means that users can't use that name.
That's probably for the best though, because we should having too many overriding mechanisms. If they do need to use that name, the problem is that they have one too many overriding mechanisms.
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 suppose a more realistic alternative is
lib.encapsulate ({ this, extend, ... }: {
# ...
public.withSomething = s: extend ({ this, extend, super, ... }: { something = s; }); });
}
We don't have renaming for such parameters though, which can make nested use awkward, as seen with not-just-data submodules, which have the same problem.
rant and analogy with module system
Also before you know it, someone will want to extend that attrset, and we'll have encapsulateWith { specialArgs = ...; }
, even though super
can fill the role of specialArgs
. They're actually oddly similar.
Overlays have //
for "lateral" composition (idk, is that term taken?), whereas the module system has option merging for that. extendModules
then lines up with extends
. extendModules
relies on the lexical scope (ie config
) to provide the "super" context, whereas in overlays it's an explicit parameter. specialArgs
is effectively an extension of all modules' lexical scope, so it's a close analog of super
.
This seems like an overreaction though.
The lack of nested "formals" ({ this@{ foo, ... }, ... }: <body>
) could be a feature though, because then it's not possible to do the equivalent of what's currently lib.encapsulate ({ foo, ... }: <body>)
, which doesn't work, because such lambdas are strict (a phenomenon very relevant to lazyFunction
, for some context).
I've taken the first "use case", #119942 in a different direction. It was already a separate implementation of the concept, but it now also uses a different name for the function output. |
Something like this function should be the starting point for |
encapsulate = layerZero: | ||
let | ||
fixed = layerZero ({ extend = f: encapsulate (extends f layerZero); } // fixed); | ||
in fixed.public; |
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.
in fixed.public; | |
in fixed.result; |
Instead of public
, we could use result
, to avoid being too much like object-oriented programming, as that seems to be a concern.
Personally I actually prefer public
, because I find the association with object-oriented programming useful, even if it doesn't implement messages.
Similarly, imperative languages that implement functional features don't reinvent all names. For instance, when a language implements a functional list processing API, they use the simple function names that were supposed to be used with pure functions, even though that can't be enforced. map
is just a better name than traverse
, let alone traverseIO
.
This kind of thing is accepted.
If we do insist on something like result
, we should also rename encapsulate
, and make it all about "an attrset fixpoint where one of the attrs is the result", or "an overridable scope where one of the variables is returned". I have no idea how to turn that into a name.
*/ | ||
encapsulate = layerZero: | ||
let | ||
fixed = layerZero ({ extend = f: encapsulate (extends f layerZero); } // fixed); |
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.
Idea:
fixed = layerZero ({ extend = f: encapsulate (extends f layerZero); } // fixed); | |
fixed = layerZero (fixed // { extend = f: encapsulate ((fixed.extends or extends) f layerZero); }); |
This allows the base layer (or subsequent layers) to pick a "merge function" that implements extension differently - ie not overlay-style extension, but something more suitable for the use case.
Possible improvements that can be opted into this way: attribute deletion, nested merging, or a layered cousin of the module system.
Maybe the predictability of lib.extends
is an important feature though.
}; | ||
}) | ||
|
||
s = r.extend (final: previous: { |
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.
s = r.extend (final: previous: { | |
s = r.extend (this: old: { |
I like this as a convention because it has a mnemonic: the reverse, "old this" can only be a description of one of the parameters and is therefore the wrong order.
We should probably not reuse names of overlays, so that overlays and encapsulate
can be used together more easily. (For instance using an haskellPackages
overlay inside a Nixpkgs overlay is quite common, and awkward.)
self: super:
: overlaysfinal: prev:
: Nixpkgs overlaysfinalAttrs: prevAttrs:
: overrideAttrsthis: old:
: Things with overlay-like overriding and encapsulation.
Motivation for this change
Creates attrsets that have overlay-based private attrs in their closure.
It's the same idea that underpins #119942 and it's useful for defining
mkPackage
as proposed in NixOS/rfcs#92 (comment)#119942 is a lot like
mkDerivation = encapsulate (self: { public = mkDerivationImpl self; })
, but doesn't use this function for backcompat and micro-optimization reasons.I guess one way
encapsulate
could be better/different is by making itencapsulate (self: super@{extends}: .....)
. That seems a bit cleaner.Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)nixos/doc/manual/md-to-db.sh
to update generated release notes