ehaynes99 commented 2 months ago

Description

I've long had ignore patterns for lockfiles, dependency dirs that I don't want to be included in search, etc. e.g.

file_ignore_patterns = {
  '.git/',
  'node_modules',
  'package-lock.json',
  'pnpm-lock.yaml',
  -- ...
},

After https://github.com/nvim-telescope/telescope.nvim/pull/3173, actions like lsp_definitions no longer work for any dependencies where the source/definition matches any of the exclusions.

Neovim version

NVIM v0.10.0
Build type: Release
LuaJIT 2.1.1716656478

Operating system and version

Arch Linux

Telescope version / branch / rev

7bd2f9b72f8449780b79bcf351534e2cd36ec43a

checkhealth telescope

==============================================================================
telescope: health#telescope#check

Checking for required plugins ~
- OK plenary installed.
- OK nvim-treesitter installed.

Checking external dependencies ~
- OK rg: found ripgrep 14.1.0
- OK fd: found fd 10.1.0

===== Installed extensions ===== ~

Telescope Extension: `fzf` ~
- OK lib working as expected
- OK file_sorter correctly configured
- OK generic_sorter correctly configured

Telescope Extension: `live_grep_args` ~
- No healthcheck provided

Steps to reproduce

Hopefully since I'm pointing to specific lines of code, you don't need me to figure out a minimal config that has TypeScript LSP wired up, but I can if you insist. But fundamentally, the issue is that lsp_definitions, lsp_implementations, etc. on any value from a library will (correctly) include matches that are in the excluded node_modules directory. If I wrap this line: https://github.com/nvim-telescope/telescope.nvim/blob/7bd2f9b72f8449780b79bcf351534e2cd36ec43a/lua/telescope/builtin/__lsp.lua#L199 with:

print('before: ' .. vim.inspect(items))
items = filter_file_ignore_patters(items, opts)
print('after: ' .. vim.inspect(items))

I get this output:

before: { {
    col = 13,
    filename = "/home/ehaynes99/workspace/backend/node_modules/.pnpm/@sinclair+typebox@0.32.31/node_modules/@sinclair/typebox/build/cjs/type/static/static.d.ts",
    lnum = 33,
    text = "export type Static<T extends TSchema, P extends unknown[] = []> = (T & {",
    user_data = {
      originSelectionRange = {
        ["end"] = {
          character = 52,
          line = 11
        },
        start = {
          character = 46,
          line = 11
        }
      },
      targetRange = {
        ["end"] = {
          character = 13,
          line = 34
        },
        start = {
          character = 0,
          line = 32
        }
      },
      targetSelectionRange = {
        ["end"] = {
          character = 18,
          line = 32
        },
        start = {
          character = 12,
          line = 32
        }
      },
      targetUri = "file:///home/ehaynes99/workspace/backend/node_modules/.pnpm/%40sinclair%2Btypebox%400.32.31/node_modules/%40sinclair/typebox/build/cjs/type/static/static.d.ts"
    }
  } }
after: {}

Expected behavior

The editor would navigates to the file containing the declaration

Actual behavior

A message is printed

[telescope.builtin.lsp_definitions]: No LSP Definitions found

Minimal config

local root = vim.fn.fnamemodify("./.repro", ":p")

-- set stdpaths to use .repro
for _, name in ipairs { "config", "data", "state", "cache" } do
  vim.env[("XDG_%s_HOME"):format(name:upper())] = root .. "/" .. name
end

-- bootstrap lazy
local lazypath = root .. "/plugins/lazy.nvim"
if not vim.uv.fs_stat(lazypath) then
  vim.fn.system {
    "git",
    "clone",
    "--filter=blob:none",
    "https://github.com/folke/lazy.nvim.git",
    lazypath,
  }
end
vim.opt.runtimepath:prepend(lazypath)

