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

Add ILspSolutionProvider in lsp #44474

Merged
merged 3 commits into from
Jun 11, 2020
Merged

Add ILspSolutionProvider in lsp #44474

merged 3 commits into from
Jun 11, 2020

Conversation

dibarbet
Copy link
Member

@dibarbet dibarbet commented May 21, 2020

Depends on #44775

Currently the LSP server uses the VisualStudioWorkspace current solution to retrieve documents. This generally works in solution open cases. However, when only a folder is opened, the LSP server is not able to respond as it can't find documents that are part of another workspace's solution (e.g. misc files). This is something we noticed recently when the solution open experience was not working.

The goal of this change is to allow LSP requests to be served for open folder in cloud envs (currently opened .cs files have no completion, no navbar, no goto def, etc). This change makes it so LSP requests can be served with an LSP misc files experience, like having references to mscorlib in completion.

The change being made here is to create a solution provider that retrieves the correct solution.
Long term we would like the request to come in with a solution snapshot which can be used to find the actual solution. Since we're not there yet, we just use the VS workspace and misc files workspace current solution so that we have at least some support for the open folder scenario in cloud env. Note that I have not added metadataassourceworkspace support as there are issues outside of roslyn (our LSP server isn't requested for those files at all).

@@ -72,5 +84,32 @@ public CodeActionsHandler(ICodeFixService codeFixService, ICodeRefactoringServic

return result.ToArrayAndFree();
}

internal static async Task<IEnumerable<CodeAction>> GetCodeActionsAsync(Document? document,
Copy link
Member Author

Choose a reason for hiding this comment

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

just moved from the old base class

@CyrusNajmabadi
Copy link
Member

This PR is quite large. WHen you make it non-draft, can you explain what's going on, and possibly break into smaller pieces?

@dibarbet
Copy link
Member Author

This PR is quite large. WHen you make it non-draft, can you explain what's going on, and possibly break into smaller pieces?

@CyrusNajmabadi I've updated the description, hopefully that explains it a bit better.
I don't think it will be easy to split into smaller pieces - this is basically adding the ILspSolutionProvider that I need to thread through all the LSP handlers. The majority of the changes are just add/removing parameters related to it and fixing up retrieval of the solutions.
I also did some deletion of a couple of unnecessary pieces that I would have otherwise had to thread through also.

@dibarbet dibarbet marked this pull request as ready for review May 21, 2020 22:06
@dibarbet dibarbet requested a review from a team as a code owner May 21, 2020 22:06
@dibarbet dibarbet added Area-IDE LSP issues related to the roslyn language server protocol implementation labels May 21, 2020
@dibarbet dibarbet marked this pull request as draft May 21, 2020 22:31
@dibarbet dibarbet force-pushed the lsp_solution branch 3 times, most recently from fc65c32 to 0491c46 Compare June 2, 2020 01:28
@dibarbet dibarbet marked this pull request as ready for review June 3, 2020 23:15
@dibarbet
Copy link
Member Author

dibarbet commented Jun 3, 2020

@CyrusNajmabadi slightly smaller now! Ready

Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

Only real request here is to switch around the arguments for the extension methods: it makes sense to me the 'this' arugment is ILspSolutionProivder and not something like TextDocumentIdentifier. If that method wasn't an extension method we'd probably put it on ILspSolutionProvider directly.

The question about whether we were better off keeping things decoupled and passing in an ILspSolutionProvider to the request handlers rather than MEF I'm happy to defer in a later PR if we want to get this in sooner.

@dibarbet dibarbet merged commit a85202d into dotnet:master Jun 11, 2020
@dibarbet dibarbet deleted the lsp_solution branch June 11, 2020 00:45
@ghost ghost added this to the Next milestone Jun 11, 2020
@dibarbet dibarbet removed this from the Next milestone Jun 30, 2020
@dibarbet dibarbet added this to the 16.7.P4 milestone Jun 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE LSP issues related to the roslyn language server protocol implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants