Co-hosting: Add API for mapping a generated document URI and range back to the host document #10712
Conversation
This change updates `DocumentContext` is several ways: 1. Fields used for caching are now assigned with `Interlocked.Initialize(...)` to ensure that the same value are used in race scenarios. 2. All public async methods now return `ValueTask` rather than `Task`. This has lower overhead since the async methods will return synchronously after the first call. 3. Members are no longer virtual. This appears to have been originally done to allow `DocumentContext` to be mocked for tests, which isn't the right way to handle testing. Instead, since `DocumentContext` delegates its implementation to its `IDocumentSnapshot`, the snapshot should be mocked to change the implementation. 4. The Identifier property has been converted to a method, since it creates a new object every time.
In order to remove `IDocumentContextFactory` from `AbstractRazorDocumentMappingService`, it's necessary to split `IRazorDocumentMappingService` into different services. The goal is to make `IRazorDocumentMappingService` provide APIs that only perform mapping using Roslyn primitives, such as `LinePosition`, `LinePositionSpan`, and `TextChange`. `IEditMappingService` contains an API moved from `IRazorDocumentMappingService` that remaps a `WorkspaceEdit`.
This allows `EditMappingService` and `AbstractRazorDocumentMappingService` to share a helper method.
This change removes `IDocumentContextFactory` from the `AbstracRazorDocumentMappingService` and replaces the single usage with a new protected abstract method.
This change also removes the `AbstractDocumentMappingService.TryGetCodeDocumentAsync(...)` method that was added in an an early commit.
davidwengier
left a comment
davidwengier
left a comment
There was a problem hiding this comment.
Love the ValueTask local methods in document context! Lovely stuff.
...azor/src/Microsoft.CodeAnalysis.Remote.Razor/DocumentMapping/RemoteDocumentMappingService.cs
Outdated
Show resolved
Hide resolved
...azor/src/Microsoft.CodeAnalysis.Remote.Razor/DocumentMapping/RemoteDocumentMappingService.cs
Outdated
Show resolved
Hide resolved
| CancellationToken cancellationToken) | ||
| { | ||
| throw new NotImplementedException(); | ||
| var razorDocumentUri = FilePathService.GetRazorDocumentUri(generatedDocumentUri); |
There was a problem hiding this comment.
Nothing to do now, but just a note that this feels fragile, and might not be possible with the source generator hooked up. I suspect FilePathService will be useless, if not actively harmful, in cohosting. Having said that, it probably depends on where generatedDocumentUri comes from, because until we have the source generator hooked up the old conventions still apply, but they might end up being false positives.
There was a problem hiding this comment.
I agree. For co-hosting, I don't think we'll want IFilePathService at all. Instead, we should probably be able to determine the original razor document from the SourceGeneratedDocument.
...azor/src/Microsoft.CodeAnalysis.Remote.Razor/DocumentMapping/RemoteDocumentMappingService.cs
Show resolved
Hide resolved
| /// Converts the specified <see cref="Uri"/> into a file path that matches | ||
| /// a Roslyn <see cref="TextDocument.FilePath"/>. | ||
| /// </summary> | ||
| public static string GetDocumentFilePath(this Uri uri) |
There was a problem hiding this comment.
I was expecting this method to only be in remote, so it can't be accidentally used in the Razor LSP, but having "Roslyn" in the comment is probably good enough.
There was a problem hiding this comment.
When I found the same code in CSharpTestLspServerHelpers, I decided that I'd rather just share the method across the code base.
There was a problem hiding this comment.
Oh I missed that, good call!
3 participants
There's a fair amount of refactoring here, but I was careful to separate everything. So, I recommend reviewing commit-by-commit.