-
Notifications
You must be signed in to change notification settings - Fork 219
Remove excess async state machines in completion #11620
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
Changes from all commits
3a0d65c
ef4a827
b533363
4a33dc1
09542f8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,14 +19,14 @@ namespace Microsoft.AspNetCore.Razor.LanguageServer.Completion; | |
| [RazorLanguageServerEndpoint(Methods.TextDocumentCompletionName)] | ||
| internal class RazorCompletionEndpoint( | ||
| CompletionListProvider completionListProvider, | ||
| CompletionTriggerAndCommitCharacters completionTriggerAndCommitCharacters, | ||
| ITelemetryReporter? telemetryReporter, | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ℹ️ Only tests and benchmarks were calling the |
||
| CompletionTriggerAndCommitCharacters triggerAndCommitCharacters, | ||
| ITelemetryReporter telemetryReporter, | ||
| RazorLSPOptionsMonitor optionsMonitor) | ||
| : IRazorRequestHandler<CompletionParams, VSInternalCompletionList?>, ICapabilitiesProvider | ||
| { | ||
| private readonly CompletionListProvider _completionListProvider = completionListProvider; | ||
| private readonly CompletionTriggerAndCommitCharacters _triggerAndCommitCharacters = completionTriggerAndCommitCharacters; | ||
| private readonly ITelemetryReporter? _telemetryReporter = telemetryReporter; | ||
| private readonly CompletionTriggerAndCommitCharacters _triggerAndCommitCharacters = triggerAndCommitCharacters; | ||
| private readonly ITelemetryReporter _telemetryReporter = telemetryReporter; | ||
| private readonly RazorLSPOptionsMonitor _optionsMonitor = optionsMonitor; | ||
|
|
||
| private VSInternalClientCapabilities? _clientCapabilities; | ||
|
|
@@ -52,46 +52,43 @@ public TextDocumentIdentifier GetTextDocumentIdentifier(CompletionParams request | |
|
|
||
| public async Task<VSInternalCompletionList?> HandleRequestAsync(CompletionParams request, RazorRequestContext requestContext, CancellationToken cancellationToken) | ||
| { | ||
| var documentContext = requestContext.DocumentContext; | ||
|
|
||
| if (request.Context is null || documentContext is null) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| var sourceText = await documentContext.GetSourceTextAsync(cancellationToken).ConfigureAwait(false); | ||
| if (!sourceText.TryGetAbsoluteIndex(request.Position, out var hostDocumentIndex)) | ||
| if (request.Context is not VSInternalCompletionContext completionContext || | ||
| requestContext.DocumentContext is not { } documentContext) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| if (request.Context is not VSInternalCompletionContext completionContext) | ||
| var autoShownCompletion = completionContext.InvokeKind != VSInternalCompletionInvokeKind.Explicit; | ||
| var options = _optionsMonitor.CurrentValue; | ||
| if (autoShownCompletion && !options.AutoShowCompletion) | ||
| { | ||
| Debug.Fail("Completion context should never be null in practice"); | ||
| return null; | ||
| } | ||
|
|
||
| var autoShownCompletion = completionContext.InvokeKind != VSInternalCompletionInvokeKind.Explicit; | ||
| if (autoShownCompletion && !_optionsMonitor.CurrentValue.AutoShowCompletion) | ||
| var sourceText = await documentContext.GetSourceTextAsync(cancellationToken).ConfigureAwait(false); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ℹ️ I've reordered some of the checks to push fast checks ahead of checks that require async work.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do any of these changes apply to the cohost completion endpoint? Though even if they do, I'm fine if you want to leave that for me, I suspect I need to audit that, and compare to this endpoint anyway
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't actually look at the co-hosting endpoint. I was primarily looking at callers of
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The options checking would be in the endpoint, which is why I thought of it, but I just looked and the ordering of the checks there is fine. |
||
| if (!sourceText.TryGetAbsoluteIndex(request.Position, out var hostDocumentIndex)) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| var correlationId = Guid.NewGuid(); | ||
| using var _ = _telemetryReporter?.TrackLspRequest(Methods.TextDocumentCompletionName, LanguageServerConstants.RazorLanguageServerName, TelemetryThresholds.CompletionRazorTelemetryThreshold, correlationId); | ||
| using (_telemetryReporter.TrackLspRequest(Methods.TextDocumentCompletionName, LanguageServerConstants.RazorLanguageServerName, TelemetryThresholds.CompletionRazorTelemetryThreshold, correlationId)) | ||
| { | ||
| var razorCompletionOptions = new RazorCompletionOptions( | ||
| SnippetsSupported: true, | ||
| AutoInsertAttributeQuotes: options.AutoInsertAttributeQuotes, | ||
| CommitElementsWithSpace: options.CommitElementsWithSpace); | ||
|
|
||
| var razorCompletionOptions = new RazorCompletionOptions( | ||
| SnippetsSupported: true, | ||
| AutoInsertAttributeQuotes: _optionsMonitor.CurrentValue.AutoInsertAttributeQuotes, | ||
| CommitElementsWithSpace: _optionsMonitor.CurrentValue.CommitElementsWithSpace); | ||
| var completionList = await _completionListProvider.GetCompletionListAsync( | ||
| hostDocumentIndex, | ||
| completionContext, | ||
| documentContext, | ||
| _clientCapabilities!, | ||
| razorCompletionOptions, | ||
| correlationId, | ||
| cancellationToken).ConfigureAwait(false); | ||
| return completionList; | ||
| return await _completionListProvider | ||
| .GetCompletionListAsync( | ||
| hostDocumentIndex, | ||
| completionContext, | ||
| documentContext, | ||
| _clientCapabilities.AssumeNotNull(), | ||
| razorCompletionOptions, | ||
| correlationId, | ||
| cancellationToken) | ||
| .ConfigureAwait(false); | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Note that this is now the only reason a
DocumentContextis passed through. Perhaps we can find a way to remove this in future.