Enable support for an LSP client to open source generated files #68771

jasonmalinowski · 2023-06-26T06:03:29Z

This allows source-generated documents to be opened by an LSP client. A custom URI format is defined which uniquely defines a source generated document by serializing the SourceGeneratedDocumentIdentity; this URI is returned any time we need to navigate to a document or reference the document in any way. A custom request is also defined to fetch the text of a document which can be used for the client; our VS Code extension will implement a TextDocumentContentProvider to call that.

src/EditorFeatures/TestUtilities/LanguageServer/AbstractLanguageServerProtocolTests.cs

src/Features/LanguageServer/Protocol/Extensions/Extensions.cs

CyrusNajmabadi · 2023-07-11T16:27:21Z

src/Features/LanguageServer/Protocol/Extensions/Extensions.cs

        {
-            var documents = solution.GetDocuments(documentIdentifier.Uri);
+            var documents = await solution.GetDocumentsAsync(documentIdentifier.Uri, cancellationToken).ConfigureAwait(false);
            return documents.Length == 0
                ? null
                : documents.FindDocumentInProjectContext(documentIdentifier, (sln, id) => sln.GetRequiredDocument(id));


Suggested change

: documents.FindDocumentInProjectContext(documentIdentifier, (sln, id) => sln.GetRequiredDocument(id));

: documents.FindDocumentInProjectContext(documentIdentifier, static (sln, id) => sln.GetRequiredDocument(id));

nit.

src/Features/LanguageServer/Protocol/Extensions/Extensions.cs

src/Features/LanguageServer/Protocol/Handler/SourceGenerators/GetTextHandler.cs

CyrusNajmabadi · 2023-07-11T16:31:49Z

src/Features/LanguageServer/Protocol/Handler/SourceGenerators/GetTextHandler.cs

+        public bool MutatesSolutionState => false;
+        public bool RequiresLSPSolution => true;
+
+        public LSP.TextDocumentIdentifier GetTextDocumentIdentifier(SourceGeneratorGetTextParams request) => request.TextDocument;


interesting, so we support getting in that uri for the SG doc in the handler itself, and having that map to the Doc that is in the context object? nice.

i'm def not quite understanding the threading/generation model here. Specifically, the client could certainly ask us about a doc that we're no longer gnerating right? If that happens, what's the 'fail' model? Do we throw deep in the server message processing? do we bail gracefully and not execute the request at all? do we synthesize a dummy empty doc? can you clarify?

interesting, so we support getting in that uri for the SG doc in the handler itself, and having that map to the Doc that is in the context object? nice.

Yep, the generated document URI is passed for any document request and we'll return it like anything else. So all other features should hopefully light up with no surprises here.

i'm def not quite understanding the threading/generation model here. Specifically, the client could certainly ask us about a doc that we're no longer gnerating right? If that happens, what's the 'fail' model? Do we throw deep in the server message processing? do we bail gracefully and not execute the request at all? do we synthesize a dummy empty doc? can you clarify?

This part isn't implemented in this PR yet (it's still draft! 😄) But the source generated files we get still produce didOpen/didChange/didClose events like any other opened document. So just like we update the LSP solution to match all the text changes that came in via didChange for regular documents, we can do the same here. We already have support in the core workspace for "treat this source generated file as having this other content" and "force this document to still exist, regardless". So in that case, if the file is still open but no longer being generated, we can just give a solution snapshot to the handler that still contains that document with the client's text. The other option is we just count it as a miscellaneous file at that point. So no matter what, the handler can run, but the document is not quite real in one way or another.

The rest of this is now implemented: when a source generated document is open, we're using the text in the editor for that document in the LSP solution snapshot, so everything lines up.

src/Features/LanguageServer/Protocol/Handler/SourceGenerators/GetTextHandler.cs

src/Features/LanguageServer/Protocol/Handler/SourceGenerators/SourceGeneratorGetTextParams.cs

src/Features/LanguageServer/Protocol/Handler/SourceGenerators/SourceGeneratedDocumentText.cs

