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/modules: add attrArgName #177576

Closed

Conversation

roberth
Copy link
Member

@roberth roberth commented Jun 13, 2022

Description of changes

Make the name module parameter configurable, so that extendModules/moduleType can be reused as a submodule type more than once, and so that the parameter can be domain-specific.

Closes #177564.

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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@roberth roberth force-pushed the lib-modules-add-attrArgName branch from ee992e1 to d06f5fa Compare June 14, 2022 22:01
@infinisil
Copy link
Member

infinisil commented Jun 15, 2022

I don't like this fix. The fact that a duplicate definition error occurs is an implementation detail, so it shouldn't have a user-facing fix. However, the capability of being able to specify the attribute name for "name" seems useful in itself. In fact, this is very similar to this PR and relates to that discussion: #97378 (comment)

@roberth
Copy link
Member Author

roberth commented Jun 15, 2022

The fact that a duplicate definition error occurs is an implementation detail, so it shouldn't have a user-facing fix.

I think I framed this solution wrong. The root of the problem is that it's currently impossible to differentiate the name arguments that are set at two different levels when type = moduleType or type = (extendModules f).type.
Modules defined in such options are merged with their parent module, causing a correct message about the duplicate name argument.

This PR isn't so much about solving the error message, as it is about being able to differentiate those names and use both their values in the resulting nested submodules.

However, the capability of being able to specify the attribute name for "name" seems useful in itself.
In fact, this is very similar to this PR and relates to that discussion: #97378 (comment)

I agree. Renaming or qualifying the name always felt like fighting the module system, while using multiple names is a perfectly valid thing to do.

I did think about your nameStack solution when I encountered the problem, and I saw the following challenges:

  • The nameStack value would have to correspond to the innermost set of modules in case of extendModules/moduleType. Otherwise, some names will be lost. This can not be achieved with a regular type, unless we use some sort of counter to set priorities. A special specialArgs might be an easier solution, but no less ugly imo.
  • Having a extendModules always cause the nameStack parameter to be overridden breaks the unwritten rule that an option with type = moduleType and no definitions is a clone of its parent module (modulo prefix).
  • Reconstructing the names from nameStack is finicky, having to track the order in which names were set, and through which options, so the correct number of pops can be determined.

@roberth
Copy link
Member Author

roberth commented Jun 15, 2022

The problem solved by this PR is not blocking other work anymore, at least not for me.

I've found another way of dealing with this. The #97378 discussion reminded me that it doesn't have to be attrsOf moduleType. By making it attrsOf (submodule (f moduleType)), I can avoid #177564.

@roberth
Copy link
Member Author

roberth commented Jun 22, 2022

Not terrible, not great, but not needed, so closing.

Fwiw, I also looked into making attrsOf pass the name explicitly, but couldn't find an easy solution.
Rough idea: types have a parameter that represents what context they expect to receive. (a crude coeffect system? Don't quote me on this)

So if attrsOf has now has type type -> type, we could make that

attrsOf   :: type (str -> c) -> type c

for all "type contexts" c.

A type context would be the same as, or convertible to module args.

This also enables currying; perhaps a working principled behind an alternative solution to #97378

x: attrsOf (attrsOf x)   :: type (str -> str -> c) -> type c

We can now also give descriptive types to other functions:

int :: type a
  # for any a

submoduleWith  :: { modules :: [module], contextualModule :: f module, ... } -> type (f module)
  # for any function f, ie not just type constructors

mkOption       :: { type :: type module, ... } -> option
  # most types are `type module` by virtue of being `forall a. type a`
  # while hardcoding `module` here is probably fine and it encodes
  # the completeness of modules nicely, we might want to support
  # something more general here, although I don't think we have any other
  # context aware types.
  # These types are for design purposes only anyway, so it doesn't really matter.

I would really like such semantics to back the feature and it seems quite feasible to implement it.

However, I'm dealing with plenty of more practical matters now, so I'll close it.

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.

Repeated use of moduleType or (extendModules foo).type in attrsOf can not use the name parameter.
2 participants