Skip to content
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.

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

lib/types: Introduce lazyAttrsOf #70138

Merged
merged 6 commits into from Jan 10, 2020
Merged

lib/types: Introduce lazyAttrsOf #70138

merged 6 commits into from Jan 10, 2020

Conversation

@Infinisil
Copy link
Member

@Infinisil Infinisil commented Sep 30, 2019

Motivation for this change

This introduces lazyAttrsOf, which is like attrsOf, but lazy in its values. So a single attribute can be accessed without forcing all others.

The tradeoff is that it has worse support for conditional definitions, so when defining foo.some-attr = mkIf false 10 for some lazyAttrsOf option foo, evaluating foo ? some-attr will be true even though it shouldn't be because of the mkIf false. So this option should ideally only be used where conditional definitions aren't needed.

I saw this originally in @oxij's #57123, where I thought the justification wasn't very convincing. But since then I found 2 other uses for such a type:

  • Described in #70138 (comment). In short: By making _module.args be lazy in its values, we can evaluate parts of NixOS configurations without forcing all arguments. Conditional definitions are completely useless for module arguments, so this has no downside. This PR implements this.
  • Described in NixOS/rfcs#42 (comment). In short: It allows lazyAttrsOf values to depend on other values in the same lazyAttrsOf without having infinite recursion. This is very useful in case absent values are encoded with nullOr, in which case conditional definitions can be replaced with assigment of null.

Best reviewed commit-by-commit.

TODO

  • Write test
  • Ensure backwards compatibility
  • Write docs, mentioning what the drawback of this is
  • Write comments and commit subject

cc @oxij @nbp @Profpatsch @rycee

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Oct 2, 2019

Wow! @arcnmx asked why setting _module.args.bar = throw "Bar evaluated!" would throw even when bar wasn't supposed to be evaluated. Here's a reproducible example:

with import <nixpkgs/lib>;
(evalModules {
  modules = [
    ({ foo, bar, ... }: {
      config._module.args = {
        foo = "Value of foo";
        bar = throw "Bar evaluated!";
      };
    })
  ];
}).config._module.args.foo

Well turns out args is defined as an attrsOf! With lazyAttrsOf this is no problem anymore, and I think this can be the first justified use of lazyAttrsOf in nixpkgs (especially since it doesn't make sense to have mkIf false on such a value).

@Infinisil Infinisil force-pushed the Infinisil:lazyAttrsOf branch from 22ddd8a to 9ee204c Oct 12, 2019
@Infinisil Infinisil force-pushed the Infinisil:lazyAttrsOf branch 2 times, most recently from baab1ff to 33c443c Oct 12, 2019
@Infinisil Infinisil marked this pull request as ready for review Oct 12, 2019
@Infinisil Infinisil requested review from edolstra and nbp as code owners Oct 12, 2019
@Infinisil Infinisil requested review from Profpatsch and rycee Oct 12, 2019
@Infinisil Infinisil force-pushed the Infinisil:lazyAttrsOf branch from 33c443c to eb49928 Oct 14, 2019
@Infinisil Infinisil force-pushed the Infinisil:lazyAttrsOf branch from eb49928 to be840b1 Nov 7, 2019
@domenkozar
Copy link
Member

@domenkozar domenkozar commented Nov 7, 2019

@domenkozar domenkozar mentioned this pull request Nov 7, 2019
0 of 1 task complete
Copy link
Contributor

@roberth roberth left a comment

mkIf false

I don't think this needs to be part of this PR, but I'd like to share my thoughts.

We could add an 'empty' or 'placeholder' value to the type in cases where mkIf false ... can be assigned a sane value. It is up to the consuming code to handle this case correctly though.
It requires some care from the module author though, because the 'empty' object must be treated the same as an attribute that doesn't exist.

In my implementation (thanks Domen for linking) I wrote a single generic function that can do both. In hindsight that approach complicated the code unnecessarily. If it's implemented it should probably be a separate type lazyAttrsWithDefault emptyValue t.

I wrote it that way to make the type to match the capability of attrsOf as closely as possible but I'm not certain that it's a useful niche to fill.

<para>
An attribute set of where all the values are of
<replaceable>t</replaceable> type. Multiple definitions result in the
joined attribute set. This is the lazy version of <varname>types.attrsOf

This comment has been minimized.

@roberth

roberth Nov 7, 2019
Contributor

Suggested change
joined attribute set. This is the lazy version of <varname>types.attrsOf
merged attribute set where duplicate attributes are merged according to <replaceable>t</replaceable>. This is a lazy version of <varname>types.attrsOf
Copy link
Contributor

@roberth roberth left a comment

The documentation should point out that the type checking of all attributes still happens.
In my implementation for example it doesn't check the attributes, because my goal was to improve performance.

To replicate that behavior we could add type functions unchecked and lazyChecked which wrap a type to disable the check or run it before returning the value. That way you can have attrsOf (lazyChecked package) to prevent instantiating some packages if you don't need them, but still want to prevent non-package things from being used there.
These functions don't need to block this pr though.

@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Nov 11, 2019

@roberth After thinking about it for a while, what you suggested with a default value sounds like a good idea. Because how this PR is now, there's no way to recover from a mkIf false, since the.option ? foo returns true but accessing it errors. I think we could do with a much simpler interface though, even going as far as using null as that special value without being able to customize it. Because I think all such attrsOf-like options should allow unsetting keys with null, so there one shouldn't need to distinguish between null and mkIf false. This however also implies that the given elemType needs to be nullOr, which leads to this nicely sounding type:

let {
  nullableAttrsOf = elemType:
    /* Implementation behaves like `lazyAttrsOf (nullOr elemType)`
       description could be `attribute set of (null for unset or elemType)`
    */;
}

This would work really well for NixOS/rfcs#42 too. What do you think of this?

Copy link
Contributor

@roberth roberth left a comment

I like the simplicity of "just null it", but it has the problem you mentioned. We can rely on the type parameter to provide the answer.
It removes the need for nullableAttrsOf, which I find to be non-obvious and potentially ambiguous as to which part can be null: the whole attrset, or the attribute value?

if type.check def.value then res
else throw "The option value `${showOption loc}' in `${def.file}' is not of type `${type.description}'.")
(type.merge loc defsFinal) defsFinal
else throw "The option `${showOption loc}' is used but not defined.";

