Update FindImplementationsForInterfaceMemberAsync to correctly handle ambiguous type returns#35853
Conversation
src/Workspaces/Core/Portable/Shared/Extensions/ITypeSymbolExtensions.cs
Outdated
Show resolved
Hide resolved
|
Title of this should be changed to more explain what impact this has. |
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
Something seems fishy here.
Request changes
Submit feedback suggesting changes.
|
@CyrusNajmabadi I updated the comment to better describe the issue. |
|
@ryzngard did you push new commits? i'm not seeing them... |
|
@CyrusNajmabadi the only code change looked like the simplification of the null expression, which I hadn't pushed. Let me know if you have more concerns about this. |
|
Any way to write a test for this? |
Working on that now. I've updated the comment a bit more with details, since it still wasn't very clear what the problem was. Trying to get a repro in a test now. |
src/Workspaces/Core/Portable/Shared/Extensions/ITypeSymbolExtensions.cs
Outdated
Show resolved
Hide resolved
| @@ -180,7 +182,7 @@ public static async Task<ImmutableArray<SymbolAndProjectId>> FindImplementations | |||
| interfaceMember, | |||
| solution, | |||
| typeSymbolCompilation, | |||
There was a problem hiding this comment.
so, the input is basically, current symbol (actual implementation) and interface symbol (target interface symbol to match) ? and compilation of each symbol belong to?
wouldn't it be much cleaner if SymbolAndProjectId keep passed down and OriginalSymbolsMatch become Async method?
There was a problem hiding this comment.
by the way, when typeSymbolCompilation is null, VerifyForwardType seems doing this
http://source.roslyn.io/#Microsoft.CodeAnalysis.Workspaces/FindSymbols/SymbolFinder_Hierarchy.cs,508
to handle that case?
you might do the same for symbolToMatch?
I can see case where typeSymbolCompilation is null case?
http://source.roslyn.io/#Microsoft.CodeAnalysis.Workspaces/FindSymbols/FindReferences/Finders/AbstractReferenceFinder.cs,202
There was a problem hiding this comment.
wouldn't it be much cleaner if SymbolAndProjectId keep passed down and OriginalSymbolsMatch become Async method?
Maybe? At this time it'd be a lot of refactoring. I don't feel strongly enough that it would be cleaner, but if it would it should be a separate change.
…yforwardedtypes_null
|
@heejaechang @CyrusNajmabadi is there anything left blocking this? |
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
Nope! LGTM. Thanks!
Approve
Submit feedback approving these changes.
FindImplementationsForInterfaceMemberAsyncchecks for forwarded types usingSymbolFinder.OriginalSymbolsMatch, which expects non-null compilation for both the project containing the interface and the project containing the symbol to compare to. Until now, most instances have found equivalence without falling through, but in cases where the type forwarding isn't completely verified a non-null compilation for the interface symbol is needed as well. See VerifyForwardedTypes for the logic being used.In #35786 the type System.Text.Encoding is provided by a a NuGet reference and could be ambiguous based on the build target. The OOP service attempts to find the correct type resolution but needs the original symbol compilation to verify.
Without this fix, FindAllReferences may cause a null deref and fail.
Fixes #35786