fix: util.root_pattern prioritises pattern order

[emilioziniades]

Because of the way util.root_dir is defined, .csproj files are found before .sln files and the incorrect root directory is set. This prevents go-to-definition between multiple projects in a single solution and causes a few other issues. See #2612 for a more in-depth explanation.

This updates the default configuration to avoid this footgun.

EDIT:

The changes now cover the above issue, but does so in a more general way by updating the util.root_pattern function.

closes #2612
related to razzmatazz/csharp-language-server#57

[justinmk]

why can't the fix be done in util.root_pattern instead of special-cased for csharp?

[emilioziniades]

Yeah there's no reason why not. I agree that having the special case is weird. Will put something together and we can take a look.

[emilioziniades]

@justinmk would you mind taking another look? I've updated util.root_pattern as discussed.

Previously, the function did this:

for each parent directory:
    for each pattern:
        if match:
            return match

I have switched the two loops around so that it now does the following:

for each pattern:
    for each parent directory:
        if match:
            return match

There is a slight performance hit because instead of traversing parent directories once, it now traverses parent directories n times for n patterns.

On the plus side though this resolves the issue not only for csharp_ls, but also for a whole bunch of other language servers which use a similar pattern of doing return util.root_pattern("pattern1")(path) or util.root_pattern("pattern2")(path) or .... Some examples:

• omnisharp
• tsserver
• cairo_ls
• hls

[justinmk]

Thanks for the clear explanation!

• should mention "in order" in the doc for root_pattern

  nvim-lspconfig/doc/lspconfig.txt

  Lines 371 to 374 in 0a0682d

  	- `util.root_pattern`: function which takes multiple arguments, each
  	corresponding to a different root pattern against which the contents of the
  	current directory are matched using |vim.fn.glob()| while traversing up the
  	filesystem.

• @glepnir any objections? this might accidentally change behavior of existing configs, but they can be updated.

After this change if a config has root_pattern {'package.json', '.git'} then parents are traversed until package.json is found, a random .git won't incorrectly define the workspace. If someone wants .git to be higher precedence, then the root_pattern should be changed to {'.git', 'package.json'}

[justinmk]

otoh , does make it impossible to define root markers with "equal precedence"? e.g.

nvim-lspconfig/lua/lspconfig/server_configurations/tsserver.lua

Lines 15 to 18 in 90a28fd

	root_dir = function(fname)
	return util.root_pattern 'tsconfig.json'(fname)
	or util.root_pattern('package.json', 'jsconfig.json', '.git')(fname)
	end,

is that important?

[emilioziniades]

Force pushed to update spelling of "prioritises" in commit message, and updated util.root_pattern docs.

otoh , does make it impossible to define root markers with "equal precedence"? e.g.

Yes it does.

is that important?

Not sure. Struggling to think of a situation where equal precedence would be desirable/useful.

Case A

directory/
    marker_a
    marker_b

Case B

directory/
    marker_a
    subdirectory/
        marker_b

Case C

directory/
    marker_b
    subdirectory/
        marker_a

In case A, precedence doesn't matter, any order of markers will identify the same root directory.

The only situation where equal precedence would be useful is if you always wanted to identify directory/subdirectory as the root, regardless of whether marker_a or marker_b is present. i.e. you have one project that is like case B, and another project using the same language server that is like case C. Does this actually ever happen?

[glepnir]

it's better use vim.fs module

[glepnir]

thought after 0.10 we can use vim.iter with find .

[justinmk]

Force pushed to update spelling of "prioritises" in commit message

Did CI complain about that? 😆

after 0.10 we can use vim.iter with find .

We can save the vim.fs refactoring until 0.10 then?

Since we have carefully considered the implications and the behavior of this PR seems to match user intention, I think we should go ahead and try this.

In a followup PR we should cleanup the existing configs since the or cases are now redundant.

[glepnir]

nope. ci has been fixed on master. just need rebase :)

[emilioziniades]

Did CI complain about that? 😆

lol no it was just bothering me 😝

nope. ci has been fixed on master. just need rebase :)

Rebased master and CI is passing now.