CyrusNajmabadi · 2023-07-11T16:34:29Z

src/Features/LanguageServer/Protocol/Handler/SpellCheck/AbstractSpellCheckingHandler.cs

+                        // they should just consider the file deleted and should remove all spans information they've
+                        // cached for it.
+                        progress.Report(CreateReport(requestParams.TextDocument, ranges: null, resultId: null));
+                    }


was this a goof on my part?

question. why merge these? is removing all the old, then adding the new/changed not a nice way to ahndle things?

General cleanup: both functions looped to the same list (although in slightly different ways), both called GetDocument, and then one only operated if the document was null, one if it wasn't. It didn't even cross my mind this might have been an intentional pattern -- it looked like a refactoring that ended up in a silly place and nobody noticed.

...lStudio/Xaml/Impl/Implementation/LanguageServer/Handler/Definitions/GoToDefinitionHandler.cs

src/Workspaces/Core/Portable/Workspace/Workspace_Editor.cs

CyrusNajmabadi · 2023-07-11T16:36:39Z

Looking good overall. Def would like to understand some parts a little better. Feel free to include info here, or just ping me directly :)

dibarbet

just realized its draft, I only ended up looking at part of it

src/Features/LanguageServer/Protocol/Extensions/Extensions.cs

src/Features/LanguageServer/Protocol/Handler/SourceGenerators/GetTextHandler.cs

src/Features/LanguageServer/Protocol/Extensions/Extensions.cs

This method has a flag to throw if the document isn't there and otherwise gracefully handle it not being there; however we called GetRequiredDocument which means we'd throw regardless.

This means any LSP request targeting a source generated document is able to operate as normal.

I don't imagine returning .editorconfigs is a problem here since nothing is really going to take that path, but this makes it more consistent with everything else.

Now it's easier to update both at once.

This will allow us to use other methods for freezing generated documents which rely on having all the information for correctness. This also changes to putting just the hint path into the URI's LocalPath since that displays better in VS Code.

This ensures that when we get a request for source generated document that ranges match up, and ranges pointing to a source generated document also match.

jasonmalinowski · 2024-02-17T02:22:01Z

src/Features/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs

@@ -421,7 +442,7 @@ internal string GetLanguageForUri(Uri uri)
            languageId = trackedDocument.LanguageId;
        }

-        var documentFilePath = ProtocolConversions.GetDocumentFilePathFromUri(uri);
+        var documentFilePath = uri.Scheme == SourceGeneratedDocumentUris.Scheme ? uri.LocalPath : ProtocolConversions.GetDocumentFilePathFromUri(uri);


I decided to put this check here.

I could have put it in GetDocumentFilePathFromUri, since that's where we have the special rule about how file:// URIs work versus other things in documents, but I imagine that would break other things: imagine the case where the client opens some other document and gives it a roslyn-source-generated URI; that we might put in the miscellaneous files workspace just like we do with anything else with a funky URI, and in that case I think we'd end up adding a document with the full URI in the file path. But having this check in GetDocumentFilePathFromUri would break that in various icky ways.

Put another way: this method doesn't return the file path from a URI. It returns the Document.FilePath property which is used in the workspace for that URI, and that's just a different thing.

I decided to put this check in GetDocumentFilePathFromUri

Decided to, or not to? I think I would prefer if this check was handled in GetDocumentFilePathFromUri

@dibarbet Re-reading my comment it made zero sense so I just rewrote it entirely. Let me know if that helps.

@jasonmalinowski Actually just noticed - do we even need to do this check? I see this implementation for GetDocumentFilePathFromUri:

public static string GetDocumentFilePathFromUri(Uri uri) => uri.IsFile ? uri.LocalPath : uri.AbsoluteUri;

Which looks like it should handle the non-file case already

So the non-file case means we use AbsoluteUri which includes the query string. Consider the case of two git files, say git:Foo.cs?version=1234 and git:Foo.cs?version=2345. We need to count those as separate documents in miscellaneous files, so it's correct that GetDocumentFilePathFromUri is using AbsolouteUri in this case.

