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

🐛fix(client): we dont want to attach a source if its command is not executable. #124

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Zeioth
Copy link

@Zeioth Zeioth commented May 11, 2024

(for sources which command is different than nil).

Problem

A explicitly registered external source will be attached even if the source command is not actually executable. This create several unwanted scenarios:

  • When a user uninstall the mason package of a registered external source, none-ls currently attach it anyway.
  • Trying to run the non existing command will display an error.

This is inconsistent with the behavior of builtins, which will only attach if the executable is available. This inconsistent behavior is likely to be a regression.

Example

The next formatter appear attached even though we uninstalled it and rebooted neovim (bottom right of the image).
screenshot_2024-05-10_18-49-50_149890376

Trying to run the formatter will of cause an error.
screenshot_2024-05-10_18-51-58_029003400

Solution

  • By not allowing clients to be attached if their executable do not exist, it's not possible to have an inconsistent state anymore.
  • Builtins and external sources now behave the same.

@Zeioth
Copy link
Author

Zeioth commented May 11, 2024

closes #90

@mochaaP
Copy link
Member

mochaaP commented May 11, 2024

Correct me if I'm wrong: seems

M.is_available = function(source, filetype, method)
if source._disabled or source.generator._failed then
return false
end
return (not filetype or matches_filetype(source, filetype)) and (not method or matches_method(source, method))
end
already handled this (generator._failed)?

@Zeioth
Copy link
Author

Zeioth commented May 12, 2024

Ok I just discovered the next:

none-ls architecture

  • always makes you explicitly load the clients you want to use in sources.
  • if you explicitely load a client, and its executable is not available, you will get a warning informing you (as in the screenshot above).

mason-null-ls

  • it iterates all available builtins, and load them for you.
  • But because mason-null-ls only look for builtins, it won't correctly load external sources automatically. As they live in require("none-ls.formatting.mysource"), require("none-ls.formatting.mysource"), etc while regular builtins live in require("null-ls").builtins.formatting.mysource

This provokes mason-null-ls is not able to find external sources.

Possible solution A

Fixing the issue in mason-null-ls.

Pros: Fixes the issue. Everything keep working the same.

Possible solution B

Fixing the issue in none-ls (as in this PR).

Pros: It fixes the issue.
Cons: It won't inform to users if the executable is not present anymore. It will just not load the source.

@Zeioth
Copy link
Author

Zeioth commented May 12, 2024

For now I'm gonna try to implement the solution in mason-null-ls. but I'm keeping the PR open.

@Zeioth
Copy link
Author

Zeioth commented May 20, 2024

I've implemented a plugin to load internal and external sources intelligently when using mason: https://github.com/Zeioth/none-ls-autoload.nvim

But I would still consider merging this PR, so we don't register things we know for a fact are not going to work when trying to run them.

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

Successfully merging this pull request may close these issues.

None yet

2 participants