Conversation
ryzngard
left a comment
There was a problem hiding this comment.
overall LGTM. More questions for my learning than anything.
|
|
||
| if (!codeDocument.Source.Text.TryGetAbsoluteIndex(position, out var hostDocumentIndex)) | ||
| { | ||
| return NoFurtherHandling; |
There was a problem hiding this comment.
❓: Where is this defined? I'm assuming it's something similar to RemoteResponse<null>?
There was a problem hiding this comment.
Yeah, Its on RemoteResponse: https://github.com/dotnet/razor/blob/main/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Remote/RemoteResponse.cs#L14
Brought in by the using static above. In my VS is bold, which makes it look less magical.
| return NoFurtherHandling; | ||
| } | ||
|
|
||
| // Finally, call into C#. |
| ? referenceItem.Location | ||
| : result.Second; | ||
|
|
||
| if (location is null) |
There was a problem hiding this comment.
What's the use case for the location being null here? I would have assumed it just didn't get added to the results?
There was a problem hiding this comment.
Honestly, I've no idea, but referenceItem.Location is annotated as nullable so this defensiveness is just to make the compiler happy. I suspect this falls out as a quirk in the VS LSP client, where the underlying VS type doesn't have to specify a location, because navigation can be done by a callback, but callbacks aren't part of LSP.
| referenceItem.DisplayPath = mappedUri.AbsolutePath; | ||
| referenceItem.DocumentName = mappedUri.AbsolutePath; |
There was a problem hiding this comment.
I'm only alive by caffeine right now so excuse my ignorance. Is DisplayName and DocumentName supposed to be user friendly in some way? Wouldn't we want something to manipulate this from the AbsolutePath? Or is that just how it is?
There was a problem hiding this comment.
The docs here are not really clear, so I just looked at what was returned by Roslyn, which was full paths, and took that as gospel. I couldn't find any visual evidence in VS that the values are used. The docs say things like DisplayPath is for when "the actual path would be confusing", but whether that means we're doing the right thing because generated document paths are confusing, or the wrong thing because full paths are confusing, is entirely up to individual interpretation.
|
|
||
| namespace Microsoft.CodeAnalysis.Remote.Razor; | ||
|
|
||
| internal sealed class RemoteFindAllReferencesService(in ServiceArgs args) : RazorDocumentServiceBase(in args), IRemoteFindAllReferencesService |
There was a problem hiding this comment.
❓ This is the pattern everywhere but I'm not sure I get it. Is it just to avoid making copies of the struct?
There was a problem hiding this comment.
This is code written by Dustin, inspired by code written by Tomas, so I'm going to go out on a limb and say "Yes" 😁
Fixes #11237
Needs dotnet/roslyn#76002 and a version bump
Very simple one, though I was a little cheeky. There is basically no code that could be shared, except for code that removes
__oandk_BackingFieldfrom the results, but those methods operate on VS LSP types. Given cohosting largely uses Roslyn LSP types, and will be exclusively FUSE, and FUSE doesn't have__o, I just skipped that bit. Confirmed in VS that having cohosting and FUSE on makes for nice looking results with no generated code artifacts.Side note: I don't think anything will ever have
k_BackingFieldin the results, but there is zero test coverage so who knows! 😁