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: init attrsWith #344216

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

hsjobeki
Copy link
Contributor

@hsjobeki hsjobeki commented Sep 24, 2024

Description of changes

Unify the code path between attrsOf and lazyAtrrsOf

  • Set lazy = true to receive set lazy type
  • Also now allow to set the "<name>" placeholder for option 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.

I first tried to wrap the attrsOf type and only override getSubOptions with a function taking a custom prefix. But it seems i am missing something.

Context

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 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: module system About "NixOS" module system internals 6.topic: lib The Nixpkgs function library labels Sep 24, 2024
@DavHau
Copy link
Member

DavHau commented Sep 24, 2024

LGTM

@roberth
Copy link
Member

roberth commented Sep 24, 2024

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

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

@hsjobeki
Copy link
Contributor Author

hsjobeki commented Sep 25, 2024

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
          ];
        }
      );
   };

@roberth
Copy link
Member

roberth commented Sep 25, 2024

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)

@hsjobeki hsjobeki changed the title lib.types: init namedAttrsOf lib.types: init attrsWith Sep 27, 2024
@hsjobeki hsjobeki force-pushed the lib/namedAttrsOf branch 3 times, most recently from d8d8643 to 6432843 Compare September 27, 2024 07:43
@hsjobeki
Copy link
Contributor Author

hsjobeki commented Sep 27, 2024

@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 {
      ...

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

In summary

  • This would be a good opportunity to fix showOption properly
  • Some microoptimization

# If the part is a named placeholder of the form "<...>" don't escape it.
# Required for compatibility with: namedAttrsOf
# Can lead to misleading escaping if somebody uses literally "<...>" in their option names.
# This is the trade-off to allow for named placeholders in option names.
Copy link
Member

Choose a reason for hiding this comment

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

Right, we actually have two types of option paths: abstract ones containing placeholders, and real ones that are just data.
Treating them the same isn't quite correct, and merging a workaround could complicate a real fix.
Also note that some of these path items are not module options but attrsOf attributes, and "*" will occur as an attribute name in certain configurations to represent a "pattern" that matches everything; for example when an HTTP server doesn't responds for an unknown virtual host, etc.

We could solve this in at least two ways

a. Leave the "option path" type as is, but use two functions

  • leave showOption as is, suitable for concrete option paths
  • add showAbstractOption, which implements these new rules
    b. Only improve the representation
  • represent abstract path items with a value like { _type = "optionPathPlaceholder"; metaVariable = "name"; __toString = this: "<${this.metaVariable}>"; }

I think (b) may have good backward compatibility and it solves the problem for module options; not just attrsOf keys.
__toString is mostly for compatibility with existing code that kind of works when a placeholder string is passed. This happens when generating option docs.

Copy link
Member

Choose a reason for hiding this comment

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

This is pre-existing tech debt, so while this is a good opportunity to fix it before making it slightly worse, perhaps this should be done in a follow-up PR instead.

Copy link
Contributor Author

@hsjobeki hsjobeki Sep 28, 2024

Choose a reason for hiding this comment

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

I'd prefer to do this in a follow up PR if you are okay with it. Should i remove the changes from this one?

expr = lib.showOption ["<name>" "<myName>" "*" "{foo}"];
expected = "<name>.<myName>.*.\"{foo}\"";
};

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for writing tests ❤️

I'd swear I've tested using optionAttrSetToDocList before, but that must have been on an unmerged PR or external project. Anyway, it'd be good to also test this in a more end-to-end way. You can keep the tests you already wrote.

expr = lib.showOption ["<name>" "<myName>" "*" "{foo}"];
expected = "<name>.<myName>.*.\"{foo}\"";
};

testCartesianProductOfEmptySet = {
Copy link
Member

Choose a reason for hiding this comment

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

thought:
Most of the tests are in lib/tests/modules{,.sh}.
We should put all module system tests in one place. (but not in this PR)

}:
let
typeName = "attrsOf";
lazyMergeFn = loc: defs:
Copy link
Member

Choose a reason for hiding this comment

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

Inlining single-use bindings and constants saves a small amount of evaluator memory every time this type is instantiated.

Not refactoring these two merge functions into a single one is good, removing a conditional from the hot path.
So I think the most efficient way to do this is to inline mergeFn and lazyMergeFn, but then leave their implementations exactly as they are.

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 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<name> as prefix in types.attrsOf types.submodule is often unhelpful
3 participants