chore: add type annotations and resolve LSP warnings #2555

Akmadan23 · 2023-11-24T11:05:40Z

Initially discussed in #2455
WIP: There are still some decisions to be made here

Akmadan23

Then there two general discussion points:

Style inconsistency: the majority of docs comments is of type ---@ but there are still some --- @. I don't know if one should be enforced instead of the other (---@ should be the official one), unfortunately stylua hasn't an option to check it.
Setup functions: this is almost irrelevant, I did not add type annotations to the various M.setup functions since they are all the same and are intuitive, but I would still add them just for completeness

Akmadan23 · 2023-11-24T11:09:46Z

lua/nvim-tree.lua

+---@class FileNode: BaseNode
+---@field extension string
+
+---@alias Node ParentNode|DirNode|FileNode


This is the Node type I managed to come up with, let me know if it's ok and most importantly where it should go. I put it here just to put it temporarily somewhere.

where it should go.

Where is indeed difficult. We could just create nvim-tree/node.lua just to hold the definitions.

When we eventually get to creating proper node classes they can live in there.

This is the Node type I managed to come up with, let me know if it's ok

It's great! It contains all that we know about.

LLS can tell us if we're using an unknown field and we can add as we discover them.

Akmadan23 · 2023-11-24T11:19:28Z

lua/nvim-tree/actions/fs/remove-file.lua

 local function remove_dir(cwd)
  local handle = vim.loop.fs_scandir(cwd)
  if type(handle) == "string" then
-    return notify.error(handle)
+    notify.error(handle)
+    return


In some cases I added an explicit nil return instead of returning the call of a function that returns nil.

Akmadan23 · 2023-11-24T11:21:17Z

lua/nvim-tree/actions/root/dir-up.lua

  else
    local newdir = vim.fn.fnamemodify(utils.path_remove_trailing(core.get_cwd()), ":h")
    require("nvim-tree.actions.root.change-dir").fn(newdir)
-    return require("nvim-tree.actions.finders.find-file").fn(node.absolute_path)
+    require("nvim-tree.actions.finders.find-file").fn(node.absolute_path)


Removed pointless and misleading returns as well

Akmadan23 · 2023-11-24T11:23:37Z

lua/nvim-tree/git/runner.lua

 function Runner:_run_git_job(callback)
  local handle, pid
  local stdout = vim.loop.new_pipe(false)
  local stderr = vim.loop.new_pipe(false)
  local timer = vim.loop.new_timer()

+  if stdout == nil or stderr == nil or timer == nil then
+    return
+  end


Added this little check to silence the "Need nil check" warnings.

This is where we really gain the value.

alex-courtis · 2023-11-25T04:20:34Z

Style inconsistency: the majority of docs comments is of type ---@ but there are still some --- @. I don't know if one should be enforced instead of the other (---@ should be the official one), unfortunately stylua hasn't an option to check it.

---@ does seem the convention.

It doesn't look like the LLS formatter has it either: https://github.com/CppCXY/EmmyLuaCodeStyle/blob/master/docs/format_config_EN.md

We could run find lua -type f -exec sed -i 's/^--- @/---@/g' {} \; every now and then if people aren't following it.

Alternatively, add grep -r "^--- @" lua to the Makefile.

alex-courtis · 2023-11-25T04:30:47Z

Setup functions: this is almost irrelevant, I did not add type annotations to the various M.setup functions since they are all the same and are intuitive, but I would still add them just for completeness

Agreed, minimal value. I'm happy enough to do nothing.

In the future we could do an Options class however that would be a lot of maintenance...

alex-courtis

This is incredible. This will pay off immediately and continually.

alex-courtis · 2023-11-25T04:09:31Z

lua/nvim-tree.lua

+---@class ParentNode
+---@field name string
+
+---@class BaseNode


This is fantastic... finding all these bits every time always slows me down.

alex-courtis · 2023-11-25T04:22:13Z

