nixpkgs icon indicating copy to clipboard operation
nixpkgs copied to clipboard

lib.types: init attrsWith

Open hsjobeki opened this issue 1 year ago • 12 comments

Description of changes

Unify the code path between attrsOf and lazyAtrrsOf

  • Set lazy = true to receive set lazy type
  • Set name="name" to configure the <name> placeholder for the docs. This is particularly useful when "<name>" doesn't make sense or when dealing with nested attrsOf submodule. Which would yield "<name>.<name>"

Apart from that everything should behave the same.

Usage example

mkOption {
  type = types.attrsWith {
    elemType = types.str;
    name = "userName";
    lazy = true;
  };
  default = "root";
}

Context

  • Closes https://github.com/NixOS/nixpkgs/issues/295872

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.11 Release Notes (or backporting 23.11 and 24.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.

Add a :+1: reaction to pull requests you find important.

hsjobeki avatar Sep 24 '24 13:09 hsjobeki

LGTM

DavHau avatar Sep 24 '24 13:09 DavHau

Apart from that everything should behave the same.

Seems like a wasted opportunity actually. The type could accept a function from name to type so you really have a name, that you can even feed into the submodule specialArgs using a different name name, etc.

Anyway, I guess what I'm getting at is that the attrs types should be factored into a single more capable function, because this property is not mutually exclusive with the other property about laziness (attrsOf/lazyAttrsOf).

See also

  • https://github.com/NixOS/nixpkgs/issues/295872

I'll add that to the description as well, because this would close that.

roberth avatar Sep 24 '24 17:09 roberth

Seems like a wasted opportunity actually. The type could accept a function from name to type so you really have a name, that you can even feed into the submodule specialArgs using a different name name, etc.

So before i start factoring this, and make sure the checks pass.

Do you mean something like this?

    # Interface
    # elemTypeFn :: String -> OptionType
    namedAttrsOf = attrName: elemTypeFn: mkOptionType rec {
     # ... 
    }
 # Simple Usage with submodule taking a function itself
 # I choose username as concrete name name here.
 # Couldn't make up a nice name for `namedAttr` but `name` is probably bad, since it already used in types.submodule in a different way.
   mkOption {
      type = namedAttrsOf "username" (attrName: submoduleWith {
          specialArgs = {
             inherit attrName;
          };
          modules = [
           # ... other modules receiving the name
          ];
        }
      );
   };

hsjobeki avatar Sep 25 '24 08:09 hsjobeki

I was thinking something along the lines of

attrsWith {
  name = "username";
  itemType = submoduleWith { modules = [ <...> ]; };
  # or, perhaps later:
  # itemTypeFunction = name: submoduleWith { modules = f name; specialArgs = g name; };
  # and perhaps later:
  # lazy = true;
};

This is more extensible and will let us cover lazyAttrsOf as well.

Also, instead of attrsWith we could do dict, because a submodule value is also an attrset but very different. (See also the issue and the defintiion of dictionary in https://nix.dev/manual/nix/2.24/development/json-guideline)

roberth avatar Sep 25 '24 11:09 roberth

@roberth okay. I refactored everthing accordingly. Lets see if all checks pass. I also added some smoke eval tests.

Also when looking at the usage. It might be more consistent if we name elemType -> just type. Unsure because the parent attribute as also called type.

options = {
  foo = mkOption {
    type = attrsWith {
      elemType = submodule {
      ...

hsjobeki avatar Sep 27 '24 08:09 hsjobeki

@infinisil Just added some little documentation and fixed the missing functor attributes (wrapped, type), that got removed from my previous commit. Nixos manual should be able to build.

hsjobeki avatar Oct 08 '24 20:10 hsjobeki

https://github.com/NixOS/nixpkgs/pull/344216#discussion_r1781965008 really needs to be added as tests, because those wrapped and type attributes aren't needed and actually break it :sweat_smile:. lib/tests/modules.sh would be a good entry-point for testing that.

infinisil avatar Oct 08 '24 23:10 infinisil

#344216 (comment) really needs to be added as tests, because those wrapped and type attributes aren't needed and actually break it 😅. lib/tests/modules.sh would be a good entry-point for testing that.

Is it breaking? I ran those tests that you gave me and they where all fine. I'll run the other ones lib/tests/modules.sh as well. Shouldn't those run with CI because all jobs passed as well?

EDIT: Just ran nix-build lib/tests/release.nix which seems to run the lib/tests/modules.sh in turn. Just exited with tests ok

hsjobeki avatar Oct 09 '24 08:10 hsjobeki

I mean that in the comment I linked, I showed you two additional test cases that should be added to modules.sh in this PR

infinisil avatar Oct 09 '24 09:10 infinisil

@infinisil Was somehow still stuck with the name merging. Previously attrsOf used:

functor = (defaultFunctor name) // { wrapped = elemType; }; Note that wrapped = elemType;

Then in defaultTypeMerge we got this pice of code:

...
    else if (f.wrapped != null && f'.wrapped != null) && (wrapped != null)
       then f.type wrapped
    # value types
    else if (f.payload != null && f'.payload != null) && (payload != null)
       then f.type payload

This means if we omit wrapped then f.type is called with payload and f.binOp is executed to merge the name. But if wrapped is omited, the manual wont build. (Not sure why and if this is a bug in the manual, since all other tests pass)

I found a possible solution: To switch the order of payload and wrapped not sure if this affects lazyness. All tests where fine, the manual could be built and name merging worked.

    # value types
    else if (f.payload != null && f'.payload != null) && (payload != null)
       then f.type payload
    # composed types
    else if (f.wrapped != null && f'.wrapped != null) && (wrapped != null)
       then f.type wrapped

I also noticed that maybe both wrapped and payload should return null if they are null instead of continuing to the next condition branch.

    # value types
    else if (f.payload != null && f'.payload != null)
       # both f and f' have payload, If merged payload is null we should return null and not try f.type wrapped instead because the merge of payload failed already.
       then 
         if payload != null 
         then f.type payload 
         else null
       
    # composed types
    else if (f.wrapped != null && f'.wrapped != null)
      # same with wrapped
       then          
         if wrapped != null
         then f.type wrapped 
         else null

hsjobeki avatar Oct 17 '24 14:10 hsjobeki

@infinisil @roberth what do you think about this. I have this questions still in mind:

  • [ ] Should <name> be configured via parameter name ? Because in the linked Issue everyone seemed convince that it shouldn't but it seems fine if i look at this implementation? (some alternatives: label,keyName s, keys, descriptor ....
  • [ ] Should defaultTypeMerge return early? (see https://github.com/NixOS/nixpkgs/pull/344216#issuecomment-2419681449)
  • [ ] Is it required that the functor still needs wrapped? (Because that means we need both payload and wrapped, which seems is a new case that never happened before?)

hsjobeki avatar Oct 17 '24 19:10 hsjobeki

Should <name> be configured via parameter name ?

This should be considered in the context of also having e.g. https://github.com/NixOS/nixpkgs/pull/218812/files, which is a laterally related but independent parameter. This is somewhat of a red herring though, because we have good reasons to prefer a different name name for other reasons.

Picking good names makes a big difference in the UX, as they're used over and over. "Name" is not great because it can refer to any of:

  • the default presentation of the placeholder in docs and diagnostics, <name>
  • the module argument by the name of name
  • now also: the setting that controls the placeholder
  • potentially: the setting that controls the attribute name of the module argument

This means we have to often qualify the name of name, which requires more thinking than just having separate terms. It'd be easier to just say label or placeholder

  • separate term, no need to decide whether to qualify it when talking or thinking about it
  • describes the purpose
  • unambiguous (especially placeholder)
    • sometimes labels are key-value pairs, like https://github.com/NixOS/nixpkgs/issues/305741

Compare:

  • "name" is the default name
  • "name" is the default label
  • "name" is the default placeholder

Compare:

  • an attribute set type constructor has a name for documentation
    • carries too much weight, implying that they are unique in some imaginary namespace, which is not the case
  • an attribute set type constructor has a label for documentation
    • similar to "name" but weaker implication
  • an attribute set type constructor has a placeholder for documentation

I think placeholder is the best out of these three.

As for the other two points, I think @infinisil has more experience with type merging.

roberth avatar Oct 18 '24 09:10 roberth

@infinisil If we remove wrapped from the functor then the following error hapens when building the nixos manual:

error:
       … while calling anonymous lambda

         at /nix/store/n9qgjvaqaipfi7y5skc0r1l27y487axh-nixos/lib/eval-cacheable-options.nix:1:1:

            1| { libPath
             | ^
            2| , pkgsLibPath

       … from call site

         at /nix/store/n9qgjvaqaipfi7y5skc0r1l27y487axh-nixos/lib/make-options-doc/default.nix:130:17:

          129|   filteredOpts = lib.filter (opt: opt.visible && !opt.internal) transformedOpts;
          130|   optionsList = lib.flip map filteredOpts
             |                 ^
          131|    (opt: opt

       … while calling 'flip'

         at /nix/store/cr32l4lf8qjmx7vnfkfqqgxx7dfdvx7k-lib/trivial.nix:317:16:

          316|   */
          317|   flip = f: a: b: f b a;
             |                ^
          318|

       … from call site

         at /nix/store/n9qgjvaqaipfi7y5skc0r1l27y487axh-nixos/lib/make-options-doc/default.nix:127:13:

          126| let
          127|   rawOpts = lib.optionAttrSetToDocList options;
             |             ^
          128|   transformedOpts = map transformOptions rawOpts;

       … while calling 'optionAttrSetToDocList''

         at /nix/store/cr32l4lf8qjmx7vnfkfqqgxx7dfdvx7k-lib/options.nix:320:32:

          319|
          320|   optionAttrSetToDocList' = _: options:
             |                                ^
          321|     concatMap (opt:

       … while calling anonymous lambda

         at /nix/store/cr32l4lf8qjmx7vnfkfqqgxx7dfdvx7k-lib/options.nix:321:16:

          320|   optionAttrSetToDocList' = _: options:
          321|     concatMap (opt:
             |                ^
          322|       let

       … from call site

         at /nix/store/cr32l4lf8qjmx7vnfkfqqgxx7dfdvx7k-lib/options.nix:358:26:

          357|         # builtins.trace opt.loc
          358|         [ docOption ] ++ optionals subOptionsVisible subOptions) (collect isOption options);
             |                          ^
          359|

       … while calling 'optionals'

         at /nix/store/cr32l4lf8qjmx7vnfkfqqgxx7dfdvx7k-lib/lists.nix:820:5:

          819|     cond:
          820|     elems: if cond then elems else [];
             |     ^
          821|

       … from call site

         at /nix/store/cr32l4lf8qjmx7vnfkfqqgxx7dfdvx7k-lib/options.nix:353:31:

          352|           let ss = opt.type.getSubOptions opt.loc;
          353|           in if ss != {} then optionAttrSetToDocList' opt.loc ss else [];
             |                               ^
          354|         subOptionsVisible = docOption.visible && opt.visible or null != "shallow";

       … while calling 'optionAttrSetToDocList''

         at /nix/store/cr32l4lf8qjmx7vnfkfqqgxx7dfdvx7k-lib/options.nix:320:32:

          319|
          320|   optionAttrSetToDocList' = _: options:
             |                                ^
          321|     concatMap (opt:

       … from call site

         at /nix/store/cr32l4lf8qjmx7vnfkfqqgxx7dfdvx7k-lib/options.nix:358:67:

          357|         # builtins.trace opt.loc
          358|         [ docOption ] ++ optionals subOptionsVisible subOptions) (collect isOption options);
             |                                                                   ^
          359|

       … while calling 'collect'

         at /nix/store/cr32l4lf8qjmx7vnfkfqqgxx7dfdvx7k-lib/attrsets.nix:867:5:

          866|     pred:
          867|     attrs:
             |     ^
          868|     if pred attrs then

       … while calling 'collect'

         at /nix/store/cr32l4lf8qjmx7vnfkfqqgxx7dfdvx7k-lib/attrsets.nix:867:5:

          866|     pred:
          867|     attrs:
             |     ^
          868|     if pred attrs then

       … from call site

         at /nix/store/cr32l4lf8qjmx7vnfkfqqgxx7dfdvx7k-lib/attrsets.nix:868:8:

          867|     attrs:
          868|     if pred attrs then
             |        ^
          869|       [ attrs ]

       … while calling 'isType'

         at /nix/store/cr32l4lf8qjmx7vnfkfqqgxx7dfdvx7k-lib/types.nix:76:18:

           75| rec {
           76|   isType = type: x: (x._type or "") == type;
             |                  ^
           77|

       … from call site

         at /nix/store/cr32l4lf8qjmx7vnfkfqqgxx7dfdvx7k-lib/types.nix:931:32:

          930|             # is just to avoid conflicts with potential options from the submodule
          931|             _freeformOptions = freeformType.getSubOptions prefix;
             |                                ^
          932|           };

       … while calling 'getSubOptions'

         at /nix/store/cr32l4lf8qjmx7vnfkfqqgxx7dfdvx7k-lib/types.nix:618:23:

          617|       emptyValue = { value = {}; };
          618|       getSubOptions = prefix: elemType.getSubOptions (prefix ++ ["<${placeholder}>"]);
             |                       ^
          619|       getSubModules = elemType.getSubModules;

       error: value is null while a set was expected
Cacheable portion of option doc build failed.
Usually this means that an option attribute that ends up in documentation (eg `default` or `description`) depends on the restricted module arguments `config` or `pkgs`.

Rebuild your configuration with `--show-trace` to find the offending location. Remove the references to restricted arguments (eg by escaping their antiquotations or adding a `defaultText`) or disable the sandboxed build for the failing module by setting `meta.buildDocsInSandbox = false`.

error: builder for '/nix/store/0w0qq6927cvy20qmlzdr5ydsxr91f42a-lazy-options.json.drv' failed with exit code 1;
       last 20 log lines:
       >           930|             # is just to avoid conflicts with potential options from the submodule
       >           931|             _freeformOptions = freeformType.getSubOptions prefix;
       >              |                                ^
       >           932|           };
       >
       >        … while calling 'getSubOptions'
       >
       >          at /nix/store/cr32l4lf8qjmx7vnfkfqqgxx7dfdvx7k-lib/types.nix:618:23:
       >
       >           617|       emptyValue = { value = {}; };
       >           618|       getSubOptions = prefix: elemType.getSubOptions (prefix ++ ["<${placeholder}>"]);
       >              |                       ^
       >           619|       getSubModules = elemType.getSubModules;
       >
       >        error: value is null while a set was expected
       > Cacheable portion of option doc build failed.
       > Usually this means that an option attribute that ends up in documentation (eg `default` or `description`) depends on the restricted module arguments `config` or `pkgs`.
       >
       > Rebuild your configuration with `--show-trace` to find the offending location. Remove the references to restricted arguments (eg by escaping their antiquotations or adding a `defaultText`) or disable the sandboxed build for the failing module by setting `meta.buildDocsInSandbox = false`.
       > 
       For full logs, run 'nix log /nix/store/0w0qq6927cvy20qmlzdr5ydsxr91f42a-lazy-options.json.drv'.
error: 1 dependencies of derivation '/nix/store/bqpgk6sy8f9kbx385hhbjpl4c3ylfnsj-options.json.drv' failed to build
error: 1 dependencies of derivation '/nix/store/rajbr0l6pky8s4rmc61g6i0fqdphkv2f-nixos-manual-html.drv' failed to build

hsjobeki avatar Oct 26 '24 09:10 hsjobeki

Could we perhaps merge the changes that are mere refactors first? Unfortunately the first commit already introduces a new feature.

Just having a very basic attrsWith { lazy? } merged would simplify the diff and unblock #351888.

roberth avatar Nov 01 '24 13:11 roberth

If we merge this one before it solves the issue with wrapped and payload. https://github.com/NixOS/nixpkgs/pull/350906

Which would clean those changes.

We can then merge only the lazy part

I can open up seperate PR for only the name placeholder.

I'll keep this PR. And opened up a new one to init attrsWith {lazy }

hsjobeki avatar Nov 07 '24 17:11 hsjobeki

@infinisil I just found why omitting functor.wrapped breaks the manual:

https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/services/mail/public-inbox.nix#L10

Seems like this line was added in https://github.com/NixOS/nixpkgs/pull/104457 And unfortunately depends on the internal structure of the attrsOf type.

I made this PR to fix the problem: https://github.com/NixOS/nixpkgs/pull/354800

hsjobeki avatar Nov 09 '24 15:11 hsjobeki

@roberth @infinisil #354738 introduced attrsWith itself. I'll rebase this; So the scope is to only allow customizing the placeholder itself.

hsjobeki avatar Dec 03 '24 07:12 hsjobeki

Eval summary

  • Added packages: 0
  • Removed packages: 11
  • Changed packages: 1
  • Rebuild Linux: 1
  • Rebuild Darwin: 1

github-actions[bot] avatar Dec 03 '24 08:12 github-actions[bot]

Cool, thank you!

Ma27 avatar Dec 09 '24 15:12 Ma27

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

https://discourse.nixos.org/t/use-cases-of-option-type-internals/57317/1

nixos-discourse avatar Dec 12 '24 15:12 nixos-discourse