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

Add types.attrTag #284551

Merged
merged 23 commits into from
Apr 9, 2024
Merged

Add types.attrTag #284551

merged 23 commits into from
Apr 9, 2024

Conversation

roberth
Copy link
Member

@roberth roberth commented Jan 28, 2024

Description of changes

The purpose of attrTag is to describe values that are already structured with this exact syntax, in RFC 42 settings.

It can also be used as an alternative to other sum type representations that aren't merged yet (edit: in some cases). This may or may not be nice, but that's irrelevant because attrTag is needed for a specific purpose anyway, as mentioned.
My goal here is only to introduce mechanism, not policy. For instance, if we want to pick a favorite sum type representation, or formulate some other kinds of recommendations, that can be done separately, later, when we have practical experience with multiple sum types.

I've updated the docs to lay out the new concept and how it relates to the existing "union-like" types.

Other sum type in development (not suitable for my use case as mentioned)

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 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.

Add a 👍 reaction to pull requests you find important.

@roberth roberth requested a review from infinisil as a code owner January 28, 2024 15:39
@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 This PR adds or changes documentation 6.topic: module system About "NixOS" module system internals 6.topic: lib The Nixpkgs function library labels Jan 28, 2024
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

This is looking great! This really feels like a hole in the module system that nobody bothered filling until now.

There appears to be a problem when trying to build the manual with options of this type:

