Move to a single simple type for internal NavTo results by CyrusNajmabadi · Pull Request #52315 · dotnet/roslyn

CyrusNajmabadi · 2021-03-31T21:22:58Z

This change makes two important changes to how navto internally reports results when searching C# and VB projects.

First, it decouples the result from having an actual Document instance. We want this in the future as we want to be able to search OOP without necessarily hydrating Document values on the remote side (which saves the synchronization cost).

Second, it removes a couple of intermediary conversion steps. THese were left around from multiple rewrites to this area over the years. NOw, we have a simple internal format for a navto result, and we only convert it to the final form we need as the very last step before giving it to the editor.

…ation

CyrusNajmabadi · 2021-03-31T22:07:52Z

src/Features/Core/Portable/NavigateTo/AbstractNavigateToSearchService.InProcess.cs

        public static Task SearchProjectInCurrentProcessAsync(
            Project project, ImmutableArray<Document> priorityDocuments, string searchPattern,
-            IImmutableSet<string> kinds, Func<INavigateToSearchResult, Task> onResultFound, bool isFullyLoaded, CancellationToken cancellationToken)
+            IImmutableSet<string> kinds, Func<RoslynNavigateToItem, Task> onResultFound, bool isFullyLoaded, CancellationToken cancellationToken)


First change is that internally we use this simple data type RoslynNavigateToItem as the only currency for producing and passing along nav-to results.

CyrusNajmabadi · 2021-03-31T22:08:49Z

src/Features/Core/Portable/NavigateTo/AbstractNavigateToSearchService.InProcess.cs

+                kind, matchKind,
+                isCaseSensitive,
+                matchedSpans.ToImmutable(),
+                ComputSecondarySort(declaredSymbolInfo));


the type only holds onto the basic data that is needed to rehydrate the UI at the end. importantly, it also doesn't need a Document to hold onto. A doc-id is sufficiient. This is important for a followup PR where we allow searchign just by doc-ids.

CyrusNajmabadi · 2021-03-31T22:09:06Z

src/Features/Core/Portable/NavigateTo/AbstractNavigateToSearchService.InProcess.cs

+                ComputSecondarySort(declaredSymbolInfo));
+        }
+
+        private static string ComputSecondarySort(DeclaredSymbolInfo declaredSymbolInfo)


these function moved from AbstractNavigateToSearchService.SearchResult.cs, but are otherwise almost hte same.

CyrusNajmabadi · 2021-03-31T22:10:33Z

src/Features/Core/Portable/NavigateTo/AbstractNavigateToSearchService.cs


            await SearchDocumentInCurrentProcessAsync(
-                document, searchPattern, kinds, onResultFound, isFullyLoaded, cancellationToken).ConfigureAwait(false);
+                document, searchPattern, kinds, onItemFound, isFullyLoaded, cancellationToken).ConfigureAwait(false);


both the remote side and the local side only deal with RoslynNavigateToItem internally. Only on the boundary (this method) do we convert it to the INavigateToResult that consumers need.

CyrusNajmabadi · 2021-03-31T22:11:07Z

src/Features/Core/Portable/NavigateTo/NavigateToItem.cs

+        public readonly ImmutableArray<TextSpan> NameMatchSpans;
+
+        [DataMember(Order = 15)]
+        public readonly string SecondarySort;


it's definitely a lot of fields. but all are used.

CyrusNajmabadi · 2021-03-31T22:11:30Z

src/Features/Core/Portable/NavigateTo/NavigateToItem.cs

+                var project = solution.GetProject(DocumentId.ProjectId);
+                if (project != null)
+                    document = project.TryGetSourceGeneratedDocumentForAlreadyGeneratedId(DocumentId);
+            }


@jasonmalinowski is this the right way to map a doc id back to a possibly source-generated document?

Is it possible to just make this path async? Because then you can just call GetDocumentAsync().

And the longer answer: that API I really wish I could mark as friend of the other parts of the workspace so it wasn't available but we don't have that in C#. This might work though, but asyncifying is preferrable.

Is it possible to just make this path async? Because then you can just call GetDocumentAsync().

i don't want this to be async. if it references a source generated document, then we better have that document. i do not want the documents to be generated.

So two things to be aware of:

If this type being serialized across a wire, we might have a source generated document generated in-proc but not OOP yet or vice versa if one side is ahead of the other. We do want to get it updated so a generators only run OOP and in-proc will just pull the contents over, but that would have to be behind an async call.

This method will not return stale results, at all. I don't know what your lifetime model is, but if say project B depends on A, and A is updated, there's no guarantees this will work.

I haven't reviewed the rest of this PR to figure out what's going on and whether this is safe to use. The intent of the API here was only to implement GetDocument() that takes a syntax tree. I've always been switching code over to GetDocumentAsync() in any sort of OOP-rehydration because calling the async method is either:

fast and cheap if the document does exist

won't crash if the document doesn't exist

CyrusNajmabadi · 2021-03-31T22:12:38Z

src/Features/Core/Portable/NavigateTo/NavigateToItem.cs

+    internal readonly struct RoslynNavigateToItem
+    {
+        [DataMember(Order = 0)]
+        public readonly bool IsStale;


stale-ness is a concept that will be in the next pr. currently all results are non-stale. however, in the next PR it will be ok for a stale-result to be returned. the host can then filter it out if that doc doesn't exist in teh current solution. Navigation will also have to be resilient to the span possibly being out of date.

CyrusNajmabadi · 2021-03-31T22:13:38Z

src/Features/Core/Portable/NavigateTo/NavigateToItem.cs

+                        }
+                    }
+                }
+            }


