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

dibarbet · 2024-09-20T00:50:30Z

A new version of #68771

I was having a heck of a time with the conflicts, so I ended up just re-doing it (and copying changes over that I thought were relevant).

I believe I addressed the comments, however I still do have some questions.

Server side of dotnet/vscode-csharp#6426

Note - this does not yet implement support for refreshing the file on changes

dibarbet · 2024-09-20T00:51:22Z

src/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs

+                        // Source generated documents cannot go through OnDocumentOpened/Closed.
+                        // There is a separate OnSourceGeneratedDocumentOpened/Closed method, but there is no need
+                        // for us to call it in LSP - it deals with mapping TextBuffers to text containers.
+                        return;


@jasonmalinowski @CyrusNajmabadi for thoughts. I decided to not call it - otherwise I'd have to implement similar TryOnSourceGeneratedDocumentOpen/Closed like we have for regular documents. It didn't seem necessary as the open was only trying to associate things with text buffers.

Since these are readonly (and content managed by the server), there is nothing to update in the workspace either

i think the comment coudlo be beefed up.

So I've forgotten, for regular documents, why are we opening/closing the documents? Is it just so other code can ask "what's open" from the workspace or are we still getting some other benefit?

I honestly can't remembner anything that would require the regular documents to actually be opened in the workspace. Generally LSP features should be checking if LSP thinks the document is opened.

And there isn't any source text container stuff to do either, sicne we have our own LSP fork management.

src/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs

src/LanguageServer/Protocol/LanguageInfoProvider.cs

src/EditorFeatures/TestUtilities/LanguageServer/AbstractLanguageServerProtocolTests.cs

src/LanguageServer/Protocol/Extensions/Extensions.cs

JoeRobich · 2024-09-25T07:25:15Z

src/LanguageServer/Protocol/LanguageInfoProvider.cs

@@ -43,24 +43,31 @@ internal class LanguageInfoProvider : ILanguageInfoProvider
            { ".mts", s_typeScriptLanguageInformation },
        };

-        public LanguageInformation GetLanguageInformation(string documentPath, string? lspLanguageId)
+        public LanguageInformation GetLanguageInformation(Uri uri, string? lspLanguageId)


Is lspLanguageId the language selected in the VS Code status bar for a particular document? If so, why would we not want to honor that over the file extension, in the case that the user has chosen something different?

It is the languageId the LSP client gives us (which I believe in VSCode will change based on the language selected in the status bar).

