Skip to content

lib: Add beforeApply option attribute #302905

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.

Sign up for GitHub

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

Closed
dasJ wants to merge 1 commit into from
Closed

lib: Add beforeApply option attribute #302905

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions lib/modules.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -807,6 +807,9 @@ let

in warnDeprecation opt //
{ value = builtins.addErrorContext "while evaluating the option `${showOption loc}':" value;
# The merged value before applying the options `apply` function to it.
# In general though, `apply` should not be used, it's an anti-pattern.
beforeApply = res.mergedValue;
Copy link
Member

@roberth roberth Apr 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds the cost of a whole attribute to every option, but delivers almost no value in almost all cases.
Also it doesn't fully solve the problem. What if opt.beforeApply is wrong, and a non-trivial number of modules relies on opt.beforeApply? mkOption { modifyBeforeApply = f; }? opt.beforeModifyApply?

I think at some point we just have to admit that modules aren't fit for certain options or option trees anymore.
What if instead of an option tree or submodule, we could have a types.adapter, which transforms all definitions arbitrarily, transforms the returned config and options arbitrarily, and can be implemented in whichever way the use case demands?
That way we don't have to further bloat the module system with mostly unnecessary attributes, and actually do a better job at being compatible.

Copy link
Member

@infinisil infinisil Apr 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if opt.beforeApply is wrong

Not sure what you mean by that?

and a non-trivial number of modules relies on opt.beforeApply

It only makes sense to rely on beforeApply if the option has an apply, otherwise it's the same as the options value. And apply is being discouraged already, you won't find much new uses. The main use case here really is just allowing certain reasonable configs for older options that already use apply and can't be migrated away from easily.

I agree that it's a high cost to pay for very little value though..

Copy link
Member Author

@dasJ dasJ Apr 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also a good time to mention #299736 as an alternative implementation that doesn't add a new attribute to all options.

Copy link
Member

@roberth roberth Apr 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if opt.beforeApply is wrong

Not sure what you mean by that?

This time opt.value is wrong. Next time opt.beforeApply is wrong. Why would this sequence stop?

And apply is being discouraged already, you won't find much new uses.

I'm not convinced of that.

Copy link
Member

@infinisil infinisil Apr 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

opt.value is "wrong" this time because apply exists, so we need beforeApply. The sequence stops here because we don't have a "pre-apply" step. After the values get merged, it gets passed to apply directly.

I'm not convinced of that.

At least I'm heavily discouraging apply whenever I see it :)

Copy link
Member

@roberth roberth Apr 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I might have overreacted.

Bloat argument still stands though, or are we betting on replacing evalModules in the foreseeable future?
I like the idea of having time to do that... :)

Copy link
Member

@roberth roberth Apr 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another angle is the current impossibility of using #257511 to simplify submodule. This can't currently be done, in large part because submodules have two values:

  • the value also known as config
  • the value with _type = "configuration", aka the evalModules result, containing config, as well as options and other "meta" or "out of band" things.

What if opt.beforeApply was already thing, submodule returned the _type = "configuration", and by convention, each submodule-typed option had apply = x: x.config or apply = x: removeAttrs ["_module"] x.config?
While that would be a little cumbersome to write, especially for more complicated hierarchies with multiple submodule/attrsOf nestings, it would be more capable, more efficient (fewer removeAttrs calls that copy almost everything), and possible to orthogonalize submodule into individually usable types, such as more or less attempted in #257511.

Copy link
Member

@infinisil infinisil Apr 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds interesting, though I don't think having beforeApply would be blocking that.

Once we deprecate apply entirely, beforeApply will vanish too :)

Copy link
Member

@roberth roberth Apr 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once we deprecate apply entirely, beforeApply will vanish too :)

I think apply will stick around until we replace modules themselves. Do we really want to inflict its removal on users?

inherit (res.defsFinal') highestPrio;
definitions = map (def: def.value) res.defsFinal;
files = map (def: def.file) res.defsFinal;
Expand Down
2 changes: 2 additions & 0 deletions lib/tests/modules.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ checkConfigOutput '^"one two"$' config.result ./shorthand-meta.nix

checkConfigOutput '^true$' config.result ./test-mergeAttrDefinitionsWithPrio.nix

checkConfigOutput '^true$' config.okChecks ./before-apply.nix

# Check that a module argument is passed, also when a default is available
# (but not needed)
#
Expand Down
24 changes: 24 additions & 0 deletions lib/tests/modules/before-apply.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
{ options, lib, ... }:

# Tests wheter a apply function properly generates the `beforeApply` option attribute

{
options = {
optionWithoutApply = lib.mkOption {
default = false;
type = lib.types.bool;
};

optionWithApply = lib.mkOption {
default = false;
type = lib.types.bool;
apply = x: !x;
};

okChecks = lib.mkOption {};
};
config.okChecks = builtins.addErrorContext "while evaluating the assertions" (
assert options.optionWithoutApply.beforeApply == options.optionWithoutApply.value;
assert options.optionWithApply.beforeApply == !options.optionWithApply.value;
true);
}