Improve callPackage error message!

[infinisil]

Motivation for this change

Oftentimes arguments in package declarations are misspelled, leading to errors like

error: anonymous function at /home/infinisil/src/nixpkgs/pkgs/tools/text/ripgrep/default.nix:1:1
  called without required argument 'fetchFromGithub', at /home/infinisil/src/nixpkgs/lib/customisation.nix:69:16

which doesn't give any indication as to what the correct attribute is. Well with this change it does! The error now is

error: Function in /home/infinisil/src/nixpkgs/pkgs/tools/text/ripgrep/default.nix 
  called without required argument "fetchFromGithub", 
  did you mean "fetchFromGitHub" or "fetchFromGitLab"?

Which imo, is pretty damn fancy!

The implementation searches through all attributes in pkgs for one with a Levenshtein distance of 2 or less, catching most misspellings. Had to jump through some hoops to make this fast though.

Ping @alyssais @capsensitive @worldofpeace @Synthetica9 @roberth @Profpatsch

Better alternative to changes like #79854

Things done

• Wrote tests
• Checked how this influenced performance (it shouldn't influence it much, since in the working case not much extra is done). <- That's correct, see Improve callPackage error message! #79877 (comment)

[roberth]

This looks like it won't work for invocations that only become valid after overriding. I hope I'm wrong.
Also this is demanding missingArgs on all callPackage invocations. What's the performance impact?

[infinisil]

I guess you mean something like this:

(lib.callPackageWith {} ({ foo }: foo) {}).override { foo = 10; }

However this also throws an error even before this change.

Yeah the performance needs to be investigated, I think it's going to be minor though

[roberth]

As much as I want this feature, I doubt that it should be implemented in Nixpkgs rather than Nix itself.
When implemented in Nix itself, this functionality will be available in all attrset-function invocations, rather than just callPackage, without any overhead; neither for the interpreter hot path nor for the Nixpkgs lib reader.

[roberth]

This is the lookup in Nix: https://github.com/NixOS/nix/blob/c3181e21e75d9e31254cd97c8e56858be7072d5f/src/libexpr/eval.cc#L1161

[infinisil]

@roberth Unless callPackageWith becomes a builtin, I'm pretty sure this actually can't be implemented in Nix itself. All such functions are defined like { stdenv } (without ellipsis), meaning exactly those arguments need to be passed in, no more. To do this, function arguments are intersected with the automatic arguments (pkgs), and only passing the result of that to the function. At this point, Nix doesn't know anything about the automatic arguments anymore -> it's unable to look those up.

What could be implemented in Nix however is the same thing for attribute access like pkgs.fetchFromGithub

[grahamc]

I wonder what it would take to implement this in Nix itself, to get this goodness everywhere! edit: GitHub hadn't loaded the comments made since I opened this page.

[infinisil]

Yeah so I'm pretty sure now. This only works due to the fact that callPackageWith has knowledge of all available packages in addition to the function arguments. To implement this in Nix would mean to implement callPackageWith as a builtin. Since this function depends on many other things however, this would mean implementing pretty much all of lib/customisation.nix as builtins. That might not even be too unreasonable, so if anybody wants to do this feel free.

Edit: Though really, there's not much benefit to implementing this in Nix with something like builtins.callPackageWith, because the only use of that builtin would be as lib.callPackageWith = builtins.callPackageWith. It's not functionality that can reasonably be introduced into the syntax of the language, so it's only something available with such a callPackageWith function, at which point people may as well use the one from nixpkgs. This is in contrast with builtins like builtins.isFunction and builtins.sort, which are builtins because it's not possible to implement them otherwise or it would be too slow respectively.

[infinisil]

After some more tinkering I was able to come up with this change:

diff --git a/lib/customisation.nix b/lib/customisation.nix
index ac234e3b8c6..9944d3330f2 100644
--- a/lib/customisation.nix
+++ b/lib/customisation.nix
@@ -118,7 +118,10 @@ rec {
     let
       f = if lib.isFunction fn then fn else import fn;
       auto = builtins.intersectAttrs (lib.functionArgs f) autoArgs;
-    in makeOverridable f (auto // args);
+      allPotentialArgs = autoArgs // args;
+      filled = lib.mapAttrs (name: value: allPotentialArgs.${name})
+        (lib.filterAttrs (name: value: ! value) (lib.functionArgs f));
+    in makeOverridable f (filled // auto // args);
 
 
   /* Like callPackage, but for a function that returns an attribute

This results in this error now:

error: attribute 'fetchFromGithub' missing, at /home/infinisil/src/nixpkgs/lib/customisation.nix:122:43
(use '--show-trace' to show detailed location information)

So this change makes it look up all arguments in the available packages, resulting in an attribute access error instead of a function call error for missing arguments. With this, implementing such a feature for attribute access in Nix directly (as in pkgs.fetchFromGithub would throw a nice error), would automatically get it for callPackagesWith too! Unfortunately the error message doesn't show the location of the function anymore, which is rather unfortunate.

So, this could be implemented in Nix directly, either by:

• Implementing this error for attribute access and using above patch, only downside is the lost error location
• Implementing this error through a builtin callPackageWith, with the downside of being very complex and having no real benefit

If losing error location (without --show-trace) is acceptable, then using the former is most reasonable. If it is not, then this PR is the next best alternative, at no significant cost (other than not having such errors for all attribute accesses). Perhaps implementing attribute access errors orthogonally would be best.

[roberth]

autoArgs is huge, so you probably want to perform "two" lookups instead of doing a big merge:

(name: value: autoArgs.${name} or args.${name} or autoArgs.${name})

The function location can be recovered by allowing functions as the first argument to builtins.addErrorContext. This does require --show-trace.

Alternatively, builtins.throw/abort can be changed to take a list which is concatMapped internally with a toString that does print function locations. This does require suggestions to be implemented in Nix, but if it improves performance for the hot path that's worth it.

The readability concern is minor if the big let binding is moved out into a separate function.

A possible benefit of generating suggestions in Nix is you can more easily add domain-specific logic. For example giving special treatment to common errors for which Levenshtein distance doesn't work too well, like adding/removing a lib prefix. However, I think there's really not that much preventing Nix itself from doing that too.

(If we go with a Nix implementation) Did you try canonicalizing the names before running Levenshtein? You could

• lowercase everything
• remove _ and -
• remove lib prefix
• remove version number suffixes (maybe too much loss, maybe slow?)

[infinisil]

@roberth I like your idea of being able to have more domain-specific behavior with an implementation in Nix code. I think doing those suggestions here would decrease code readability a bit too much for now, and the current approach has some leeway for things like python-28 -> python38/python27 still.

So, I'm kind of convinced this PR is a good idea because:

• Implementing this builtin into Nix with above patch would lose error location without workarounds
• Implementing this builtin into Nix with a callPackageWith would be very complex
• Doing this in Nix code directly is relatively simple and doesn't lead to worse errors
• Doing this in Nix code directly allows for more flexibility and domain-specifics in the future
• This PR has real benefits without any costs. Just because it doesn't implement these suggestions for all of Nix doesn't make those benefits any worse

Imo this can be merged after performance is confirmed to not be dramatically worse (which I doubt). And after some tests are added for the levenshtein functions

[infinisil]

The time to evaluate nix-instantiate nixos/release-combined.nix -A tested with 4 samples (stats calculated with sta):

state	min	max	mean	stddev	stderr
Before PR	211.659	217.278	214.293	2.61731	1.30866
After PR	211.653	217.456	214.504	2.19853	1.09927

So I think it's safe to say that performance is influenced minimally by this.

Edit: Updated stats to discard the first sample for warmup

[grahamc]

I like this PR.

Given the recent evaluation memory wall we hit (and are still having trouble with) I am -1 on this PR merging right now. I would like to see that stabilize before adding this. I know it may not make it worse, but I don't want to tempt fate today.

[edolstra]

It's a nice feature, but Nix is not a general purpose language so this definitely should not be implemented in Nix code.

[roberth]

@infinisil The evaluation time of your benchmark may be dominated by time spent in the module system. Benchmarking Nixpkgs by itself should help to make the results more significant, considering that standard error estimate is currently rather high.

[infinisil]

@edolstra I guess you're referring to the levenshtein distance being implemented in nix code? That's the only thing that's "general-purpose" here. Yeah I guess a builtins.levenshtein[AtMost] might be a good idea in Nix itself, but there is no harm having this implemented in Nix code.

[edolstra]

@infinisil Yes, exactly. Nix isn't really suited for implementing anything that has the word "algorithm" in it :-) E.g. Nix doesn't have a type system, the evaluator is not very fast, etc.

I don't think the solution is to have builtins.levenshtein. Rather, the evaluator could give better "undefined variable" messages by using Levenshtein internally.

[infinisil]

@edolstra As explained above already, this either has the disadvantage of: Losing error context if using #79877 (comment) or significant complexity in Nix builtins using #79877 (comment). If you know of a way to get an error message without losing context and without too much complexity, let me know, because I don't see it.

And as such, I still think this PR is the best way to implement this.

[roberth]

Isn't callPackage a thing we can get rid of?
This works fine:

> import ./foo.nix pkgs      
error: anonymous function at /home/user/h/nixpkgs/foo.nix:1:1 called without required argument 'qux', at (string):1:1

Is there any reason to use reflection and intersectAttrs, other than

• having a bunch of function definitions without { , ... }
• possibly improved performance when callPackage x { someOverride = y; } is used

Both of these seem solvable.
Most callPackage invocations don't seem to have overrides, so shallow copying pkgs only a few times shouldn't be a problem and it does save many intersectAttrs calls + allocations.

What we get in returns is better error messages as implemented in Nix. We want to have it there for the general case of attrset invocations anyway. Furthermore, we simplify Nixpkgs by getting rid of needless use of reflection and we get the same behavior as with the module system, which also requires ....

Also we can replace this entire nix pill https://nixos.org/nixos/nix-pills/callpackage-design-pattern.html by a footnote about how laziness is working really well. No need to waste people's time on legacy.

Please correct me if I'm wrong :)

EDIT (Nov 13): I'll abuse this comment to keep notes. First of all thanks for clarifying the relevance of makeOverridable in callPackage, so we probably only want to get rid of intersectAttrs-related aspects of callPackage. Apparently nobody here cross-compiles, because cross compilation changes callPackage to do its splicing magic. I haven't really looked into that yet (trying to avoid rabbit holes when I'm in one) but I do not think it relies on the intersectAttrs bits.

[infinisil]

@roberth For one, callPackage also calls makeOverridable which adds .override and .overrideAttrs, so a bit more would still be needed.

I like the idea of redesigning how packages are declared in nixpkgs, the current way seems clumsy in many ways (multiple dependency listings, what's up with optional dependencies, what's up with configuration, why callPackage with overrides, why not auto-call all these package files). However anything like that should have some serious thought put into it before it is applied, because it involves tree-wide changes and requires people to know about the new ways. As you suggest, we might get this feature for free once that is done and Nix implements such suggestions, but this will be a long ways away.

[roberth]

@infinisil I agree, except for the long ways away. It depends on the migration path we choose, but either way it's going to be a 'mechanical' process; simple and boring, which is good.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/fetchfromgithub-in-configuration-nix/5927/10

[grahamc]

I think a few people have misunderstood @edolstra's position here. He doesn't want to implement this language in code evaluated by the evaluator -- ie: he doesn't want this solution to be done with the Nix Expression Language. Instead, he would like to see the solution implemented as a patch to the Nix Evaluator's error messages, in the NixOS/nix repo.

[edolstra]

If we're on the topic of simplifying package expressions, we should ask whether packages should be functions at all. The main reason why they are functions is because you don't want to define all packages in one giant file. But things become much simpler if you do put everything in a giant file, see for example perl-packages.nix which doesn't use package functions (dependencies aren't passed around through function arguments but are obtained from the lexical environment). Since such a file becomes unwieldy, we could an include mechanism, i.e.

firefox = include ./path/to/firefox.nix;
libfoo = include ./path/to/libfoo.nix;
libbar = include ./path/to/libbar.nix;
stdenv = ...;

where firefox.nix looks like

stdenv.mkDerivation {
  buildInputs = [ libfoo libbar ... ];
}

i.e. it doesn't take any arguments but just expects to be included in a lexical environment containing stdenv, libfoo, etc.

This style does break .override but arguably that's a good thing.

Another possibility is Bazel-style dependencies where packages themselves specify where to get their dependencies, so something like

buildInputs =
  [ (import ../path/to/libfoo.nix)
    (import ../path/to/libbar.nix)
    ...
  ];

but this changes semantics a lot.

[7c6f434c]

This style does break `.override` but arguably that's a good thing.

We even use overrides in Nixpkgs proper, and this _is_ useful functionality.

[Profpatsch]

I propose we merge then.

[roberth]

I don't think it's a particularly good solution, but it solves a problem. If it causes any issues, it can be reverted.

For readability, you could factor the error checking and throw into a helper function as much as possible. That should leave a concise implementation of what callPackageWith really does. Should be easy because you need to rebase anyway.
If you could do that, you have my approval.

[edolstra]

I still don't think that the Nixpkgs standard library is the right place for functions like levenshtein. This is complexity that we need to support forever once added.

[jtojnar]

Could we name it _internalLevenshtein or even require the user to call it as _internalLevenshtein {__acceptThisFunctionIsInternalAndCanBeRemovedAtAnyTime = true; } a b to rid us of such support requirements?

[Profpatsch]

Just move it to a let binding inside the place it’s used instead of exposing it from lib?

…

On Fri, Oct 9, 2020 at 12:10 PM Jan Tojnar ***@***.***> wrote: Could we name it _internalLevenshtein, or require the user to call it as _internalLevenshtein {__acceptThisFunctionIsInternalAndCanBeRemovedAtAnyTime = true; } a b to rid us of such support requirements? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#79877 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAYB5ZUBXBLCABR2NRA6FFDSJ3ORRANCNFSM4KTNQRHQ> .

[jtojnar]

Private definitions are harder to run tests for.

[infinisil]

To be exact, the levenshtein function itself isn't actually needed for this functionality, because it only relies on levenshteinAtMost 2, which has a custom implementation that doesn't rely on the generic levenshtein function.

[infinisil]

But also, I don't think there's a problem with having a generic levenshtein function or the likes in nixpkgs. These functions don't need constant maintenance. And even if they do, it's probably much less than all other parts of nixpkgs, and I'd be happy to do it.

[infinisil]

I wouldn't mind this being merged, though I might continue my attempt at implementing this in Nix directly soon, which then would make this PR redundant.

[Profpatsch]

So let’s get rid of the generic levenshtein function in this PR and only add the specialized version for now, it seems like this was the only objection left.

…

On Sat, Oct 10, 2020 at 12:17 AM Silvan Mosberger ***@***.***> wrote: I wouldn't mind this being merged, though I might continue my attempt at implementing this in Nix directly soon, which then would make this PR redundant. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#79877 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAYB5ZQVTCVKXVDZALGQGILSJ6DV5ANCNFSM4KTNQRHQ> .

[stale]

I marked this as stale due to inactivity. → More info

[Profpatsch]

I’d still like to get this merged. I am okay with the nix levenshtein implementation.

[Profpatsch]

I took the liberty of rebasing on current master (only the tests had a merge conflict).

[Profpatsch]

It still works for me:

> nix-build -A ripgrep
error: Function called without required argument "fetchFromGitHu" at /home/philip/nixpkgs/pkgs/tools/text/ripgrep/default.nix:2, did you mean "fetchFromGitHub", "fetchFromGitea" or "fetchFromGithub"?

I would merge within a few days.