# configuration.nix
{ lib, ... }: {
  options.mine = lib.mkOption {
    type = lib.types.attrTag {
      a = lib.types.int;
    };
    description = "mine";
  };

  config.documentation.nixos.includeAllModules = true;
}
$ nix-build nixos --arg configuration ./configuration.nix -A config.system.build.manual.manual

       error: assertion '((opt)._type == "option")' failed

       at /home/tweagysil/src/nixpkgs/main/lib/types.nix:629:17:

          628|               builtins.addErrorContext "while checking that attrTag tag ${lib.strings.escapeNixIdentifier n} is an option with a type${inAttrPosSuffix args.tags n}" (
          629|                 assert opt._type == "option";
             |                 ^
          630|                 opt // {

nestedTypes = tags;
functor = (defaultFunctor "attrTagWith") // {
payload = { inherit tags; };
binOp =
Copy link
Member

Choose a reason for hiding this comment

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

There's a potential minor issue with supporting type merging, because users may write code like this:

{
  tag1 = 0;
  tag2 = 1;
}.${config.my.option}

And if some third-party module specifies another tag, this code breaks, without any good way to fix it.

Though I guess if we document that code needs to handle arbitrarily many tags, it might be fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Your example looks like a usage of enum.
Indeed attrTag has that same characteristic, but I don't think this will happen by accident.

Regarding the sameness:

  • enum [str1 .. strn] is dual to an attrset of values.
  • attrTag ... is dual to an attrset of functions.

lib/types.nix Outdated
};
})
tags;
substSubModules = m: attrTagWith { tags = mapAttrs (n: opt: opt // { type = (opt.type or types.unspecified).substSubModules m; }) tags; };
Copy link
Member

Choose a reason for hiding this comment

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

The types.unspecified looks odd here, is that necessary or can we avoid it?

Copy link
Member Author

Choose a reason for hiding this comment

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

In a module, the presence of opt.type would be guaranteed by fixupOptionType.
Calling that seems heavy-handed considering that this works, and is basically free, in terms of eval performance.

Copy link
Member

Choose a reason for hiding this comment

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

I'd rather avoid introducing additional instances of unspecified. That should be deprecated. I think a call to fixupOptionType is fine, it's going to make deprecation of unspecified easier.

Alternatively I'd also be fine with starting requiring type's under attrTag only for now, so like

Suggested change
substSubModules = m: attrTagWith { tags = mapAttrs (n: opt: opt // { type = (opt.type or types.unspecified).substSubModules m; }) tags; };
substSubModules = m: attrTagWith { tags = mapAttrs (n: opt: opt // { type = (opt.type or (throw "type is required")).substSubModules m; }) tags; };

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've removed substSubModules because without getSubmodules, this turns out to be dead code anyway.
So I've found while trying to test it.

I don't think getSubmodules is feasible to implement, because we don't have a single set of modules to return, let alone put back with substSubModules

I'll be happy to do something about this later if we find some sub-optimal generated docs.

lib/types.nix Show resolved Hide resolved
@infinisil infinisil added the significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc. label Jan 31, 2024
@infinisil infinisil mentioned this pull request Feb 1, 2024
12 tasks
@roberth
Copy link
Member Author

roberth commented Feb 4, 2024

There appears to be a problem when trying to build the manual with options of this type:

    type = lib.types.attrTag {
      a = lib.types.int;
    };

I've changed it to take options instead of types. This showed that assert was not good enough. I've added a custom error message for this just now.

@Ma27
Copy link
Member

Ma27 commented Feb 9, 2024

It can also be used as an alternative to other sum type representations that aren't merged yet.

Just for the reference: this is not a substitute for all use-cases of taggedSubmodule though: for instance, in Nextcloud you have e.g. the option dbtype and if that's sqlite, dbuser/dbpass/etc are invalid whereas it is required for dbtype being mysql and psql.

@infinisil
Copy link
Member

infinisil commented Feb 26, 2024

@Ma27 I replied to your comment in the other PR since it's more relevant there: #254790 (comment)

I think we should finish discussing the need for that PR first before continuing with this one.

@Lassulus
Copy link
Member

What is dislike about this solution, is that there is no apparent indicator that a specific value is a sum-type. it just looks like a normal attribute set and you either have to discover it through configuring it wrong or reading the options source code? with the taggedSubmodule type that is IMHO clearer, but this code is clearer.

@roberth
Copy link
Member Author

roberth commented Feb 28, 2024

@Lassulus We need both types, because both occur in the wild, in RFC 42 settings.

Other sum type in development

Not intended to compete, but a statement of fact. In a non-RFC 42 context they are alternatives to each other, and I don't think I care which one gets chosen by module authors.

@infinisil
Copy link
Member

infinisil commented Mar 6, 2024

What is dislike about this solution, is that there is no apparent indicator that a specific value is a sum-type. it just looks like a normal attribute set and you either have to discover it through configuring it wrong or reading the options source code? with the taggedSubmodule type that is IMHO clearer, but this code is clearer.

That's fair. Maybe it could be this instead:

{
  type = "foo";
  foo = {
    # ...
  };
}

This would be a combination of attrTag and taggedSubmodule.

roberth added 15 commits April 4, 2024 11:54
This comment was added in 73f32d0, when it was already
supposed to be an attrset.
You can find them in the sub-options now.
... well, except for the ellipses, which hide unnecessary descriptions,
which you should write!
Thank you lheckemann for pointing this out!
Keep it simple for now.
I haven't managed to trigger the error, and it turns out that this
method is optional.
Specifically, getSubmodules is unimplemented (and unimplementable),
the tests pass, and we seem to have good location info.
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Looking good, let's merge!

The problems I mentioned in #284551 (review) are still not addressed, but it doesn't block this PR :)

@infinisil infinisil merged commit 4f1d724 into NixOS:master Apr 9, 2024
22 checks passed
rycee referenced this pull request in nix-community/home-manager May 13, 2024
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/home-manager-install-as-module-in-flake-users-option-and-home-manager-cli-not-available/45567/2

Copy link
Contributor

Backport failed for release-23.11, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-23.11
git worktree add -d .worktree/backport-284551-to-release-23.11 origin/release-23.11
cd .worktree/backport-284551-to-release-23.11
git switch --create backport-284551-to-release-23.11
git cherry-pick -x ca81a89839176c2e0d0b0fb01a931a4a65404fa6 0feea2dbd2eb76ffa1ba1db7d291bcb1a274d529 2ceb5558f95f978f1df70a2873b0fdf6ee87ff9f 1ad30772ea4feca0b597446e102fb323c89c41a0 6949bc21ce1de068bc1b21e273d15620c0e5770e 5b49672af44875df0449f7ff1b55965eedeec1da 42d3b54f0dab73e42e6643e0a3c0ee140b6c8112 0bc978322178fe6dbe91fe1477d275e1005d5e77 e090bb55f0599afcdfda63f3e7e27bfd1cfd0691 4c7d990badc4a6ef9adcffb0790902de94faa51e 2e1d470569f5a2a1a9154efdf6f21d12c659631d 475a55b2f0433d5324817ab9882102db65e3733e c0f54d3dea047729d754dc2419ecfe8ab2cce23b fa8b46adf467eec196da8808d62c9e03d69caa37 bcd774606a257fd6f14bda50effac67474005447 1465777b63d38988d5ecd81683d2975321e59d1a 47e4a18d018be9efaa93a199e24fbeedc80f14be 2d791b5f7b6575f2153f5971a2046bec15f637f7 f354686536823a0bedb95abf13b090b961485bf9 74831d8b38ad4754940d25a03a39ce66b6b6cf4f cf4968a9045e7404ba54598ee9608f7e33458006 22d7f146a4b11f89118fc4dd5939e09d2e4e652d 35fe538330b738b90d527d908ee487e38d42de40

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: lib The Nixpkgs function library 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 This PR adds or changes documentation 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants