fix: autostart=false: attach when editing new, nonexistent file (#2712 attempt 2)

[Diomendius]

This is a fixed version of #2712, which was merged and reverted.

Fixes #2711

[glepnir]

I don't think schedule is correct here. r pyright started in single file mode which means we send empty root to the server instead of pwd. Register a FileType event to check if it matches pwd. does this works ?

diff --git a/lua/lspconfig/configs.lua b/lua/lspconfig/configs.lua
index 391a329..8f166ea 100644
--- a/lua/lspconfig/configs.lua
+++ b/lua/lspconfig/configs.lua
@@ -177,6 +177,20 @@ function configs.__newindex(t, config_name, config_def)
           end
           local pseudo_root = #bufname == 0 and pwd or util.path.dirname(util.path.sanitize(bufname))
           M.manager:add(pseudo_root, true, bufnr)
+          api.nvim_create_autocmd('FileType', {
+            pattern = config.filetypes or '*',
+            callback = function(arg)
+              local parent = vim.fs.dirname(api.nvim_buf_get_name(arg.buf))
+              if not (parent == pseudo_root) then
+                return
+              end
+              if #M.manager:clients() == 0 then
+                return true
+              end
+              M.manager:try_add_wrapper(arg.buf, root_dir)
+            end,
+            group = lsp_group,
+          })
         end
       end)
     end

[justinmk]

shouldn't try_add / try_add_wrapper check nvim_buf_is_valid? they are "try" patterns, after all.

[Diomendius]

I considered this myself, actually, but didn't have a strong opinion either way.

• Both functions would need a check, so code size/complexity is about the same either way.
• Moving the checks might prevent another bug like this in the future.
• try_add_wrapper() would do the check twice, but that's not really a problem.

If your intuition is to move the checks, I might as well.

[justinmk]

which means we send empty root to the server instead of pwd.

how is that related to the use of schedule? in this PR, schedule is sending the root_dir that would have been passed even without schedule.

[glepnir]

No new BufNewFile is needed and no vim.schedule is needed. Single file mode does not send the root dir to the server. What is missing here is a FileType event registration for the current pseudo directory. gif is working with diff.

[justinmk]

Avoiding the extra BufNewFile handlers is an argument in favor of @glepnir's solution (or at least makes them ~equivalent). Also @glepnir's solution seems clearer because it's contained in the single_file_support condition.

In any case, a "try" function should check whether the buf is valid, IMO.

[Diomendius]

Personally, I wouldn't expect the server to auto-attach to newly-opened files in single file mode, though this isn't a bad thing.

This doesn't fix the problem when not running in single file mode, though.

The only reason vim.schedule() is needed is when triggering on BufNewFile, because it can't be relied on to fire after FileType, and the only reason BufNewFile is needed is when not triggering on FileType, as BufReadPost doesn't trigger for new files. Handling everything in a FileType autocommand using * as the fallback pattern works, unless a server should be able to function on a buffer with an empty filetype, though I'm not sure if this is a valid use case.

[Diomendius]

Maybe it wasn't obvious enough in #2711, but the init script to reproduce that issue forces the workspace root to be the working directory when the server is started:

root_dir = function() return vim.fn.getcwd() end,

[justinmk]

to unblock this, i suggest:

• move the "buf valid" check to the try_x functions
• we can try the schedule() approach again
• @glepnir can send an improvement later, if possible

[glepnir]

root_dir = function() return vim.fn.getcwd() end,

We never recommend setting the root directory like this. https://github.com/neovim/nvim-lspconfig/blob/master/CONTRIBUTING.md#adding-a-server-to-lspconfig I'm guessing is because the eval functions like getcwd cwd etc are not api-fast.

Another point. Do we need BufReadPost at all? First it will only work if the file exists. Registering this event will match the pattern for any file opened. Second lsp has specific file type support, so shouldn't it only need FileType? And FileType will trigger even if the buffer does not exist. :e non_exist | set ft= xxx This should also let lsp connect. this should be equivalent to the previous behavior. And more sensible

diff --git a/lua/lspconfig/configs.lua b/lua/lspconfig/configs.lua
index 391a329..81f46be 100644
--- a/lua/lspconfig/configs.lua
+++ b/lua/lspconfig/configs.lua
@@ -142,9 +142,12 @@ function configs.__newindex(t, config_name, config_def)
         end

         if root_dir then