But you are pointing to a deeper problem I think: GetLanguageForUri should never be calling AbsoluteUri anywhere (even indirectly) since that means if we take that path we're passing in the "?version=1234" into https://github.com/jasonmalinowski/roslyn/blob/f3da440d747e7ec80e97ed8f748dfda529bca23a/src/Features/LanguageServer/Protocol/LanguageInfoProvider.cs#L38 and that doesn't end well. So my change here is wrong, and the existing code is also wrong. GetLanguageForUri should always be using local path or something that drops the query string under the assumption the main part of the file has a useful extension. (and if that assumption isn't right then we're in trouble since at that point the URI itself is untrustworthy.)

I also should take a second look at this -- I fixed this up as a part of some testing and I'm now seeing I probably shouldn't have even gotten there in the first place -- the initial check for URI + language name probably should have worked, I'd imagine? So maybe I have another bug somewhere.

jasonmalinowski · 2024-02-17T02:23:46Z

src/Features/LanguageServer/Protocol/Extensions/Extensions.cs

+        /// <summary>
+        /// Finds the document for a TextDocumentIdentifier, potentially returning a source-generated file.
+        /// </summary>
+        public static async ValueTask<Document?> GetDocumentAsync(this Solution solution, TextDocumentIdentifier documentIdentifier, CancellationToken cancellationToken)


These methods all match the DocumentId-taking methods we have on Solution itself, just these take the identifiers.

Should we remove the GetDocument method? At a glance it seemingly looks like it has different behavior from the non-async version? It isn't checking the URI scheme

Some places don't care about source generated documents so they can still call the other method. But let me check -- an ancient form of this PR did do that approach and I can't recall if it was mostly just tests impacted at this point.

jasonmalinowski · 2024-02-17T02:28:18Z

src/Features/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs

+            var document = await lspSolution.GetTextDocumentAsync(textDocumentIdentifier, cancellationToken).ConfigureAwait(false);
+            if (document != null)


This means we'll catch source generated docs too for request. The async-ness shouldn't actually matter for the most part: if a document is open, we will have forked in the text that's current open in the editor and this should complete quickly.

jasonmalinowski · 2024-02-17T02:30:16Z

...able/Workspace/Solution/SolutionCompilationState.GeneratedFileReplacingCompilationTracker.cs

-                // TODO: This only needs to be implemented if a feature that operates from a source generated file then makes
-                // further mutations to that project, which isn't needed for now. This will be need to be fixed up when we complete
-                // https://github.com/dotnet/roslyn/issues/49533.
-                throw new NotImplementedException();


In VS for Windows, we'd only freeze a single generated document if you invoked a feature from that open document, and other open source generated documents were still being computed even if they were open. For LSP we're stricter here: we'll freeze everything to keep everything in sync, but that means you're more likely to have one of the replacing trackers. So this is something we can hit now.

src/EditorFeatures/TestUtilities/LanguageServer/AbstractLanguageServerProtocolTests.cs

src/Features/LanguageServer/Protocol/Extensions/Extensions.cs

jasonmalinowski · 2024-02-17T02:33:41Z

src/Features/LanguageServer/Protocol/Handler/SourceGenerators/GetTextHandler.cs

+        public bool MutatesSolutionState => false;
+        public bool RequiresLSPSolution => true;
+
+        public LSP.TextDocumentIdentifier GetTextDocumentIdentifier(SourceGeneratorGetTextParams request) => request.TextDocument;


The rest of this is now implemented: when a source generated document is open, we're using the text in the editor for that document in the LSP solution snapshot, so everything lines up.

src/Features/LanguageServer/Protocol/Handler/SourceGenerators/GetTextHandler.cs

...lStudio/Xaml/Impl/Implementation/LanguageServer/Handler/Definitions/GoToDefinitionHandler.cs

dibarbet · 2024-02-21T00:13:41Z

src/Features/LanguageServer/Protocol/Extensions/Extensions.cs

