-
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
Conversation
1d0c2af
to
92459b8
Compare
src/Features/Core/Portable/ExternalAccess/VSTypeScript/VSTypeScriptNavigableItemWrapper.cs
Show resolved
Hide resolved
var location = await ProtocolConversions.TextSpanToLocationAsync( | ||
item.Document, item.SourceSpan, item.IsStale, cancellationToken).ConfigureAwait(false); | ||
document, item.SourceSpan, item.IsStale, cancellationToken).ConfigureAwait(false); |
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.
i don't think this is safe anymore. item.IsStale was previously used to say that the result info might not match teh current snapshot. But now that you're not rooting an actual snapshot, it may always be the case that things may be stale (e.g. spans map to things that are now no longer legal).
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.
We use context.Solution
to obtain the document, which is the same solution used for the call to GetItemsFromPreferredSourceLocations
. It seems like this would allow it to be correct.
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 thread is covered by #68068 (comment)
{ | ||
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 comment
The 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 comment
The 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 EnsureInitialized
operates on a StrongBox<T>
specifically because it allows the initialized value to be null.
roslyn/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Utilities/LazyInitialization.cs
Lines 89 to 101 in 806dba8
/// <summary> | |
/// Ensure that the given target value is initialized in a thread-safe manner. This overload supports the | |
/// initialization of value types, and reference type fields where <see langword="null"/> is considered an | |
/// initialized value. | |
/// </summary> | |
/// <typeparam name="T">The type of the target value.</typeparam> | |
/// <param name="target">A target value box to initialize.</param> | |
/// <typeparam name="U">The type of the <paramref name="state"/> argument passed to the value factory.</typeparam> | |
/// <param name="valueFactory">A factory delegate to create a new instance of the target value. Note that this delegate may be called | |
/// more than once by multiple threads, but only one of those values will successfully be written to the target.</param> | |
/// <param name="state">An argument passed to the value factory.</param> | |
/// <returns>The target value.</returns> | |
public static T? EnsureInitialized<T, U>([NotNull] ref StrongBox<T?>? target, Func<U, T?> valueFactory, U state) |
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.
It's not clear which item you are referring to
Api is: public INavigableItem.NavigableDocument Document
which is non-null. my guess is the file is not NRT annotated though. If that's the case, you don't have to change anything.
If this is NRT annotated, something is wrong :)
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 file does not have null annotations enabled currently.
}; | ||
|
||
internal ValueTask<Document> GetDocumentAsync(Solution solution, CancellationToken cancellationToken) | ||
=> solution.GetRequiredDocumentAsync(Id, includeSourceGenerated: IsSourceGeneratedDocument, cancellationToken); |
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 doesn't fele safe. Since we're not rooting the solution, then items may refer to docs that now no longer exist. we should reflect that in this api and make sure downstream consumers are resilient to that happening.
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.
Wouldn't a failure here require the deletion of a document containing a Navigate To result while the Navigate To UI was active? I can try to make it not required though.
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.
➡️ There was only one call to GetDocumentAsync
(directly or indirectly) which didn't pass in the same solution used to create the items in the first place. It was the call to GetTextSynchronously
, which has now been updated to TryGetTextSynchronously
and the caller updated to handle the case where the document was no longer available.
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.
The navto system (now called AIOSP) supports pinning the results, and going back to them and interacting with them post search. So it would be possible to potentially run into a doc that no longer exists. Def wonky for sure. I'm ok if you punt on that. just a note in the code would be good.
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.
i support this given that this passes in the original solution. Can you doc this though that's mandatory for this to be safe.
I think you should also rename this to GetRequiredDocumentAsync to make it clear it will die if not there, and that's a bug in the caller for passing bad data.
If you do the above, i'm fine with this approach :)
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.
➡️ Documented and renamed
return await document.GetValueTextAsync(cancellationToken).ConfigureAwait(false); | ||
} | ||
|
||
internal SourceText GetTextSynchronously(Solution solution, CancellationToken cancellationToken) |
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.
what's teh scenario where the text is needed synchronously?
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 method contains an extra parameter (solution
) relative to the original use, so all calls to this method will appear in the diff if you search for GetTextSynchronously
. There is one call in NavigateToItemDisplay.cs.
|
||
namespace Microsoft.CodeAnalysis.Editor.Implementation.NavigateTo | ||
{ | ||
internal sealed class DefaultNavigateToPreviewService : INavigateToPreviewService | ||
{ | ||
public int GetProvisionalViewingStatus(Document document) | ||
=> 0; | ||
public __VSPROVISIONALVIEWINGSTATUS GetProvisionalViewingStatus(INavigableItem.NavigableDocument document) |
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.
@@ -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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
➡️ #68104
/azp run roslyn-integration-corehost |
Azure Pipelines successfully started running 1 pipeline(s). |
@CyrusNajmabadi this one finally has a passing build. Good to merge? |
@@ -243,14 +244,14 @@ private string ComputeSecondarySort() | |||
int ComputeFolderDistance() | |||
{ | |||
// No need to compute anything if there is no active document. Consider all documents equal. | |||
if (_activeDocument == null) | |||
if (_activeDocument is not { } activeDocument) |
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.
feels unnecessary right? since we did a null check, the later lines below shoudl be safe to refer to _activeDocument, right?
(that's my personal preference, as i always find it harder to reason when there are two varaibles (with almost identical names) pointing to the same value in teh same scope.
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.
➡️ _activeDocument
is Nullable<T>
, but activeDocument
is T
.
public record NavigableDocument(NavigableProject Project, string Name, string? FilePath, IReadOnlyList<string> Folders, DocumentId Id) | ||
{ | ||
public required bool IsSourceGeneratedDocument { get; init; } | ||
public required Workspace Workspace { get; init; } |
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.
why have some methods be parameters, and some be required props?
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.
➡️ Moved all to parameters
internal ValueTask<Document> GetDocumentAsync(Solution solution, CancellationToken cancellationToken) | ||
=> solution.GetRequiredDocumentAsync(Id, includeSourceGenerated: IsSourceGeneratedDocument, cancellationToken); | ||
|
||
internal async ValueTask<SourceText> GetTextAsync(Solution solution, CancellationToken cancellationToken) |
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.
GetRequiredText.
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.
➡️ GetText
is always required in Roslyn.
@@ -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 comment
The 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 comment
The 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 comment
The 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.
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.
Signing off, with the feedback on a few things to touch up. If you disagree, lmk and we can discuss. Primarily it was doc'ing a couple of things, and then using 'Required' in the names of things to convey the expectations. Thanks, this looks great.
Addresses 11.1GB retained set observed in a local heap dump.
Submitted as draft for PR validation.
Follow-up to #68071