Move Quick Info content production to the Features layer and add Razor EA co-hosting APIs for Hover and Completion #75226

DustinCampbell · 2024-09-24T23:25:57Z

Razor is moving toward an architecture where its LSP handlers are loaded within the Roslyn LSP and are implemented via services that run in Roslyn OOP within VS Service Hub. Of the handlers that Razor needs to implement, it turns out that 'hover' and 'completion' (and likely, 'documentSymbol' and 'references') present a special challenge. In Roslyn, these handler implementations rely on host-provided services to provide the results for a default LSP client or richer results for the VS LSP client. However, the VS-targeted services are implemented in the EditorFeatures layer, which is never loaded in the Roslyn OOP.

This pull requests decouples these services from their VS dependencies and pushes the service logic into the Features, or into the Roslyn language server itself.

A small data model has been added to the Features layer to represent Quick Info content.
An abstraction has been added to handle the production of navigation action links for Quick Info content.
The relevant GUID/ids for VS images have been copied to the Features layer to avoid any VS dependencies. (Note: these are defined in Microsoft.VisualStudio.Imaging.dll as constants, so VS can never change them without breaking consumers.)
Logic for producing Quick Info content has been moved to the Features layer and refactored to reduce allocations.
All the "ILspFeatureNameResultCreationService" logic has been moved from the EditorFeatures layer to the Roslyn language server.
HoverHandler, CompletionHandler, and CompletionResolveHandler have been refactored to provide static methods called from the Razor EA.

While this change is needed to support Razor co-hosting, it would also be needed if Roslyn ever wanted to move its VS language server out-of-process.

cc @dibarbet and @davidwengier

This is necessary to produce rich VS adornment-based hover tooltips in Roslyn OOP. Currently this logic lives in the EditorFeatures layer.

IntellisenseQuickInfoBuilderContext has four(!) properties that are used to create navigation Actions. I've wrapped this in NavigationActionFactory class.

IntellisenseQuickInfoBuilderContext renamed to QuickInfoContentBuilderContext

EditorFeaturesOnTheFlyDocsElement renamed to QuickInfoOnTheFlyDocsElement

This change is a rewrite of BuildInteractiveTextElements to reduce allocations and prepare it to be moved to the Features layer. In addition, it is no exposed as an extension method on ImmutableArray<TaggedText> named ToInteractiveTextElements(...).

With the logic to produce rich Quick Info content in the Features layer, this service is no longer and its logic can be moved into HoverHandler.

CyrusNajmabadi · 2024-09-25T00:57:29Z

I can review the tomorrow!

src/Features/Core/Portable/Common/GlyphExtensions.KnownImageIds.cs

src/Features/Core/Portable/QuickInfo/Presentation/QuickInfoContentBuilder.cs

CyrusNajmabadi · 2024-09-25T18:25:50Z

src/EditorFeatures/Core/IntelliSense/NavigationActionFactory.cs