lua/nvim-tree/git/runner.lua

 function Runner:_run_git_job(callback)
  local handle, pid
  local stdout = vim.loop.new_pipe(false)
  local stderr = vim.loop.new_pipe(false)
  local timer = vim.loop.new_timer()

+  if stdout == nil or stderr == nil or timer == nil then
+    return
+  end


This is where we really gain the value.

alex-courtis · 2023-11-25T04:32:56Z

lua/nvim-tree.lua

+---@class FileNode: BaseNode
+---@field extension string
+
+---@alias Node ParentNode|DirNode|FileNode


where it should go.

Where is indeed difficult. We could just create nvim-tree/node.lua just to hold the definitions.

When we eventually get to creating proper node classes they can live in there.

alex-courtis · 2023-11-25T04:39:19Z

lua/nvim-tree.lua

+---@class FileNode: BaseNode
+---@field extension string
+
+---@alias Node ParentNode|DirNode|FileNode


This is the Node type I managed to come up with, let me know if it's ok

It's great! It contains all that we know about.

LLS can tell us if we're using an unknown field and we can add as we discover them.

alex-courtis · 2023-11-25T04:41:29Z

What do you think about a SymlinkDirNode and SymlinkFileNode? lua does support multiple inheritence...

Akmadan23 · 2023-11-25T20:37:03Z

What do you think about a SymlinkDirNode and SymlinkFileNode?

Right, I completely forgot about symlinks.

Alternatively, add grep -r "^--- @" lua to the Makefile.

Or better yet add it to the style check so that it's done automatically.

We could just create nvim-tree/node.lua just to hold the definitions.

Yeah, I'm not sure if LLS cares about files that are not imported tho...
Otherwise we can put it in the lib module which is the most generic I guess.

alex-courtis · 2023-11-25T23:11:32Z

Yeah, I'm not sure if LLS cares about files that are not imported tho...
Otherwise we can put it in the lib module which is the most generic I guess.

It looks like they've added the @meta annotation to handle this case.

Or better yet add it to the style check so that it's done automatically.

Yes please! If you can find a simple way that would be fantastic.

Right, I completely forgot about symlinks.

Only if it's a small amount of effort. We can iterate on this.

alex-courtis · 2023-11-26T00:23:34Z

I had a play with #2546 against this branch... down to 16 warnings! Nice work.

uvlib.patch.gz adds libuv so that we can reference uv.uv_fs_t and friends, which takes us to 11.

Akmadan23 · 2023-11-26T10:29:24Z

It looks like they've added the @meta annotation to handle this case.

Nice, I didn't know that.

If you can find a simple way that would be fantastic.

I'm working on it, seems to be pretty straight forward.

uvlib.patch.gz adds libuv so that we can reference uv.uv_fs_t and friends, which takes us to 11.

Feel free to push to this branch if you need to.

hinell · 2023-11-26T12:03:29Z

Nice. We actually need to document basic strcture: Node.

It seems like there is no class for that.

alex-courtis · 2023-11-26T21:34:39Z

Feel free to push to this branch if you need to.

Done.

alex-courtis · 2023-11-26T22:57:24Z

I've taken the liberty of adding scripts/luals-check and fixing some nits.

I'd like to free you up to continue the actual work.

Some recipes:
scripts/luals-check | grep "file:"
scripts/luals-check | jq --color-output
scripts/luals-check | yq --colors -p json

alex-courtis · 2023-11-26T22:58:52Z

Nice. We actually need to document basic strcture: Node.

It seems like there is no class for that.

Please Seek First To Understand: https://github.com/nvim-tree/nvim-tree.lua/blob/type-annotations/lua/nvim-tree/node.lua

Akmadan23 · 2023-11-26T23:46:35Z

I've taken the liberty of adding scripts/luals-check and fixing some nits.