+            if (documentUri.Scheme != SourceGeneratedDocumentUris.Scheme)
+                return solution.GetDocumentIdsWithFilePath(ProtocolConversions.GetDocumentFilePathFromUri(documentUri));
+
+            var documentId = SourceGeneratedDocumentUris.DeserializeIdentity(solution, documentUri)?.DocumentId;


we were given a source generated document identity, should we throw if we can't deserialize?

throwing seems good :)

We'll throw if we can't deserialize; the case where we won't throw but return null is if we are being given a document ID that we know is stale, say because the project has been unloaded. I can make that throw too if we want, but that might hit the user.

(and the project ID check is mostly so we can recapture the "real" project ID for purposes of debugging)

Hmm. The unloaded case I assume would be better handled once we are able to refresh the document. But even if we tell the client it is gone, it is possible the client already has a request in flight by the time it gets told it is removed.

In that case returning null for a documentId seems reasonable - since the document was not found.

it is possible the client already has a request in flight by the time it gets told it is removed.

Yep you're correctly thinking about the potential race here.

dibarbet · 2024-02-21T00:16:47Z

src/Features/LanguageServer/Protocol/Extensions/Extensions.cs

+            }
+        }
+
+        public static async ValueTask<TextDocument?> GetTextDocumentAsync(this Solution solution, TextDocumentIdentifier documentIdentifier, CancellationToken cancellationToken)


I'm getting a bit confused between all these overloads we have. Should GetDocumentAsync be calling into GetTextDocumentAsync and verifying its a Document, instead of implementing it again?

Ah, sure, I can do that now. GetTextDocumentAsync didn't exist when I wrote the earlier bit.

dibarbet · 2024-02-21T00:18:45Z

src/Features/LanguageServer/Protocol/Extensions/Extensions.cs

+        /// <summary>
+        /// Finds the document for a TextDocumentIdentifier, potentially returning a source-generated file.
+        /// </summary>
+        public static async ValueTask<Document?> GetDocumentAsync(this Solution solution, TextDocumentIdentifier documentIdentifier, CancellationToken cancellationToken)


Should we remove the GetDocument method? At a glance it seemingly looks like it has different behavior from the non-async version? It isn't checking the URI scheme

dibarbet · 2024-02-21T00:22:11Z

src/Features/LanguageServer/Protocol/Extensions/SourceGeneratedDocumentUris.cs

+
+            // If we have a path (which is technically optional) also append it
+            if (identity.Generator.AssemblyPath != null)
+                uri += "&assemblyPath=" + Uri.EscapeDataString(identity.Generator.AssemblyPath);


Am slightly concerned about the size of the URI - wondering if having all this data present in the URI itself could hit serialization perf.

Is all of this strictly necessary to include, or could we infer it back on the server side based on the document id?

do we need this assmebly path?

Is all of this strictly necessary to include, or could we infer it back on the server side based on the document id?

Initially I was just doing document ID; the problems began once we actually were freezing the source generated documents to match the contents of the text changes from LSP. There we use the identity as a general concept for matching up the source generated document data at the workspace layer, because the underlying workspace APIs allow a frozen generated document to be created "out of thin air" if we realized asynchronously that the generated document doesn't exist anymore.

We could reduce this back to document ID and hold onto the identity on the server side for open source generated documents as a separate map, but it gets a bit funky that way -- when do we put something into that map? Unlike VS for Windows when it's a very clear "here's a generated document, now we know we're navigating to it" gesture, here we're just returning a URI and we might get a later request for it. Or maybe we won't. There's no guarantee so just packing the data in the URI seemed the easiest way to do it.

I expect the serialization cost to only be paid any time a user command like go to def or find refs touches a generated document. In the former case, the cost is trivial; the latter case could be expensive. But I'd rather think we should measure the cost first to understand if there's an easier change; for example if the expensive bit is the Uri.EscapeDataString() then we could easily stick that bit in a cache.

do we need this assmebly path?

