Bug: `FzfLua files` with enabled 'mini.icons' sometimes returns not all paths

[echasnovski]

RTFM Checklist

• [X] I have searched exisiting issues / discussions
• [ ] I have read the Wiki including the Advanced section
• [ ] I have read man fzf / I am well versed in shell fzf

Operating system

EndeavourOS Linux x86_64, 6.10.1-arch1-1

Shell

zsh

Neovim version (nvim --version)

0.10.0

Fzf version (fzf --version)

0.54.0 (9e92b6f1)

Output of :lua print(os.getenv('FZF_DEFAULT_OPTS'))

--layout=reverse --inline-info --height 25%

Is the problem reproducible with mini.sh?

• [ ] My issue is reproducible with mini.sh
• [ ] My issue IS NOT reproducible with mini.sh
• [X] I have not tested with mini.sh (not relevant, requires LSP, Windows, etc)

Fzf-lua configuration

-- Full config using 'mini.nvim' (and 'mini.deps'):
-- Clone 'mini.nvim' manually in a way that it gets managed by 'mini.deps'
local path_package = vim.fn.stdpath('data') .. '/site/'
local mini_path = path_package .. 'pack/deps/start/mini.nvim'
if not vim.loop.fs_stat(mini_path) then
  vim.cmd('echo "Installing `mini.nvim`" | redraw')
  local clone_cmd = { 'git', 'clone', '--filter=blob:none', 'https://github.com/echasnovski/mini.nvim', mini_path }
  vim.fn.system(clone_cmd)
  vim.cmd('packadd mini.nvim | helptags ALL')
  vim.cmd('echo "Installed `mini.nvim`" | redraw')
end

require('mini.deps').setup({ path = { package = path_package } })
local add, now, later = MiniDeps.add, MiniDeps.now, MiniDeps.later

now(function() require('mini.icons').setup() end)
now(function()
  add('ibhagwan/fzf-lua')
  require('fzf-lua').setup()
end)

Describe the bug / steps to reproduce

Executing :FzfLua files with 'mini.icons' enabled results in unstable results In some directories. I am currently able to reproduce this only in 'mini.nvim' repo root.

• Create '~/.config/nvim-fzf-lua/init.lua' with the full config posted in previous section.
• Inside '~/.local/share/nvim-fzf-lua/site/pack/deps/start/mini.nvim' execute NVIM_APPNAME=nvim-fzf-lua nvim. Wait for 'mini.nvim' and 'fzf-lua' to install. Restart (just to be safe).
• NVIM_APPNAME=nvim-fzf-lua nvim
• Execute :FzfLua files several times. The total number of found files should be 2214, however it almost always lower than that and sometimes even 0.

Important notes:

• This does not happen on Nightly 0.11.
• The breaking commit seems to be 6edea7e87647, which is aimed to have a better MiniIcons.get() mock regarding vim.filetype.match(). Everything seems to work on d1493cfac8e5, albeit a bit choppier then on latest commit on 0.11.

• I was not able to reproduce this in any other repo apart from 'mini.nvim' (Hmmm.... :) ).

  I have absolutely no idea what here can be the issue...

Here is a screencast:

https://github.com/user-attachments/assets/1dde819a-ac2f-4c3d-8ad8-ce08b5cfdb68

[ibhagwan]

The breaking commit seems to be https://github.com/ibhagwan/fzf-lua/commit/6edea7e876477a2a5811c1a172ff6d2a25669e1f, which is aimed to have a better MiniIcons.get() mock

Hence my initial aversion to the monkey code patches of vim.filetype.match :)

I have absolutely no idea what here can be the issue...

It’s probably a random crash in the external process due to an unhandled exception which is a bit tricky to track in the external process, that’s why we have the debug option :FzfLua files file_icons=mini multiprocess=false, it will run on the main thread (slower and laggy) but we’ll be able to see the unhandled exception as an error when it happens.