this logic moved from teh main searcher. It depends on actual Document/Project info, so we don't want to do it in OOP, where i want NavigateTo to not depend on those. Instead, it does it on the host side before passing back out to vs.

CyrusNajmabadi · 2021-03-31T22:14:06Z

src/Features/Core/Portable/NavigateTo/NavigateToItem.cs

+
+            Glyph INavigableItem.Glyph => GetGlyph(_item.DeclaredSymbolInfoKind, _item.DeclaredSymbolInfoAccessibility);
+
+            private static Glyph GetPublicGlyph(DeclaredSymbolInfoKind kind)


moved from NavigableItemFactory.DeclaredSymbolNavigableItem.cs

CyrusNajmabadi · 2021-03-31T22:14:19Z

src/Features/Core/Portable/Navigation/NavigableItemFactory.DeclaredSymbolNavigableItem.cs

-{
-    internal partial class NavigableItemFactory
-    {
-        internal class DeclaredSymbolNavigableItem : INavigableItem


another pointless intermediary type. removed.

CyrusNajmabadi · 2021-03-31T22:14:32Z

src/Features/Core/Portable/Remote/RemoteArguments.cs

-    #region NavigateTo
-
-    [DataContract]
-    internal readonly struct SerializableNavigateToSearchResult


more remvoed marshalling types.

sharwell

LGTM on condition that you've tried it in experimental instance on Roslyn for each of the various types we support

src/Features/Core/Portable/NavigateTo/RoslynNavigateToItem.cs

sharwell · 2021-03-31T23:02:36Z

src/Workspaces/Core/Portable/FindSymbols/DeclaredSymbolInfo.cs

+                Intern(stringTable, name),
+                Intern(stringTable, nameSuffix),
+                Intern(stringTable, containerDisplayName),
+                Intern(stringTable, fullyQualifiedContainerName),


❔ Do we not want to move this into the constructor?

no. i wanted to be able to use thsi as a DataContract. so the pure data part is in the tyep/constructor. but the types that create tehse on the OOP side go through teh factory method to share stirngs there.

…ation

CyrusNajmabadi · 2021-04-01T15:55:54Z

@jasonmalinowski is this a better way to do things?

dibarbet

lgtm, but I can't comment on the source generators bit.

CyrusNajmabadi · 2021-04-01T18:54:25Z

src/Features/Core/Portable/NavigateTo/AbstractNavigateToSearchService.InProcess.cs

+                matchedSpans.ToImmutable());
+        }
+
+        private static string ComputeCombinedProjectName(Document document, ImmutableArray<Project> additionalMatchingProjects)


this logic is the same. t he method just moved.

…ation

Move to a single simple type for internal NavTo results

e233c62

CyrusNajmabadi requested review from davidwengier, dibarbet and jasonmalinowski March 31, 2021 21:22

CyrusNajmabadi requested a review from a team as a code owner March 31, 2021 21:22

ghost added the Area-IDE label Mar 31, 2021

CyrusNajmabadi added 3 commits March 31, 2021 14:23

Merge branch 'simplifyNavTo' into simpleNavToSerialization

6f3e25e

Merge remote-tracking branch 'upstream/main' into simpleNavToSerializ…

f5c6199

…ation

Merge branch 'simplifyNavTo' into simpleNavToSerialization

450d6ee

CyrusNajmabadi commented Mar 31, 2021

View reviewed changes

CyrusNajmabadi added 2 commits March 31, 2021 15:42

Simplify more

0f1c081

Simplify more

7734b93

sharwell reviewed Mar 31, 2021

View reviewed changes

CyrusNajmabadi and others added 7 commits March 31, 2021 16:43

Merge remote-tracking branch 'upstream/main' into simpleNavToSerializ…

b8c706b

…ation

NRT

bb60640

Simplify

9313266

Merge remote-tracking branch 'upstream/main' into simpleNavToSerializ…

cc3308e

…ation

PRfeedback

0076c29

Merge remote-tracking branch 'upstream/main' into simpleNavToSerializ…

9e0adbe

…ation

Build

481e4b8

dibarbet approved these changes Apr 1, 2021

View reviewed changes

CyrusNajmabadi commented Apr 1, 2021

View reviewed changes

move project resolution back to host side.

74b4385

CyrusNajmabadi enabled auto-merge April 1, 2021 20:33

Merge remote-tracking branch 'upstream/main' into simpleNavToSerializ…

8a8e7aa

…ation

CyrusNajmabadi mentioned this pull request Apr 1, 2021

Avoid OOP syncing when doing a nav-to search during the initial load of a solution. #52351

Merged

CyrusNajmabadi merged commit b969565 into dotnet:main Apr 2, 2021

ghost added this to the Next milestone Apr 2, 2021

CyrusNajmabadi deleted the simpleNavToSerialization branch April 11, 2021 18:20

dibarbet modified the milestones: Next, 16.10.P3 Apr 26, 2021


		Glyph INavigableItem.Glyph => GetGlyph(_item.DeclaredSymbolInfoKind, _item.DeclaredSymbolInfoAccessibility);

		private static Glyph GetPublicGlyph(DeclaredSymbolInfoKind kind)

Conversation

CyrusNajmabadi commented Mar 31, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Mar 31, 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!

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!

Choose a reason for hiding this comment

Uh oh!

sharwell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Apr 1, 2021

Uh oh!

dibarbet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CyrusNajmabadi Mar 31, 2021 •

edited

Loading