@ElvishJerricco, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ttuegel, @peti and @edolstra to be potential reviewers.

@ElvishJerricco Looking good! Some of the complexity of the *scope stuff I think @nbp and @peti want to remove altogether, but that can be done later of course.

I think iself and WithSelf should become output and WithOutput---I don't want to confuse people over iself vs self. Despite what I said earlier in person, I think output might be a clear name than interface--so the function would be like mkOutput or something.

@Ericson2314 yea that sounds like a mostly better name, although output is generally a term that refers to Nix store outputs.

Oh, I was unaware of this work :). I'll look when I'm back at my desk in a couple hours.

Wait... Is that travis failure a memory leak in the Nix expressions?... Did I do that?

@ElvishJerricco So, I need a little bit more time to think this over. In the interim, I'd like to point out -- and get your thoughts on -- another solution I played with: #15095 (comment)

There might be something there that you can draw inspiration from.

Alright, I've looked through this pretty extensively, and it all looks good to me.

I see from your example:

obj.overridePackage (output: self: super: { args = super.args // {b = self.val.a;}; })

It's apparent that overridePackage can serve the purpose of both override and extend, and solves the issue I was trying to address in #15095 (comment) (where I wanted a way to allow for extensions to refer to the package/package-set's original input arguments), at the expense of being a bit more verbose than either. And for completeness, it looks like you could also elide existing attrs in either case, should the need arise, which is nice new feature. I suspect that override and extend will suit 99% of use cases, while overridePackage makes possible that last 1%, so I think the flexibility justifies the interface. Very nice work!

I don't want to bike-shed too much, but I do wonder if we can come up with more intuitive name. I've been pushing for a change in vernacular for a while now, where override would indicate changing input arguments whereas extend allows for modification of a resulting package/-set. I don't know where that leaves us for naming what is essentially both in one, though. Considering that it applies equally well to both individual packages and sets of packages, the "Package" in overridePackage might be a little misleading.

Another thing I'm excited about is being able to use extend with mkDerivation. overideDerivation doesn't compose, and overrideAttrs doesn't give you the fixedpoint, so having override+extend+overridePackage is a huge step up.

@cstrahan Thanks =) I agree about the naming of overridePackage; I think it's definitely the wrong name. I'm just not sure how to encapsulate the idea that it can both override and extend in one name. I guess the easy answer would be overrideAndExtend =P

As I mentioned before, if this does not belong in Nixpkgs, then this can go in an overlay [1], where you are free to do anything, and where this will not add burden to maintain Nixpkgs.

[1] https://github.com/mozilla/nixpkgs-mozilla/blob/master/lib-overlay.nix

I'm very reluctant to merge this because a) it makes a pretty non-trivial change to a core function; b) changes to makeOverridable may have a significant performance / memory impact; c) I have no idea what it does or why; d) I don't have time to review it at the moment. Sorry about that.

Can someone point out an actual problem with this code? Something that will break, something that will slow down, something that will make future code harder to reason about, anything?

We have several people who have taken the time to understand this code who think it's a good solution to real-world problems, a few people who have reviewed it who aren't sure it's the best solution but haven't proposed another, and a few people who haven't reviewed it in detail but don't like it anyway. Why shouldn't we merge in this case? git revert is very cheap.

I should also point out that the code is structured so that each commit stands alone is readable. @ElvishJerricco has specifically done this to make this otherwise complex change decipherable.

BTW, this is oversimplified but the ultimate theoretical justification for this change is the same as the justification for protected class members: We have details we want to encapsulate from consumers of our interface but make available to extenders of it.

@shlevy Merge conflicts are fixed btw.

@edolstra Please let me know if your objection is a hard "no" to this being merged, otherwise I'll merge soon.

To touch on the performance concerns: if we don't have a readily available means of profiling built into Nix, that ought to be a priority for us, rather than punting on design improvements.

This broke evaluation of pkgs/top-level/release.nix

@LnL7 751d397 , thanks

@shlevy looks good now

Tarball is broken still, fixing...

Done

This PR breaks the evaluation of callPackage ./foo {} where foo/default.nix requires a stdenv argument. For example, I have in my ~/.config/nixpkgs/config.nix:

haskellPackages = super.haskellPackages.override {
  overrides = self: super: {
    Cabal-git = self.callPackage /home/simons/src/cabal/Cabal {};
    cabal-install-git = (self.callPackage /home/simons/src/cabal/cabal-install {}).overrideScope (self: super: {
      Cabal = self.Cabal-git;
      hackage-security = pkgs.haskell.lib.dontCheck super.hackage-security;
    });
  };
};

and /home/simons/src/cabal is a recent checkout of git://github.com/haskell/cabal.git. This used to work fine, but now I get the following error:

error: anonymous function at /home/simons/src/cabal/cabal-install/default.nix:1:1 called without required argument ‘stdenv’, at /home/simons/.nix-defexpr/lib/customisation.nix:218:76

git bisect says that e11dbc3 is the first commit to cause that failure.

@ElvishJerricco @Ericson2314 Any idea here?

Please revert. This makes an already bad idea (makeOverridable) even more complicated. For example, the full description of makeOverridableWithInterface is:

  # A more powerful version of `makeOverridable` with features similar
  # to `makeExtensibleWithInterface`.

WTF does that mean? What is makeExtensibleWithInterface? It's not like the code is self-explanatory, with all the selfs and supers and functors and dynamic attributes etc.

Regarding performance metrics, we have those, and they show a significant spike starting today: https://hydra.nixos.org/job/nixpkgs/trunk/metrics#tabs-charts. E.g. nix-env.qa.allocations went from 374 MB to 631 MB.

The claim that reverts are cheap is false in my experience; once changes like these are in and other commits start building on them, it quickly becomes very hard to untangle them.

Reverting

Done

Fair. I understand that this change was complicated. But this still leaves us with issues like #26561. extend is supposed to be a fundamental way to mess with haskellPackages, but it's just broken. What is the correct solution for that? How else do we solve all the various problems outlined in this discussion?

The existence of an overrideScope attribute that creates a complete, modified copy of the package set for the context of the given attribute may have been a mistake in the first place. One might very well argue that this method should exists only for the package set, but not for every package inside of it. It's my understanding that this interpretation is also in-line with the way overlays work.

Anyhow, technical discussion of this issue probably belongs into an RFC, not into a pull request -- unless someone feels like spending a lot of effort creating a PR that (a) works, (b) does not affect performance nor memory requirements, (c) is well documented, and (d) works out of the box without breaking evaluation of existing code.