-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Send back icons for FAR via LSP #47882
Changes from 18 commits
fe82ec6
8b9f26d
d66e4e5
e30456b
3abf5c2
c703f1a
0b60e89
c5312b1
a805ec6
1213208
711e42d
b206219
9f10b94
20bf6f1
263c416
eab8465
e380fe6
51bd904
0f09454
9b05079
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 |
---|---|---|
|
@@ -41,6 +41,12 @@ public FindAllReferencesHandler(IMetadataAsSourceFileService metadataAsSourceFil | |
return Array.Empty<LSP.VSReferenceItem>(); | ||
} | ||
|
||
// We only support streaming results back, so no point doing any work if the client doesn't | ||
if (referenceParams.PartialResultToken == null) | ||
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. Once pull diagnostics goes in we should switch this to use the progress implementation there. 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. 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. Woops, closed mine as dupe! |
||
{ | ||
return Array.Empty<LSP.VSReferenceItem>(); | ||
} | ||
|
||
var findUsagesService = document.GetRequiredLanguageService<IFindUsagesLSPService>(); | ||
var position = await document.GetPositionFromLinePositionAsync( | ||
ProtocolConversions.PositionToLinePosition(referenceParams.Position), cancellationToken).ConfigureAwait(false); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,9 +4,13 @@ | |
|
||
#nullable disable | ||
|
||
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Microsoft.CodeAnalysis.Editor.Shared.Extensions; | ||
using Microsoft.VisualStudio.Text.Adornments; | ||
using Roslyn.Test.Utilities; | ||
using Xunit; | ||
using LSP = Microsoft.VisualStudio.LanguageServer.Protocol; | ||
|
@@ -45,7 +49,44 @@ void M2() | |
Assert.Equal("M", results[1].ContainingMember); | ||
Assert.Equal("M2", results[3].ContainingMember); | ||
|
||
AssertValidDefinitionProperties(results, 0); | ||
AssertValidDefinitionProperties(results, 0, Glyph.FieldPublic); | ||
} | ||
|
||
[WpfFact(Skip = "https://github.com/dotnet/roslyn/issues/43063")] | ||
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. does this need to be skipped still? 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 can't see the build results in the linked issue to see why they failed, but with the handler only supporting streaming I can't see how they ever passed, so was planning on unskipping when adding non-streaming support. |
||
public async Task TestFindAllReferencesAsync_Class() | ||
{ | ||
var markup = | ||
@"class {|reference:A|} | ||
{ | ||
public int someInt = 1; | ||
void M() | ||
{ | ||
var i = someInt + 1; | ||
} | ||
} | ||
class B | ||
{ | ||
int someInt = {|reference:A|}.someInt + 1; | ||
void M2() | ||
{ | ||
var j = someInt + {|caret:|}{|reference:A|}.someInt; | ||
} | ||
}"; | ||
using var workspace = CreateTestWorkspace(markup, out var locations); | ||
|
||
var results = await RunFindAllReferencesAsync(workspace.CurrentSolution, locations["caret"].First()); | ||
AssertLocationsEqual(locations["reference"], results.Select(result => result.Location)); | ||
|
||
var textElement = results[0].Text as ClassifiedTextElement; | ||
Assert.NotNull(textElement); | ||
var actualText = string.Concat(textElement.Runs.Select(r => r.Text)); | ||
|
||
Assert.Equal("class A", actualText); | ||
Assert.Equal("B", results[1].ContainingType); | ||
Assert.Equal("B", results[2].ContainingType); | ||
Assert.Equal("M2", results[2].ContainingMember); | ||
|
||
AssertValidDefinitionProperties(results, 0, Glyph.ClassInternal); | ||
} | ||
|
||
[WpfFact(Skip = "https://github.com/dotnet/roslyn/issues/43063")] | ||
|
@@ -80,7 +121,7 @@ void M2() | |
Assert.Equal("M", results[1].ContainingMember); | ||
Assert.Equal("M2", results[3].ContainingMember); | ||
|
||
AssertValidDefinitionProperties(results, 0); | ||
AssertValidDefinitionProperties(results, 0, Glyph.FieldPublic); | ||
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 we want some test for something other than FieldPublic? |
||
} | ||
|
||
[WpfFact] | ||
|
@@ -116,12 +157,13 @@ void M() | |
Assert.NotNull(results[0].Location.Uri); | ||
} | ||
|
||
private static LSP.ReferenceParams CreateReferenceParams(LSP.Location caret) => | ||
private static LSP.ReferenceParams CreateReferenceParams(LSP.Location caret, IProgress<object> progress) => | ||
new LSP.ReferenceParams() | ||
{ | ||
TextDocument = CreateTextDocumentIdentifier(caret.Uri), | ||
Position = caret.Range.Start, | ||
Context = new LSP.ReferenceContext(), | ||
PartialResultToken = progress | ||
}; | ||
|
||
private static async Task<LSP.VSReferenceItem[]> RunFindAllReferencesAsync(Solution solution, LSP.Location caret) | ||
|
@@ -131,17 +173,23 @@ private static LSP.ReferenceParams CreateReferenceParams(LSP.Location caret) => | |
SupportsVisualStudioExtensions = true | ||
}; | ||
|
||
var progress = new ProgressCollector<LSP.VSReferenceItem>(); | ||
|
||
var queue = CreateRequestQueue(solution); | ||
return await GetLanguageServer(solution).ExecuteRequestAsync<LSP.ReferenceParams, LSP.VSReferenceItem[]>(queue, LSP.Methods.TextDocumentReferencesName, | ||
CreateReferenceParams(caret), vsClientCapabilities, null, CancellationToken.None); | ||
await GetLanguageServer(solution).ExecuteRequestAsync<LSP.ReferenceParams, LSP.VSReferenceItem[]>(queue, LSP.Methods.TextDocumentReferencesName, | ||
CreateReferenceParams(caret, progress), vsClientCapabilities, null, CancellationToken.None); | ||
|
||
return progress.GetItems(); | ||
} | ||
|
||
private static void AssertValidDefinitionProperties(LSP.ReferenceItem[] referenceItems, int definitionIndex) | ||
private static void AssertValidDefinitionProperties(LSP.VSReferenceItem[] referenceItems, int definitionIndex, Glyph definitionGlyph) | ||
{ | ||
var definition = referenceItems[definitionIndex]; | ||
var definitionId = definition.DefinitionId; | ||
Assert.NotNull(definition.DefinitionText); | ||
|
||
Assert.Equal(definitionGlyph.GetImageId(), definition.DefinitionIcon.ImageId); | ||
|
||
for (var i = 0; i < referenceItems.Length; i++) | ||
{ | ||
if (i == definitionIndex) | ||
|
@@ -150,9 +198,22 @@ private static void AssertValidDefinitionProperties(LSP.ReferenceItem[] referenc | |
} | ||
|
||
Assert.Null(referenceItems[i].DefinitionText); | ||
Assert.Equal(0, referenceItems[i].DefinitionIcon.ImageId.Id); | ||
Assert.Equal(definitionId, referenceItems[i].DefinitionId); | ||
Assert.NotEqual(definitionId, referenceItems[i].Id); | ||
} | ||
} | ||
|
||
private sealed class ProgressCollector<T> : IProgress<object> | ||
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. Making a note here to switch this as well in #48350 |
||
{ | ||
private readonly List<T> _items = new List<T>(); | ||
|
||
public T[] GetItems() => _items.ToArray(); | ||
|
||
public void Report(object value) | ||
{ | ||
_items.AddRange((T[])value); | ||
} | ||
} | ||
} | ||
} |
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.
TriggerCharacter being
null
in a proper functioning LSP client results in completions when typing out an identifier. I don't think returningnull
here is correct.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.
Also, to be clear, Razor translates the LSP platform's requests into spec abiding LSP requests which ends up hitting this call path. If you checked this in it'd stop returning C# identifier completions in Razor scenarios 😢
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.
I think for now we can just match the spec and say invoked if we get null. If we actually get null, this will result in completions coming up unexpectedly (as invoked is generally more aggressive). But VS is currently passing the trigger char 'incorrectly' so we'll only hit this in Razor right now.
Then we can followup with a longterm fix from https://github.com/microsoft/ms-lsp/issues/57#issuecomment-703863301 (probably some kind of new property)
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.
I've updated the PR to what I believe is spec-compliant behaviour, specifically in
0f09454
(#47882). PTAL.