-          api.nvim_create_autocmd('BufReadPost', {
-            pattern = fn.fnameescape(root_dir) .. '/*',
+          api.nvim_create_autocmd('FileType', {
+            pattern = config.filetypes,
             callback = function(arg)
+              if not (vim.fs.dirname(api.nvim_buf_get_name(arg.buf)) == root_dir) then
+                return
+              end
               if #M.manager:clients() == 0 then
                 return true
               end

[Diomendius]

The method of setting root_dir is irrelevant. I could have had the repro script create an empty .git directory for the default discovery to find, and the result would be the same. I didn't and still don't expect opening a buffer, whether backed by an existing file or not, to automatically attach a server running in single file mode. I have no problem with this functionality being implemented, but it is unrelated to the issue I originally reported.

Currently, a server is automatically started/attached in any of these cases, when autostart is enabled:

• Opening a buffer whose detected filetype matches a configured server, regardless of whether it is backed by an existing file
• Opening a buffer backed by an existing file, regardless of whether filetypes is set

A server is NOT started/attached, when autostart is enabled, when:

• Opening a buffer NOT backed by an existing file, when filetypes is falsy.

Regardless of autostart, a server is automatically attached when:

• Opening a buffer whose path is descended from the root_dir of a running server, if a file exists at that path

Unless autostart is enabled and a server is configured with the buffer's filetype, a server is NOT automatically attached when:

• Opening a buffer whose path is descended from the root_dir of a running server, if a file does NOT exist at that path

In any of the above cases, if a server would be started/attached and filetypes is set, but the filetype of the buffer is not in filetypes, the server is not started/attached, but this is expected behaviour.

I can see these solutions:

• Additionally trigger autocommands on BufNewFile when they currently trigger on BufReadPost (the solution implemented in this PR)
  • This requires vim.schedule() or a similar workaround ONLY because BufNewFile sometimes triggers before FileType. If the autocommand is set up during execution of the user's init.vim/init.lua, it will trigger before FileType. If it is set up later, it will trigger after.
  • Instead of using vim.schedule() in the autocommand callback, the creation of the autocommand could be delayed (probably also via vim.schedule()). I don't know how reliable this would be, and it may cause problems when opening a buffer on the command line, as the buffer would probably be opened before the callback to schedule() runs.
• Trigger both the autostart autocommand and the root-based autocommand only on FileType, using a * pattern if filetypes is falsy
  • This would not work for any buffers with no detected filetype at all. This isn't typically a problem, but would interfere with language servers that operate on plain text files. A spell-checking language server might be one practical example that would be affected.

Your alternate root detection logic would work, but not for files with no detected filetype. To handle nesting, the condition needs to be changed to something like if (api.nvim_buf_get_name(arg.buf):sub(1, #root_dir) ~= root_dir), though.

[glepnir]

we also have vim.fs.parents(vim.api.nvim_buf_get_name(0)) for nested folder . vim.schedule puts it into the next event loop for processing. If there are callbacks that need lsp later in the current loop, none of them will work. The problem is not the scheduling timing. Instead, BufReadPost will only be triggered by existing files. For lsp without a specific file type, you can use the wildcard config.filetypes or '*'. FileType will be triggered even if the file does not exist. So just using FileType is enough. BufReadPost This is the behavior designed by the previous maintainer. I am confused here 🤔 any situation that FileType will not work must BufReadPost?

[Diomendius]

FileType triggers when filetype is set, including when filetype detection sets it. FileType does not trigger when a buffer is opened if filetype is not set, such as for text files and unrecognized extensions. If filetype detection does set filetype this will happen regardless of whether the buffer is backed by a file, unless filetype detection would depend on the contents of the file, like with #! detection for shell scripts with no extension.

BufReadPost triggers after reading the contents of an existing file into a buffer. It does not trigger when no file exists and always triggers after filetype detection, if that happens.

BufNewFile triggers when a buffer is created, but only when not reading from an existing file, and so is mutually exclusive with BufReadPost. Whether this triggers before or after filetype detection depends on how early in Nvim init the autocommand was created, see neovim/neovim#7367.

If there are callbacks that need lsp later in the current loop, none of them will work.

Is this possible? If a server is already attached, this doesn't matter, and if a server isn't attached, what would try to interact with it that can be triggered without the server already attached?

[Diomendius]

These are the only servers under lspconfig/configs without filetypes, so they may no longer attach in some cases if only FileType is used to auto-attach:

• contextive
• custom_elements_ls
• typos_lsp

[glepnir]

if no filetypes can't we use wildcards like * in FIleType and callback still check the parent is before cwd or pseudo ?

[Diomendius]

FileType does not trigger at all (for any pattern including *) when running, for instance, :e foo.txt.

Both BufReadPost and BufNewFile would be needed to handle this case. If there is some logic besides config.filetypes that decides whether a server should be attached that depends on the buffer's filetype, we need schedule() or an equivalent workaround to wait for filetype detection before attaching the server.

If, however, we can assume that the buffer's filetype doesn't matter at all unless the server we're trying to attach has config.filetypes set, then schedule() is unnecessary (as its only purpose is to ensure filetype is up to date) and we can avoid this whole mess entirely.

[justinmk]

FileType does not trigger at all (for any pattern including *) when running, for instance, :e foo.txt.

it triggers in that case because filetype=text. though it doesn't trigger for :e foo, is that important?

[Diomendius]

Okay, that was a terrible example. I should have checked that first. 😆

*.log files don't set filetype, as another example, as do extensionless files as you mentioned. It's not that uncommon to see text files without an extension.

I don't know how important it is to preserve autostart/autoattach for these files, as I don't need this myself, but it should currently work (as long as the file already exists), so it would be nice not to go backwards here.

I'm hoping filetype isn't actually needed for servers with no configured filetypes. schedule() is unnecessary for FileType, which works for every other case.

[justinmk]

I don't know how important it is to preserve autostart/autoattach for these files

It might actually be a bug if lsp is activated for unknown buffers.

servers with no configured filetypes

I don't understand why lsp configs would not have any filetype associations. Do people want lsp activated for every random file regardless of its type?

If it simplifies the code, maybe it's not worth supporting that.

[justinmk]

My last comment was question(s) for @glepnir . Main question is, can we try this out, or is there an obvious, serious risk that I'm missing?