Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve util.root_pattern() #3621

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

tmillr
Copy link
Contributor

@tmillr tmillr commented Feb 19, 2025

  • Accept root patterns matching broken symlinks. If the dirent exists, that should be good enough.

  • perf: don't check/resolve/follow symlinks when matching root patterns

  • Remove pointless conditionals such as if match ~= nil then return match

  • Remove pointless check for file existence (vim.fn.glob() already guarantees this)

  • perf: avoid unnecessary table creations (e.g. used only to concatenate strings; string.format() or .. should be preferred for simple/static string concatenations)

  • perf: avoid synchronous vim.loop.fs*() calls where possible (they tend to be slower than their builtin counterparts)

  • Account for the fact that functions like string.match() and string.gsub() return multiple values (e.g. use return (s:gsub(...)) to return the modified string)

  • Add types to some of the utils

@tmillr tmillr requested a review from glepnir as a code owner February 19, 2025 19:19
Copy link
Member

@justinmk justinmk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most (or all) of util.lua is on the path to deprecation. However, improvements to it, especially simplifications, are still helpful because it makes it easier to understand what is missing from "upstream" Nvim stdlib.

@tmillr tmillr marked this pull request as draft February 19, 2025 19:45

Verified

This commit was signed with the committer’s verified signature.
tmillr Tyler Miller
- For each dir level, try all root patterns (instead of all levels for
  each pattern), returning on the first match. Prefer deeper matches,
  regardless of which pattern matches, over an earlier pattern matching
  in a parent dir.

- Accept root patterns matching broken symlinks. If the dirent exists,
  that should be good enough.

- perf: don't check/resolve/follow symlinks when matching root patterns

- Remove pointless check for file existence (`vim.fn.glob()` already
  guarantees this)

- perf: avoid unnecessary table creations (e.g. used only to concatenate
        strings; `string.format()` or `..` should be preferred for
        simple/static string concatenations)

- perf: avoid synchronous `vim.loop.fs*()` calls where possible (they
        tend to be slower than their builtin counterparts)

- Account for the fact that functions like `string.match()` and
  `string.gsub()` return multiple values (e.g. use `return (s:gsub(...))`
  to return the modified string)

- Add types to some of the utils
@tmillr
Copy link
Contributor Author

tmillr commented Feb 21, 2025

It's basically ready now, but going through the file one more time, I observed and decided to fix the bug in root_pattern(), only to go down the issue-ref rabbit-hole and realize that said bug is actually feature now.

The way root_pattern() was modified to prefer pattern order over directory depth feels very wrong to me, and I think the issue that caused it to get changed should have instead been:

  1. Fixed locally (e.g. by removing certain patterns, or calling root_pattern() multiple times separately). There was never anything keeping people from calling it multiple times with certain (higher-precedence) patterns at a time if that was absolutely needed, and the algorithm would have been the same (as it is today).
  2. Fixed by introducing another, separate, root pattern util which works differently (i.e. as it does today)

And it seems to have led to some ugly/hacky code in order to workaround the new (and IMO unintuitive) behavior, and it has caused at least a couple bugs as well (#3508).

But that change was over a year ago, and perhaps things were different then. The way I understand it now is that the root_dir callback drives/defines a workspace and its members/roots? Or in other words, it is the only way to achieve a multi-root/multi-member workspace (maybe some servers determine this internally on their own—e.g. rust workspace members are explicitly defined—but I know at least lua_ls doesn't and relies on the client). This is another reason why I think root_pattern() should've stayed the way it was. A .luarc.json in a nested subdir should reuse the server but also create a new/separate workspace member/context, and it should probably even do the same upon finding a nested .git. But there may be some nuance here that I am unaware of.

But anyway, even though it should definitely be changed back to how it was, I understand (now) that that could potentially cause some servers' configuration to break as they may have been changed in the meantime in order to adapt. So just let me know whether to change it, or leave it (hopefully to be addressed later).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants