-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid rooting document instances in NavigateToSearchResult #68068
Changes from 1 commit
ae18d23
92459b8
092a9ec
a9f7d94
60bcec9
78e6e08
35dcfa7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ | |
using Microsoft.CodeAnalysis.Text.Shared.Extensions; | ||
using Microsoft.VisualStudio.Imaging.Interop; | ||
using Microsoft.VisualStudio.Language.NavigateTo.Interfaces; | ||
using Microsoft.VisualStudio.Shell.Interop; | ||
using Microsoft.VisualStudio.Text; | ||
using Microsoft.VisualStudio.Utilities; | ||
using Roslyn.Utilities; | ||
|
@@ -63,7 +64,7 @@ private ReadOnlyCollection<DescriptionItem> CreateDescriptionItems() | |
return new List<DescriptionItem>().AsReadOnly(); | ||
} | ||
|
||
var sourceText = document.GetTextSynchronously(CancellationToken.None); | ||
var sourceText = document.GetTextSynchronously(document.Workspace.CurrentSolution, CancellationToken.None); | ||
var span = NavigateToUtilities.GetBoundedSpan(_searchResult.NavigableItem, sourceText); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ick. i never noticed this before. This really violates a core principle of navto (that no work is done client side to avoid costly UI perf). This is on the UI thread and goes and potentially does IO. Can you make issue that says that server should compute and add this info to items (ideally only if "show descriptions" is selected by user), and the client just has to present. no need for GetTextSynchronously then. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ➡️ #68104 |
||
|
||
var items = new List<DescriptionItem> | ||
|
@@ -111,13 +112,13 @@ public int GetProvisionalViewingStatus() | |
var document = _searchResult.NavigableItem.Document; | ||
if (document == null) | ||
{ | ||
return 0; | ||
return (int)__VSPROVISIONALVIEWINGSTATUS.PVS_Disabled; | ||
} | ||
|
||
var workspace = document.Project.Solution.Workspace; | ||
var workspace = document.Workspace; | ||
var previewService = workspace.Services.GetService<INavigateToPreviewService>(); | ||
|
||
return previewService.GetProvisionalViewingStatus(document); | ||
return (int)previewService.GetProvisionalViewingStatus(document); | ||
} | ||
|
||
public void PreviewItem() | ||
|
@@ -128,7 +129,7 @@ public void PreviewItem() | |
return; | ||
} | ||
|
||
var workspace = document.Project.Solution.Workspace; | ||
var workspace = document.Workspace; | ||
sharwell marked this conversation as resolved.
Show resolved
Hide resolved
|
||
var previewService = workspace.Services.GetService<INavigateToPreviewService>(); | ||
|
||
previewService.PreviewItem(this); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,9 +2,12 @@ | |
// The .NET Foundation licenses this file to you under the MIT license. | ||
// See the LICENSE file in the project root for more information. | ||
|
||
#nullable disable | ||
|
||
using System; | ||
using System.Collections.Generic; | ||
using System.Collections.Immutable; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Microsoft.CodeAnalysis.Shared.Extensions; | ||
using Microsoft.CodeAnalysis.Text; | ||
|
||
namespace Microsoft.CodeAnalysis.Navigation | ||
|
@@ -33,7 +36,7 @@ internal interface INavigableItem | |
/// </summary> | ||
bool IsImplicitlyDeclared { get; } | ||
|
||
Document Document { get; } | ||
NavigableDocument Document { get; } | ||
sharwell marked this conversation as resolved.
Show resolved
Hide resolved
|
||
TextSpan SourceSpan { get; } | ||
|
||
/// <summary> | ||
|
@@ -45,5 +48,39 @@ internal interface INavigableItem | |
bool IsStale { get; } | ||
|
||
ImmutableArray<INavigableItem> ChildItems { get; } | ||
|
||
public record NavigableDocument(NavigableProject Project, string Name, string? FilePath, IReadOnlyList<string> Folders, DocumentId Id) | ||
{ | ||
public required Workspace Workspace { get; init; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why have some methods be parameters, and some be required props? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ➡️ Moved all to parameters |
||
|
||
public static NavigableDocument FromDocument(Document document) | ||
=> new(NavigableProject.FromProject(document.Project), document.Name, document.FilePath, document.Folders, document.Id) { Workspace = document.Project.Solution.Workspace }; | ||
|
||
internal async ValueTask<Document> GetDocumentAsync(Solution solution, CancellationToken cancellationToken) | ||
{ | ||
if (solution.GetDocument(Id) is { } document) | ||
return document; | ||
|
||
return await solution.GetRequiredDocumentAsync(Id, includeSourceGenerated: true, cancellationToken).ConfigureAwait(false); | ||
sharwell marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
internal async Task<SourceText> GetTextAsync(Solution solution, CancellationToken cancellationToken) | ||
{ | ||
var document = await GetDocumentAsync(solution, cancellationToken).ConfigureAwait(false); | ||
return await document.GetTextAsync(cancellationToken).ConfigureAwait(false); | ||
} | ||
|
||
internal SourceText GetTextSynchronously(Solution solution, CancellationToken cancellationToken) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's teh scenario where the text is needed synchronously? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ➡️ This method contains an extra parameter ( |
||
{ | ||
var document = solution.GetRequiredDocument(Id); | ||
return document.GetTextSynchronously(cancellationToken); | ||
} | ||
} | ||
|
||
public record struct NavigableProject(string Name, ProjectId Id) | ||
{ | ||
public static NavigableProject FromProject(Project project) | ||
=> new(project.Name, project.Id); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -5,8 +5,10 @@ | |||||||||||||||||||||||||||
#nullable disable | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
using System.Collections.Immutable; | ||||||||||||||||||||||||||||
using System.Runtime.CompilerServices; | ||||||||||||||||||||||||||||
using Microsoft.CodeAnalysis.Shared.Extensions; | ||||||||||||||||||||||||||||
using Microsoft.CodeAnalysis.Text; | ||||||||||||||||||||||||||||
using Roslyn.Utilities; | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
namespace Microsoft.CodeAnalysis.Navigation | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
|
@@ -17,6 +19,7 @@ private class SymbolLocationNavigableItem : INavigableItem | |||||||||||||||||||||||||||
private readonly Solution _solution; | ||||||||||||||||||||||||||||
private readonly ISymbol _symbol; | ||||||||||||||||||||||||||||
private readonly Location _location; | ||||||||||||||||||||||||||||
private StrongBox<INavigableItem.NavigableDocument> _lazyDocument; | ||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe doc this? That null means, uncomputed, but we could compute and still point at null? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ⏱️ Enabling nullable reference types on this file to improve clarity There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ➡️ Ended up documenting this, but not changing the nullability. There is a discrepancy in nullability which existed prior to this work, and I opted to defer fixing that. |
||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
public SymbolLocationNavigableItem( | ||||||||||||||||||||||||||||
Solution solution, | ||||||||||||||||||||||||||||
|
@@ -38,8 +41,21 @@ public SymbolLocationNavigableItem( | |||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
public bool IsImplicitlyDeclared => _symbol.IsImplicitlyDeclared; | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
public Document Document | ||||||||||||||||||||||||||||
=> _location.IsInSource ? _solution.GetDocument(_location.SourceTree) : null; | ||||||||||||||||||||||||||||
public INavigableItem.NavigableDocument Document | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
get | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
return LazyInitialization.EnsureInitialized( | ||||||||||||||||||||||||||||
ref _lazyDocument, | ||||||||||||||||||||||||||||
static self => | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
return (self._location.IsInSource && self._solution.GetDocument(self._location.SourceTree) is { } document) | ||||||||||||||||||||||||||||
? INavigableItem.NavigableDocument.FromDocument(document) | ||||||||||||||||||||||||||||
: null; | ||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is concerning. We can return null, but the API says it's not null. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not clear which item you are referring to (there are two return values in question). This overload of roslyn/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/LazyInitialization.cs Lines 89 to 101 in 806dba8
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Api is: If this is NRT annotated, something is wrong :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ➡️ This file does not have null annotations enabled currently. |
||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||
this); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
public TextSpan SourceSpan => _location.SourceSpan; | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't love exposing this API directly. Can we have our own enum to wrap?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
➡️ This non-public API is tightly coupled to Visual Studio, so wrapping seems unnecessary. In addition to duplicating values that already exist, it makes it more difficult to find documentation and other references to the same structure/values.