Support go to definition from metadata source #883

filipw · 2017-06-02T16:35:27Z

This is an attempt to fix #876
I am not sure if this is the best way to do it, but I wanted to fit into the existing endpoint structure to avoid breaking changes.

The current flow of the client trying to go to metadata is the following

Invoke /gotodefinition. The server code recognizes the symbol location is in metadata, and uses Roslyn's MetadataAsSourceService to generate a temporary document a provide basic metadata information to the caller - such as the position of the symbol in the metadata source. No metadata source is returned though.
Invoke /metadata and pass in the meta information (i.e. TypeName, AssemblyName). The server will then once again call into MetadataAsSourceService, generate a temporary document again, and return the source text of that document, along with its temporary name.
The client stitches these pieces of information together - displays the metadata source text, with its temporary name, and positions the user correctly based on the location obtained from the original /gotodefinition call.

It's impossible at the moment to continue the navigation from metadata to other metadata sources, because that would require going through /gotodefinition endpoint again, and it doesn't support starting points that are not a document inside the workspace.

This PR changes the above flow in the following, non-breaking way

We still start by invoking /gotodefinition. The code recognizes the symbol location is in metadata, and uses Roslyn's MetadataAsSourceService to generate a temporary document a provide basic metadata information to the caller - such as the position of the symbol in the metadata source. No metadata source is returned though. The difference is that the temporary document document containing metadata source is stored in memory for future reuse (not added to current workspace, just stored on a side in a dictionary).
Invoke /metadata and pass in the meta information (i.e. TypeName, AssemblyName). The server now doesn't call into MetadataAsSourceService again (so we have a perf boost), but rather it will use the cached document and return the source text of that document, along with its temporary name.
The client stitches these pieces of information together - displays the metadata source text, with its temporary name, and positions the user correctly based on the location obtained from the original /gotodefinition call.
The client can now call /gotodefinition and ask for anything based on the location in the metadata source document - that is because the /gotodefinition endpoint has also been modified to not only work with documents that are part of Workspace but also with metadata sources using the aformentioned metadata cache to look up the sources. From there the process goes back to point 1, and we can have a neverending navigation cycle between metadata documents.

Let me know what you think, and whether this approach makes sense - we can absolutely change it to something better.
I also specifically avoided storing the metadata source documents inside the workspace as that started impacting a lot of other endpoints in an unwelcome way (i.e. go to symbol).

david-driscoll

Couple minor changes, otherwise 👍 .

Expanding to include perhaps find usages, and type info would be cool, but certainly not needed on the first pass.

david-driscoll · 2017-06-02T17:22:02Z

src/OmniSharp.Roslyn.CSharp/Services/Navigation/GotoDefinitionService.cs

-            var document = _workspace.GetDocument(request.FileName);
+            var document = request.IsMetadataFile() ? 
+                _metadataHelper.FindDocumentInMetadataCache(request.FileName) : 
+                _workspace.GetDocument(request.FileName);


This could technically work for other endpoints as well right? Thinking of /findusages, /typelookup and maybe even /highlight.

Would be great if we could simplify this.

We could probably just make it _metadataHelper.FindDocumentInMetadataCache(request.FileName) ?? _workspace.GetDocument(request.FileName); to avoid the first if there.

yes this approach should work for other endpoints too

david-driscoll · 2017-06-02T17:25:49Z

src/OmniSharp.Roslyn/MetadataHelper.cs

+            }
+
+            Document metadataDocument = null;
+            if (!_metadataDocumentCache.TryGetValue(fileName, out metadataDocument))


could use out var metadataDocument

Definitely. 😄

david-driscoll · 2017-06-02T17:26:46Z

src/OmniSharp.Roslyn/OmniSharpWorkspace.cs

@@ -127,6 +127,8 @@ public IEnumerable<Document> GetDocuments(string filePath)

        public Document GetDocument(string filePath)
        {
+            if (filePath == null) return null;


Should probably use string.IsNullOrEmpty()

Or string.IsNullOrWhitespace(...)

DustinCampbell

Looks good! Just a few "clean up" suggestions. Also, the CI failure looks like a real test failure.

DustinCampbell · 2017-06-05T18:22:06Z

src/OmniSharp.Roslyn/MetadataHelper.cs

        }

