Don't hold onto documents in 'Find' features#55635
Don't hold onto documents in 'Find' features#55635CyrusNajmabadi merged 38 commits intodotnet:mainfrom
Conversation
src/EditorFeatures/Core.Cocoa/NavigationCommandHandlers/FindExtensionMethodsCommandHandler.cs
Show resolved
Hide resolved
| return threadingContext.JoinableTaskFactory.Run(() => | ||
| streamingPresenter.TryNavigateToOrPresentItemsAsync( | ||
| threadingContext, workspace, title, definitions, cancellationToken)); | ||
| threadingContext, solution, title, definitions, cancellationToken)); |
There was a problem hiding this comment.
this method went away as it was identical to the one above (which already took a solution).
|
|
||
| var definitionItem = overriddenSymbol.ToNonClassifiedDefinitionItem(document.Project.Solution, true); | ||
| await context.OnDefinitionFoundAsync(definitionItem, cancellationToken).ConfigureAwait(false); | ||
| await context.OnDefinitionFoundAsync(solution, definitionItem, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
instead of passing along a DefinitionItem (or SourceReferenceItem) which holds onto the Solution, we now pass along a lighterweight object with just the DocId and we also pass along the SOlution as a method-param for clients that need it. but that way they can grab what they need and hold onto the DefItem without rooting the solution.
| /// for cases like partial types/members. | ||
| /// </summary> | ||
| public ImmutableArray<DocumentSpan> SourceSpans { get; } | ||
| public ImmutableArray<SerializableDocumentSpan> SourceSpans { get; } |
There was a problem hiding this comment.
this is the core part of the change that ensures that anything holding onto a DefItem doesn't root the solution.
| /// The location of the source item. | ||
| /// </summary> | ||
| public DocumentSpan SourceSpan { get; } | ||
| public SerializableDocumentSpan SourceSpan { get; } |
There was a problem hiding this comment.
this is the otehr part that ensures we're not rooting the solution if someone holds onto a RefItem.
src/Features/Core/Portable/FindUsages/DefinitionItem.DocumentLocationDefinitionItem.cs
Outdated
Show resolved
Hide resolved
src/VisualStudio/Core/Def/Implementation/Workspace/VisualStudioSymbolNavigationService.cs
Outdated
Show resolved
Hide resolved
| [Obsolete("Override CanNavigateToAsync instead", error: false)] | ||
| public abstract bool CanNavigateTo(Workspace workspace, CancellationToken cancellationToken); | ||
| [Obsolete("Override TryNavigateToAsync instead", error: false)] | ||
| public abstract bool TryNavigateTo(Workspace workspace, bool showInPreviewTab, bool activateTab, CancellationToken cancellationToken); |
There was a problem hiding this comment.
would like to remove thse, but they are used by typescript. @DanielRosenwasser can you add appropriate contacts so i can walk you guys through moving to the new async members?
| projectName, | ||
| projectFlavor, | ||
| document.FilePath, | ||
| documentSpan.SourceSpan, |
There was a problem hiding this comment.
i didn't want to padd 'documentSpan' along (in case it got accidentally captured in the future), so i explicitly broke out the data we computed from it and passed it along here insetad.
| ImmutableDictionary<string, string> customColumnsData) | ||
| { | ||
| var document = documentSpan.Document; | ||
| var (guid, projectName, projectFlavor) = GetGuidAndProjectInfo(document); |
There was a problem hiding this comment.
these moved to the caller so we could avoid passing DocumentSpan into this method.
|
|
||
| base.PreprocessNavigate(entry, e); | ||
| await supportsNavigation.TryNavigateToAsync(e.IsPreview, context.UserCancellationToken).ConfigureAwait(false); | ||
| } |
There was a problem hiding this comment.
this logic changed pretty heavily. previously we would navigate synchronously (which coudl block the UI). Now we navigate asynchronously, with a cancellable TWD
...Studio/Core/Def/Implementation/FindReferences/VisualStudioDefinitionsAndReferencesFactory.cs
Outdated
Show resolved
Hide resolved
3418dca to
a7f8ce9
Compare
...Studio/Core/Def/Implementation/FindReferences/VisualStudioDefinitionsAndReferencesFactory.cs
Show resolved
Hide resolved
| protected override Task FindActionAsync(IGoToBaseService service, Document document, int caretPosition, IFindUsagesContext context, CancellationToken cancellationToken) | ||
| => service.FindBasesAsync(document, caretPosition, context, cancellationToken); | ||
|
|
||
| protected override Task<Solution> GetSolutionAsync(Document document, CancellationToken cancellationToken) |
There was a problem hiding this comment.
💡 Often completes synchronously with a non-cached result
| protected override Task<Solution> GetSolutionAsync(Document document, CancellationToken cancellationToken) | |
| protected override ValueTask<Solution> GetSolutionAsync(Document document, CancellationToken cancellationToken) |
| } | ||
| return CanNavigateToMetadataSymbol(solution.Workspace, symbolKey); | ||
|
|
||
| if (await this.SourceSpans[0].TryRehydrateAsync(solution, cancellationToken).ConfigureAwait(false) is not DocumentSpan span) |
There was a problem hiding this comment.
💡 Can use implicit type here
| if (await this.SourceSpans[0].TryRehydrateAsync(solution, cancellationToken).ConfigureAwait(false) is not DocumentSpan span) | |
| if (await this.SourceSpans[0].TryRehydrateAsync(solution, cancellationToken).ConfigureAwait(false) is not { } span) |
|
|
||
| public static SerializableDocumentSpan Dehydrate(DocumentSpan documentSpan) | ||
| => new(documentSpan.Document.Id, documentSpan.SourceSpan); | ||
| public static SerializableDocumentSpan Dehydrate(DocumentIdSpan documentSpan) |
There was a problem hiding this comment.
💡 Can you file a follow-up issue to remove SerializableDocumentSpan? I don't want to clutter this pull request, but it should be unnecessary now.
|
Note: i'd like #55696 to go in first. We can then take this second, but also revert it if it turns out we run into any issues. |
|
going to move this to 17.1 to ensure we have lots of time to validate this. |
Extract async navigation portions of #55635
|
i didn't merge this. investigating what happened here. |
Instead of using a Document+Span as the internal currency for locations, use a DocumentId+Span.
Note: features like FAR did not actually need the Document that was passed along after the initial creation of the entry in the UI.
Fixes AB#1371724