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

Introduce types.raw and types.unconditional #132448

Closed
wants to merge 6 commits into from

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Aug 2, 2021

Motivation for this change

This PR contains a number of changes which are related or weakly dependent on each other:

  • Fixes the emptyValue of types.listOf, which should be [] not {}
  • For options that don't have any definitions, but whose type has an emptyValue, the emptyValue is used instead of throwing an error. Types who have an emptyValue:
    • nullOr: null
    • attrsOf: {}
    • listOf: []
    • submodule: {}
    • And wrapper types of the above
      This means that it's not necessary anymore to define default = {} for submodules or attrsOf to make it work without any definitions.
  • Introduce types.raw as a type that doesn't do any nested processing for mkIf, mkForce, etc. on its values. This is useful when this processing would throw errors or where it would be too expensive (such as with package sets like nixpkgs).
  • Use types.raw for _module.args's type. This finally fixes _module.args.name gets merged on assignment #53458
  • 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.

This is probably the last piece needed before we can finally deprecate types.attrs and types.unspecified.

Ping @roberth @nbp @danbst @sphaugh

Things done
  • Write tests
    • For emptyValue's being used
    • For types.raw
    • For types.unconditional <elemType>
  • Write docs
    • For types.raw
    • For types.unconditional <elemType>

@infinisil infinisil added the 6.topic: module system About "NixOS" module system internals label Aug 2, 2021
@infinisil infinisil changed the title Introduce types.raw and types.conditional Introduce types.raw and types.unconditional Aug 2, 2021
@roberth
Copy link
Member

roberth commented Aug 2, 2021

unconditional is neat!

However, I don't think pkgs is a valid use case for raw and I don't know what would be.

The crux of it is that "attrsets and lists can be big" is not a good intuition. Let me explain.

The thing is, you can always insert a minimal check. For an option to make sense it must be used in some way. The minimal way of using a thunk is to evaluate it to weak head normal form (whnf)*. This means that we always have the opportunity to put a check in front of the config's thunk, so that the check evaluates it to whnf and shallowly checks the type before returning it to whatever needed the value. The overhead of this is absolutely tiny compared to the rest of the module system and does not increase with value size, no more of the value is evaluated than would be without the check.

Of course this is only possible because of the fact that definitions are only checked when used. That doesn't usually feel like a relevant property and there's something unpleasant about it, but it works out rather well.

Another thing to note is that whnf doesn't always give you a lot of information, but discerning between types like int, list, attrset, string, bool is possible and useful.

I guess another way to put it is that checking whnf only doesn't increase strictness and is therefore of constant time (and small, because what can you really do).

* I don't think whnf has been defined anywhere for Nix, but the concept applies. It's essentially Expr::forceValue; the minimal amount of work before you can get any information out of it that is not an implementation detail.

@infinisil
Copy link
Member Author

@roberth Good point. But doing such an extra check can be done by types.addCheck, and types.raw can be the used as its base type.

However, after thinking about it, I think you're right, we don't need types.raw. The real motivation was to be able to replace all those types.attrs and types.unspecified with something better (since those two have weird merging behavior). However as you say, we need to evaluate values at least to WNHF, which for atomic values like ints/strings/bools/etc means evaluating the whole thing.

The only place we could therefore use such a types.raw is for attrsOf raw and listOf raw. However, instead of using types.raw for this, we should instead implement it as an unchecked types.attrs/types.list, which explicitly only checks for isAttrs/isList, without making any attempts at merging multiple definitions of the same attribute. This then doesn't need to involve the module system really and can be implemented via the type interface.

Or maybe we should still just have types.raw, but we don't need to do the whole "not processing any mkIf's, etc." involving the module system. Instead we can simply implement it defining a new type with merge = mergeOneOption. Additional checks, e.g. for it being an attribute set, can be added with types.addCheck. Or alternatively types.attrsOf types.raw works too.

@infinisil
Copy link
Member Author

I guess there's one potential use case of a types.raw that doesn't do any processing at all: If the value of an option has the _type attribute set to something that messes up the module system. Because it uses _type = "if" for mkIf's, and so on. But that's really very niche, I don't expect anybody to run into this.

@infinisil
Copy link
Member Author

I changed types.raw to not do this no-processing thing as mentioned above

@infinisil infinisil force-pushed the special-types branch 2 times, most recently from de783e0 to 7129dfc Compare October 12, 2021 16:00
@infinisil infinisil force-pushed the special-types branch 2 times, most recently from 9679e9d to 91cc1ca Compare October 15, 2021 15:32
@infinisil infinisil marked this pull request as ready for review October 15, 2021 15:33
@infinisil
Copy link
Member Author

Wrote tests and docs for everything now

Comment on lines 217 to 219
element type in `types.attrsOf` and `types.listOf`, the attribute keys and
the list length respectively can be known without evaluating each
individual attribute/list value strictly, making these structures lazy.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
element type in `types.attrsOf` and `types.listOf`, the attribute keys and
the list length respectively can be known without evaluating each
individual attribute/list value strictly, making these structures lazy.
element type in `types.attrsOf` and `types.listOf`, the attribute keys and
the list length respectively can be known without evaluating each
individual attribute/list value, keeping these structures lazy.
For instance, this prevents a case of infinite recursion by allowing elements
in an attribute set to reference other elements of the same attribute set
via the `config` module argument.

strictly -> no evaluation at all.
making lazy -> keeping lazy
add example use case

I decided not to define spine laziness here, although that could be a useful term.

Copy link
Member Author

Choose a reason for hiding this comment

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

Applied!

An empty list is [], not {}!

Also, non-empty lists shouldn't have a default of an empty list!
This patch makes it so that if an option has no definitions, but its
type specifies an `emptyValue`, that is used instead of throwing an error.

This means that for e.g. `attrsOf`, there's no need to specify a default
of `default = {}` anymore. Other types that have such a default are
`nullOr` (default `null`), `submodule` (default `{}`) and `listOf`
(default `[]`).
@infinisil
Copy link
Member Author

Updated to resolve merge conflicts after #148315

Allows replacing `types.lazyAttrsOf elemType` with `types.attrsOf
(types.unconditional elemType)`. Also works with lists: `types.listOf
(types.unconditional elemType)`.
Fixes NixOS#53458, as types.raw
doesn't allow setting multiple values
@infinisil
Copy link
Member Author

This PR is a bit too convoluted with too many separate changes. I split off the emptyValue fixes into #160487

I also thought about it some more: Making options default to the emptyValue of their type is a bit weird and might break some assumptions. Specifically currently there's the assumption that users get an error when a type is not set but it's used, which would kind of not be a thing anymore for values with such a default. So I'm not doing that in the above PR

@infinisil infinisil mentioned this pull request Feb 17, 2022
2 tasks
@infinisil
Copy link
Member Author

Regarding types.raw, this is now split off into #160489

@infinisil
Copy link
Member Author

Finally, the types.unconditional has been split off into #160491

@infinisil infinisil closed this Feb 17, 2022
@infinisil infinisil deleted the special-types branch February 17, 2022 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

_module.args.name gets merged on assignment
2 participants