It's used to differentiate different generators having the same assembly name. We don't support that per se but we've had fun bugs when users (even accidentally) did that. Many gold bars, many complaints. So at this point we just carry this around too.

I think the main area that large URIs would bite us is something like completion or code lens - the data objects there often contain URIs, which if they get large can be quite expensive.

Completion I don't think is as much of an issue - these are readonly documents so completion shouldn't be active. Code lens would be active though, so we'd be serializing this for each code lens item in the document.

I guess we can try and see if we have issues, but generally the less data we can put in there the better.

these are readonly documents so completion shouldn't be active

Hi, it's me, Razor.

I guess we can try and see if we have issues, but generally the less data we can put in there the better.

This was generally my approach: this is "correct" in that it accurately represents the underlying Roslyn model. If it's slow but the only reason it's slow is because of the code creating the string, we can easily add a cache. If it's slow because just having a big string is slow even if it's instant to make, then we can revisit.

And in the completion request I imagine there might be one bigger URI for the request, but the response is still massive, right? Or we even seeing places where the request is too big?

Ah right - if Razor is querying the c# source generated docs we'd put it in the data. But now I'm thinking about it more, I believe we promote the data onto the list instead of each item (to avoid this with existing URIs)

I should be clear here, Razor is not currently querying source generated docs via LSP, but it might depending on order of cohost-y things.

dibarbet · 2024-02-21T00:29:57Z

src/Features/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs

+                    .SelectAsArray(tuple => (tuple.identity!.Value, tuple.Text));
+
+            var doesAllTextMatch = await DoesAllTextMatchWorkspaceSolutionAsync(documentsInWorkspace, cancellationToken).ConfigureAwait(false);
+            if (doesAllTextMatch && !sourceGeneratedDocuments.Any())


I'm not quite understanding the behavior difference here. If the source generated text already matches the workspace sln text for source generated documents, why is it not valid to use the workspace sln?

Also - could we add a test or two with source generated documents in the lsp workspace manager? Like making sure we get the expected re-used workspace instance or forked solution instance where appropriate.

Ah maybe I need to rename the method here: DoesAllTextMatchWorkspaceSolutionAsync is still checking regular documents only.

Kind of feels like DoesAllTextMatchWorkspaceSolutionAsync should check every kind of document and shouldn't special case source gen docs here

So this is what's kinda fun here -- it could check if the source generated documents match...by potentially running generators! Otherwise it needs to call the other magic workspace level method that deals with the potential mismatch. This keeps this effectively synchronous in the dispatch queue, since I don't ever want dispatching to be slowing things down.

dibarbet · 2024-02-21T00:34:55Z

src/Features/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs

@@ -421,7 +442,7 @@ internal string GetLanguageForUri(Uri uri)
            languageId = trackedDocument.LanguageId;
        }

-        var documentFilePath = ProtocolConversions.GetDocumentFilePathFromUri(uri);
+        var documentFilePath = uri.Scheme == SourceGeneratedDocumentUris.Scheme ? uri.LocalPath : ProtocolConversions.GetDocumentFilePathFromUri(uri);


I decided to put this check in GetDocumentFilePathFromUri

Decided to, or not to? I think I would prefer if this check was handled in GetDocumentFilePathFromUri

CyrusNajmabadi · 2024-02-21T00:49:34Z

src/Features/LanguageServer/Protocol/Extensions/SourceGeneratedDocumentUris.cs

+    // roslyn-source-generated://E7D5BCFA-E345-4029-9D12-3EDCD0FB0F6B/Generated.cs?documentId=8E4C0B71-4044-4247-BDD0-04AF4C9E1677&assembly=Generator...
+    //
+    // where the first GUID is the project ID, the second GUID is the document ID.
+    internal static class SourceGeneratedDocumentUris


nit: being plural confused me.

Fair, will change.

CyrusNajmabadi · 2024-02-21T00:50:06Z

src/Features/LanguageServer/Protocol/Extensions/SourceGeneratedDocumentUris.cs

