FindAllReferences on Deconstruct should consider all documents #24223
Conversation
|
i'm a bit confused in how any of this worked before. where was the code that was searching for deconstructs previously? |
|
n/m. i see how it was working before. |
|
@dotnet/roslyn-ide for review. A 15.6 fix. Thanks |
| @@ -93,7 +93,11 @@ protected override bool CanFind(IMethodSymbol symbol) | |||
| ? await FindDocumentsWithForEachStatementsAsync(project, documents, cancellationToken).ConfigureAwait(false) | |||
| : ImmutableArray<Document>.Empty; | |||
|
|
|||
| return ordinaryDocuments.Concat(forEachDocuments); | |||
| var deconstructDocuments = IsDeconstructMethod(methodSymbol) | |||
There was a problem hiding this comment.
❓ Can/should we further narrow this down to cases where the parameter list only contains out parameters (or for static methods, only out parameters after a this parameter)? #Resolved
There was a problem hiding this comment.
We could, but I'm not sure it's worth it. The purpose here is to prune potential documents and this simple heuristic is effective for that purpose.
For comparison, we apply a similar (simplistic) heuristic for detecting possible enumerator methods (see IsForEachMethod a few lines below), without checkout parameters or return type.
I feel that's ok to leave as-is.
In reply to: 161816905 [](ancestors = 161816905)
| @@ -394,6 +394,15 @@ protected Task<ImmutableArray<Document>> FindDocumentsWithForEachStatementsAsync | |||
| }, cancellationToken); | |||
| } | |||
|
|
|||
| protected Task<ImmutableArray<Document>> FindDocumentsWithDeconstructionAsync(Project project, IImmutableSet<Document> documents, CancellationToken cancellationToken) | |||
There was a problem hiding this comment.
💡 Consider refactoring these two similar methods to use a helper that takes a Func<WhateverTheIndexTypeIs, bool>. At this point I wouldn't move the helper to a different class or anything, but it does help show the common pattern for this type of document lookup.
|
@Pilchie Please approve for 15.6 ask-mode. Thanks |
Customer scenario
Use FindAllReferences on a
Deconstructmethod. Only references in the same document will be found, but all documents (that are indexed as having at least one deconstruction) should be considered.Bugs this fixes
Fixes #24184
Workarounds, if any
None
Risk
Performance impact
Low. This is following the design of FindAllReferences for
GetEnumeratormethods very closely. The new code is only invoked when using FAR on a method calledDeconstruct.Is this a regression from a previous update?
No.
Root cause analysis
I should have included a test case with multiple documents, and also done some testing with dev hive on Roslyn codebase.
How was the bug found?
FAR for Deconstruct has not shipped yet (I implemented it in 15.6, in PR #22934) and I found this problem while using it on Roslyn codebase.
Verified the fix on Roslyn:
