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

Improved submodule `name` passing #97378

Open
wants to merge 2 commits into
base: master
from
Open

Conversation

@Infinisil
Copy link
Member

@Infinisil Infinisil commented Sep 7, 2020

Motivation for this change

Submodules receive the name argument which tells them which attribute they are defined under. E.g.

{ lib, ... }: {
  options.foo = lib.mkOption {
    type = lib.types.attrsOf (lib.types.submodule ({ name, ... }: {
      options.name = lib.mkOption { default = name; };
    }));
  };
  config.foo.bar = {};
}

This makes the foo option evaluate to { bar = { name = "bar"; }; }

However there are also some problems with this, namely (hah):

  1. If the submodule isn't defined under an attrsOf, the name isn't relevant at all. E.g.

    { lib, ... }: {
      options.foo = lib.mkOption {
        type = lib.types.submodule ({ name, ... }: {
          options.name = lib.mkOption { default = name; };
        });
      };
      config.foo = {};
    }

    makes the foo option evaluate to { name = "foo"; }. This isn't useful at all since the name is just the option name, which you can already know without this.

  2. If the submodule is defined under multiple attrsOf, the name only tells you about the latest one. E.g.

    { lib, ... }: {
      options.foo = lib.mkOption {
        type = lib.types.attrsOf (lib.types.attrsOf (lib.types.submodule ({ name, ... }: {
          options.name = lib.mkOption { default = name; };
        })));
      };
      config.foo.bar.baz = {};
    }

    makes the foo option evaluate to { bar = { baz = { name = "baz"; }; }; }. You can see that the submodule doesn't get the information that it's under the bar attribute.

This changes fixes both of these problems by:

  • Pointing name to the last relevant attribute the submodule is defined under, if any. For problem 1 this means that you now get an error that the name argument isn't passed. Or if the submodule was defined in a deeper attribute set, it would return the attribute name of it, instead of the option name. This is backwards incompatible for submodules that relied on the option name it's defined under to change its behavior, but I think that's fine, because that isn't the intended use of name.
  • Exposing all attributes a submodule is defined under via the names argument, which contains them in reverse order. For problem 2 this means that names gives you [ "baz" "bar" ]. This is in reverse such that a static list index can be used to access elements, since the list can be arbitrarily long depending on where the submodule is evaluated.

Ping @roberth

@rycee You'll be able to remove this workaround with this change: https://github.com/nix-community/home-manager/blob/249650a07ee2d949fa599f3177a8c234adbd1bee/modules/lib/types-dag.nix#L24-L28

Things done
  • Added a test
@Infinisil Infinisil requested review from edolstra and nbp as code owners Sep 7, 2020
@Infinisil Infinisil mentioned this pull request Sep 7, 2020
2 of 2 tasks complete
@Infinisil Infinisil changed the title Submodule name Improved submodule `name` passing Sep 7, 2020
@Infinisil Infinisil force-pushed the Infinisil:submodule-name branch from 3744a37 to bbf6d96 Sep 7, 2020
@roberth
Copy link
Contributor

@roberth roberth commented Sep 8, 2020

This looks alright. I really like the "stack" approach of having the innermost name first. Perhaps this could be reflected in the name, like nameStack? After all the innermost thing doesn't have multiple names and "stack" gives a good clue.

I'm also wondering about lists. I'm not actually a big fan of those, but perhaps it's useful to include list indices in it? Not sure what the name should be in that case. pathStack? I guess it can be added later with a name like that if needed.

@Infinisil Infinisil force-pushed the Infinisil:submodule-name branch from bbf6d96 to 16b35bf Sep 8, 2020
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Sep 8, 2020

@roberth I like the suggestion of calling it nameStack instead, applied that.

I think using a list like that is fine for now.

Infinisil added 2 commits Sep 7, 2020
@Infinisil Infinisil force-pushed the Infinisil:submodule-name branch from 16b35bf to dcc0b20 Sep 21, 2020
@Infinisil
Copy link
Member Author

@Infinisil Infinisil commented Oct 9, 2020

@roberth You think this is looking good now? If there aren't any complaints I'll merge it

Copy link
Contributor

@roberth roberth left a comment

I'd prefer to read modules where information like name is passed down into submodules via the lexical scope, so I don't think we should recommend using this with list indexing operations like elemAt. I had to look that one up; don't think I've used it before and for good reason. List indexes have no meaning by themselves and can always be avoided by using high-level list operations like concatMap and whatnot, unless you're not managing configuration but implementing clever algorithms which is an entirely separate domain for which the module system is unfit. /rant

tl;dr name things, don't number things. Simple lexical scoping is one of the nicest things in functional programming, so let's use it.

Still I think this feature could be useful for some generic purpose. Did you have something like that in mind?

# For backwards compatibility, expose the most relevant name
# directly
Comment on lines +472 to +473

This comment has been minimized.

@roberth

roberth Oct 10, 2020
Contributor

Suggested change
# For backwards compatibility, expose the most relevant name
# directly
# Conveniently expose the most relevant name directly

This is the most used one, so let's keep it around.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.