-        public Task<Document> GetDocumentFromMetadata(Project project, ISymbol symbol, CancellationToken cancellationToken = new CancellationToken())
+        public async Task<MetadataDocumentResult> GetAndAddDocumentFromMetadata(Project project, ISymbol symbol, CancellationToken cancellationToken = new CancellationToken())


What a beautiful place to use a C# 7 tuple!

DustinCampbell · 2017-06-05T18:22:15Z

src/OmniSharp.Roslyn/MetadataHelper.cs

+            }
+
+            Document metadataDocument = null;
+            if (!_metadataDocumentCache.TryGetValue(fileName, out metadataDocument))


Definitely. 😄

DustinCampbell · 2017-06-05T18:23:27Z

src/OmniSharp.Roslyn/MetadataHelper.cs

-            var topLevelSymbol = GetTopLevelContainingNamedType(symbol);
+            var fileName = GetFilePathForSymbol(project, symbol);
+
+            Project metadataProject = null;


Tiny, tiny, minuscule nit: I don't think you need = null here because `metadataProject is definitely assigned before it is used below.

DustinCampbell · 2017-06-05T18:24:32Z

src/OmniSharp.Roslyn/OmniSharpWorkspace.cs

@@ -127,6 +127,8 @@ public IEnumerable<Document> GetDocuments(string filePath)

        public Document GetDocument(string filePath)
        {
+            if (filePath == null) return null;


Or string.IsNullOrWhitespace(...)

DustinCampbell · 2017-06-05T18:24:41Z

tests/OmniSharp.Roslyn.CSharp.Tests/FindReferencesFacts.cs

@@ -6,6 +6,8 @@
 using TestUtility;
 using Xunit;
 using Xunit.Abstractions;
+using OmniSharp.Models.GotoDefinition;


Sort usings?

DustinCampbell · 2017-06-05T18:28:50Z

tests/OmniSharp.Roslyn.CSharp.Tests/GoToDefinitionFacts.cs

+                var metadataTree = CSharpSyntaxTree.ParseText(metadataResponse.Source);
+                var iComparable = metadataTree.GetCompilationUnitRoot().DescendantNodesAndSelf().OfType<BaseTypeDeclarationSyntax>().
+                    First().BaseList.Types.FirstOrDefault(x => x.Type.ToString() == "IComparable");
+


Nit: extra blank line

DustinCampbell · 2017-06-05T18:29:57Z

tests/OmniSharp.Roslyn.CSharp.Tests/GoToDefinitionFacts.cs

+        [Theory]
+        [InlineData("bar.cs")]
+        [InlineData("bar.csx")]
+        public async Task ReturnsDefinitionInMetadata_FromMetadata_WhenSymbolIsType(string filename)


Again, unifying the pattern for calling each handler would be helpful for a minor readability improvement.

Thanks for the comments though! Very helpful!

DustinCampbell · 2017-06-05T18:31:01Z

tests/OmniSharp.Roslyn.CSharp.Tests/GoToDefinitionFacts.cs

+                    FileName = testFile.FileName,
+                    Line = point.Line,
+                    Column = point.Offset,
+                    Timeout = 60000,


Is it possible to call this endpoint without a timeout?

DustinCampbell · 2017-06-05T18:31:38Z

tests/OmniSharp.Roslyn.CSharp.Tests/GoToDefinitionFacts.cs

+                // use the source to locate "IComparable" which is an interface implemented by Int32 struct
+                var metadataTree = CSharpSyntaxTree.ParseText(metadataResponse.Source);
+                var iComparable = metadataTree.GetCompilationUnitRoot().DescendantNodesAndSelf().OfType<BaseTypeDeclarationSyntax>().
+                    First().BaseList.Types.FirstOrDefault(x => x.Type.ToString() == "IComparable");


Would love to see this broken down onto separate lines for readability.

DustinCampbell · 2017-06-05T19:22:52Z

src/OmniSharp.Roslyn/MetadataHelper.cs

                    .WithCompilationOptions(project.CompilationOptions)
-                    .WithMetadataReferences(project.MetadataReferences)
-                : project;
+                    .WithMetadataReferences(project.MetadataReferences);


Should this be indented?

filipw · 2017-06-06T12:47:26Z

thanks for the comments 👍 and sorry for the delay (long weekend) 🌴
I'll make the changes you mentioned

filipw · 2017-06-06T13:57:00Z

I addressed the comments, squashed the commits to remove the noise I managed to generate and rebased this on top of the latest dev.

DustinCampbell

LGTM

DustinCampbell · 2017-06-10T15:33:58Z

Are you good here @filipw? If so, I think you can merge this once CI completes again.

filipw · 2017-06-10T15:58:27Z

I think so!

filipw requested review from DustinCampbell and david-driscoll June 2, 2017 16:35

david-driscoll requested changes Jun 2, 2017

View reviewed changes

DustinCampbell approved these changes Jun 5, 2017

View reviewed changes

filipw force-pushed the feature/go-to-metadata-2 branch from d68e040 to 2a8be67 Compare June 6, 2017 13:55

filipw added 2 commits June 6, 2017 15:55

added support for go to definition for metadata files

f302015

addressed PR review feedback

2a8be67

removed unnecessary using

f57c98c

filipw force-pushed the feature/go-to-metadata-2 branch 2 times, most recently from dd627bd to c317826 Compare June 6, 2017 14:03

better naming and formatting in tests

c317826

DustinCampbell approved these changes Jun 6, 2017

View reviewed changes

david-driscoll approved these changes Jun 6, 2017

View reviewed changes

filipw and others added 3 commits June 7, 2017 08:11

Merge branch 'dev' into feature/go-to-metadata-2

645f28f

Merge branch 'dev' into feature/go-to-metadata-2

a83e33f

Merge branch 'dev' into feature/go-to-metadata-2

65ecfe7

DustinCampbell merged commit 4fa5de5 into OmniSharp:dev Jun 12, 2017

filipw mentioned this pull request Jul 3, 2017

Support go to definition from metadata dotnet/vscode-csharp#1620

Merged

razzmatazz mentioned this pull request Jul 16, 2017

go-to-definition in metadata OmniSharp/omnisharp-emacs#334

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support go to definition from metadata source #883

Support go to definition from metadata source #883

filipw commented Jun 2, 2017

david-driscoll left a comment

david-driscoll Jun 2, 2017

david-driscoll Jun 2, 2017

filipw Jun 6, 2017

david-driscoll Jun 2, 2017

DustinCampbell Jun 5, 2017

david-driscoll Jun 2, 2017

DustinCampbell Jun 5, 2017

DustinCampbell left a comment •

edited

Loading

DustinCampbell Jun 5, 2017

DustinCampbell Jun 5, 2017

DustinCampbell Jun 5, 2017

DustinCampbell Jun 5, 2017

DustinCampbell Jun 5, 2017

DustinCampbell Jun 5, 2017

DustinCampbell Jun 5, 2017

DustinCampbell Jun 5, 2017

DustinCampbell Jun 5, 2017

DustinCampbell Jun 5, 2017

DustinCampbell Jun 5, 2017

filipw commented Jun 6, 2017

filipw commented Jun 6, 2017

DustinCampbell left a comment

DustinCampbell commented Jun 10, 2017

filipw commented Jun 10, 2017

Support go to definition from metadata source #883

Support go to definition from metadata source #883

Conversation

filipw commented Jun 2, 2017

The current flow of the client trying to go to metadata is the following

This PR changes the above flow in the following, non-breaking way

david-driscoll left a 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

DustinCampbell left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipw commented Jun 6, 2017

filipw commented Jun 6, 2017

DustinCampbell left a comment

Choose a reason for hiding this comment

DustinCampbell commented Jun 10, 2017

filipw commented Jun 10, 2017

DustinCampbell left a comment •

edited

Loading