-- install plugins
local plugins = {
  {
    "nvim-telescope/telescope.nvim",
    dependencies = {
      "nvim-lua/plenary.nvim",
    },
    config = function()
      -- ADD INIT.LUA SETTINGS THAT ARE _NECESSARY_ FOR REPRODUCING THE ISSUE
      require("telescope").setup {
        file_ignore_patterns = {
          'node_modules',
        },
      }
    end,
  },
}

require("lazy").setup(plugins, {
  root = root .. "/plugins",
})

jamestrew commented 2 months ago

The LSP pickers always honored file_ignore_patterns. The only exception to this rule was when there's only one result so we'd bypass the actual picker. I think there's an argument to be made for this exception to be considered a bug. Hence #3173

One way to restore the something like the previous behavior would be to exclude node_modules from the file_ignore_patterns table just for LSP pickers.

Probably just passing an empty table should work since most other file patterns won't be something LSPs will pick up anyways.

Does that seem reasonable?

ehaynes99 commented 2 months ago

The LSP pickers always honored file_ignore_patterns.

Respectfully, this worked until now, so I'm not sure that's true. lsp_definitions will navigate to the type definition in node_modules until af78ba3b7da51ea5780bdb2c4b990ae2ea2ce716, but not in 58ec6eca1c3704d130d749843e16fbf6c8cdc57e.

The only exception to this rule was when there's only one result so we'd bypass the actual picker. I think there's an argument to be made for this exception to be considered a bug.

I personally LOVE this behavior (I actually thought I remembered this being configurable, but I can't find it). Happy to live with that decision, though, but the inability to jump to definitions in libraries excluded from text search is definitely not going to work. If this behavior stands, I feel like I'm going to have to start spreading the exclusions to things like the ripgrep options in vimgrep_arguments, and... whatever I need to do for fd to search files, and... not sure what else. I doubt I'll be the only one that is surprised by this, though.

https://github.com/nvim-telescope/telescope.nvim/assets/2187664/e19069b5-e60f-4711-b8b1-d57c7a26de31

jamestrew commented 1 month ago

Respectfully, this worked until now, so I'm not sure that's true. lsp_definitions will navigate to the type definition in node_modules until af78ba3b7da51ea5780bdb2c4b990ae2ea2ce716, but not in 58ec6eca1c3704d130d749843e16fbf6c8cdc57e.

This is only true is there's only one result to jump to - which is the exception I'm talking about. But when there's more than one result and the telescope UI would open, file_ignore_patterns would be respected and filter out matching results. See https://github.com/nvim-telescope/telescope.nvim/issues/3168

I understand this change might be a little jarring but it has been the expectation for several years and has been long documented under :h telescope.defaults.file_ignore_patterns:

    file_ignore_patterns: ~
        A table of lua regex that define the files that should be ignored.
        Example: { "^scratch/" } -- ignore all files in scratch directory
        Example: { "%.npz" } -- ignore all npz files
        See: https://www.lua.org/manual/5.1/manual.html#5.4.1 for more
        information about lua regex
        Note: `file_ignore_patterns` will be used in all pickers that have a
        file associated. This might lead to the problem that lsp_ pickers
        aren't displaying results because they might be ignored by
        `file_ignore_patterns`. For example, setting up node_modules as ignored
        will never show node_modules in any results, even if you are
        interested in lsp_ results.

        If you only want `file_ignore_patterns` for `find_files` and
        `grep_string`/`live_grep` it is suggested that you setup `gitignore`
        and have fd and or ripgrep installed because both tools will not show
        `gitignore`d files on default.

        Default: nil

3173 only fixes an edge case related to this.

Like mentioned above, fd and rg should already take care of ignoring node_modules as long as it's in your .gitignore. If you don't use git, your can instead use a .ignore file which fd and rg will both respect. It's my opinion that users should use file_ignore_patterns as little as possible as the pattern matching is handled in the main lua event loop and is orders of magnitude slower than letting external tools such as fd and rg handle file ignoring.

If you still want to use file_ignore_patterns, my recommendation is to pass them specific to pickers. eg.

require('telescope.builtin').lsp_definitions({ file_ignore_patterns = {} })

This is what I meant by but I was only on my phone earlier, couldn't go quite into detail.