OmniSharp / omnisharp-roslyn

OmniSharp server (HTTP, STDIO) based on Roslyn workspaces
MIT License
1.76k stars 420 forks source link

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

Open Hoffs opened 2 years ago

Hoffs commented 2 years ago

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:

https://github.com/OmniSharp/omnisharp-roslyn/blob/657b06470be88f4759e7db5969f84ce1bc0b14d2/src/OmniSharp.LanguageServerProtocol/Handlers/OmniSharpDefinitionHandler.cs#L35-L40

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

https://github.com/OmniSharp/omnisharp-roslyn/blob/657b06470be88f4759e7db5969f84ce1bc0b14d2/src/OmniSharp.Roslyn.CSharp/Services/Navigation/GotoDefinitionServiceV2.cs#L33-L34

https://github.com/OmniSharp/omnisharp-roslyn/blob/657b06470be88f4759e7db5969f84ce1bc0b14d2/src/OmniSharp.Roslyn/BaseExternalSourceService.cs#L17-L25

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.

tadeokondrak commented 2 years ago

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 commented 2 years ago

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

kflu commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

+1

griggsca91 commented 1 year ago

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 commented 7 months ago

+1. This is also problematic on Helix editor. https://github.com/helix-editor/helix/issues/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 commented 6 months ago

Hey, guys, do we have any updates?

KiLLeRRaT commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

KiLLeRRaT commented 6 months ago

+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.