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

Zeioth commented 3 months ago

(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 commented 3 months ago

closes https://github.com/nvimtools/none-ls.nvim/issues/90

mochaaP commented 3 months ago

Correct me if I'm wrong: seems https://github.com/nvimtools/none-ls.nvim/blob/f5632db2491fbe02b54f1a321a98548a8ba2bd15/lua/null-ls/sources.lua?plain=1#L101-L107 already handled this (generator._failed)?

Zeioth commented 3 months ago

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 commented 3 months ago

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

Zeioth commented 3 months ago

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.

Zeioth commented 2 months ago

Because the tests are not passing, and we already have the plugin to solve this. Let's close the PR.

nvimtools / none-ls.nvim