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

[LSP] Untangle retrieving the document identifier from the executing handler #71114

Open
dibarbet opened this issue Dec 6, 2023 · 9 comments
Assignees
Labels
Area-IDE Feature Request LSP issues related to the roslyn language server protocol implementation
Milestone

Comments

@dibarbet
Copy link
Member

dibarbet commented Dec 6, 2023

As we implement allowing different LSP request handlers for different documents, its become clear that we need to move the code that retrieves the text document off of the handler itself.

This will allow us to consistently retrieve the document without specifically calling a handler that may not be appropriate for the request. For example, we would like to avoid C# completion resolve handler to find the document when resolving a XAML completion item.

We might be able to repurpose the ITextDocumentIdentifierHandler to untie it from a specific handler and be a separately exported LSP service that we use to resolve the textdocument before we call into a specific handler.

public class ExportTextDocumentIdentifierHandler : ExportAttribute
{
    public ExportTextDocumentProviderAttribute(string methodName)
    {
        ...
    }
}

[ExportTextDocumentIdentifierHandler("codeAction/resolve")
public class CodeActionResolveTextDocumentProvider : ITextDocumentIdentifierHandler<CodeAction, TextDocumentIdentifier>
{
    TextDocumentIdentifier GetTextDocumentIdentifier(CodeAction request)
    {
        return ((JToken)request.Data!).ToObject<CodeActionResolveData>()!.TextDocument;
    }
}
@dibarbet dibarbet added Area-IDE Feature Request LSP issues related to the roslyn language server protocol implementation labels Dec 6, 2023
@dibarbet dibarbet self-assigned this Dec 6, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Dec 6, 2023
@dibarbet
Copy link
Member Author

dibarbet commented Dec 6, 2023

cc @CyrusNajmabadi - would like your thoughts here too

mgoertz-msft added a commit to mgoertz-msft/roslyn that referenced this issue Jan 22, 2024
…) via cherry-pick of PR commit. The Roslyn insertion will need to be coordinated with a Razor update to switch from the obsolete IRequestContextFactory to AbstractRequestContextFactory.

Addresses dotnet#69471 and dotnet#71114
@mgoertz-msft
Copy link
Contributor

@arkalyanms, I see that this is still marked as untriaged, so I wanted to make sure that you are aware that our XAML LSP implementation for VS Code has a dependency on this work.

FYI @dibarbet, @etvorun

@arunchndr arunchndr removed the untriaged Issues and PRs which have not yet been triaged by a lead label Feb 14, 2024
@arunchndr arunchndr added this to the 17.10 P3 milestone Feb 14, 2024
@mgoertz-msft
Copy link
Contributor

FYI, I started looking at this.

@dibarbet
Copy link
Member Author

dibarbet commented Mar 8, 2024

@mgoertz-msft before you get too far, take a look at #72230

It's pretty close to done, I just wanted to push the language determination into the queue loop, instead of before. That was going to need breaking changes in clasp, so I was working on #72237 first.
Thats basically done, just needs to be merged and then razor and webtools insert using the new source package version.

@mgoertz-msft
Copy link
Contributor

@dibarbet Thanks. I went down the other path of decoupling ITextDocumentIdentifierHandler and created an ILspService to get the TextDocumentIdentifier/Uri from a TRequest, which right now still includes a fallback doing it the existing way. While creating such handlers in our XAML code though I realized that neither approach currently handles the ResolveDataCaches we have for the CompletionResolveHandlers for example.

@mgoertz-msft
Copy link
Contributor

I cherry-picked your commit to try it myself and it looks like we have quite a bit of redundant data in the completion resolve item:

  Name Value Type
dataToken {{ "DataId": 3, "Document": { "uri": "file:///c:/Users/mgoertz/source/repos/MauiApp8/MauiApp8/MainPage.xaml" }, "TextDocument": { "uri": "file:///c:/Users/mgoertz/source/repos/MauiApp8/MauiApp8/MainPage.xaml" } }} Newtonsoft.Json.Linq.JToken {Newtonsoft.Json.Linq.JObject}
  ◢ ChildrenTokens Count = 3 System.Collections.Generic.IList<Newtonsoft.Json.Linq.JToken> {Newtonsoft.Json.Linq.JPropertyKeyedCollection}
  ▶ [0] {"DataId": 3} Newtonsoft.Json.Linq.JToken {Newtonsoft.Json.Linq.JProperty}
  ▶ [1] {"Document": { "uri": "file:///c:/Users/mgoertz/source/repos/MauiApp8/MauiApp8/MainPage.xaml" }} Newtonsoft.Json.Linq.JToken {Newtonsoft.Json.Linq.JProperty}
  ▶ [2] {"TextDocument": { "uri": "file:///c:/Users/mgoertz/source/repos/MauiApp8/MauiApp8/MainPage.xaml" }} Newtonsoft.Json.Linq.JToken {Newtonsoft.Json.Linq.JProperty}

I believe DataId 3 is the cache index but I need to check where the explicit URIs are comming from... they are quite wasteful for a completion list.

@mgoertz-msft
Copy link
Contributor

Oh, wait a minute, I think we checked a client capability somewhere to store the URI once for all items, but I think that may only work for VS Code and not VS. Anyway, I think we are sending it all right now, so I need to check our XAML code...

@mgoertz-msft
Copy link
Contributor

Turns out that code was never added to our implementation in VS, so I'll have to revisit that. I like that your proposed change is at a much lower level. I have yet to see it all working together though - right now I'm getting some MissingMethodExceptions in Roslyn - clearly something's out of sync LOL

@arunchndr arunchndr modified the milestones: 17.10 P3, Backlog Nov 13, 2024
@CyrusNajmabadi
Copy link
Member

@dibarbet do we need to do anything here? if so, assign to a milestone. if not, close out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE Feature Request LSP issues related to the roslyn language server protocol implementation
Projects
None yet
Development

No branches or pull requests

4 participants