Send back icons for FAR via LSP #47882

davidwengier · 2020-09-21T05:49:44Z

Commit-by-commit makes sense here, as most of the work was in responding to nullability changes in Microsoft.VisualStudio.LangaugeServer.Protocol.
Thanks Sam for the first commit :D

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

...tudio/Xaml/Impl/Implementation/LanguageServer/Handler/Completion/CompletionResolveHandler.cs

src/Features/LanguageServer/Protocol/Extensions/ProtocolConversions.cs

davidwengier · 2020-09-22T04:36:27Z

Retargeting this to 16.8, FYI @jinujoseph

davidwengier · 2020-09-24T01:31:40Z

@dibarbet should this be targeting a different branch? I noticed you mentioned Cyrus' PR should be retargeted, and that one and this one will now have conflicts, but they won't show up until a later merge.

dibarbet · 2020-09-24T01:36:40Z

@davidwengier I think it should target release/dev16.8-vs-deps since we're moving the editor packages to 16.8.

Once 16.8 ships we'll merge the 16.8-vs-deps branch back into non-vs-deps

…lReferencesIcons

src/Workspaces/Remote/ServiceHub/Services/LanguageServer/LanguageServer.cs

src/VisualStudio/Core/Def/Implementation/LanguageClient/InProcLanguageServer.cs

jasonmalinowski · 2020-09-28T19:28:45Z

src/VisualStudio/Core/Test.Next/Services/LspDiagnosticsTests.cs

@@ -423,7 +423,7 @@ static DiagnosticData CreateMockDiagnosticDataWithMappedLocation(Document docume
                var diagnostic = Diagnostic.Create(descriptor, location);
                return new DiagnosticData(diagnostic.Id,
                    diagnostic.Descriptor.Category,
-                    null,
+                    message: "",


See earlier question: are we sure we don't have to deal with diagnostics with no message?

src/Features/LanguageServer/ProtocolUnitTests/References/FindAllReferencesHandlerTests.cs

jasonmalinowski · 2020-09-28T19:36:03Z

src/Features/LanguageServer/ProtocolUnitTests/References/FindAllReferencesHandlerTests.cs

@@ -78,7 +81,7 @@ void M2()
            Assert.Equal("M", results[1].ContainingMember);
            Assert.Equal("M2", results[3].ContainingMember);

-            AssertValidDefinitionProperties(results, 0);
+            AssertValidDefinitionProperties(results, 0, Glyph.FieldPublic);


Do we want some test for something other than FieldPublic?

jasonmalinowski · 2020-09-28T19:43:08Z

src/EditorFeatures/TestUtilities/LanguageServer/AbstractLanguageServerProtocolTests.cs

                {
                    DisplayText = text,
                    TextDocument = requestParameters.TextDocument,
                    Position = requestParameters.Position
-                },
+                }),
+                Icon = tags != null ? new ImageElement(tags.ToImmutableArray().GetFirstGlyph().GetImageId()) : null,


Not using tags.ToImmutableArray().GetFirstGlyph().GetImageElement()? You added the extension method... 😄

🤦‍♂️
I think I need to start this PR again.. its been rebased so many times to different branches I don't even know what is real any more

NTaylorMullen · 2020-10-05T05:51:21Z

src/Features/LanguageServer/Protocol/Handler/Completion/CompletionHandler.cs

@@ -51,6 +51,11 @@ internal class CompletionHandler : IRequestHandler<LSP.CompletionParams, LSP.Com
                return null;
            }

+            if (request.Context?.TriggerCharacter == null)
+            {
+                return null;


TriggerCharacter being null in a proper functioning LSP client results in completions when typing out an identifier. I don't think returning null here is correct.

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 😢

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)

I've updated the PR to what I believe is spec-compliant behaviour, specifically in 0f09454 (#47882). PTAL.

dibarbet · 2020-10-06T00:43:39Z

src/Features/LanguageServer/Protocol/Handler/References/FindAllReferencesHandler.cs

@@ -41,6 +41,12 @@ public async Task<LSP.VSReferenceItem[]> HandleRequestAsync(ReferenceParams refe
                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)


Once pull diagnostics goes in we should switch this to use the progress implementation there.
Filed #48350

I already logged #48150 (and #48151) for this :P
I started working on, but need to make changes to the BufferedProgress to make it thread safe, so yes, was waiting for pull diagnostics to be in first.

Woops, closed mine as dupe!

dibarbet · 2020-10-06T00:46:25Z

src/Features/LanguageServer/ProtocolUnitTests/References/FindAllReferencesHandlerTests.cs

+            AssertValidDefinitionProperties(results, 0, Glyph.FieldPublic);
+        }
+
+        [WpfFact(Skip = "https://github.com/dotnet/roslyn/issues/43063")]


does this need to be skipped still?

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.

dibarbet · 2020-10-06T00:48:25Z

src/Features/LanguageServer/ProtocolUnitTests/References/FindAllReferencesHandlerTests.cs

                Assert.Equal(definitionId, referenceItems[i].DefinitionId);
                Assert.NotEqual(definitionId, referenceItems[i].Id);
            }
        }
