Add GoToDefinition for XAML LSP by LinglingTong · Pull Request #55296 · dotnet/roslyn

LinglingTong · 2021-07-30T21:07:39Z

This PR enables GoToDefinition for LSP XAML. The GoToDefinitionHandler uses IXamlGoToDefinitionService, which will be implemented in XAML language service to get the definitions of the given document and position. There are two types of XamlDefinition we are getting from the XAML language service:

XamlSourceDefinition has the file path and textSpan (or line and column). This is usually the case when the definition is in a XAML file.
XamlSymbolDefinition contains just the symbol. We then use Roslyn to find the locations of the given symbol.

LinglingTong · 2021-07-30T21:39:20Z

@mgoertz-msft @dibarbet Could you please help to take a look?

In reply to: 890167800

dibarbet · 2021-07-30T21:51:53Z

...lStudio/Xaml/Impl/Implementation/LanguageServer/Handler/Definitions/GoToDefinitionHandler.cs

+        {
+            var locations = ArrayBuilder<LSP.Location>.GetInstance();
+
+            if (definition is XamlSourceDefinition sourceDefinition)


would recommend breaking up this method a little bit so it's easier to read, e.g. have methods to handle each kind of definition (HandleSourceDefinition, HandleSymbolDefinition

dibarbet · 2021-07-30T23:08:55Z

...lStudio/Xaml/Impl/Implementation/LanguageServer/Handler/Definitions/GoToDefinitionHandler.cs

+
+        public override async Task<LSP.Location[]> HandleRequestAsync(TextDocumentPositionParams request, RequestContext context, CancellationToken cancellationToken)
+        {
+            var locations = ArrayBuilder<LSP.Location>.GetInstance();


can use arrabuilder like this - https://sourceroslyn.io/#Microsoft.CodeAnalysis.LanguageServer.Protocol/Handler/Diagnostics/WorkspacePullDiagnosticHandler.cs,64 and then just to ToArray() on it instead of ToArrayAndFree()

dibarbet · 2021-07-30T23:09:12Z

...lStudio/Xaml/Impl/Implementation/LanguageServer/Handler/Definitions/GoToDefinitionHandler.cs

+
+        private async Task<LSP.Location[]> GetLocationsAsync(XamlDefinition definition, RequestContext context, CancellationToken cancellationToken)
+        {
+            var locations = ArrayBuilder<LSP.Location>.GetInstance();


same comment as above on ArrayBuilder usage

mgoertz-msft · 2021-07-30T23:20:27Z

...lStudio/Xaml/Impl/Implementation/LanguageServer/Handler/Definitions/GoToDefinitionHandler.cs

+            }
+
+            var position = await document.GetPositionFromLinePositionAsync(ProtocolConversions.PositionToLinePosition(request.Position), cancellationToken).ConfigureAwait(false);
+            var xamlGoToDefinitionService = document.Project.LanguageServices.GetService<IXamlGoToDefinitionService>();


Looks like it could move down to before line 65 #Closed

mgoertz-msft · 2021-07-30T23:21:52Z

...lStudio/Xaml/Impl/Implementation/LanguageServer/Handler/Definitions/GoToDefinitionHandler.cs

+
+            var definitions = await xamlGoToDefinitionService.GetDefinitionsAsync(document, position, cancellationToken).ConfigureAwait(false);
+            foreach (var definition in definitions)
+            {


Optimize with Parallel.ForEach or Task.WhenAll? #Closed

I switched to use Task.WhenAll, and I also changed the locations to ConcurrentBag type so it is thread safe

mgoertz-msft · 2021-07-30T23:24:27Z

...lStudio/Xaml/Impl/Implementation/LanguageServer/Handler/Definitions/GoToDefinitionHandler.cs

+                    // Cannot find the file in solution. This is probably a file lives outside of the solution like generic.xaml
+                    // which lives in the Windows SDK folder. Try getting the SourceText from the file path.
+                    using var fileStream = new FileStream(sourceDefinition.FilePath, FileMode.Open, FileAccess.Read);
+                    var sourceText = SourceText.From(fileStream);


What's the best way to handle error conditions here? For example if the file doesn't exist or cannot be opened for some reason.

Just tried out giving it an atbitrary file path and let it throw. Don't see any error diaglog/indicator from the UI. I am thinking about let it throw if anything wrong with opening the document so we can get the exception from Telemetry late, but yeah the downside of it would be the remaining code will not execute either (if there are more definitions). @dibarbet how does csharp handle partial success for go to definition?

wouldn't this be a full failure - you have no location to go to? Depending on the kind of failure we either throw or just return no definitions.

If you do not expect to hit an exception, I would leave it as throwing so you can capture telemetry on it. If an exception here is normal and not unexpected, then it should probably be handled. Throwing here is OK, the request queue should handle the exception and report a nfw since it isn't a mutating request.

Thanks, David. That's what I am expecting. BTW do you know if there will be a dialog when fail to go to definition for LSP just like what we have in classic VS? Or no indication for failure is the design going forward?

mgoertz-msft · 2021-07-30T23:29:32Z

src/VisualStudio/Xaml/Impl/Features/Definitions/XamlSourceDefinition.cs

+    internal sealed class XamlSourceDefinition : XamlDefinition
+    {
+        private readonly TextSpan? _span;
+        private readonly int _line, _character;


_character

_column #Closed

mgoertz-msft · 2021-07-30T23:32:17Z

...lStudio/Xaml/Impl/Implementation/LanguageServer/Handler/Definitions/GoToDefinitionHandler.cs

+                Contract.ThrowIfNull(symbolDefinition.Symbol);
+                var symbol = symbolDefinition.Symbol;
+
+                if (symbol.Locations.First().IsInMetadata && _metadataAsSourceFileService != null)


symbol.Locations.First()

@dibarbet IS the first location always the definition or is there a better way to get the location for a symbol definition?

Yeah we usually use first. that being said this is what we do in non-LSP and we probably need to update our handler to do something similar
https://sourceroslyn.io/#Microsoft.VisualStudio.LanguageServices/Implementation/Workspace/VisualStudioSymbolNavigationService.cs,67

mgoertz-msft · 2021-07-30T23:33:05Z

...lStudio/Xaml/Impl/Implementation/LanguageServer/Handler/Definitions/GoToDefinitionHandler.cs

+                Contract.ThrowIfNull(symbolDefinition.Symbol);
+                var symbol = symbolDefinition.Symbol;
+
+                if (symbol.Locations.First().IsInMetadata && _metadataAsSourceFileService != null)


&& _metadataAsSourceFileService != null

Not necessary. MEF creation will fail w/o it #Closed

mgoertz-msft · 2021-08-03T14:19:28Z

src/VisualStudio/Xaml/Impl/Features/Definitions/XamlSourceDefinition.cs

+            _span = span;
+        }
+
+        public XamlSourceDefinition(string filePath, int line, int character)


character

column

mgoertz-msft · 2021-08-03T14:19:39Z

src/VisualStudio/Xaml/Impl/Features/Definitions/XamlSourceDefinition.cs

+            // Convert the line column to TextSpan
+            if (_line < text.Lines.Count)
+            {
+                var character = Math.Min(_column, text.Lines[_line].Span.Length);


character

column

mgoertz-msft · 2021-08-03T14:27:45Z

...lStudio/Xaml/Impl/Implementation/LanguageServer/Handler/Definitions/GoToDefinitionHandler.cs

+            {
+                var task = Task.Run(async () =>
+                {
+                    foreach(var location in await this.GetLocationsAsync(definition, context, cancellationToken).ConfigureAwait(false))


foreach(

insert space

mgoertz-msft · 2021-08-03T14:29:03Z

...lStudio/Xaml/Impl/Implementation/LanguageServer/Handler/Definitions/GoToDefinitionHandler.cs

+            }
+            else
+            {
+                throw new InvalidOperationException("Unexpected XamlDefinition Type");


XamlDefinition

nameof(XamlDefinition)

mgoertz-msft

Add GoToDefinitionHandler for XAML

3fc70d4

LinglingTong requested a review from a team as a code owner July 30, 2021 21:07

ghost added the Area-IDE label Jul 30, 2021

dibarbet approved these changes Jul 30, 2021

View reviewed changes

mgoertz-msft reviewed Jul 30, 2021

View reviewed changes

address code review feedback

2e68720

mgoertz-msft reviewed Aug 3, 2021

View reviewed changes

LinglingTong added 2 commits August 3, 2021 09:35

fix formatting and some names

973fd36

more code review feedbacks

a1001cd

mgoertz-msft approved these changes Aug 4, 2021

View reviewed changes

mgoertz-msft merged commit e011274 into dotnet:main Aug 4, 2021

ghost added this to the Next milestone Aug 4, 2021

dibarbet modified the milestones: Next, 17.0.P4 Aug 31, 2021

Conversation

LinglingTong commented Jul 30, 2021

Uh oh!

LinglingTong commented Jul 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgoertz-msft Jul 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgoertz-msft Jul 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgoertz-msft Jul 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgoertz-msft Jul 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgoertz-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LinglingTong commented Jul 30, 2021 •

edited

Loading

mgoertz-msft Jul 30, 2021 •

edited

Loading

mgoertz-msft Jul 30, 2021 •

edited

Loading

mgoertz-msft Jul 30, 2021 •

edited

Loading

mgoertz-msft Jul 30, 2021 •

edited

Loading