[feature] Support pre-commit hook

[Freed-Wu]

Can it support pre-commit like https://github.com/Koihik/LuaFormatter? TIA!

[carsakiller]

You may be able to write your own precommit script that utilizes --check to check for errors. I am not sure of any built-in way to support this.

[Freed-Wu]

$ lua-language-server --check XXX.lua
Diagnosis complete, 1 problems found, see /home/wzy/.cache/lua-language-server/log/check.json
$ echo $?
0

Can it return 1 when problems found? It can be used by pre-commit.

[sumneko]

AFAIK, someone has created a Github Action for this automated check, but I forgot the name of the project.

[sumneko]

https://github.com/mrcjkb/lua-typecheck-action

[Freed-Wu]

Can it return 1 when problems found

Hard, because script/cli/check.lua use multi process, main process cannot know if sub process find error.

[tomlau10]

Can it return 1 when problems found
Hard, because script/cli/check.lua use multi process, main process cannot know if sub process find error.

I guess main process can know the exit code of sub process. So if sub process uses exit 1 when finding >= 1 problems, the main process can then also exit 1 at the end 🤔 @Freed-Wu

For example:

• cli/check_worker.lua will return true if check passed, otherwise false if there are problems
• then in cli/init.lua, we can check the return value

if _G['CHECK_WORKER'] then
    local checkPassed = require 'cli.check_worker'
    os.exit(checkPassed and 0 or 1, true)
end

• finally in cli/check.lua, I believe that the process:wait() will return the exit code, then we can catch it

local checkPassed = true
for _, process in ipairs(procs) do
    checkPassed = process:wait() == 0 and checkPassed
end
...
return checkPassed

• again in cli/init.lua, we can check the return value

if _G['CHECK'] then
    local checkPassed = require 'cli.check'
    os.exit(checkPassed and 0 or 1, true)
end

[fidgetingbits]

you can see a lua-ls script in these nix pre-commit hooks https://github.com/cachix/git-hooks.nix/blob/master/modules/hooks.nix that handles checking for errors by using a a wrapper and checking check.json file contents after the run.

I'm writing a similar shell script wrapper and ran into some other problems. for example --check doesn't seem to support checking a single file(?), only a directory. but that is very slow if you only want to check changed files. if you filter the pre-commit rule for only .lua files, it will pass each lua file as an argument to the script, and also possibly make multiple invocations, so you have to cope with that to avoid constantly rerunning --check across the whole directory multiple times.

Definitely seems it would be useful if there was a --check-files option or something

[tomlau10]

using a a wrapper and checking check.json file contents after the run.

Isn't that check.json will always be created no matter if there are problems or not? When no problems found, the file content will be an empty json array [].

            text = ''
              set -e
              export logpath="$(mktemp -d)"
              lua-language-server --check $(realpath .) \
                --checklevel="${hooks.lua-ls.settings.checklevel}" \
                --configpath="${luarc-dir}/.luarc.json" \
                --logpath="$logpath"
              if [[ -f $logpath/check.json ]]; then
                echo "+++++++++++++++ lua-language-server diagnostics +++++++++++++++"
                cat $logpath/check.json
                exit 1
              fi
            '';

In my github action lint workflow, I just use jq to check the length of that json array 👀

          # exit error if any
          if [ $(jq -r 'length' ${LUALS_RESULT_FILE}) -gt 0 ]; then
            exit 1
          fi

---

--check doesn't seem to support checking a single file(?)

I also thought about this when I integrate LuaLS into my project's gha lint workflow. I have been using luacheck before and I cached the result when the files are identical to speed up the process. However there are no similar things for LuaLS. But on a second thought, I think it will just not work for LuaLS to check a single file, because many diagnosis are cross files based 🤔.

Let me give an example. In a project you have a meta.lua which defines some API, and your other files depends on it. One day you updated 1 of the APIs in this meta.lua, say by changing the type of a function param. But then you didn't change any other files. If LuaLS only checks for files that are modified, it will only check this meta.lua but not others => this will create false negative check result as all other files are actually using an outdated API 💥

This kind of problem doesn't exist in luacheck because all checking of luacheck are file based. 😕

---

but that is very slow if you only want to check changed files

Btw did you try the --num_threads=* flag? LuaLS recently supports multi-threaded check in #2638 and it can speed up the whole process up to around 60% time.

I used this in my gha workflow, but unfortunately I can only use --num_threads=2 as the free gha ubuntu vm only has 2 cores... At least it speeds up the overall time from 40s => 30s for me 😂