+
+        private sealed class ProgressCollector<T> : IProgress<object>


Making a note here to switch this as well in #48350

ghost · 2020-10-07T03:51:25Z

Hello @davidwengier!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost

Auto-approval

davidwengier added the Area-IDE label Sep 21, 2020

davidwengier requested review from a team as code owners September 21, 2020 05:49

dibarbet reviewed Sep 21, 2020

View reviewed changes

davidwengier changed the base branch from master to release/dev16.8 September 22, 2020 04:34

jinujoseph added the Needs Shiproom Approval label Sep 22, 2020

davidwengier changed the base branch from release/dev16.8 to release/dev16.8-vs-deps September 24, 2020 07:41

CyrusNajmabadi and others added 7 commits September 24, 2020 17:51

Nrt tweaks

fe82ec6

Revert

8b9f26d

Revert

d66e4e5

Revert

e30456b

Return definition icons for FindAllReference requests

3abf5c2

Unify types for completion resolve data

c703f1a

Move Glyph helper method

0b60e89

davidwengier force-pushed the LSPFindAllReferencesIcons branch from ef7b160 to 0b60e89 Compare September 24, 2020 07:55

davidwengier and others added 3 commits September 24, 2020 19:12

Update Dependency

c5312b1

Nullable changes

a805ec6

Nullable

1213208

This was referenced Sep 24, 2020

Test failures via timeline in Roslyn jaredpar/runfo#37

Closed

Test failures via timeline in Roslyn jaredpar/runfo#38

Closed

davidwengier added 2 commits September 25, 2020 06:19

Nullable

711e42d

Fix tests

b206219

davidwengier changed the base branch from release/dev16.8-vs-deps to master-vs-deps September 25, 2020 06:57

davidwengier removed the Needs Shiproom Approval label Sep 25, 2020

Merge remote-tracking branch 'upstream/master-vs-deps' into LSPFindAl…

9f10b94

…lReferencesIcons

davidwengier force-pushed the LSPFindAllReferencesIcons branch from a9802d5 to 9f10b94 Compare September 25, 2020 10:17

davidwengier requested a review from a team September 28, 2020 07:27

jasonmalinowski reviewed Sep 28, 2020

View reviewed changes

davidwengier added 5 commits September 29, 2020 12:46

PR feedback

20bf6f1

Add test

263c416

Merge branch 'master-vs-deps' into LSPFindAllReferencesIcons

eab8465

Fix bad merge, and nullable change

e380fe6

Nullable

51bd904

NTaylorMullen reviewed Oct 5, 2020

View reviewed changes

Handle completion in line with the LSP spec

0f09454

dibarbet approved these changes Oct 6, 2020

View reviewed changes

This was referenced Oct 6, 2020

Agents canceling during dotnet builds jaredpar/runfo#47

Closed

Azure Agent timeout #48030

Closed

Merge branch 'master-vs-deps' into LSPFindAllReferencesIcons

9b05079

davidwengier added the auto-merge label Oct 7, 2020

ghost approved these changes Oct 7, 2020

View reviewed changes

ghost merged commit bf030da into dotnet:master-vs-deps Oct 7, 2020

ghost added this to the Next milestone Oct 7, 2020

Cosifne modified the milestones: Next, 16.9.P1 Oct 12, 2020

davidwengier deleted the LSPFindAllReferencesIcons branch January 11, 2021 05:41

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send back icons for FAR via LSP #47882

Send back icons for FAR via LSP #47882

davidwengier commented Sep 21, 2020 •

edited by Pilchie

Loading

davidwengier commented Sep 22, 2020

davidwengier commented Sep 24, 2020

dibarbet commented Sep 24, 2020 •

edited

Loading

jasonmalinowski Sep 28, 2020

jasonmalinowski Sep 28, 2020

jasonmalinowski Sep 28, 2020

davidwengier Sep 29, 2020

NTaylorMullen Oct 5, 2020 •

edited

Loading

NTaylorMullen Oct 5, 2020

dibarbet Oct 5, 2020

davidwengier Oct 6, 2020

dibarbet Oct 6, 2020

davidwengier Oct 6, 2020

dibarbet Oct 6, 2020

dibarbet Oct 6, 2020

davidwengier Oct 6, 2020

dibarbet Oct 6, 2020

ghost commented Oct 7, 2020

ghost left a comment

Send back icons for FAR via LSP #47882

Send back icons for FAR via LSP #47882

Conversation

davidwengier commented Sep 21, 2020 • edited by Pilchie Loading

davidwengier commented Sep 22, 2020

davidwengier commented Sep 24, 2020

dibarbet commented Sep 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NTaylorMullen Oct 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Oct 7, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost left a comment

Choose a reason for hiding this comment

davidwengier commented Sep 21, 2020 •

edited by Pilchie

Loading

dibarbet commented Sep 24, 2020 •

edited

Loading

NTaylorMullen Oct 5, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.