Although I doubt this is the reason it’s worthwhile updating to the latest commit I just pushed https://github.com/ibhagwan/fzf-lua/commit/9215e798d3582042757fe5bb44958144e9c86423 and testing the new option I added for mini process1:

Due to the initial lag we had with the first mock of filetype.match I added this option to feed fzf line by line instead of bulk so users have a more immediate UI response.

:FzfLua files file_icons=mini process1=false

[ibhagwan]

Can confirm, running:

:FzfLua files file_icons=mini multiprocess=false

Causes neovim to crash randomly, that’s why the number of results vary, most likely from the vim.filetype.match mock “improvements” :)

My guess is the nvim_buf_get_lines|nvim_buf_line_count APIs are returning something unexpected, I’ll read their docs and do some testing and see what needs to be adjusted.

[ibhagwan]

Something in the test file names with the new mock methods causes the headless wrapper to crash

[ibhagwan]

Btw, I was sure it was the line count mock returning 1 when it should’ve been returning a 0 but I updated the mock to the below and it’s still crashing:

M._nvim_buf_line_count = function() return 0 end

[echasnovski]

Something in the test file names with the new mock methods causes the headless wrapper to crash

Yeah, I've bisected to that also. But it doesn't seem to be related to file names per se. I was (finally) able to reproduce in manually created directory: created 999 files with names 'file-001', 'file-002', ... 'file-999' in the temporary directory. There is crash there. So it seems to be related to vim.filetype.match() not being able to match filetype on too many paths.

Btw, I was sure it was the line count mock returning 1 when it should’ve been returning a 0 but I updated the mock to the below and it’s still crashing:

M._nvim_buf_line_count = function() return 0 end

I've just replicated what vim.api.nvim_buf_line_count(buf_id) returns for the empty scratch buffer (which is used in 'mini.icons').

[ibhagwan]

So it seems to be related to vim.filetype.match() not being able to match filetype on too many paths.

I doubt this is the issue, I tried it on a bigger folder with consistent success, also if you remove buf = 0 from the call it will most likely succeed (as was before said commit).

I just tried also mocking nvim_buf_get_name which is also used when buf = 0 but it didn’t seem to help either.

[ibhagwan]

TBH, the better solution would be to remove buf = 0 and open an upstream issue: why should file.txt isn’t recognized as ft=text without buf = 0?

[ibhagwan]

https://github.com/ibhagwan/fzf-lua/commit/0c7cd0169cb8433f4f3b102bf6d4c0c7e0a20446

@echasnovski, changed the nvim_buf_xxx mocks to an unloaded buffer (vs an empty buffer) and it seems to work.

[echasnovski]

TBH, the better solution would be to remove buf = 0 and open an upstream issue: why should file.txt isn’t recognized as ft=text without buf = 0?

I've discussed the matter privately and it most certainly won't be changed and is there by design. The buf = nil case is there to indicate that user wants to match filetype only by file name and doesn't have any content. Which will won't match many filetypes (txt, ts, etc.).

What should be possible is to have two following cases being equivalent:

• vim.filetype.match({ filename = 'hello.ts', buf = buf_id }) with empty scratch buffer buf_id.
• vim.filetype.match({ filename = 'hello.ts', contents = { '' } }).

This seems to be a welcome change, but it might require a lot of work to fix.

0c7cd01

@echasnovski, changed the nvim_buf_xxx mocks to an unloaded buffer (vs an empty buffer) and it seems to work.

It does seem to fix the issue but results in a huge performance regression.

[ibhagwan]

It does seem to fix the issue but results in a huge performance regression.

I also noticed the first call is much slower now… I dislike the idea of optimizing “hacks”.

[ibhagwan]

I've discussed the matter privately and it most certainly won't be changed and is there by design. The buf = nil case is there to indicate that user wants to match filetype only by file name and doesn't have any content. Which will won't match many filetypes (txt, ts, etc.).

