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.

Sign up for GitHub

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

Jump to bottom

Completion textedits cursor #60499

Closed
wants to merge 6 commits into from
Closed

Completion textedits cursor #60499

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0bdf1c9
Always use text edits in completion for correctness
dibarbet Mar 29, 2022
81243d8
Use LSP snippets to specify cursor location for completion
dibarbet Mar 31, 2022
5ab94d9
Add test
dibarbet Mar 31, 2022
80ba2a1
Update test to specify additional text edit behavior
dibarbet Mar 31, 2022
86bd721
Account for numerous bugs everywhere
dibarbet Apr 1, 2022
6d53689
more bugs...
dibarbet Apr 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
196 changes: 98 additions & 98 deletions src/EditorFeatures/Core/LanguageServer/Handlers/Completion/CompletionHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ namespace Microsoft.CodeAnalysis.LanguageServer.Handler
[Method(LSP.Methods.TextDocumentCompletionName)]
internal class CompletionHandler : IRequestHandler<LSP.CompletionParams, LSP.CompletionList?>
{
internal const string EditRangeSetting = "editRange";

private readonly IGlobalOptionService _globalOptions;
private readonly ImmutableHashSet<char> _csharpTriggerCharacters;
private readonly ImmutableHashSet<char> _vbTriggerCharacters;
Expand Down Expand Up @@ -64,6 +66,7 @@ public CompletionHandler(
{
var document = context.Document;
Contract.ThrowIfNull(document);
Contract.ThrowIfNull(context.Solution);

// C# and VB share the same LSP language server, and thus share the same default trigger characters.
// We need to ensure the trigger character is valid in the document's language. For example, the '{'
Expand All @@ -89,31 +92,27 @@ public CompletionHandler(

var (list, isIncomplete, resultId) = completionListResult.Value;

if (list.IsEmpty)
{
return new LSP.VSInternalCompletionList
{
Items = Array.Empty<LSP.CompletionItem>(),
SuggestionMode = list.SuggestionModeItem != null,
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting an empty list with suggestion mode on is a thing?

IsIncomplete = isIncomplete,
};
}

var lspVSClientCapability = context.ClientCapabilities.HasVisualStudioLspCapability() == true;
var snippetsSupported = context.ClientCapabilities.TextDocument?.Completion?.CompletionItem?.SnippetSupport ?? false;
var itemDefaultsSupported = context.ClientCapabilities.TextDocument?.Completion?.CompletionListSetting?.ItemDefaults?.Contains(EditRangeSetting) == true;
var commitCharactersRuleCache = new Dictionary<ImmutableArray<CharacterSetModificationRule>, string[]>(CommitCharacterArrayComparer.Instance);

// Feature flag to enable the return of TextEdits instead of InsertTexts (will increase payload size).
Contract.ThrowIfNull(context.Solution);
var returnTextEdits = _globalOptions.GetOption(LspOptions.LspCompletionFeatureFlag);

TextSpan? defaultSpan = null;
LSP.Range? defaultRange = null;
if (returnTextEdits)
{
// We want to compute the document's text just once.
documentText = await document.GetTextAsync(cancellationToken).ConfigureAwait(false);

// We use the first item in the completion list as our comparison point for span
// and range for optimization when generating the TextEdits later on.
var completionChange = await completionService.GetChangeAsync(
document, list.Items.First(), cancellationToken: cancellationToken).ConfigureAwait(false);

// If possible, we want to compute the item's span and range just once.
// Individual items can override this range later.
defaultSpan = completionChange.TextChange.Span;
defaultRange = ProtocolConversions.TextSpanToRange(defaultSpan.Value, documentText);
}
// We use the first item in the completion list as our comparison point for span
Copy link
Contributor

Choose a reason for hiding this comment

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

Lol the hack, is this ever not true?

// and range for optimization when generating the TextEdits later on.
var completionChange = await completionService.GetChangeAsync(
document, list.Items.First(), cancellationToken: cancellationToken).ConfigureAwait(false);
var defaultSpan = completionChange.TextChange.Span;
var defaultRange = ProtocolConversions.TextSpanToRange(defaultSpan, documentText);

var supportsCompletionListData = context.ClientCapabilities.HasCompletionListDataCapability();
var completionResolveData = new CompletionResolveData()
Expand All @@ -125,10 +124,19 @@ public CompletionHandler(
foreach (var item in list.Items)
{
var completionItemResolveData = supportsCompletionListData ? null : completionResolveData;
var lspCompletionItem = await CreateLSPCompletionItemAsync(
request, document, item, completionItemResolveData, lspVSClientCapability, commitCharactersRuleCache,
completionService, returnTextEdits, snippetsSupported, stringBuilder, documentText,
defaultSpan, defaultRange, cancellationToken).ConfigureAwait(false);

var lspCompletionItem = lspVSClientCapability ? new LSP.VSInternalCompletionItem() : new LSP.CompletionItem();

// Add basic properties like display text, filter text, icons, etc.
AddRoslynItemProperties(lspCompletionItem, item, stringBuilder, completionItemResolveData);

// Convert the roslyn edits to LSP text edits using the default item span where possible.
await AddTextEditsAsync(lspCompletionItem, item, document, documentText, completionService, snippetsSupported,
itemDefaultsSupported, list.Span, defaultSpan, cancellationToken).ConfigureAwait(false);

// Convert the roslyn commit character rules to LSP commit characters.
AddCommitCharacters(lspCompletionItem, item, commitCharactersRuleCache, lspVSClientCapability);

lspCompletionItems.Add(lspCompletionItem);
}

Expand All @@ -149,6 +157,14 @@ public CompletionHandler(
PromoteCommonCommitCharactersOntoList(completionList);
}

if (itemDefaultsSupported)
{
completionList.ItemDefaults = new LSP.CompletionListItemDefaults
{
EditRange = defaultRange,
};
}

var optimizedCompletionList = new LSP.OptimizedVSCompletionList(completionList);
return optimizedCompletionList;

Expand All @@ -169,101 +185,85 @@ bool IsValidTriggerCharacterForDocument(Document document, char triggerCharacter
return true;
}

static async Task<LSP.CompletionItem> CreateLSPCompletionItemAsync(
LSP.CompletionParams request,
Document document,
CompletionItem item,
CompletionResolveData? completionResolveData,
bool supportsVSExtensions,
Dictionary<ImmutableArray<CharacterSetModificationRule>, string[]> commitCharacterRulesCache,
CompletionService completionService,
bool returnTextEdits,
bool snippetsSupported,
StringBuilder stringBuilder,
SourceText? documentText,
TextSpan? defaultSpan,
LSP.Range? defaultRange,
CancellationToken cancellationToken)
static void AddRoslynItemProperties(LSP.CompletionItem lspItem, CompletionItem roslynItem, StringBuilder stringBuilder, CompletionResolveData? completionResolveData)
{
// Generate display text
stringBuilder.Append(item.DisplayTextPrefix);
stringBuilder.Append(item.DisplayText);
stringBuilder.Append(item.DisplayTextSuffix);
stringBuilder.Append(roslynItem.DisplayTextPrefix);
stringBuilder.Append(roslynItem.DisplayText);
stringBuilder.Append(roslynItem.DisplayTextSuffix);
var completeDisplayText = stringBuilder.ToString();
stringBuilder.Clear();

var completionItem = supportsVSExtensions ? new LSP.VSInternalCompletionItem() : new LSP.CompletionItem();
completionItem.Label = completeDisplayText;
completionItem.SortText = item.SortText;
completionItem.FilterText = item.FilterText;
completionItem.Kind = GetCompletionKind(item.Tags);
completionItem.Data = completionResolveData;
completionItem.Preselect = ShouldItemBePreselected(item);
lspItem.Label = completeDisplayText;
lspItem.SortText = roslynItem.SortText;
lspItem.FilterText = roslynItem.FilterText;
lspItem.Kind = GetCompletionKind(roslynItem.Tags);
lspItem.Data = completionResolveData;
lspItem.Preselect = ShouldItemBePreselected(roslynItem);

if (lspItem is LSP.VSInternalCompletionItem vsCompletionItem)
{
vsCompletionItem.Icon = new ImageElement(roslynItem.Tags.GetFirstGlyph().GetImageId());
}
}

static async Task AddTextEditsAsync(
Copy link
Contributor

Choose a reason for hiding this comment

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

Given this doesn't always add a text edit and occasionally depends on Resolve to do the needful I wonder if there's a better name we could use to represent that

LSP.CompletionItem lspItem,
CompletionItem roslynItem,
Document document,
SourceText documentText,
CompletionService completionService,
bool snippetsSupported,
bool itemDefaultsSupported,
TextSpan listSpan,
TextSpan defaultTextEditSpan,
CancellationToken cancellationToken)
{
// Complex text edits (e.g. override and partial method completions) are always populated in the
// resolve handler, so we leave both TextEdit and InsertText unpopulated in these cases.
if (item.IsComplexTextEdit && completionItem is LSP.VSInternalCompletionItem vsItem)
if (roslynItem.IsComplexTextEdit && lspItem is LSP.VSInternalCompletionItem vsItem)
{
vsItem.VsResolveTextEditOnCommit = true;
// Razor C# is currently the only language client that supports LSP.InsertTextFormat.Snippet.
// We can enable it for regular C# once LSP is used for local completion.
if (snippetsSupported)
{
completionItem.InsertTextFormat = LSP.InsertTextFormat.Snippet;
lspItem.InsertTextFormat = LSP.InsertTextFormat.Snippet;
}
}
// If the feature flag is on, always return a TextEdit.
else if (returnTextEdits)
{
var textEdit = await GenerateTextEdit(
document, item, completionService, documentText, defaultSpan, defaultRange, cancellationToken).ConfigureAwait(false);
completionItem.TextEdit = textEdit;
}
// If the feature flag is off, return an InsertText.
else
{
completionItem.InsertText = SymbolCompletionItem.TryGetInsertionText(item, out var insertionText) ? insertionText : completeDisplayText;
}

var commitCharacters = GetCommitCharacters(item, commitCharacterRulesCache, supportsVSExtensions);
if (commitCharacters != null)
{
completionItem.CommitCharacters = commitCharacters;
}

if (completionItem is LSP.VSInternalCompletionItem vsCompletionItem)
{
vsCompletionItem.Icon = new ImageElement(item.Tags.GetFirstGlyph().GetImageId());
}

return completionItem;

static async Task<LSP.TextEdit> GenerateTextEdit(
Document document,
CompletionItem item,
CompletionService completionService,
SourceText? documentText,
TextSpan? defaultSpan,
LSP.Range? defaultRange,
CancellationToken cancellationToken)
{
Contract.ThrowIfNull(documentText);
Contract.ThrowIfNull(defaultSpan);
Contract.ThrowIfNull(defaultRange);

var completionChange = await completionService.GetChangeAsync(
document, item, cancellationToken: cancellationToken).ConfigureAwait(false);
var completionChangeSpan = completionChange.TextChange.Span;
document, roslynItem, cancellationToken: cancellationToken).ConfigureAwait(false);

var textEdit = new LSP.TextEdit()
if (completionChange.TextChanges.Length > 1 && lspItem is LSP.VSInternalCompletionItem vsCompletionItem)
{
// We have more than one text change. This means the edit is broken up into a main edit around the
// cursor location and additional edits.
// Due to https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1513155 in VS we cannot provide the additional text edits
// on resolve only as the client may not call us. Instead we provide nothing and force the client to resolve us.
// This is not ideal for filtering as we have to rely on the client inferred range, but is currently the best we can do.
//
// We also cannot provide the additional edits and the main text edit upfront as Razor currently cannot performantly
Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest, I'm not sure we'd want to anyways. Additional Text Edits for every item is expensive serialization wise 😄

// map the additional text edits for all the completion items.
// Tracking issue: https://github.com/dotnet/razor-tooling/issues/6242
Copy link
Contributor

Choose a reason for hiding this comment

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

This razor-tooling issue is closed now 😁

vsCompletionItem.VsResolveTextEditOnCommit = true;
}
else
{
NewText = completionChange.TextChange.NewText ?? "",
Range = completionChangeSpan == defaultSpan.Value
? defaultRange
: ProtocolConversions.TextSpanToRange(completionChangeSpan, documentText),
};
// We have a single main edit, populate the completion item edits.
CompletionResolveHandler.AddTextEdits(lspItem, completionChange, documentText, listSpan, itemDefaultsSupported ? defaultTextEditSpan : null, snippetsSupported);
}
}
}

return textEdit;
static void AddCommitCharacters(LSP.CompletionItem lspItem, CompletionItem roslynItem,
Dictionary<ImmutableArray<CharacterSetModificationRule>, string[]> commitCharacterRulesCache, bool supportsVSExtensions)
{
var commitCharacters = GetCommitCharacters(roslynItem, commitCharacterRulesCache, supportsVSExtensions);
if (commitCharacters != null)
{
lspItem.CommitCharacters = commitCharacters;
}
}

Expand Down
Loading