lib/modules: disallow setting config, options in specialArgs of evalModules

[eclairevoyant]

Seems like an easy win to prevent breaking the module system entirely, see https://discourse.nixos.org/t/cannot-find-attribute-programs-firefox/61401/7 for context

@Aleksanaa if you're interested to review

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[Aleksanaa]

See the lines below:

nixpkgs/lib/modules.nix

Lines 154 to 196 in effb331

	Additional arguments passed to each module in addition to ones
	like `lib`, `config`,
	and `pkgs`, `modulesPath`.
	This option is also available to all submodules. Submodules do not
	inherit args from their parent module, nor do they provide args to
	their parent module or sibling submodules. The sole exception to
	this is the argument `name` which is provided by
	parent modules to a submodule and contains the attribute name
	the submodule is bound to, or a unique generated name if it is
	not bound to an attribute.
	Some arguments are already passed by default, of which the
	following *cannot* be changed with this option:
	- {var}`lib`: The nixpkgs library.
	- {var}`config`: The results of all options after merging the values from all modules together.
	- {var}`options`: The options declared in all modules.
	- {var}`specialArgs`: The `specialArgs` argument passed to `evalModules`.
	- All attributes of {var}`specialArgs`
	Whereas option values can generally depend on other option values
	thanks to laziness, this does not apply to `imports`, which
	must be computed statically before anything else.
	For this reason, callers of the module system can provide `specialArgs`
	which are available during import resolution.
	For NixOS, `specialArgs` includes
	{var}`modulesPath`, which allows you to import
	extra modules from the nixpkgs package tree without having to
	somehow make the module aware of the location of the
	`nixpkgs` or NixOS directories.
	```
	{ modulesPath, ... }: {
	imports = [
	(modulesPath + "/profiles/minimal.nix")
	];
	}
	```
	For NixOS, the default value for this option includes at least this argument:
	- {var}`pkgs`: The nixpkgs package set according to
	the {option}`nixpkgs.pkgs` option.

I doubt if passing all of these predefined arguments are unwelcome and can lead to undefined behavior. Even pkgs has a more standard way to override.

Besides that, I don't think I have enough knowledge to review changes here.

[eclairevoyant]

For pkgs, the current recommended way is to set nixpkgs.pkgs and add "${modulesPath}/misc/nixpkgs/read-only.nix" to imports in the NixOS module system. Outside of NixOS I'm not sure what would be the best way.

But that wouldn't be done here, as it'd be a layering violation to prevent setting pkgs in evalModules, because pkgs is not a standard module arg in the module system; it could be prevented in nixosSystem or elsewhere if needed.

Anyway, config seemed like an easy win, which is why I addressed that. Assuming no fallout from that, then we could expand later.

[roberth]

Individual Module System applications and submodules can and should add their own checks if they have problems with this kind of thing.

Maybe collisions between _module.args and specialArgs could be warned about generically and later forbidden. It seems feasible to check this as _module.args apply, or perhaps, somewhat more expensively, by changing applyModuleArgs to seq a new argument that performs the check provided by a let in a caller. That would be messy, but a plan B nonetheless.

[hsjobeki]

I think this a good idea in general.
I would even propose to also do the same for options. It might be not as common as config but may lead to the same kind of errors.

My only thing concern beeing the assertpreloads eager evaluation of specialArgs to all submodule evaluations. Although it is comparably small; I would still be interested how performance is impacted by this.

cc: @infinisil

... (actions log) ...
ofborg-eval — ^.^!

Why is there no performance report in this pr ?

[hsjobeki]

Can you also add a test to lib/tests/modules.sh ? I think one test at all should be fine.

[roberth]

thoughts - no action required. Can revisit and/or revert this change if needed in the future.

I find setting config rather intriguing actually. We don't currently have a way to do apply on two or more options simultaneously, (e.g. where final value a' depends on merged defs for a and b, and b' also depends on a and b) and I know this has prevented some changes and/or compatibility logic from being implemented.