I suppose generally we should use the languageId instead (if we're able to map it), otherwise fall back to the file extension.

I'm going to defer this for now though - there is some small risk of regression, especially in VS if they give us the wrong languageId. I'd rather take that in a separate PR

Agreed that in the case of a well-behaved LSP client, we should trust the language ID. Also agreed we should keep this as is for now and verify we have well-behaved clients before changing it. 😄

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

src/LanguageServer/Protocol/Extensions/SourceGeneratedDocumentUri.cs

src/LanguageServer/ProtocolUnitTests/Workspaces/LspWorkspaceManagerTests.cs

src/LanguageServer/Protocol/Extensions/SourceGeneratedDocumentUri.cs

CyrusNajmabadi · 2024-09-27T20:20:19Z

src/LanguageServer/Protocol/Extensions/SourceGeneratedDocumentUri.cs

+        var assemblyVersion = Uri.EscapeDataString(identity.Generator.AssemblyVersion.ToString());
+        var typeName = Uri.EscapeDataString(identity.Generator.TypeName);
+
+        var uri = $"{Scheme}://{projectId}/{hintPathPortion}?{DocumentIdParam}={documentId}&{HintNameParam}={hintName}&{AssemblyNameParam}={assemblyName}&{AssemblyVersionParam}={assemblyVersion}&{TypeNameParam}={typeName}";


so, the part i trust is the projectId/docId. teh rest i think should all be opaque data we effectively don't munge with (specifically referring to hintpathportion).

The project ID/document ID is still the core part, yes. But including everything else means we're able to round-trip the entire SourceGeneratedDocumentIdentity. This allows for at least one edge case to be better handled: if the user opens a source generated file, and then something means that file isn't being generated any more, we can still respond to an LSP request for this URI if the file is still open on the client, as if it's otherwise a regular SourceGeneratedDocument rather than it being a generic miscellaneous files file.

jasonmalinowski

Overall this still jives with what I expect this to look like. I feel like my old PR had some issues around opening/closing documents repeatedly and miscellaneous files breaking, but I trust you've tested that well.

My two open questions:

Regarding the DateTime.Now stuff -- what's that being used for?
For the code we have that's doing document open/closed on mutating workspaces -- is that just so code can ask what documents are open or did we have something more happening there?

src/LanguageServer/Protocol/Extensions/SourceGeneratedDocumentUri.cs

jasonmalinowski · 2024-09-28T00:37:31Z

src/LanguageServer/Protocol/Extensions/SourceGeneratedDocumentUri.cs

+        var assemblyVersion = Uri.EscapeDataString(identity.Generator.AssemblyVersion.ToString());
+        var typeName = Uri.EscapeDataString(identity.Generator.TypeName);
+
+        var uri = $"{Scheme}://{projectId}/{hintPathPortion}?{DocumentIdParam}={documentId}&{HintNameParam}={hintName}&{AssemblyNameParam}={assemblyName}&{AssemblyVersionParam}={assemblyVersion}&{TypeNameParam}={typeName}";


The project ID/document ID is still the core part, yes. But including everything else means we're able to round-trip the entire SourceGeneratedDocumentIdentity. This allows for at least one edge case to be better handled: if the user opens a source generated file, and then something means that file isn't being generated any more, we can still respond to an LSP request for this URI if the file is still open on the client, as if it's otherwise a regular SourceGeneratedDocument rather than it being a generic miscellaneous files file.

jasonmalinowski · 2024-09-28T00:52:02Z

src/LanguageServer/Protocol/Extensions/Extensions.cs

+        public static async ValueTask<Document?> GetDocumentAsync(this Solution solution, TextDocumentIdentifier documentIdentifier, CancellationToken cancellationToken)
+        {
+            var textDocument = await solution.GetTextDocumentAsync(documentIdentifier, cancellationToken).ConfigureAwait(false);
+            Contract.ThrowIfTrue(textDocument is not null && textDocument is not Document, $"{textDocument!.Id} is not a Document");


The semantics here feel a bit funky -- so if the document doesn't exist we'll return null, but if it does exist but it's an additional file we'll throw? Seems like we might end up with a bunch of accidental exceptions that way?

In my view, the accidental exceptions would be more catching unintentional bugs - someone was asking for a document - but they got an additional document (or other kind of document) instead - which likely means they should be calling a different API.

If we returned null, then the code might 'work' but not be doing what they want.

src/LanguageServer/Protocol/Extensions/Extensions.cs

jasonmalinowski · 2024-09-28T01:01:34Z

src/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs

+                        // Source generated documents cannot go through OnDocumentOpened/Closed.
+                        // There is a separate OnSourceGeneratedDocumentOpened/Closed method, but there is no need
+                        // for us to call it in LSP - it deals with mapping TextBuffers to text containers.
+                        return;


So I've forgotten, for regular documents, why are we opening/closing the documents? Is it just so other code can ask "what's open" from the workspace or are we still getting some other benefit?

src/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs

jasonmalinowski · 2024-09-28T01:20:01Z

src/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs

+                return false;
+            }
+
+            var newState = existingState.WithText(text);


I forget, how expensive is this? If the state is different, can we actually use that later rather than doing it a second time?

It doesn't seem to expensive (compares checksums, then copies over to a new Sg state. As far as I can tell any actual work is deferred - https://source.dot.net/#Microsoft.CodeAnalysis.Workspaces/Workspace/Solution/SourceGeneratedDocumentState.cs,c9182b28095a2b1c,references

if you'd prefer I can expose a version of WithFrozenSourceGeneratedDocuments that takes in the sg states.

Looked into this a little bit. Definitely possible to do with some small refactoring to WithFrozenSourceGeneratedDocuments to allow us to pass an existing state. However I don't think it will make much of a difference for the following reasons

If the state is the same, the only work we do is calculating the checksums, which we would do regardless

If the state is different, there still isn't much work done - we create a SourceGeneratedDocumentState, but all the parsing and everything else is deferred in lazy's.

We create a max of one new state here if the text is different since this returns early as soon as we detect one difference.

Think it is fine to leave it like this, but let me know what you think

jasonmalinowski · 2024-09-28T01:22:29Z

src/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs

+                _trackedDocuments.Keys.Where(static uri => uri.Scheme == SourceGeneratedDocumentUri.Scheme)
+                    .Select(uri => (identity: SourceGeneratedDocumentUri.DeserializeIdentity(workspaceCurrentSolution, uri), _trackedDocuments[uri].Text))
+                    .Where(tuple => tuple.identity.HasValue)
+                    .SelectAsArray(tuple => (tuple.identity!.Value, DateTime.Now, tuple.Text));


@CyrusNajmabadi What is the time used for here, generally? Something complicated or in the VS for Windows case just updating the gold bar so the user knows when it was last updated?

Think its just the gold bar. Right now I just picked a time because we need to pass it to WithFrozenSourceGeneratedDocuments

src/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs

jasonmalinowski · 2024-09-28T01:33:56Z

src/VisualStudio/Core/Def/NavigateTo/RoslynSearchResultViewFactory.cs

@@ -64,9 +64,9 @@ public Task<IReadOnlyList<SearchResultPreviewPanelBase>> GetPreviewPanelsAsync(S
                return null;

            Uri? absoluteUri;
-            if (document.IsSourceGeneratedDocument)
+            if (document.SourceGeneratedDocumentIdentity is not null)


If we had to pass through the identity just so we can create a URI, why not just have the document object just hold onto the URI in the first place? Then we can delete this handling here and the try/catch in favor of a document.GetURI() when the object is created?

the code creating the NavigableDocument is in features, which doesn't have any of the URI stuff from the LSP layer (see next comment)

jasonmalinowski · 2024-09-28T01:37:03Z

src/VisualStudio/Core/Def/NavigateTo/RoslynSearchResultViewFactory.cs

            {
-                absoluteUri = ProtocolConversions.CreateUriFromSourceGeneratedFilePath(filePath);
+                absoluteUri = SourceGeneratedDocumentUri.Create(document.SourceGeneratedDocumentIdentity.Value);
            }
            else
            {


And to be clear if we just pass a URI directly then we can delete this else block too. This was added as a part of #72442 which was a fine tactical fix but shouldn't be needed going forward, I'd imagine.

Going to leave this as-is - the navigable document is created at the features layer which doesn't include any of the URI stuff we have at the protocol layer. So I believe the else is still needed here since we won't be using the GetUri extension on document.

I don't see a problem with moving some of the URI stuff down to features, but I think that is best left to a separate PR (a number of helpers would need to move along with it)

dibarbet · 2024-10-02T23:15:48Z

Going to merge this - addressed Cyrus' comments, and Jason won't be available for a bit. Can take additional feedback later.

Enable support for an LSP client to open source generated files

8d00434

dibarbet requested a review from a team as a code owner September 20, 2024 00:50

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Sep 20, 2024

dibarbet requested review from jasonmalinowski and CyrusNajmabadi September 20, 2024 00:50

dotnet-policy-service bot added the VSCode label Sep 20, 2024

dibarbet commented Sep 20, 2024

View reviewed changes

src/LanguageServer/Protocol/Workspaces/LspWorkspaceManager.cs Outdated Show resolved Hide resolved

dibarbet mentioned this pull request Sep 20, 2024

Add a TextDocumentContentProvider for source-generated files dotnet/vscode-csharp#7581

Merged

JoeRobich reviewed Sep 20, 2024

View reviewed changes

src/LanguageServer/Protocol/LanguageInfoProvider.cs Show resolved Hide resolved

dibarbet added 2 commits September 23, 2024 10:46

fix f# language info

85f8f08

Fix navigable item sg URIs

b778358

build-analysis bot mentioned this pull request Sep 23, 2024

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

3 tasks

Fix diagnostics to get TextDocument instead of Document

fcdc545

JoeRobich reviewed Sep 25, 2024

View reviewed changes

dibarbet added 2 commits September 25, 2024 11:20

Udate LSIF source generator tests to expect new URI

ff6ec8d

Stabilize lsp source generator forking test

43d56b6

CyrusNajmabadi reviewed Sep 25, 2024

View reviewed changes

src/LanguageServer/Protocol/Handler/SourceGenerators/SourceGeneratedFileGetTextHandler.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Sep 25, 2024

View reviewed changes

src/LanguageServer/Protocol/Handler/SourceGenerators/SourceGeneratedFileGetTextHandler.cs Outdated Show resolved Hide resolved

dibarbet force-pushed the source_gen_vscode branch from 4b76d35 to 5477d5b Compare September 25, 2024 21:53

Do not fork when source generator document is unchanged and add tests

58c8106

dibarbet force-pushed the source_gen_vscode branch from 5477d5b to 58c8106 Compare September 25, 2024 21:55

CyrusNajmabadi reviewed Sep 26, 2024

View reviewed changes

src/LanguageServer/Protocol/Extensions/SourceGeneratedDocumentUri.cs Outdated Show resolved Hide resolved