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

No builtin before builtin functions can break NVM when those functions are overwriten #3239

Closed
Sargates opened this issue Nov 25, 2023 · 2 comments
Assignees
Labels
shell alias clobbering Anything dealing with users shadowing builtins with aliases or functions.

Comments

@Sargates
Copy link

I know the prompt said not to delete any parts. I don't care enough to fill it all out because the fix to this issue is so small

Operating system and version:

Windows 11, WSL Ubuntu, ZSH, NVM V0.39.5

How did you install nvm?

Install script

What steps did you perform?

See below

Is there anything in any of your profile files that modifies the PATH?

No

The issue and its fix is just to add the builtin keyword before each call of a builtin function.
In my case, I declared a function called hash in my .zshrc which overwrites the builtin hash function before nvm.sh gets sourced.
nvm.sh calls this new, overwritten hash function which may cause unintended behavior. I replaced the two calls of hash I could find in my build of nvm.sh with builtin hash instead and this fixed my issue.
This fix shouldn't break anything because it will always reference the intended hash function.

I know I could fix this by putting the lines

export NVM_DIR="$HOME/.nvm"
[ -s "$NVM_DIR/nvm.sh" ] && \. "$NVM_DIR/nvm.sh"  # This loads nvm
[ -s "$NVM_DIR/bash_completion" ] && \. "$NVM_DIR/bash_completion"  # This loads nvm bash_completion

before the part that sources all my custom .zsh files, but I found this issue and figured I would report it.

Side note:
I don't really use NVM or even Node, so I don't know what errors this specific case might cause.
What I did to cause this issue (overwriting the hash function) probably breaks some aspect of nvm so if someone does the same thing it might lead someone in the complete wrong direction of how to fix their issue.
The only reason I caught this was because of the way I implemented my hash function which caused there to be some output whenever I opened up a new terminal.

@Sargates Sargates changed the title No builtin before builtin functions can break NVM when those functions are overwriten No builtin before builtin functions can break NVM when those functions are overwriten Nov 25, 2023
@ljharb
Copy link
Member

ljharb commented Nov 25, 2023

The proper change would be hashcommand hash; good catch. I'll push up a fix once Vampire/setup-wsl#50 is resolved.

Note that it's a bad idea to override builtins anyways.

@ljharb ljharb self-assigned this Nov 25, 2023
@ljharb ljharb added the shell alias clobbering Anything dealing with users shadowing builtins with aliases or functions. label Nov 25, 2023
@ljharb ljharb closed this as completed in cc765cc Dec 5, 2023
@area
Copy link

area commented Dec 5, 2023

This seems to break under zsh for me, where command hash returns zsh: command not found: hash.

EDIT: See #3247

ljharb added a commit to ljharb/nvm that referenced this issue Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
shell alias clobbering Anything dealing with users shadowing builtins with aliases or functions.
Projects
None yet
Development

No branches or pull requests

4 participants
@ljharb @area @Sargates and others