+        public static Uri Create(SourceGeneratedDocumentIdentity identity)
+        {
+            // Ensure the hint path is converted to a URI-friendly format
+            var hintPathParts = identity.HintName.Split('\\');


is \ something windows specific? are all hint names separated that way?

@jjonescz @chsienki did we end up normalizing hint paths in the compiler? I can't quite recall.

Hintnames are normalized to use forward slash

roslyn/src/Compilers/Core/Portable/SourceGeneration/AdditionalSourcesCollection.cs

Line 68 in f83efca

hintName = hintName.Replace('\\', '/');

Alright I'll delete that code then.

src/Features/LanguageServer/Protocol/Extensions/SourceGeneratedDocumentUris.cs

CyrusNajmabadi · 2024-02-21T00:52:59Z

...atures/LanguageServer/Protocol/Handler/SourceGenerators/SourceGeneratedFileGetTextHandler.cs

+namespace Microsoft.CodeAnalysis.LanguageServer.Handler;
+
+[ExportCSharpVisualBasicStatelessLspService(typeof(SourceGeneratedFileGetTextHandler)), Shared]
+[Method("sourceGeneratedFile/_roslyn_getText")]


nit, my preference is _roslyn_ as the prefix of the entire method. but htis is fine as well.

We do this pattern everywhere else. I don't know why (it confuses me too) -- @dibarbet do you know the backstory?

CyrusNajmabadi · 2024-02-21T00:53:09Z

...atures/LanguageServer/Protocol/Handler/SourceGenerators/SourceGeneratedFileGetTextHandler.cs

+    public bool MutatesSolutionState => false;
+    public bool RequiresLSPSolution => true;
+
+    public LSP.TextDocumentIdentifier GetTextDocumentIdentifier(SourceGeneratorGetTextParams request) => request.TextDocument;


Suggested change

public LSP.TextDocumentIdentifier GetTextDocumentIdentifier(SourceGeneratorGetTextParams request) => request.TextDocument;

public LSP.TextDocumentIdentifier GetTextDocumentIdentifier(SourceGeneratorGetTextParams request)

=> request.TextDocument;

CyrusNajmabadi · 2024-02-21T00:53:51Z

...atures/LanguageServer/Protocol/Handler/SourceGenerators/SourceGeneratedFileGetTextHandler.cs

+        Contract.ThrowIfFalse(document is SourceGeneratedDocument);
+
+        // If a source file is open we ensure the generated document matches what's currently open in the LSP client so that way everything
+        // stays in sync and we don't have mismatched ranges. But for this particular case, we want to ignore that.


i don't really understand the comment.

I'll rewrite it.

CyrusNajmabadi · 2024-02-21T00:54:21Z

...atures/LanguageServer/Protocol/Handler/SourceGenerators/SourceGeneratedFileGetTextHandler.cs

+        document = await document.Project.Solution.WithoutFrozenSourceGeneratedDocuments().GetDocumentAsync(document.Id, includeSourceGenerated: true, cancellationToken).ConfigureAwait(false);
+
+        var text = document != null ? await document.GetTextAsync(cancellationToken).ConfigureAwait(false) : null;
+        return new SourceGeneratedDocumentText { Text = text?.ToString() };


if the text is null, should we return a null response? or is it right to return anon-null response with null interior text?

I choose null interior text so the answer was "the text of this no longer exists".

src/Features/Lsif/Generator/Generator.cs

...able/Workspace/Solution/SolutionCompilationState.GeneratedFileReplacingCompilationTracker.cs

jacobjmarks · 2024-04-26T01:41:32Z

Bump on this; I believe @jasonmalinowski has gone on leave. Are we able to reassign?

jasonmalinowski self-assigned this Jun 26, 2023

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Jun 26, 2023

jasonmalinowski mentioned this pull request Jun 26, 2023

Support opening of source-generated files over LSP dotnet/vscode-csharp#5858

Closed

jasonmalinowski force-pushed the opening-source-generated-documents-over-lsp branch from 09447c1 to 961eb62 Compare July 8, 2023 01:15