I'd be surprised if setting config works, and it wouldn't be a great solution for this problem, at least not by itself.
We'll need to at least make specialArgs a function taking module arguments, and the unadulterated config would have to be separate from the config that's defined by specialArgs.
This would make specialArgs in extendModules an extension function. That's fun.
Also need to consider whether such a feature should interact with "out of band" info propagation, i.e. the thing that would allow submodule (evalModules) to be split into a separate types.record and types.fix function without losing access to options etc - the "out of band" info.

[roberth]

My only thing concern beeing the assertpreloads eager evaluation of specialArgs to all submodule evaluations.

I believe evalModules is already strict in specialArgs for all practical purposes, so the strictness part of strictness seems to be covered. Not sure about the eagerness part of strictness. It seems that we've lost the evaluation report, which was fairly useful for assessing changes like this (after adding a factor 5× to the percentages). We really should have something like that back.

[roberth]

cc: @infinisil

ofborg-eval — ^.^!
Why is there no performance report in this pr ?

So I completely read past this assuming it was a quote. Skill issue on my side for sure, but also maybe not use blockquotes when not quoting?

[infinisil]

Never got around to implementing it with the new eval unfortunately #355847

[hsjobeki]

Okay since we dont have a performance report in ci for now. I did a comparison of two nixos system evaluations without any modules:

Master

{
  "cpuTime": 3.697922945022583,
  "envs": {
    "bytes": 116693440,
    "elements": 8386940,
    "number": 6199740
  },
  "gc": {
    "cycles": 4,
    "heapSize": 655024128,
    "totalBytes": 987361840
  },
  "list": {
    "bytes": 17014552,
    "concats": 262373,
    "elements": 2126819
  },
  "nrAvoided": 6220321,
  "nrExprs": 1405485,
  "nrFunctionCalls": 5431674,
  "nrLookups": 3366481,
  "nrOpUpdateValuesCopied": 15756596,
  "nrOpUpdates": 396272,
  "nrPrimOpCalls": 2480453,
  "nrThunks": 7638911,
  "sets": {
    "bytes": 381391424,
    "elements": 22265372,
    "number": 1571592
  },
  "sizes": {
    "Attr": 16,
    "Bindings": 16,
    "Env": 8,
    "Value": 24
  },
  "symbols": {
    "bytes": 1041105,
    "number": 87181
  },
  "time": {
    "cpu": 3.697922945022583,
    "gc": 1.228,
    "gcFraction": 0.3320783094339194
  },
  "values": {
    "bytes": 318556104,
    "number": 13273171
  }
}

Branch

{
  "cpuTime": 2.979763984680176,
  "envs": {
    "bytes": 108966392,
    "elements": 7910867,
    "number": 5709932
  },
  "gc": {
    "cycles": 4,
    "heapSize": 671805440,
    "totalBytes": 975337952
  },
  "list": {
    "bytes": 15991096,
    "concats": 262476,
    "elements": 1998887
  },
  "nrAvoided": 5854512,
  "nrExprs": 1403768,
  "nrFunctionCalls": 4939656,
  "nrLookups": 3120541,
  "nrOpUpdateValuesCopied": 16194862,
  "nrOpUpdates": 399227,
  "nrPrimOpCalls": 2353469,
  "nrThunks": 7527100,
  "sets": {
    "bytes": 386707904,
    "elements": 22593540,
    "number": 1575704
  },
  "sizes": {
    "Attr": 16,
    "Bindings": 16,
    "Env": 8,
    "Value": 24
  },
  "symbols": {
    "bytes": 1037900,
    "number": 86950
  },
  "time": {
    "cpu": 2.979763984680176,
    "gc": 0.986,
    "gcFraction": 0.3308986903222234
  },
  "values": {
    "bytes": 312670920,
    "number": 13027955
  }
}

I hope i didn't make any mistakes. But it seems number of thunks going down.

7638911 -> 7527100 => Delta -1.4%

[hsjobeki]

I think there is some follow-up work possible to improve the handling of _module.args and specialArgs collisions. Otherwise this look nice. Thanks for also adding the same strictness for options ❤️