Will it be added in this PR? I'd rather add it with a separate PR... By the way I haven't commented on #2546 yet because I tested it on this branch and it runs as expected, but on master it doesn't find any problem, where that is clearly not the case. I also experienced a similar behavior when I was adding type annotations in some files, and only then LLS displayed warnings about something completely unrelated to the annotations just added...

alex-courtis · 2023-11-27T01:00:49Z

I've taken the liberty of adding scripts/luals-check and fixing some nits.

Will it be added in this PR? I'd rather add it with a separate PR... By the way I haven't commented on #2546 yet because I tested it on this branch and it runs as expected, but on master it doesn't find any problem, where that is clearly not the case. I also experienced a similar behavior when I was adding type annotations in some files, and only then LLS displayed warnings about something completely unrelated to the annotations just added...

Pulling it out now.

Are you cool with the little fixes?

This reverts commit c70229c.

alex-courtis · 2023-11-27T23:25:26Z

lua/nvim-tree/marks/init.lua

@@ -4,21 +4,24 @@ local NvimTreeMarks = {}

 local M = {}

---@param node Node
+---@class MinimalNode


Wow... I like this one... I'm seeing many possibilities.

…ecks

Akmadan23

According to my tests we should be down to just 4 warnings, 3 of which start from a non-issue in my opinion. More details in the comments

Akmadan23 · 2023-11-28T10:39:50Z

lua/nvim-tree/actions/fs/create-file.lua

 local function create_and_notify(file)
  events._dispatch_will_create_file(file)
  local ok, fd = pcall(vim.loop.fs_open, file, "w", 420)
  if not ok then
    notify.error("Couldn't create file " .. notify.render_path(file))
    return
  end
-  vim.loop.fs_close(fd)
+  vim.loop.fs_close(tonumber(fd) or error "This will never happen")


Maybe it's not the best solution, but it works...

This is difficult. We're struggling with similar issues at my workplace right now...

Can we revert this one? It's correct and it passes checks.

Dealing with the vim. luals issues is a job for another day, if we even want to do it.

Akmadan23 · 2023-11-28T10:41:41Z

lua/nvim-tree/core.lua

@@ -31,10 +34,12 @@ function M.reset_explorer()
  TreeExplorer = nil
 end

+---@return string


This should be string|nil, but many internal functions rely on always receiving a string, so that's something I need to fix.

Akmadan23 · 2023-11-28T10:43:21Z

lua/nvim-tree/node.lua

+---@field link_to string
+
+---@alias SymlinkNode SymlinkDirNode|SymlinkFileNode
+---@alias Node ParentNode|DirNode|FileNode|SymlinkNode|Explorer


Adding Explorer here seems to be the best solution, otherwise we would have many Node/Explorer inconsistencies.

Akmadan23 · 2023-11-28T10:47:33Z

lua/nvim-tree/lib.lua

@@ -81,6 +85,8 @@ function M.get_last_group_node(node)
  return next_node


I really don't get why it complains about a missing nil check here... And consequently a possible nil return

Akmadan23 · 2023-11-28T10:49:03Z

lua/nvim-tree/explorer/init.lua

 function Explorer.new(cwd)
-  cwd = vim.loop.fs_realpath(cwd or vim.loop.cwd())
+  cwd = vim.loop.fs_realpath(cwd or vim.fn.getcwd())


This always returns a string instead of string|nil

Akmadan23 · 2023-11-28T10:57:13Z

Apologies again for rudely pushing to your branch and interrupting your work!

Oh, don't worry, I was just a bit confused by the unexpected output on my side.

alex-courtis · 2023-12-02T01:38:31Z

The issues with vim and uv are getting in the way of the real goal here. I apologise for steering us in that direction.

Could be be practical and remove the libraries for now?
We can add temporary definitions for now and deal with vim, uv etc. later.

node.lua

-- TODO add "${3rd}/luv/library" to "workspace.library"
---@class uv.uv_req_t: table
---@class uv.uv_fs_t: uv.uv_req_t

runner.lua

