suggestion to use `inherit`

[Aleksanaa]

Is your feature request related to a problem? Please describe.

let
  a = 1;
in
{
  inherit a;
}

Looks more readable than

let
  a = 1;
in
{
  a = a;
}

Describe the solution you'd like

1. Suggest a = a to be written in inherit a and a = foo.bar.a in inherit (foo.bar) a, with a hint.
2. Suggest inherit (foo) a; inherit (foo) b to be written in inherit (foo) a b.

Describe alternatives you've considered Don't implement it

[pbsds]

Try running statix on nixpkgs with only manual_inherit and manual_inherit_from enabled and see what you get. There are IMO many cases where i feel it lessens readability, especially in the middle of other attribute assignments that don't use inherit.

[Aleksanaa]

There are IMO many cases where i feel it lessens readability, especially in the middle of other attribute assignments that don't use inherit.

Could you give me some cases directly? I think I may not be sure which cases are you referring to.

[pbsds]

--- a/nixos/modules/security/pam.nix
+++ b/nixos/modules/security/pam.nix
@@ -99,7 +99,7 @@ let
     }));
   };
 
-  package = config.security.pam.package;
+  inherit (config.security.pam) package;
   parentConfig = config;
 
   pamOpts = { config, name, ... }: let cfg = config; in let config = parentConfig; in {
@@ -653,11 +653,11 @@ let
           { name = "kanidm"; enable = config.services.kanidm.enablePam; control = "sufficient"; modulePath = "${config.services.kanidm.package}/lib/pam_kanidm.so"; settings = {
             ignore_unknown_user = true;
           }; }
-          { name = "sss"; enable = config.services.sssd.enable; control = if cfg.sssdStrictAccess then "[default=bad success=ok user_unknown=ignore]" else "sufficient"; modulePath = "${pkgs.sssd}/lib/security/pam_sss.so"; }
-          { name = "krb5"; enable = config.security.pam.krb5.enable; control = "sufficient"; modulePath = "${pam_krb5}/lib/security/pam_krb5.so"; }
+          { name = "sss"; inherit (config.services.sssd) enable; control = if cfg.sssdStrictAccess then "[default=bad success=ok user_unknown=ignore]" else "sufficient"; modulePath = "${pkgs.sssd}/lib/security/pam_sss.so"; }
+          { name = "krb5"; inherit (config.security.pam.krb5) enable; control = "sufficient"; modulePath = "${pam_krb5}/lib/security/pam_krb5.so"; }
           { name = "oslogin_login"; enable = cfg.googleOsLoginAccountVerification; control = "[success=ok ignore=ignore default=die]"; modulePath = "${pkgs.google-guest-oslogin}/lib/security/pam_oslogin_login.so"; }
           { name = "oslogin_admin"; enable = cfg.googleOsLoginAccountVerification; control = "[success=ok default=ignore]"; modulePath = "${pkgs.google-guest-oslogin}/lib/security/pam_oslogin_admin.so"; }
-          { name = "systemd_home"; enable = config.services.homed.enable; control = "sufficient"; modulePath = "${config.systemd.package}/lib/security/pam_systemd_home.so"; }
+          { name = "systemd_home"; inherit (config.services.homed) enable; control = "sufficient"; modulePath = "${config.systemd.package}/lib/security/pam_systemd_home.so"; }
           # The required pam_unix.so module has to come after all the sufficient modules
           # because otherwise, the account lookup will fail if the user does not exist
           # locally, for example with MySQL- or LDAP-auth.
@@ -677,26 +677,26 @@ let
           { name = "ssh_agent_auth"; enable = config.security.pam.sshAgentAuth.enable && cfg.sshAgentAuth; control = "sufficient"; modulePath = "${pkgs.pam_ssh_agent_auth}/libexec/pam_ssh_agent_auth.so"; settings = {
             file = lib.concatStringsSep ":" config.security.pam.sshAgentAuth.authorizedKeysFiles;
           }; }
-          (let p11 = config.security.pam.p11; in { name = "p11"; enable = cfg.p11Auth; control = p11.control; modulePath = "${pkgs.pam_p11}/lib/security/pam_p11.so"; args = [
+          (let inherit (config.security.pam) p11; in { name = "p11"; enable = cfg.p11Auth; inherit (p11) control; modulePath = "${pkgs.pam_p11}/lib/security/pam_p11.so"; args = [
             "${pkgs.opensc}/lib/opensc-pkcs11.so"
           ]; })
-          (let u2f = config.security.pam.u2f; in { name = "u2f"; enable = cfg.u2fAuth; control = u2f.control; modulePath = "${pkgs.pam_u2f}/lib/security/pam_u2f.so"; inherit (u2f) settings; })
-          (let ussh = config.security.pam.ussh; in { name = "ussh"; enable = config.security.pam.ussh.enable && cfg.usshAuth; control = ussh.control; modulePath = "${pkgs.pam_ussh}/lib/security/pam_ussh.so"; settings = {
+          (let inherit (config.security.pam) u2f; in { name = "u2f"; enable = cfg.u2fAuth; inherit (u2f) control; modulePath = "${pkgs.pam_u2f}/lib/security/pam_u2f.so"; inherit (u2f) settings; })
+          (let inherit (config.security.pam) ussh; in { name = "ussh"; enable = config.security.pam.ussh.enable && cfg.usshAuth; inherit (ussh) control; modulePath = "${pkgs.pam_ussh}/lib/security/pam_ussh.so"; settings = {
             ca_file = ussh.caFile;
             authorized_principals = ussh.authorizedPrincipals;
             authorized_principals_file = ussh.authorizedPrincipalsFile;
             inherit (ussh) group;
           }; })
-          (let oath = config.security.pam.oath; in { name = "oath"; enable = cfg.oathAuth; control = "requisite"; modulePath = "${pkgs.oath-toolkit}/lib/security/pam_oath.so"; settings = {
+          (let inherit (config.security.pam) oath; in { name = "oath"; enable = cfg.oathAuth; control = "requisite"; modulePath = "${pkgs.oath-toolkit}/lib/security/pam_oath.so"; settings = {
             inherit (oath) window digits;
             usersfile = oath.usersFile;
           }; })
-          (let yubi = config.security.pam.yubico; in { name = "yubico"; enable = cfg.yubicoAuth; control = yubi.control; modulePath = "${pkgs.yubico-pam}/lib/security/pam_yubico.so"; settings = {
+          (let yubi = config.security.pam.yubico; in { name = "yubico"; enable = cfg.yubicoAuth; inherit (yubi) control; modulePath = "${pkgs.yubico-pam}/lib/security/pam_yubico.so"; settings = {
             inherit (yubi) mode debug;
             chalresp_path = yubi.challengeResponsePath;
             id = mkIf (yubi.mode == "client") yubi.id;
           }; })
-          (let dp9ik = config.security.pam.dp9ik; in { name = "p9"; enable = dp9ik.enable; control = dp9ik.control; modulePath = "${pkgs.pam_dp9ik}/lib/security/pam_p9.so"; args = [
+          (let inherit (config.security.pam) dp9ik; in { name = "p9"; inherit (dp9ik) enable; inherit (dp9ik) control; modulePath = "${pkgs.pam_dp9ik}/lib/security/pam_p9.so"; args = [
             dp9ik.authserver
           ]; })
           { name = "fprintd"; enable = cfg.fprintAuth; control = "sufficient"; modulePath = "${config.services.fprintd.package}/lib/security/pam_fprintd.so"; }
@@ -722,7 +722,7 @@ let
               || cfg.duoSecurity.enable
               || cfg.zfs))
             [
-              { name = "systemd_home-early"; enable = config.services.homed.enable; control = "optional"; modulePath = "${config.systemd.package}/lib/security/pam_systemd_home.so"; }
+              { name = "systemd_home-early"; inherit (config.services.homed) enable; control = "optional"; modulePath = "${config.systemd.package}/lib/security/pam_systemd_home.so"; }
               { name = "unix-early"; enable = cfg.unixAuth; control = "optional"; modulePath = "${package}/lib/security/pam_unix.so"; settings = {
                 nullok = cfg.allowNullPassword;
                 inherit (cfg) nodelay;
@@ -738,21 +738,21 @@ let
               { name = "mount"; enable = cfg.pamMount; control = "optional"; modulePath = "${pkgs.pam_mount}/lib/security/pam_mount.so"; settings = {
                 disable_interactive = true;
               }; }
-              { name = "kwallet"; enable = cfg.kwallet.enable; control = "optional"; modulePath = "${cfg.kwallet.package}/lib/security/pam_kwallet5.so"; }
+              { name = "kwallet"; inherit (cfg.kwallet) enable; control = "optional"; modulePath = "${cfg.kwallet.package}/lib/security/pam_kwallet5.so"; }
               { name = "gnome_keyring"; enable = cfg.enableGnomeKeyring; control = "optional"; modulePath = "${pkgs.gnome-keyring}/lib/security/pam_gnome_keyring.so"; }
-              { name = "intune"; enable = config.services.intune.enable; control = "optional"; modulePath = "${pkgs.intune-portal}/lib/security/pam_intune.so"; }
-              { name = "gnupg"; enable = cfg.gnupg.enable; control = "optional"; modulePath = "${pkgs.pam_gnupg}/lib/security/pam_gnupg.so"; settings = {
+              { name = "intune"; inherit (config.services.intune) enable; control = "optional"; modulePath = "${pkgs.intune-portal}/lib/security/pam_intune.so"; }
+              { name = "gnupg"; inherit (cfg.gnupg) enable; control = "optional"; modulePath = "${pkgs.pam_gnupg}/lib/security/pam_gnupg.so"; settings = {
                 store-only = cfg.gnupg.storeOnly;
               }; }
-              { name = "faildelay"; enable = cfg.failDelay.enable; control = "optional"; modulePath = "${package}/lib/security/pam_faildelay.so"; settings = {
+              { name = "faildelay"; inherit (cfg.failDelay) enable; control = "optional"; modulePath = "${package}/lib/security/pam_faildelay.so"; settings = {
                 inherit (cfg.failDelay) delay;
               }; }
-              { name = "google_authenticator"; enable = cfg.googleAuthenticator.enable; control = "required"; modulePath = "${pkgs.google-authenticator}/lib/security/pam_google_authenticator.so"; settings = {
+              { name = "google_authenticator"; inherit (cfg.googleAuthenticator) enable; control = "required"; modulePath = "${pkgs.google-authenticator}/lib/security/pam_google_authenticator.so"; settings = {
                 no_increment_hotp = true;
               }; }
-              { name = "duo"; enable = cfg.duoSecurity.enable; control = "required"; modulePath = "${pkgs.duo-unix}/lib/security/pam_duo.so"; }
+              { name = "duo"; inherit (cfg.duoSecurity) enable; control = "required"; modulePath = "${pkgs.duo-unix}/lib/security/pam_duo.so"; }
             ]) ++ [
-          { name = "systemd_home"; enable = config.services.homed.enable; control = "sufficient"; modulePath = "${config.systemd.package}/lib/security/pam_systemd_home.so"; }
+          { name = "systemd_home"; inherit (config.services.homed) enable; control = "sufficient"; modulePath = "${config.systemd.package}/lib/security/pam_systemd_home.so"; }
           { name = "unix"; enable = cfg.unixAuth; control = "sufficient"; modulePath = "${package}/lib/security/pam_unix.so"; settings = {
             nullok = cfg.allowNullPassword;
             inherit (cfg) nodelay;
@@ -767,17 +767,17 @@ let
             ignore_unknown_user = true;
             use_first_pass = true;
           }; }
-          { name = "sss"; enable = config.services.sssd.enable; control = "sufficient"; modulePath = "${pkgs.sssd}/lib/security/pam_sss.so"; settings = {
+          { name = "sss"; inherit (config.services.sssd) enable; control = "sufficient"; modulePath = "${pkgs.sssd}/lib/security/pam_sss.so"; settings = {
             use_first_pass = true;
           }; }
-          { name = "krb5"; enable = config.security.pam.krb5.enable; control = "[default=ignore success=1 service_err=reset]"; modulePath = "${pam_krb5}/lib/security/pam_krb5.so"; settings = {
+          { name = "krb5"; inherit (config.security.pam.krb5) enable; control = "[default=ignore success=1 service_err=reset]"; modulePath = "${pam_krb5}/lib/security/pam_krb5.so"; settings = {
             use_first_pass = true;
           }; }
-          { name = "ccreds-validate"; enable = config.security.pam.krb5.enable; control = "[default=die success=done]"; modulePath = "${pam_ccreds}/lib/security/pam_ccreds.so"; settings = {
+          { name = "ccreds-validate"; inherit (config.security.pam.krb5) enable; control = "[default=die success=done]"; modulePath = "${pam_ccreds}/lib/security/pam_ccreds.so"; settings = {
             action = "validate";
             use_first_pass = true;
           }; }
-          { name = "ccreds-store"; enable = config.security.pam.krb5.enable; control = "sufficient"; modulePath = "${pam_ccreds}/lib/security/pam_ccreds.so"; settings = {
+          { name = "ccreds-store"; inherit (config.security.pam.krb5) enable; control = "sufficient"; modulePath = "${pam_ccreds}/lib/security/pam_ccreds.so"; settings = {
             action = "store";
             use_first_pass = true;
           }; }
@@ -785,7 +785,7 @@ let
         ]);
 
         password = autoOrderRules [
-          { name = "systemd_home"; enable = config.services.homed.enable; control = "sufficient"; modulePath = "${config.systemd.package}/lib/security/pam_systemd_home.so"; }
+          { name = "systemd_home"; inherit (config.services.homed) enable; control = "sufficient"; modulePath = "${config.systemd.package}/lib/security/pam_systemd_home.so"; }
           { name = "unix"; control = "sufficient"; modulePath = "${package}/lib/security/pam_unix.so"; settings = {
             nullok = true;
             yescrypt = true;
@@ -801,8 +801,8 @@ let
             config_file = "/etc/security/pam_mysql.conf";
           }; }
           { name = "kanidm"; enable = config.services.kanidm.enablePam; control = "sufficient"; modulePath = "${config.services.kanidm.package}/lib/pam_kanidm.so"; }
-          { name = "sss"; enable = config.services.sssd.enable; control = "sufficient"; modulePath = "${pkgs.sssd}/lib/security/pam_sss.so"; }
-          { name = "krb5"; enable = config.security.pam.krb5.enable; control = "sufficient"; modulePath = "${pam_krb5}/lib/security/pam_krb5.so"; settings = {
+          { name = "sss"; inherit (config.services.sssd) enable; control = "sufficient"; modulePath = "${pkgs.sssd}/lib/security/pam_sss.so"; }
+          { name = "krb5"; inherit (config.security.pam.krb5) enable; control = "sufficient"; modulePath = "${pam_krb5}/lib/security/pam_krb5.so"; settings = {
             use_first_pass = true;
           }; }
           { name = "gnome_keyring"; enable = cfg.enableGnomeKeyring; control = "optional"; modulePath = "${pkgs.gnome-keyring}/lib/security/pam_gnome_keyring.so"; settings = {
@@ -817,12 +817,12 @@ let
           }; }
           { name = "unix"; control = "required"; modulePath = "${package}/lib/security/pam_unix.so"; }
           { name = "loginuid"; enable = cfg.setLoginUid; control = if config.boot.isContainer then "optional" else "required"; modulePath = "${package}/lib/security/pam_loginuid.so"; }
-          { name = "tty_audit"; enable = cfg.ttyAudit.enable; control = "required"; modulePath = "${package}/lib/security/pam_tty_audit.so"; settings = {
+          { name = "tty_audit"; inherit (cfg.ttyAudit) enable; control = "required"; modulePath = "${package}/lib/security/pam_tty_audit.so"; settings = {
             open_only = cfg.ttyAudit.openOnly;
             enable = cfg.ttyAudit.enablePattern;
             disable = cfg.ttyAudit.disablePattern;
           }; }
-          { name = "systemd_home"; enable = config.services.homed.enable; control = "required"; modulePath = "${config.systemd.package}/lib/security/pam_systemd_home.so"; }
+          { name = "systemd_home"; inherit (config.services.homed) enable; control = "required"; modulePath = "${config.systemd.package}/lib/security/pam_systemd_home.so"; }
           { name = "mkhomedir"; enable = cfg.makeHomeDir; control = "required"; modulePath = "${package}/lib/security/pam_mkhomedir.so"; settings = {
             silent = true;
             skel = config.security.pam.makeHomeDir.skelDirectory;
@@ -855,8 +855,8 @@ let
             config_file = "/etc/security/pam_mysql.conf";
           }; }
           { name = "kanidm"; enable = config.services.kanidm.enablePam; control = "optional"; modulePath = "${config.services.kanidm.package}/lib/pam_kanidm.so"; }
-          { name = "sss"; enable = config.services.sssd.enable; control = "optional"; modulePath = "${pkgs.sssd}/lib/security/pam_sss.so"; }
-          { name = "krb5"; enable = config.security.pam.krb5.enable; control = "optional"; modulePath = "${pam_krb5}/lib/security/pam_krb5.so"; }
+          { name = "sss"; inherit (config.services.sssd) enable; control = "optional"; modulePath = "${pkgs.sssd}/lib/security/pam_sss.so"; }
+          { name = "krb5"; inherit (config.security.pam.krb5) enable; control = "optional"; modulePath = "${pam_krb5}/lib/security/pam_krb5.so"; }
           { name = "otpw"; enable = cfg.otpwAuth; control = "optional"; modulePath = "${pkgs.otpw}/lib/security/pam_otpw.so"; }
           { name = "systemd"; enable = cfg.startSession; control = "optional"; modulePath = "${config.systemd.package}/lib/security/pam_systemd.so"; }
           { name = "xauth"; enable = cfg.forwardXAuth; control = "optional"; modulePath = "${package}/lib/security/pam_xauth.so"; settings = {
@@ -873,14 +873,14 @@ let
             order = "user,group,default";
             debug = true;
           }; }
-          { name = "kwallet"; enable = cfg.kwallet.enable; control = "optional"; modulePath = "${cfg.kwallet.package}/lib/security/pam_kwallet5.so"; settings = lib.mkIf cfg.kwallet.forceRun { force_run = true; }; }
+          { name = "kwallet"; inherit (cfg.kwallet) enable; control = "optional"; modulePath = "${cfg.kwallet.package}/lib/security/pam_kwallet5.so"; settings = lib.mkIf cfg.kwallet.forceRun { force_run = true; }; }
           { name = "gnome_keyring"; enable = cfg.enableGnomeKeyring; control = "optional"; modulePath = "${pkgs.gnome-keyring}/lib/security/pam_gnome_keyring.so"; settings = {
             auto_start = true;
           }; }
-          { name = "gnupg"; enable = cfg.gnupg.enable; control = "optional"; modulePath = "${pkgs.pam_gnupg}/lib/security/pam_gnupg.so"; settings = {
+          { name = "gnupg"; inherit (cfg.gnupg) enable; control = "optional"; modulePath = "${pkgs.pam_gnupg}/lib/security/pam_gnupg.so"; settings = {
             no-autostart = cfg.gnupg.noAutostart;
           }; }
-          { name = "intune"; enable = config.services.intune.enable; control = "optional"; modulePath = "${pkgs.intune-portal}/lib/security/pam_intune.so"; }
+          { name = "intune"; inherit (config.services.intune) enable; control = "optional"; modulePath = "${pkgs.intune-portal}/lib/security/pam_intune.so"; }
         ];
       };
     };

Anyway, my brain skipped past the whole "suggestion" bit. Enforcing this is too opinionated, but as a helpful tip I'm all for it! :+1:

[Aleksanaa]

So a = a -> inherit a can be a good suggestion anyway. inherit (foo.bar) a can be more opinionated, and can be implemented but leave disabled by default.

[pbsds]

As a editor LSP i think both are fine, in nixpkgs CI i think neither should be enforced.

[Aleksanaa]

I don't think there is a case where a = a makes sense?

[inclyc]

After I read the comments above, I think we should distinguish inherit a and inherit (expr) a.

Pre-implementation NB: statix's linting / auto-suggestion did not take consideration for squashing multiple attributes inherited, or combine to existing inherit expression. See issue https://github.com/oppiliappan/statix/issues/83.

[pbsds]

I don't think there is a case where a = a makes sense?

i feel it lessens readability, especially in the middle of other attribute assignments that don't use inherit.

consider

{
   foo = bar;
   quux = lorem-ipsum;
   inherit baz;
   dependant = foobar;
}

When scanning that attribute set with my monkey brain i read the keys ["foo" "quux" "inherit" "dependant"], which is wrong. inherit statements are read backwards. This is likely why inherits are usually put at the top of attribute sets.

inherit of some but not all assignments also mess up alphabetical sorting, which in multiple cases are beneficial.

To reiterate, I'm not against a nifty editor suggestion which you're free to ignore or not, I'm against pestering nixpkgs contributors with ill considered and opinionated lints. It only adds noise and friction, an most will assume it is the consensus of whole of nixpkgs when a CI action does it and will as such not contest it no matter how silly the suggestion might be.

[inclyc]

inherit of some but not all assignments also mess up alphabetical sorting, which in multiple cases are beneficial.

So maybe we should suggest putting new inherit expressions at the top.

[pbsds]

also a neat editor suggestion :+1: