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

textDocument/definition URI is incompatible with (...)ExternalSourceService cache #2238

Open
Hoffs opened this issue Sep 26, 2021 · 12 comments
Labels

Comments

@Hoffs
Copy link

Hoffs commented Sep 26, 2021

Hi,

I've been working on extending LSP client to pull metadata/file source when textDocument/definition returns result with $metadata$ URI (e.g. uri = "file:///%24metadata%24/Project/nvim-csharp-metadata/Assembly/System/Console/Symbol/System/Console.cs").

The problem I encountered that it seems to be impossible to get subsequent results using textDocument/definition when being run from $metadata$ file. For example, first definition request is done from normal file and gets:

{
  bufnr = 1,
  client_id = 1,
  method = "textDocument/definition",
  params = {
    position = {
      character = 20,
      line = 8
    },
    textDocument = {
      uri = "file:///home/hoffs/projects/nvim-csharp-metadata/Program.cs"
    }
  }
}
----- response ------
  {
    range = {
      end = {
        character = 36,
        line = 1774
      },
      start = {
        character = 27,
        line = 1774
      }
    },
    uri = "file:///%24metadata%24/Project/nvim-csharp-metadata/Assembly/System/Console/Symbol/System/Console.cs"
  }

Then that file can be fetched with custom call to o#/metadata, but then once in that temporary/pseudo file buffer, named $metadata$/Project/nvim-csharp-metadata/Assembly/System/Console/Symbol/System/Console.cs any textDocument/definition request is incompatible to properly resolve to cached document.

I believe the issue lies at FromUri call at:

var omnisharpRequest = new GotoDefinitionRequest()
{
FileName = FromUri(request.TextDocument.Uri),
Column = Convert.ToInt32(request.Position.Character),
Line = Convert.ToInt32(request.Position.Line)
};

As this Uri->FileName gets translated to incompatible string for FindDocumentInCache.

var document = externalSourceService.FindDocumentInCache(request.FileName) ??
_workspace.GetDocument(request.FileName);

public Document FindDocumentInCache(string fileName)
{
if (_cache.TryGetValue(fileName, out var metadataDocument))
{
return metadataDocument;
}
return null;
}

LSP URI FileName/Cache Key attempted
file://$metadata$/Project/nvim-csharp-metadata/Assembly/System/Console/Symbol/System/Console.cs \\\\$metadata$\\Project\\nvim-csharp-metadata\\Assembly\\System\\Console\\Symbol\\System\\Console.cs
file:///$metadata$/Project/nvim-csharp-metadata/Assembly/System/Console/Symbol/System/Console.cs /$metadata$/Project/nvim-csharp-metadata/Assembly/System/Console/Symbol/System/Console.cs

The desired cache key is $metadata$/Project/nvim-csharp-metadata/Assembly/System/Console/Symbol/System/Console.cs which I believe is impossible to get when going through FromUri.

So while the metadata functionality is a bit outside the standard LSP spec, since it already returns and saves the $metadata$ documents in cache (without even calling o#/metadata), I believe it would be nice if it would properly work if textDocument/definition requests come from $metadata$ file.

If thats fine, this could either be done by not using FromUri or having special case for when URI is for $metadata$ file. IMO 2nd row in the table, where $metadata$ is supposedly under root (/$metadata$/...) should be the happy scenario when providing URI for $metadata$ documents, so then in the handler it could just have a check for StartsWith('/$metadata$/'... or something.

@filipw filipw added the lsp label Sep 27, 2021
@tadeokondrak
Copy link

Hi, not sure if this is helpful, but I'm also trying to get metadata working in a non-VSCode LSP client, and for your example, I think file:///Project/nvim-csharp-metadata/Assembly/System/Console/Symbol/System/[metadata] Console.cs would work. That seems to be the URL used in VSCode.

@svermeulen
Copy link

Is there a way to at least disable this $metadata$ feature so that us LSP users don't run into this error constantly?

@kflu
Copy link

kflu commented Jan 23, 2022

is it reasonable to say that omnisharp-roslyn lsp should not return any URI that contains $metadata$, which does not comply with LSP standard, and most lsp clients don’t know how to handle it.

I’m using vim-lsp and also bump into this.

@joaotavora
Copy link

And, for good measure here's my +1 to this issue. In the Eglot client, I get a nonsensical URL like /$metadata$/Project/my-project/Assembly/.... This $metadata$ variable does sound like something which could be set, somehow.

@tilupe
Copy link

tilupe commented Jan 15, 2023

+1

@griggsca91
Copy link

Just would like to say, I started a new job in a dotnet shop, and would love to be able to have this support with my neovim editor

@Multirious
Copy link

Multirious commented Jan 18, 2024

+1. This is also problematic on Helix editor. helix-editor/helix#2430

Helix also dumped out a lot of received malformed notification from Language Server: Unhandled that has been coming from OmniSharp too. Not sure if this issue been mentioned before.

@AugustoDeveloper
Copy link

Hey, guys, do we have any updates?

@KiLLeRRaT
Copy link

KiLLeRRaT commented Feb 25, 2024

I have just installed and configured https://github.com/Hoffs/omnisharp-extended-lsp.nvim and it seems to be working well for me. I have only used it for around 30 minutes so far, but so far, so good...

Edit: I did find that if I bind straight to this, my goto definition stops working for other file types. I've added an ftplugin specific binding just for cs files. As per https://github.com/KiLLeRRaT/.dotfiles/blob/65839bcdb41558d543395c0efa329f2fab6cc644/nvim-lua/.config/nvim/after/ftplugin/cs.lua#L9C1-L9C96

@AugustoDeveloper
Copy link

I have just installed and configured https://github.com/Hoffs/omnisharp-extended-lsp.nvim and it seems to be working well for me. I have only used it for around 30 minutes so far, but so far, so good...

Yes, it works, but if you try to go to definition on metadata file, an error will be displayed.

@jknopp
Copy link

jknopp commented Mar 9, 2024

@KiLLeRRaT
Copy link

+1 on this using https://github.com/Hoffs/omnisharp-extended-lsp.nvim

See my edit on my first post about this and the ftplugin binding.

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

No branches or pull requests