[Freed-Wu]

I guess main process can know the exit code of sub process.

Sorry to find it for late. Can you provide a PR for your prove of concept?
In my test, add os.exit(1) after

    if count == 0 then
        print(lang.script('CLI_CHECK_SUCCESS'))
    else
        print(lang.script('CLI_CHECK_RESULTS', count, outpath))
    end

cannot work.

[Freed-Wu]

lua-language-server --check /the/path/of/file.lua cannot work. lua-language-server --check /the/path/of/a/directory can work. we still need some change.

[tomlau10]

Sorry to find it for late. Can you provide a PR for your prove of concept?
In my test, add os.exit(1) after

Simply adding os.exit in cli/check_worker.lua will not work, because this file is executed under the context of subprocess.
You have to catch this subprocess exit code in cli/check.lua by checking the return value of process:wait(), as explained in my previous comment: #2343 (comment)

I see that you have opened #2998 which largely follows my suggestion above. Glad that it works out 😄

---

lua-language-server --check /the/path/of/file.lua cannot work. lua-language-server --check /the/path/of/a/directory can work. we still need some change.

AFAIK, since some version of LuaLS, --check now expect passing directory instead of a file. The example in official wiki also states that a directory is needed: https://luals.github.io/wiki/usage/#--check

--check
Type: string
Perform a "diagnosis report" where the results of the diagnosis are written to the logpath
Example: --check=C:\Users\Me\path\to\workspace

Some reported that before v3.8 (?) it supports passing just a file: #2989, #2749
I can't tell because I first started to use LuaLS at v3.9.3.

---

In my own opinion, since the diagnostics performed by LuaLS is a workspace-wise (some meta definition maybe defined in another file in the workspace), it makes little sense to support passing a single file? 🤔
It needs the workspace root in order to load the .luarc.json, and then preload the whole workspace to know all the types.

There is no single file mode diagnostics (?), as opposed to luacheck where all checking are performed in single file based.

PS: I am no maintainer, so my understanding may not be 100% correct 🙈

[Freed-Wu]

I try to support check single file:

diff --git a/script/cli/check_worker.lua b/script/cli/check_worker.lua
index a2e0bff..eb1a491 100644
--- a/script/cli/check_worker.lua
+++ b/script/cli/check_worker.lua
@@ -27,8 +27,14 @@ function export.runCLI()
         print(lang.script('CLI_CHECK_ERROR_TYPE', type(CHECK_WORKER)))
         return
     end
+    local filepath = fs.path(CHECK_WORKER)
+    local is_directory = fs.is_directory(filepath)

-    local rootPath = fs.absolute(fs.path(CHECK_WORKER)):string()
+    local path = filepath
+    if not is_directory then
+        path = fs.path('.')
+    end
+    local rootPath = fs.absolute(path):string()
     local rootUri = furi.encode(rootPath)
     if not rootUri then
         print(lang.script('CLI_CHECK_ERROR_URI', rootPath))
@@ -102,6 +108,9 @@ function export.runCLI()
         config.set(rootUri, 'Lua.diagnostics.neededFileStatus', diagStatus)

         local uris = files.getChildFiles(rootUri)
+        if not is_directory then
+            uris = { furi.encode(fs.absolute(filepath):string()) }
+        end
         local max  = #uris
         table.sort(uris)    -- sort file list to ensure the work distribution order across multiple threads
         for i, uri in ipairs(uris) do

However, it cannot work:

❯ lua-language-server --check .
rootUri: file:///dev/shm/tellenc.nvim/spec/.
------------------------------
Initializing ...
uris: file:///dev/shm/tellenc.nvim/spec/./test_spec.lua
Diagnosis complete, 2 problems found, see /tmp/lua-language-server-1000/instance.lVAZ/log/check.json
❯ lua-language-server --check test_spec.lua
rootUri: file:///dev/shm/tellenc.nvim/spec/.
------------------------------
Initializing ...
uris: file:///dev/shm/tellenc.nvim/spec/test_spec.lua
Diagnosis completed, no problems found

Support pre-commit needs:

• exit with 1 when linter find error (had better print error to console)
• check single file, becuase pre-commit only use linter to check file modified in git index
• can be installed by one package manager, Any thoughts on publishing to luarocks? #623 said it use a fork of lua 5.4 named bee.lua, which provide a builtin module named bee to support filesytem and subprocess. so even it can be installed by luarocks, we still need a bee.lua installed by pre-commit.