Btw, about this, in what world does txt not point to a text a file? While I can understand other cases IMHO a txt extension clearly indicates a text file regardless of contents.

More so, I can see why detection results can differ based on the contents of the file (e.g sh file with !#/bin/pyhton), but why should the filetype results change when the buffer contents is empty or non-existent? I find this non-intuitive and confusing.

[echasnovski]

So it seems to be related to vim.filetype.match() not being able to match filetype on too many paths.

I doubt this is the issue, I tried it on a bigger folder with consistent success, also if you remove buf = 0 from the call it will most likely succeed (as was before said commit).

I mostly positive that it is related on the number of vim.filetype.match() calls done inside particular :FzfLua files execution. Here is the steps for me (on 9215e798d358 commit):

• Scenario number 1:
  • Create a directory with 346 files named like 'file-001', 'file-002', ... 'file-346'. Number 346 is found empirically and might depend on machine/OS/moon phase.
  • Inside that directory run :FzfLua files file_icons=mini multiprocess=false. It works fine. Makes 346 calls to vim.filetype.match().
  • Create another 100 files: 'file-347', ..., 'file-446'.
  • Run :FzfLua files file_icons=mini multiprocess=false. It again works fine. Makes 100 calls to vim.filetype.match() due to caching previous ones.
• Scenario number 2:
  • Create a directory with 347 files named like 'file-001', 'file-002', ... 'file-347'.
  • Inside that directory run :FzfLua files file_icons=mini multiprocess=false. It crashes.

So here my machine can't handle 347 calls to vim.filetype.match() but can 346.

So I'd ask to take another look here, because dealing with 'fzf-lua' child process is beyond my abilities.

---

Btw, about this, in what world does txt not point to a text a file? While I can understand other cases IMHO a txt extension clearly indicates a text file regardless of contents.

As I said earlier, it can conflict with 'help' filetype which should be set by modeline.

... but why should the filetype results change when the buffer contents is empty or non-existent?

The former was my initial refactor (neovim/neovim#29596) and 0.11 indeed skips content check if buffer is empty or contents is { '' }. The latter is because of:

I've discussed the matter privately and it most certainly won't be changed and is there by design. The buf = nil case is there to indicate that user wants to match filetype only by file name and doesn't have any content. Which will won't match many filetypes (txt, ts, etc.).

[ibhagwan]

It does seem to fix the issue but results in a huge performance regression.

Doesn’t seem to be a big difference between using file name only (which should be the fastest):

command:

:FzfLua files file_icons=mini profile=true query='took

filename only

filename and buf=0

filename and contents ={}

[ibhagwan]

So I'd ask to take another look here, because dealing with 'fzf-lua' child process is beyond my abilities.

Did you manage to crash it post the fix commit? If it’s now working consistently and never crashes doesn’t that disprove the theory of “too many calls”?

Even so, if it turns out to be true and too many calls are the issue I can’t think of any solution that isn’t upstream anyways.

As I said earlier, it can conflict with 'help' filetype which should be set by modeline.

I still argue that having no buffer or contents specified should return a result similar to when buffer contents is empty or buffer is unloaded, it makes sense to me but YMMV.

The buf = nil case is there to indicate that user wants to match filetype only by file name and doesn't have any content. Which will won't match many filetypes (txt, ts, etc.).

I personally find this logic unintuitive, 99% of the time txt indicates a text file, for the one case where the user specifies contents and it turns out to be a help file to make all other 99% use cases return nil.

[ibhagwan]

@echasnovski, let me try to make my case again, if I open a .lua file (not text) and run (latest nightly or 0.10, result is the same:

:lua print("ft:", vim.filetype.match({ filename = "test.txt", buf = 0  }))

The result is:

ft: text nil

While if I run:

:lua print("ft:", vim.filetype.match({ filename = "test.txt"  }))

The result is:

ft:

My current buffer buf=0 is of filetype lua, why is the API returning text? if the contents doesn't matter to this extent (i.e. only looking for modeline, etc) why should omitting buf = 0 not return the same result as if a buffer with random or no content was specified?

[echasnovski]

It does seem to fix the issue but results in a huge performance regression.

Doesn’t seem to be a big difference between using file name only (which should be the fastest):

My bad. I tried it on 'a_large_project' but forgot to use git_icons=false and panicked :) Indeed, comparing 0c7cd0169cb8 and d1493cfac8e5 (because 6edea7e87647 crashes) gives similar times on first and second :FzfLua files file_icons=mini git_icons=false.

So I'd ask to take another look here, because dealing with 'fzf-lua' child process is beyond my abilities.

Did you manage to crash it post the fix commit?

No, now all seems to be good.

If it’s now working consistently and never crashes doesn’t that disprove the theory of “too many calls”?

Not necessarily. Maybe there was some mismatched calls that resulted in child process emitting too many things and it crashed. Or something else. I can not see another interpretation to two verified scenarios apart from "too many vim.filetype.match() calls".

I still argue that ... ... I personally find this logic unintuitive, ... let me try to make my case again, ...

I agree with most of your thoughts on vim.filetype.match(). But it is not I who has to be convinced. I tried to ask around and got the answer described here.

[ibhagwan]

My bad. I tried it on 'a_large_project' but forgot to use git_icons=false and panicked :)

Btw, I disabled git_files=true for grep and reduced the profiling warning to 0.5 seconds - It is now only enabled by default in files, args and git_files.

No, now all seems to be good.

Great!

I agree with most of your thoughts on vim.filetype.match(). But it is not I who has to be convinced. I tried to ask around and got the answer https://github.com/ibhagwan/fzf-lua/issues/1360#issuecomment-2254515851.

Might be worth a revisit? I think I make a pretty compelling case of incosistency and confusion here https://github.com/ibhagwan/fzf-lua/issues/1360#issuecomment-2254533596 - don't you think?

[echasnovski]

I agree with most of your thoughts on vim.filetype.match(). But it is not I who has to be convinced. I tried to ask around and got the answer #1360 (comment).

Might be worth a revisit? I think I make a pretty compelling case of incosistency and confusion here #1360 (comment) - don't you think?

On surface - sure. For what vim.filetype.match() is originally design (be a Lua rewrite of how Vim detects filetype) - there might be unexpected issues.

What should be possible is to have two following cases being equivalent:

* `vim.filetype.match({ filename = 'hello.ts', buf = buf_id })` with empty scratch buffer `buf_id`.

* `vim.filetype.match({ filename = 'hello.ts', contents = { '' } })`.

This seems to be a welcome change, but it might require a lot of work to fix.

I think ^this^ has the best combination of being useful for users and high probability of not huge amount of bikeshedding in issues.

[ibhagwan]

I think ^this^ has the best combination of being useful for users and high probability of not huge amount of bikeshedding in issues.

At least results would be consistent, in this case you’ll probably need to add extensions such as txt to mini.icons as it will no longer be detected in mini (since a scratch buffer is used)?

[echasnovski]

At least results would be consistent, in this case you’ll probably need to add extensions such as txt to mini.icons as it will no longer be detected in mini (since a scratch buffer is used)?

Not quite. If only buffer is supplied - its contents are used. If only contents is supplied - it is used as contents (which now they do not in full). So this change would be just a way to get full vim.filetype.match() experience without the need to maintain an empty scratch buffer (and for 'fzf-lua' to monkey-patch vim.api methods).

[ibhagwan]

At least results would be consistent, in this case you’ll probably need to add extensions such as txt to mini.icons as it will no longer be detected in mini (since a scratch buffer is used)?

Not quite. If only buffer is supplied - its contents are used. If only contents is supplied - it is used as contents (which now they do not in full). So this change would be just a way to get full vim.filetype.match() experience without the need to maintain an empty scratch buffer (and for 'fzf-lua' to monkey-patch vim.api methods).

In that case it would be a great change.