-- TODO add "${3rd}/luv/library" to "workspace.library"
---@class uv.uv_handle_t: table
---@class uv.uv_stream_t: uv.uv_handle_t
---@class uv.uv_pipe_t: uv.uv_stream_t

We could even back out the changes to get around the spurious warnings from vim.cmd etc.

Akmadan23 · 2023-12-02T19:12:41Z

Could be be practical and remove the libraries for now?
We can add temporary definitions for now and deal with vim, uv etc. later.

Sure, no problem.

Akmadan23 · 2023-12-04T21:57:37Z

Now we should have 0 warnings related to our internal type annotations.

We could even back out the changes to get around the spurious warnings from vim.cmd etc.

Yes, makes sense to do this since those changes aren't directly related to the annotations added here. I can stash them and open a new PR with other corrections of that kind...

Akmadan23 · 2023-12-04T22:14:25Z

lua/nvim-tree/lib.lua

  end
-  return next_node
+
+  ---@diagnostic disable-next-line: return-type-mismatch -- it can't be nil


I can't see the possibility of a nil return given that the node parameter is not nil, or am I wrong?

Looks like luals is getting confused.

We could refactor but this is fine.

We can revisit suppressions once we have types annotations present and correct.

Akmadan23 · 2023-12-04T22:33:03Z

As for #2575, I'd rather update the various --- @ here so that we can merge it later without collateral changes.

alex-courtis · 2023-12-05T00:16:10Z

All the warnings are resolved!

What's left to do?

alex-courtis · 2023-12-05T00:24:23Z

lua/nvim-tree/view.lua

-  pcall(vim.cmd, "buffer " .. M.get_bufnr())
+  pcall(function()
+    vim.cmd("buffer " .. M.get_bufnr())
+  end)


Could we revert this one as well? I'm the idiot who put it in but it's no longer necessary.

This function needs a rewrite anyway - we need to listen for failures and handle them.

Akmadan23 · 2023-12-05T10:30:04Z

What's left to do?

Doc comments style
Revert unrelated changes

Everything should be good to go now.

alex-courtis

Lets go!

alex-courtis · 2023-12-09T00:23:30Z

lua/nvim-tree/node.lua

+---@class SymlinkFileNode: FileNode
+---@field link_to string
+
+---@alias SymlinkNode SymlinkDirNode|SymlinkFileNode


I do like this inheritence approach; it removes the need for annoying/unnecessary casts.

alex-courtis · 2023-12-09T00:24:44Z

lua/nvim-tree/marks/bulk-delete.lua

@@ -9,7 +9,7 @@ local M = {
 }

 --- Delete nodes; each removal will be optionally notified
--- @param nodes table
+---@param nodes Node[]


alex-courtis · 2023-12-09T00:26:14Z

lua/nvim-tree/log.lua

--- @vararg any arguments for string.format
+---@param typ string as per log.types config
+---@param fmt string for string.format
+---@param ... any arguments for string.format


alex-courtis · 2023-12-09T00:37:37Z

Just incredible work @Akmadan23
The devspeed improvements will be immediately gained and the safety is a massive win.

chore: add type annotations to (almost) all functions

fabea33

Akmadan23 commented Nov 24, 2023

View reviewed changes

Akmadan23 and others added 2 commits November 24, 2023 12:45

stylua

0b31f06

Merge branch 'master' into type-annotations

adeec1a

alex-courtis reviewed Nov 25, 2023

View reviewed changes

alex-courtis mentioned this pull request Nov 25, 2023

ci: lua language server and Makefile #2546

Merged

Add classes for symlink nodes

76c88de

Replace deprecated @vararg

2a6aa31

Akmadan23 added 2 commits November 26, 2023 11:32

Move node classes to node module

3b66519

Fix Symlink* classes

3af30ee

add vim and libuv runtime for luals, qualify libuv types

8393f7c

alex-courtis added 5 commits November 27, 2023 08:56

add scripts/luals-check, not quite ready for CI