[tomlau10]

However, it cannot work:

Just by looking at the output, I guess if the rootUri contains a . at the end, then your child file uris also have to contain this . 😕

• rootUri: file:///dev/shm/tellenc.nvim/spec/.
• uris: file:///dev/shm/tellenc.nvim/spec/./test_spec.lua ✅
• uris: file:///dev/shm/tellenc.nvim/spec/test_spec.lua ❌
  because it doesn't match the rootUri prefix file:///dev/shm/tellenc.nvim/spec/./ (?)

---

I try to support check single file:

To support checking only single file, I have a few questions:

• When you specify --check <file>, how can luals know the workspace root?
  Since luals binary can be run at any path, I don't think you can just assume . as the workspace root?

  • by using lua-language-server --check=. or lua-language-server --check=/dev/shm/tellenc.nvim/spec
    => luals can know the workspace root to be /dev/shm/tellenc.nvim/spec
  • but what if the current directory is /temp and someone execute lua-language-server --check=/dev/shm/tellenc.nvim/spec/test_spec.lua
    => how can luals know the root to be /dev/shm/tellenc.nvim/spec?
    but not /dev/shm/tellenc.nvim or even /dev/shm ?

• IIUC, each run of luals diagnostics requires preloading the whole workspace in order to process all defined meta data types. And typically in a single commit, there will probably be more than 1 files being edited.

  • If a commit modifies 20 files, do your commit hook execute single file check 20 times? 😳
  • but that will waste a lot of processing time, because each time start up of luals has to preload the whole workspace ?

[Freed-Wu]

how can luals know the workspace root?

know it from $PWD, or search .git

but that will waste a lot of processing time

Perhaps lua-language-server --check a b c can save more time than check them standalone.

[tomlau10]

know it from $PWD, or search .git

This may work in your pre commit hook case, but I don't think this will work in general. 😕
That's because the --check also accepts absolute path, and LuaLS (currently) doesn't assume anything related to $PWD.

• when a user run lua-language-server --check dir1/a.lua, certainly you can use $PWD as workspace root
• however when a user run lua-language-server --check /dir1/a.lua, we cannot assume anything
  => the $PWD maybe in /dir2
• moreover luals doesn't assume the workspace to be a git project, so I don't think searching .git is appropriate
  (at least this logic should not be within the server IMO)

In short, just assuming $PWD as workspace root seems will create many undefined behavior, when user supplies absolute paths as argument.

---

Perhaps lua-language-server --check a b c can save more time than check them standalone.

That's a great idea 👍
In order to tackle the undefined behavior related to absolute path mentioned above, I suggest that the --check should supports 2 modes 🤔

directory mode

• --check [directory]
• the path can be relative or absolute
• it must be a directory path
• only 1 argument is allowed, i.e.
  • --check dir1 ✅
  • --check /dir1 ✅
  • --check dir1 dir2 => should be error ❌
  • --check dir1 dir2/file => should be error as well ❌
• workspace root detection follows the current behavior
  i.e. the provided directory will be the workspace root

file(s) mode

• --check [file...]
• the paths can only be relative
• can be multiple arguments, but all of them should be file path, i.e.
  • --check a ✅
  • --check dir1/a dir1/b ✅
  • --check dir1/a dir2 ❌ => because one of them is a directory
• workspace root is always assumed to be $PWD

---

By forcing relative paths arguments in file mode, then it makes sense to assume $PWD as the root 🤔

PS: these are just my opinions, it doesn't represent maintainers' opinions 😄

[tomlau10]

Wait a minute, I still think that single file mode is not suitable for LuaLS.
As I have explained, the diagnostics of LuaLS is workspace-wise
=> i.e. editing 1 file can break another file. 🤦‍♂️

Lets consider the following minimal example:

• meta.lua

---@class Foo

• test.lua

---@type Foo
local foo

A very simple one, where meta.lua defined a Foo class and test.lua used this class

---

Now someone edited the meta.lua to

---@class Bar

and then commit it

• I believe your pre commit hook will then only check the modified file => i.e. the meta.lua (?)
• however meta.lua itself is good
• the problem lies in test.lua, in which now there is no more Foo class ‼️

This is what I want to express, LuaLS diagnostics is workspace-wise.
You just can't simply depends on the check result of edited files 😕 because new problems can occur in non edited files.
This is opposed to luacheck where all diagnostics are performed in single file basis.