This comment has been minimized.

@roberth

roberth Nov 22, 2019
Contributor

This could ask type for a placeholder value; something like

Suggested change
else throw "The option `${showOption loc}' is used but not defined.";
else t.placeholder or (throw "The option `${showOption loc}' is used but not defined.");

nullOr can then have { placeholder = null; }.

Not too complicated I think, while giving anyone the opportunity to add placeholders as appropriate for their use cases.

This comment has been minimized.

@jtojnar

jtojnar Jan 9, 2020
Contributor

Do not forget to add the synchronization comment from valueDefined.

This comment has been minimized.

@Infinisil

Infinisil Jan 9, 2020
Author Member

Done that

@Infinisil Infinisil force-pushed the Infinisil:lazyAttrsOf branch 2 times, most recently from cd3ed29 to ddfcad1 Dec 13, 2019
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Dec 13, 2019

@roberth I updated the code with your suggestion. I've decided to use emptyValue to represent such an empty value, since placeholder is already a builtin nix function name, and it really represents an empty value. See commit df5478f. I updated the tests and the documentation on this too. Let me know if this looks good to you

@Infinisil Infinisil force-pushed the Infinisil:lazyAttrsOf branch from ddfcad1 to 00fdd10 Jan 7, 2020
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Jan 7, 2020

Rebased to fix conflicts

lib/types.nix Outdated Show resolved Hide resolved
lib/types.nix Show resolved Hide resolved
lib/types.nix Show resolved Hide resolved
@Infinisil Infinisil force-pushed the Infinisil:lazyAttrsOf branch from 9870d44 to 47d26a9 Jan 8, 2020
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Jan 8, 2020

@roberth Applied your first 2 suggestions

lib/modules.nix Show resolved Hide resolved
@Infinisil Infinisil force-pushed the Infinisil:lazyAttrsOf branch 2 times, most recently from 12ac360 to e3e6205 Jan 9, 2020
@roberth
roberth approved these changes Jan 9, 2020
@Infinisil Infinisil force-pushed the Infinisil:lazyAttrsOf branch from e3e6205 to 5a95c50 Jan 9, 2020
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Jan 9, 2020

I noticed that I didn't implement type merging, so I did that now. E.g. this now works (foo.one.bar can be evaluated without an exception):

{ config, lib, pkgs, ... }: {

  imports = [
    {
      options.foo = lib.mkOption {
        type = lib.types.lazyAttrsOf (lib.types.submodule {
          options.bar = lib.mkOption {
            default = false;
          };
        });
      };
    }
    {
      options.foo = lib.mkOption {
        type = lib.types.lazyAttrsOf (lib.types.submodule {
          options.baz = lib.mkOption {
            default = false;
          };
        });
      };
    }
  ];

  config = {
    boot.loader.grub.device = "nodev";
    fileSystems."/".device = "x";

    foo = {
      one.bar = true;
      two = throw "nothing";
    };
  };
}

Using attrsOf elemType // wasn't worth it anymore, and it also didn't seem to harmonize with type merging well at all, so I'm just declaring all attributes separately in lazyAttrsOf.

@roberth
roberth approved these changes Jan 9, 2020
Infinisil and others added 6 commits Oct 12, 2019
Without this change, accessing `mergedValue` from `mergeDefinitions` in
case there are no definitions will throw an error like

  error: evaluation aborted with the following error message: 'This case should never happen.'

This change makes it throw the appropriate error

  error: The option `foo' is used but not defined.

This is fully backwards compatible.
Co-Authored-By: Robert Hensing <roberth@users.noreply.github.com>
The standard attrsOf is strict in its *values*, meaning it's impossible to
access only one attribute value without evaluating all others as well.
lazyAttrsOf is a version that doesn't have that problem, at the expense
of conditional definitions not properly working anymore.
@Infinisil Infinisil force-pushed the Infinisil:lazyAttrsOf branch from 5a95c50 to 9e97e64 Jan 10, 2020
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Jan 10, 2020

Rebased to fix conflicts, will merge after ofborg is happy

@Infinisil Infinisil merged commit 5239b32 into NixOS:master Jan 10, 2020
12 checks passed
12 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@Infinisil Infinisil deleted the Infinisil:lazyAttrsOf branch Jan 10, 2020
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Feb 5, 2020

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

https://discourse.nixos.org/t/january-2020-in-nixos/5771/1

eadwu added a commit to eadwu/nixpkgs that referenced this pull request Apr 4, 2020
This reverts commit 5239b32, reversing
changes made to de26ac1.
eadwu added a commit to eadwu/nixpkgs that referenced this pull request Apr 4, 2020
This reverts commit 5239b32, reversing
changes made to de26ac1.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.