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 types.unconditional #160491

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Feb 17, 2022

Motivation for this change

Introduce types.unconditional <elemType> as a type, which when used as e.g. attrsOf (unconditional str) can replace lazyAttrsOf str. This type ensures that conditional mkIf definitions can't influence whether the option is defined or not, and therefore make the attribute set values be evaluated lazily. This notably also works with listOf (unconditional str), making the list elements evaluated lazily, which was not possible before.

Split off from #132448

Things done
  • Write and run tests
  • Write docs

Allows replacing `types.lazyAttrsOf elemType` with `types.attrsOf
(types.unconditional elemType)`. Also works with lists: `types.listOf
(types.unconditional elemType)`.
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation labels Feb 17, 2022
@infinisil infinisil added the 6.topic: module system About "NixOS" module system internals label Feb 17, 2022
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Feb 17, 2022
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/tweag-nix-dev-update-25/18003/1

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Sep 21, 2022
Copy link
Member

@ncfavier ncfavier left a comment

Choose a reason for hiding this comment

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

Let's get this in while we wait for NixOS/nix#4090? I think this is a welcome simplification with no downsides.

@@ -151,9 +151,12 @@ rec {
# issue deprecation warnings recursively. Can also be used to reuse
# nested types
nestedTypes ? {}
# Whether `mkIf` assignments can cause the definiton to be optional in
# the parent option. If `processed` is false, this is implicitly false too
Copy link
Member

Choose a reason for hiding this comment

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

processed does not seem to have survived, whatever it was

isDefined = defsFinal != [];
# Note: We use `or true` in case `lib.types` from an older nixpkgs version
# that doesn't set `conditional` is used. Avoids some trouble down the road
isDefined = type.conditional or true -> defsFinal != [];
Copy link
Member

Choose a reason for hiding this comment

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

Let's maybe factor out the two type.conditional or trues

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 15, 2022
@@ -162,6 +165,10 @@ rec {
# nixos/doc/manual/development/option-types.xml!
types = rec {

unconditional = elemType: elemType // {
conditional = false;
Copy link
Member

@ncfavier ncfavier Dec 15, 2022

Choose a reason for hiding this comment

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

Suggested change
conditional = false;
conditional = false;
description = "unconditional ${optionDescriptionPhrase (class: class == "noun" || class == "composite") elemType}";
descriptionClass = "composite";

Maybe? Or lazy? IDK

@roberth
Copy link
Member

roberth commented Dec 16, 2022

Let's get this in while we wait for NixOS/nix#4090? I think this is a welcome simplification with no downsides.

That proposed change only allows access to attributes on the right hand side of // (or whatever can be translated into //s), and will still require evaluation of left hand sides when the attr is missing on the right. Crucially it will still force evaluation of all the attribute names when the selected attribute is missing, which is a problem because it elevates an easy to debug error (missing attribute) to a difficult error (infinite recursion, aka "uninformative" module system trace deluge).
In case a self-reference is optional, unconditional is the only solution, because it allows self.xyz or null to actually return a null instead of an infinite recursion.

(EDIT) haven't tested

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 18, 2023
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 20, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: module system About "NixOS" module system internals 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants