fix(command_resolver): fix resolve executable on Windows system

[IronLu233]

fix #1118

[jose-elias-alvarez]

Nit: can we actually add this to our path utils and read the value from there? I'd rather avoid duplicating the check.

[jose-elias-alvarez]

Two potential issues with this:

1. I'm not sure if mutating this directly on the params table will have any unexpected effects, but it seems risky.
2. If we're looking for eslint and don't find a local executable, then we'll be looking for eslint.cmd in $PATH (I'm actually not sure what this will do, but I suspect it won't work).

I think it would be less risky to declare local command_to_find = params.command and use that, then fall back to params.command.

[IronLu233]

Unlike Linux, Windows determine a file is executable or not, is rely on file's extension. when file's extension changes, the file will not be executable.

I rename prettier.cmd to prettier.cmd3, and it can not be executed.

[IronLu233]

Manually, I test two cases:

1. the source file is not in a npm project, means no package.json and node_modules.
2. the source file is a npm project, null-ls should use the local binary executable file

[jose-elias-alvarez]

I rebased the PR to fix CI and also removed deepcopy, which after further examination should be unnecessary, I think. I'm still concerned about this case:

If we're looking for eslint and don't find a local executable, then we'll be looking for eslint.cmd in $PATH (I'm actually not sure what this will do, but I suspect it won't work)

Based on your comment, my understanding is that with these changes, we can still fall back to the global eslint executable when a local executable isn't available. Is that right? @YangShaoyue Would you also be able to test this?

[jose-elias-alvarez]

I don't really know about Windows shells, but is neovim/neovim#21175 related to this at all?

[YangShaoyue]

I rebased the PR to fix CI and also removed deepcopy, which after further examination should be unnecessary, I think. I'm still concerned about this case:

If we're looking for eslint and don't find a local executable, then we'll be looking for eslint.cmd in $PATH (I'm actually not sure what this will do, but I suspect it won't work)

Based on your comment, my understanding is that with these changes, we can still fall back to the global eslint executable when a local executable isn't available. Is that right? @YangShaoyue Would you also be able to test this?

Hi, I tested those changes, it worked well for current situation. But I also have some concerns:

1. As you mentioned before, mutating params table directly may not be a good choice only for Windows platform. Maybe we can chose the executable file when it is about to be executed, or add one key/value pair represents executable file path on Windows platform (I don't know if there are some Lua's best pratices, it's just my own opinoin).
2. In this issue comment , for Situation 2, it seems that output of :echo exepath('eslint') is not the same as null-ls choosen executable file path which is local installed eslint file. And also the output of :echo exepath('eslint') doesn't have file extension, maybe neovim/neovim#21175 is related to this problem.

[jose-elias-alvarez]

If I understand the original issue correctly: it looks like the null-ls command resolver does correctly find local binaries when they're available, but spawning then fails because the path lacks the .cmd suffix. If that's the case, does this work?

diff --git a/lua/null-ls/helpers/command_resolver.lua b/lua/null-ls/helpers/command_resolver.lua
index e1d1ab2..89d85d7 100644
--- a/lua/null-ls/helpers/command_resolver.lua
+++ b/lua/null-ls/helpers/command_resolver.lua
@@ -45,11 +45,10 @@ end
 M.from_node_modules = function()
     local node_modules_resolver = M.generic(u.path.join("node_modules", ".bin"))
     return function(params)
-        if u.path.is_windows then
+        local resolved_executable = node_modules_resolver(params)
+        if resolved_executable and u.path.is_windows then
             params.command = params.command .. ".cmd"
         end
-
-        local resolved_executable = node_modules_resolver(params)
         return resolved_executable or params.command
     end
 end

This would avoid the mutation issue but I'm not sure it'll work.

[YangShaoyue]

I think what causes Situation 2 in that issue is Neovim/Vim exepath function's behavior, because null-ls find local executable file path correctly, revealed in NullLsLog:

If I understand the original issue correctly: it looks like the null-ls command resolver does correctly find local binaries when they're available, but spawning then fails because the path lacks the .cmd suffix. If that's the case, does this work?

diff --git a/lua/null-ls/helpers/command_resolver.lua b/lua/null-ls/helpers/command_resolver.lua
index e1d1ab2..89d85d7 100644
--- a/lua/null-ls/helpers/command_resolver.lua
+++ b/lua/null-ls/helpers/command_resolver.lua
@@ -45,11 +45,10 @@ end
 M.from_node_modules = function()
     local node_modules_resolver = M.generic(u.path.join("node_modules", ".bin"))
     return function(params)
-        if u.path.is_windows then
+        local resolved_executable = node_modules_resolver(params)
+        if resolved_executable and u.path.is_windows then
             params.command = params.command .. ".cmd"
         end
-
-        local resolved_executable = node_modules_resolver(params)
         return resolved_executable or params.command
     end
 end

This would avoid the mutation issue but I'm not sure it'll work.

But those changes don't work any more, maybe because resolved_executable is not changed from beginning to end?

[jose-elias-alvarez]

I'm still having a hard time understanding the behavior of exepath and related functions here (the Lua side seems pretty clear). If this isn't urgent, I'd like to try setting up Node on Windows and trying this for myself.