c70229c

additional nil checks for git/init.lua and git/runner.lua

b4570bd

additional nil checks for nvim-tree.lua

15f05f8

wrap vim.cmd-as-a-function calls inside functions

6988062

vim.tbl_filter predicate returns booleans

87d2d4f

Revert "add scripts/luals-check, not quite ready for CI"

e201c5b

This reverts commit c70229c.

alex-courtis reviewed Nov 27, 2023

View reviewed changes

Akmadan23 and others added 4 commits November 28, 2023 10:50

Fix various LSP warnings

ee60886

Merge branch 'master' into type-annotations

dcd8cb8

stylua

b4ea048

Fix Explorer class, update related annotations and add necessary ch…

e0cacc9

…ecks

Akmadan23 commented Nov 28, 2023

View reviewed changes

Akmadan23 changed the title ~~chore: add type annotations to (almost) all functions~~ chore: add type annotations and resolve LSP warnings Nov 28, 2023

Add missing annotations to live-filter

d16f289

Akmadan23 added 2 commits December 4, 2023 21:37

Add temporary aliases for uv.* types

a937cda

Resolve remaining LSP warnings

ec16b1a

Akmadan23 commented Dec 4, 2023

View reviewed changes

Akmadan23 mentioned this pull request Dec 4, 2023

ci: add style check for doc comments #2575

Merged

alex-courtis reviewed Dec 5, 2023

View reviewed changes

Akmadan23 and others added 4 commits December 5, 2023 10:58

Merge branch 'master' into type-annotations

aafccdc

Revert changes not related to internal types

d14eb6f

Minor adjustments

edc0d63

Update doc comments style

9ff04a9

Akmadan23 marked this pull request as ready for review December 5, 2023 10:30

Minor adjustments (pt. 2)

7fbc416

alex-courtis approved these changes Dec 9, 2023

View reviewed changes

alex-courtis merged commit 13f967f into master Dec 9, 2023

alex-courtis deleted the type-annotations branch December 9, 2023 00:34

		@@ -81,6 +85,8 @@ function M.get_last_group_node(node)
		return next_node

chore: add type annotations and resolve LSP warnings #2555

chore: add type annotations and resolve LSP warnings #2555

Conversation

Akmadan23 commented Nov 24, 2023 • edited Loading

Akmadan23 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-courtis commented Nov 25, 2023 • edited Loading

alex-courtis commented Nov 25, 2023

alex-courtis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-courtis commented Nov 25, 2023

Akmadan23 commented Nov 25, 2023

alex-courtis commented Nov 25, 2023 • edited Loading

alex-courtis commented Nov 26, 2023 • edited Loading

Akmadan23 commented Nov 26, 2023

hinell commented Nov 26, 2023

alex-courtis commented Nov 26, 2023

alex-courtis commented Nov 26, 2023

alex-courtis commented Nov 26, 2023

Akmadan23 commented Nov 26, 2023

alex-courtis commented Nov 27, 2023

Choose a reason for hiding this comment

Akmadan23 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-courtis Dec 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Akmadan23 commented Nov 28, 2023

alex-courtis commented Dec 2, 2023 • edited Loading

Akmadan23 commented Dec 2, 2023

Akmadan23 commented Dec 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Akmadan23 commented Dec 4, 2023

alex-courtis commented Dec 5, 2023

Choose a reason for hiding this comment

Akmadan23 commented Dec 5, 2023

alex-courtis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-courtis commented Dec 9, 2023

Akmadan23 commented Nov 24, 2023 •

edited

Loading

Akmadan23 left a comment •

edited

Loading

alex-courtis commented Nov 25, 2023 •

edited

Loading

alex-courtis commented Nov 25, 2023 •

edited

Loading

alex-courtis commented Nov 26, 2023 •

edited

Loading

alex-courtis Dec 2, 2023 •

edited

Loading

alex-courtis commented Dec 2, 2023 •

edited

Loading