trueNAHO commented 10 months ago

Description

Unlike aliases, functions can be lazy loaded from certain paths. Consequently, translating aliases to their equivalent function definitions allows them to be lazy loaded, and thus improve fish's already slow startup time.

Removing programs.fish.shellAliases in favor of programs.fish.functions is undesired because home.shellAliases elegantly passes aliases to supported shells:

https://github.com/nix-community/home-manager/blob/c3ab5ea047e6dc73df530948f7367455749d8906/modules/home-environment.nix#L516-L518

Implementation Proposal

Do not add aliases to the main configuration file by removing the following lines:

https://github.com/nix-community/home-manager/blob/89df56fefe728bed13b4b5b99af196017e330dd5/modules/programs/fish.nix#L145-L146

https://github.com/nix-community/home-manager/blob/89df56fefe728bed13b4b5b99af196017e330dd5/modules/programs/fish.nix#L384-L385

Adapt the following function call to transform cfg.shellAliases to functions with only body definitions:

https://github.com/nix-community/home-manager/blob/89df56fefe728bed13b4b5b99af196017e330dd5/modules/programs/fish.nix#L394-L421

Essentially,

programs.fish.shellAliases = {
  g = "git";
};

Should translate to

programs.fish.functions = {
  g = "git";
};

or to

programs.fish.functions = {
  g = {
    body = "git";
  };
};

rycee commented 9 months ago

I don't use or know fish so I can't judge on this proposal. But it looks good to me. I'm just not certain why fish doesn't itself do this transformation if it is better to use functions over aliases?

I think it would be good if the fish module had some listed maintainers, I certainly cannot give good advice in these matters 😉

trueNAHO commented 9 months ago

I'm just not certain why fish doesn't itself do this transformation if it is better to use functions over aliases?

The fish documentation states:

Unlike other shells, this just makes functions - fish has no separate concept of an “alias”, we just use the word for a simple wrapping function like this. alias immediately creates a function.

The startup time could be improved by defining aliases in paths that can be lazy loaded, instead of defining them in the main configuration file.

pvsr commented 9 months ago

Alias doesn't do a lot but it does prevent recursion in a way that I don't think we can replicate in HM. For example fish.shellAliases.vim = "vim ./scratch" works fine since it gets rewritten to command vim ./scratch at runtime, but fish.functions.vim.body = "vim ./scratch" fails with "The function “vim” calls itself immediately, which would result in an infinite loop." We can detect when the alias name is the same as the command easily enough, but the problem is that we need to choose between command and builtin depending on the command and that decision depends on the contents of builtin --names in the fish instance that runs the function.

trueNAHO commented 9 months ago

We can detect when the alias name is the same as the command easily enough, but the problem is that we need to choose between command and builtin depending on the command and that decision depends on the contents of builtin --names in the fish instance that runs the function.

Fish claims that its alias command can automatically determine infinite alias recursions:

If the alias has the same name as the aliased command, you need to prefix the call to the program with command to tell fish that the function should not call itself, but rather a command with the same name. If you forget to do so, the function would call itself until the end of time. Usually fish is smart enough to figure this out and will refrain from doing so (which is hopefully in your interest).

(Source: https://fishshell.com/docs/current/language.html#defining-aliases)

The following three aliases demonstrate your previous examples:

$ alias echo="echo echo"
$ functions ech
function echo --description 'alias echo=echo echo'
 builtin echo echo $argv                                                                                             
end

$ alias nvim="nvim ."
$ functions nvim
function nvim --description 'alias nvim=nvim .'
 command nvim . $argv
end

$ alias undefined="nvim ."
$ functions undefined
function undefined --wraps='nvim .' --description 'alias undefined=nvim .'
  nvim . $argv
end

[T]he problem is that we need to choose between command and builtin depending on the command and that decision depends on the contents of builtin --names in the fish instance that runs the function.

Either we hard-code the current builtin --names output into the fish module and replicate the official implementation, or we close this issue to reduce the maintenance of keeping the builtin --names up-to-date.

However, it might be safe to assume that the language builtin --names are stable and will not receive drastic changes in the future. In that case, hard-coding the current builtin --names output into the fish module seems like the simplest solution, unless the Home Manager deployment system would update the builtin --names output.

terlar commented 9 months ago

Could perhaps the alias command be used to generate the function body. With runcommand or similar? At least if it could be used to write the function file directly.

trueNAHO commented 8 months ago

Could perhaps the alias command be used to generate the function body. With runcommand or similar? At least if it could be used to write the function file directly.

stale[bot] commented 5 months ago

Thank you for your contribution! I marked this issue as stale due to inactivity. Please be considerate of people watching this issue and receiving notifications before commenting 'I have this issue too'. We welcome additional information that will help resolve this issue. Please read the relevant sections below before commenting.

If you are the original author of the issue

* If this is resolved, please consider closing it so that the maintainers know not to focus on this. * If this might still be an issue, but you are not interested in promoting its resolution, please consider closing it while encouraging others to take over and reopen an issue if they care enough. * If you know how to solve the issue, please consider submitting a Pull Request that addresses this issue.

If you are not the original author of the issue

* If you are also experiencing this issue, please add details of your situation to help with the debugging process. * If you know how to solve the issue, please consider submitting a Pull Request that addresses this issue.

Memorandum on closing issues

Don't be afraid to manually close an issue, even if it holds valuable information. Closed issues stay in the system for people to search, read, cross-reference, or even reopen – nothing is lost! Closing obsolete issues is an important way to help maintainers focus their time and effort.

9ary commented 4 months ago

This seems like the most appropriate place to share this so: I've got an implementation I'm satisfied with for my own use. It doesn't really rely on any existing functionality from the fish module, in fact I'm currently sidestepping things a bit to make it work without conflicting with h-m.

Here's the commit: https://codeberg.org/novenary/nicks/commit/200965285e71116f86ee1f6e9612aa480ccb5289. I'd be happy to collaborate on making this upstreamable, but I wanted to gauge interest before opening a pull request. Currently there's a problem with aliasing builtins, if it comes down to it we can always do the pipe into source trick that fish's alias function uses. That aside I think it should be on par in terms of functionality.

stale[bot] commented 1 month ago