+    {
+        try
+        {
+            using var token = asyncListener.BeginAsyncOperation(nameof(NavigateToTargetAsync));


for the big blocks of code, can you link to the place it got moved from?

DustinCampbell · 2024-09-25T18:32:22Z

src/EditorFeatures/Core/IntelliSense/NavigationActionFactory.cs

+            navigationTarget, workspace, documentId, threadingContext, operationExecutor, asyncListener, streamingPresenter.Value).Forget();
+    }
+
+    private static async Task NavigateToTargetAsync(


📝 This method was moved from

roslyn/src/EditorFeatures/Core/IntelliSense/Helpers.cs

Lines 190 to 232 in 4da55b7

private static async Task NavigateToQuickInfoTargetAsync(

string navigationTarget,

Workspace workspace,

DocumentId documentId,

IThreadingContext threadingContext,

IUIThreadOperationExecutor operationExecutor,

IAsynchronousOperationListener asyncListener,

IStreamingFindUsagesPresenter streamingPresenter)

{

try

{

using var token = asyncListener.BeginAsyncOperation(nameof(NavigateToQuickInfoTargetAsync));

using var context = operationExecutor.BeginExecute(EditorFeaturesResources.IntelliSense, EditorFeaturesResources.Navigating, allowCancellation: true, showProgress: false);

var cancellationToken = context.UserCancellationToken;

var solution = workspace.CurrentSolution;

SymbolKeyResolution resolvedSymbolKey;

try

{

var project = solution.GetRequiredProject(documentId.ProjectId);

var compilation = await project.GetRequiredCompilationAsync(cancellationToken).ConfigureAwait(false);

resolvedSymbolKey = SymbolKey.ResolveString(navigationTarget, compilation, cancellationToken: cancellationToken);

}

catch

{

// Ignore symbol resolution failures. It likely is just a badly formed URI.

return;

}

if (resolvedSymbolKey.GetAnySymbol() is { } symbol)

{

var location = await GoToDefinitionHelpers.GetDefinitionLocationAsync(

symbol, solution, threadingContext, streamingPresenter, cancellationToken).ConfigureAwait(false);

await location.TryNavigateToAsync(threadingContext, new NavigationOptions(PreferProvisionalTab: true, ActivateTab: true), cancellationToken).ConfigureAwait(false);

}

}

catch (OperationCanceledException)

{

}

catch (Exception ex) when (FatalError.ReportAndCatch(ex, ErrorSeverity.Critical))

{

}

}

.

DustinCampbell · 2024-09-25T18:34:47Z

src/Features/Core/Portable/QuickInfo/Presentation/TaggedTextExtensions.cs

+
+namespace Microsoft.CodeAnalysis.QuickInfo.Presentation;
+
+internal static class TaggedTextExtensions


📝 This was moved and refactored from https://github.com/dotnet/roslyn/blob/main/src/EditorFeatures/Core/IntelliSense/Helpers.cs.

DustinCampbell · 2024-09-25T18:35:56Z

src/Features/Core/Portable/QuickInfo/Presentation/QuickInfoContentBuilder.cs

+
+namespace Microsoft.CodeAnalysis.QuickInfo.Presentation;
+
+internal static class QuickInfoContentBuilder


📝 The bulk of this was moved and refactored from https://github.com/dotnet/roslyn/blob/main/src/EditorFeatures/Core/IntelliSense/QuickInfo/IntellisenseQuickInfoBuilder.cs.

DustinCampbell · 2024-09-25T18:38:37Z

src/Features/Core/Portable/QuickInfo/Presentation/QuickInfoOnTheFlyDocsElement.cs

+
+namespace Microsoft.CodeAnalysis.QuickInfo.Presentation;
+
+internal sealed class QuickInfoOnTheFlyDocsElement(Document document, OnTheFlyDocsInfo info) : QuickInfoElement


📝 This was moved and adjusted from https://github.com/dotnet/roslyn/blob/main/src/EditorFeatures/Core/IntelliSense/QuickInfo/EditorFeaturesOnTheFlyDocsElement.cs.

DustinCampbell · 2024-09-25T18:40:43Z

src/Features/Core/Portable/QuickInfo/Presentation/QuickInfoContentBuilderContext.cs

+
+namespace Microsoft.CodeAnalysis.QuickInfo.Presentation;
+
+internal sealed class QuickInfoContentBuilderContext(


📝 This was essentially moved from https://github.com/dotnet/roslyn/blob/main/src/EditorFeatures/Core/IntelliSense/QuickInfo/IntellisenseQuickInfoBuilderContext.cs.

DustinCampbell · 2024-09-25T18:43:44Z

src/LanguageServer/Protocol/Handler/Completion/CompletionResultFactory.cs

 using Roslyn.Utilities;
 using LSP = Roslyn.LanguageServer.Protocol;

 namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Completion
 {
-    internal abstract class AbstractLspCompletionResultCreationService : ILspCompletionResultCreationService
+    internal static class CompletionResultFactory


📝 This is essentially a merge of AbstractLspCompletionResultCreationService, DefaultLspCompletionResultCreationService, and EditorLspCompletionResultCreationService

Per code review feedback, this extension shouldn't be defined in the Features layer. Instead, it can move to LanguageServer.Protocol and be leveraged from the EditorFeatures layer.

src/LanguageServer/Protocol/Handler/Completion/CompletionResolveHandler.cs

src/LanguageServer/Protocol/Handler/Completion/CompletionResultFactory.cs

src/Tools/ExternalAccess/Razor/Cohost/Handlers/Completion.cs

davidwengier

EA bits look great, thanks for the extra work!
Skimmed the rest and the broad strokes made sense, I'll leave Roslyn folks to comment on the Roslyn-isms.

DustinCampbell added 21 commits September 24, 2024 07:53

Introduce Quick Info presentation model in the Features layer

c9c3ca7

This is necessary to produce rich VS adornment-based hover tooltips in Roslyn OOP. Currently this logic lives in the EditorFeatures layer.

Add helpers to convert QuickInfo presentation elements to LSP elements

f05390c

Add helpers to convert presentation elements to VS adornments

d594024

Introduce NavigationActionFactory in IntellisenseQuickInfoBuilderContext

59b4479

IntellisenseQuickInfoBuilderContext has four(!) properties that are used to create navigation Actions. I've wrapped this in NavigationActionFactory class.

Move IntellisenseQuickInfoBuilderContext to Features layer and rename

b0c46a8

IntellisenseQuickInfoBuilderContext renamed to QuickInfoContentBuilderContext

Rename OnTheFlyDocsElement to OnTheFlyDocsInfo

e7c2899

Move EditorFeaturesOnTheFlyDocsElement to Features layer and rename

888d075

EditorFeaturesOnTheFlyDocsElement renamed to QuickInfoOnTheFlyDocsElement

Move remaining navigation action logic into NavigationActionFactory

912ae5a

Clean up IndentationHelper

9a1c3b1

Rewrite BuildInteractiveTextElements

b649f1e

This change is a rewrite of BuildInteractiveTextElements to reduce allocations and prepare it to be moved to the Features layer. In addition, it is no exposed as an extension method on ImmutableArray<TaggedText> named ToInteractiveTextElements(...).

Move TaggedTextExtensions to Features layer

030efe6

Move Quick Info's BuildInteractiveContentAsync to Features layer

2cf6fcb

Clean up and reduce allocations in QuickInfoContentBuilder

2b0aa54

Push logic to compute VS image Id from glyphs to Features layer

bcac320

Move helpers that convert to Roslyn LSP elements to language server

0f49c9f

Remove ILspHoverResultCreationService

581f55c

With the logic to produce rich Quick Info content in the Features layer, this service is no longer and its logic can be moved into HoverHandler.

Remove ILspReferencesResultCreationService

0d92e27

Remove ILspSymbolInformationCreationService

a086f84

Remove ILspCompletionResultCreationService

18c234b

Add GetHoverAsync(...) to ExternalAccess.Razor

8901af8

Add GetCompletionListAsync and ResolveCompletionItemAsync to Razor EA

76c324f

DustinCampbell requested a review from a team as a code owner September 24, 2024 23:25

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Sep 24, 2024

dotnet-policy-service bot added the VSCode label Sep 24, 2024

DustinCampbell requested review from davidwengier and dibarbet September 24, 2024 23:26

Fix formatting

a1fb88d

CyrusNajmabadi reviewed Sep 25, 2024

View reviewed changes

src/Features/Core/Portable/Common/GlyphExtensions.KnownImageIds.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Sep 25, 2024

View reviewed changes

src/Features/Core/Portable/QuickInfo/Presentation/QuickInfoContentBuilder.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Sep 25, 2024

View reviewed changes

DustinCampbell commented Sep 25, 2024

View reviewed changes

DustinCampbell added 2 commits September 25, 2024 12:55

Move Glyph.GetVsImageData() extension method to LanguageServer.Protocol

206199f

Per code review feedback, this extension shouldn't be defined in the Features layer. Instead, it can move to LanguageServer.Protocol and be leveraged from the EditorFeatures layer.

Return FrozenDictionary from CreateGlyphToElementMap

a60dd72

DustinCampbell force-pushed the razor-cohosting-hover branch from 06451df to a60dd72 Compare September 25, 2024 19:55

dibarbet approved these changes Sep 25, 2024

View reviewed changes

davidwengier approved these changes Sep 26, 2024

View reviewed changes

DustinCampbell added 3 commits September 26, 2024 10:37

Update shared handler methods to take IGlobalOptionService

87b859f

Remove unnecessary commented code

51842c9

Merge branch 'main' into razor-cohosting-hover

b7b74aa

build-analysis bot mentioned this pull request Sep 26, 2024

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

3 tasks

Merge branch 'main' into razor-cohosting-hover

fc5687b

DustinCampbell merged commit 7c66b29 into dotnet:main Sep 30, 2024
25 checks passed

DustinCampbell deleted the razor-cohosting-hover branch September 30, 2024 18:09

dotnet-policy-service bot added this to the Next milestone Sep 30, 2024

This was referenced Oct 1, 2024

Broken NGEN in 4.13 #75323

Closed

Revert "Move Quick Info content production to the Features layer and add Razor EA co-hosting APIs for Hover and Completion" #75329

Closed

This was referenced Oct 9, 2024

[Automated] PRs inserted in VS build feature.debugger.main-35407.82 #75445

Closed

[Automated] PRs inserted in VS build main-35407.57 #75446

Closed

[Automated] PRs inserted in VS build feature.debugger.shadowDebug-35410.215 #75481

Closed

akhera99 modified the milestones: Next, 17.13 P1 Oct 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Quick Info content production to the Features layer and add Razor EA co-hosting APIs for Hover and Completion #75226

Move Quick Info content production to the Features layer and add Razor EA co-hosting APIs for Hover and Completion #75226

DustinCampbell commented Sep 24, 2024

CyrusNajmabadi commented Sep 25, 2024

CyrusNajmabadi Sep 25, 2024

DustinCampbell Sep 25, 2024

DustinCampbell Sep 25, 2024 •

edited

Loading

DustinCampbell Sep 25, 2024 •

edited

Loading

DustinCampbell Sep 25, 2024

DustinCampbell Sep 25, 2024

DustinCampbell Sep 25, 2024

davidwengier left a comment

	private static async Task NavigateToQuickInfoTargetAsync(
	string navigationTarget,
	Workspace workspace,
	DocumentId documentId,
	IThreadingContext threadingContext,
	IUIThreadOperationExecutor operationExecutor,
	IAsynchronousOperationListener asyncListener,
	IStreamingFindUsagesPresenter streamingPresenter)
	{
	try
	{
	using var token = asyncListener.BeginAsyncOperation(nameof(NavigateToQuickInfoTargetAsync));
	using var context = operationExecutor.BeginExecute(EditorFeaturesResources.IntelliSense, EditorFeaturesResources.Navigating, allowCancellation: true, showProgress: false);

	var cancellationToken = context.UserCancellationToken;
	var solution = workspace.CurrentSolution;
	SymbolKeyResolution resolvedSymbolKey;
	try
	{
	var project = solution.GetRequiredProject(documentId.ProjectId);
	var compilation = await project.GetRequiredCompilationAsync(cancellationToken).ConfigureAwait(false);
	resolvedSymbolKey = SymbolKey.ResolveString(navigationTarget, compilation, cancellationToken: cancellationToken);
	}
	catch
	{
	// Ignore symbol resolution failures. It likely is just a badly formed URI.
	return;
	}

	if (resolvedSymbolKey.GetAnySymbol() is { } symbol)
	{
	var location = await GoToDefinitionHelpers.GetDefinitionLocationAsync(
	symbol, solution, threadingContext, streamingPresenter, cancellationToken).ConfigureAwait(false);
	await location.TryNavigateToAsync(threadingContext, new NavigationOptions(PreferProvisionalTab: true, ActivateTab: true), cancellationToken).ConfigureAwait(false);
	}
	}
	catch (OperationCanceledException)
	{
	}
	catch (Exception ex) when (FatalError.ReportAndCatch(ex, ErrorSeverity.Critical))
	{
	}
	}


		namespace Microsoft.CodeAnalysis.QuickInfo.Presentation;

		internal static class TaggedTextExtensions


		namespace Microsoft.CodeAnalysis.QuickInfo.Presentation;

		internal static class QuickInfoContentBuilder


		namespace Microsoft.CodeAnalysis.QuickInfo.Presentation;

		internal sealed class QuickInfoOnTheFlyDocsElement(Document document, OnTheFlyDocsInfo info) : QuickInfoElement


		namespace Microsoft.CodeAnalysis.QuickInfo.Presentation;

		internal sealed class QuickInfoContentBuilderContext(

Move Quick Info content production to the Features layer and add Razor EA co-hosting APIs for Hover and Completion #75226

Move Quick Info content production to the Features layer and add Razor EA co-hosting APIs for Hover and Completion #75226

Conversation

DustinCampbell commented Sep 24, 2024

CyrusNajmabadi commented Sep 25, 2024

CyrusNajmabadi Sep 25, 2024

Choose a reason for hiding this comment

DustinCampbell Sep 25, 2024

Choose a reason for hiding this comment

DustinCampbell Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

DustinCampbell Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

DustinCampbell Sep 25, 2024

Choose a reason for hiding this comment

DustinCampbell Sep 25, 2024

Choose a reason for hiding this comment

DustinCampbell Sep 25, 2024

Choose a reason for hiding this comment

davidwengier left a comment

Choose a reason for hiding this comment

DustinCampbell Sep 25, 2024 •

edited

Loading

DustinCampbell Sep 25, 2024 •

edited

Loading