Skip to content

[exploratory] Module system types.record, types.fix#257511

Draft
roberth wants to merge 2 commits intoNixOS:masterfrom
hercules-ci:records
Draft

[exploratory] Module system types.record, types.fix#257511
roberth wants to merge 2 commits intoNixOS:masterfrom
hercules-ci:records

Conversation

@roberth
Copy link
Member

@roberth roberth commented Sep 26, 2023

Description of changes

An exploratory PR to help me and others understand the module system design space.

The submodule type (or evalModules) can be thought of as a combination of

  • trees of options
  • a fixpoint

Splitting these concepts out might

  • make the module system easier to understand
  • speed it up by removing unneeded features, such as fixpoints in submodules that are purely data
  • lead to interesting insights

Observations so far:

  • The submodule fixpoint is not a fixpoint of merely the config, but also options.
    The option type interface is currently not conducive to the transfer of any kind of metadata. This also plagues solutions for built-in assertions and warnings.

  • record and fix were easy to implement. I can't imagine implementing submodule in a remotely similar time frame.

This was written at nix.camp 🏕️ .

Could implement #158598 except not the types.fix part of module evaluation this PR was originally going for.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • 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/)
  • 23.11 Release Notes (or backporting 23.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.

@roberth roberth added 0.kind: enhancement Add something new or improve an existing system. 6.topic: module system About "NixOS" module system internals labels Sep 26, 2023
@github-actions github-actions bot added the 6.topic: lib The Nixpkgs function library label Sep 26, 2023
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Sep 26, 2023
@roberth roberth removed the 0.kind: enhancement Add something new or improve an existing system. label Sep 26, 2023
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-10-31-nixpkgs-architecture-team-meeting-45/34846/1

Copy link
Member Author

Choose a reason for hiding this comment

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

I feel like fix is rather useless in a world where we still have (more capable) submodules to handle this aspect.
We could first, or only, make record useful on its own, for data modeling use cases, whereas submodule (or usually just evalModules) can continue to fill a more programming-like role.

mergeDefinitions mkOptionType isAttrs
;

record = args@{ fields, wildcard ? null }:
Copy link
Member Author

Choose a reason for hiding this comment

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

What's missing is something like optionalFields.
It's best not to mix required fields and optional fields, because

  • required fields can be returned without looking at their declarations, making it a bit more lazy, which is good for performance and robustness
  • if we don't have optional or wildcard fields, we can return all the fields without even looking at the definitions
  • it avoids having to partition the required and optional fields at runtime, which we'd then have to do; it's more efficient this way

then
if extraData == {}
then requiredFieldValues
else throw "A definition for option `${showOption loc}' has an unknown field."
Copy link
Member Author

Choose a reason for hiding this comment

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

Which one? Print at least one attribute name.

then requiredFieldValues
else throw "A definition for option `${showOption loc}' has an unknown field."
else
requiredFieldValues //
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe flip the //? I believe their mututally exclusive, so it doesn't really matter, but if @infinisil's lazy attrset update is implemented, I think we'll want to have the more predictable set on the right.

)
extraData;
nestedTypes = fields // {
# potential collision with `_wildcard` field
Copy link
Member Author

Choose a reason for hiding this comment

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

Unimportant, but I guess we could rename the original fields of that satisfy regex _*wildcard to get an extra underscore. (The word wildcard looks weird in a regex, but it's just the literal word)

Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be an error for nested types to have a wildcard option?

@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Aug 14, 2024
builtins.addErrorContext ("while evaluating the wildcard field `${fieldName}' of option `${showOption loc}'") (
(mergeDefinitions
(loc ++ [fieldName])
wildcard.type
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
wildcard.type
optionalFields.${fieldName} or wildcard.type
  • move all of this outside the wildcard == null conditional above.

@roberth
Copy link
Member Author

roberth commented Aug 14, 2024

I think we should forget about types.fix for now and make record good, with optional fields and by improving one or two error messages. Add docs and it should be good.
More importantly, I see now that it is worth having, and worth having record by itself.

@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 2, 2025
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: lib The Nixpkgs function library 6.topic: module system About "NixOS" module system internals 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

4 participants