Enforce that package argument defaults are applied, cleans up optional dependency convention

[infinisil]

Motivation for this change

Nixpkgs features this pattern a lot for declaring optional dependencies in package definitions:

{ enableBar ? true
, bar ? null
}: /* ... */

One problem with this is that the bar ? null has no effect, because pkgs.bar exists (it actually does!), meaning that when this file gets callPackage'd, pkgs.bar will override the default value. The fact that ? null is specified can be very confusing.

This is in contrast to enableBar ? true, which uses the default of true, because pkgs.enableBar doesn't exist. However here there's also a problem, because what if at some point in the future enableBar does exist? This can mess up the result of this expression, giving errors or wrong results.

This PR implements a very simple fix for these problems: If an argument has a default, the default has to be used, it can't be overridden by an implicit attribute in the scope. Specifically above example will throw this warning:

trace: warning: In /nixpkgs/some/path/default.nix, the argument "bar" has a default value (`bar ? <default>`)
which is not allowed because the attribute "bar" exists in the call scope, therefore overriding the default
value. If "bar" should be a package configuration, changeable via `.override { bar = <value>; }`, rename the
argument to something that doesn't already exist. If "bar" should be optional dependency (commonly done
with `bar ? null`), remove the default value and use an argument like `enableFeature ? true` to decide when
to include "bar" as a dependency.

This means you'll have to update above example to be

{ enableBar ? true
, bar
}: /* ... */

And when someone at some point defines pkgs.enableBar, a warning will be thrown for that too.

Doing this also has the nice side effects that arguments with defaults are now only used for package configuration and not dependencies. This allows people to easily identify what's configuration and what's a dependency.

The main problem with this change is that it might slow down evaluation too much, since this extra checking is not free, and necessary for every package expression. This could be mitigated by introducing a Nix builtin to handle this (#79877 could also benefit from a Nix-native callPackageWith implementation)

Ping @adrianparvino @pstn because you recently ran into such a problem

Things done

• Ran nix-env -qaf . to see all the current warnings and fix them
• Checked package sets that aren't evaluated by nix-env -qaf . for warnings/errors
• Tested how much this slows down evaluation. 1.8% eval time increase, see Enforce that package argument defaults are applied, cleans up optional dependency convention #131271 (comment)

[infinisil]

This is one case where this PR actually finds a bug in nixpkgs: This expression was previously called with both pass and pass-otp, however the latter of which is just an alias of the former with an extension:

nixpkgs/pkgs/top-level/aliases.nix

Line 556 in b27f145

pass-otp = pass.withExtensions (ext: [ext.pass-otp]); # added 2018-05-04

CI however checks that aliases aren't used by not including them in the scope. The fact that this expression used ? null allowed it to get around the CI check, because then just the default of null was used!

This is notably the only package that is getting rebuilt, and possibly this change breaks the package, depending on which pass took priority beforehand.

[infinisil]

CI is finally green. While fixing the eval failures I found a couple of potential bugs that showed themselves via this change, one of which is the above for krunner.

As somewhat expected, this increases eval time by a bit, @GrahamcOfBorg shows

stat	before	after	Δ	Δ%
cpuTime	323.85	328.14	4.29	1.33%

We'd have to do more evaluations to get a good statistical estimate of it though.

This is really the only problem with this PR imo. Could be fixed by:

• For now making sure that CI runs with the callPackageWith changes, but not normal evaluation.
• In the long term, implement callPackageWith as a Nix builtin

[FRidh]

these throws should be moved to python2-packages.nix. You can then write futures = disabled super.futures;.

[FRidh]

Downside is this still evaluates super.futures.

[infinisil]

If I move them to python2-packages.nix they won't be available for Python 3 right? The point of these definitions is that they define these attributes for Python 3.

[FRidh]

You are right, I mixed up 2 and 3.

It's a pity this is needed. The idea is to get rid of all mention of Python 2 from python-packages.nix. Contributors should not need to think about Python 2. If we now need to start adding throw when a package is not available for Python 3, the whole splitting into python-packages.nix and python2-packages.nix has been for nothing.

[infinisil]

We could introduce a mechanism that automatically mirrors all attributes to Python 3 with such a throw

[nixos-discourse]

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

https://discourse.nixos.org/t/tweag-nix-dev-update-16/14379/1

[Artturin]

Anything blocking this?

[infinisil]

@Artturin The only thing that prevents me from merging this is that there's an overhead to it.

I measured it again, and it turns out to be about an 1.8% increase in evaluation time for nix-env -qa --file . for a sample size of 15 (takes about 2 minutes each). Here's a graph:

	old	new
commit	parent of b229fbc	b229fbc
mean	7.17s	7.30s
std err	0.013s	0.013s

Considering this, I'm not sure if this is worth it. 1.8% is not a lot, but it adds up.

[mohe2015]

I have also experienced this I believe and it was not easy to find the problem.

[sternenseemann]

@infinisil maybe hide this codepath between a nixpkgs config flag and set it to true in some ofborg check?

[nixos-discourse]

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

https://discourse.nixos.org/t/2024-01-09-nixpkgs-architecture-team-meeting-47/38037/1

[Aleksanaa]

I measured it again, and it turns out to be about an 1.8% increase in evaluation time for nix-env -qa --file . for a sample size of 15 (takes about 2 minutes each). Here's a graph:

Could you provide how did you generate that graph?

[infinisil]

@Aleksanaa Iirc I pretty much copied what I did here: NixOS/nix#4154 (comment)