-
Notifications
You must be signed in to change notification settings - Fork 676
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support opening of source-generated files over LSP #5858
Support opening of source-generated files over LSP #5858
Conversation
@@ -662,6 +663,17 @@ function registerRazorCommands(context: vscode.ExtensionContext, languageServer: | |||
})); | |||
} | |||
|
|||
function registerSourceGeneratedFilesContentProvider(context: vscode.ExtensionContext, languageServer: RoslynLanguageServer) { | |||
context.subscriptions.push(vscode.workspace.registerTextDocumentContentProvider('source-generated', new class implements vscode.TextDocumentContentProvider { | |||
// TODO: also implement eventing for when the file changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now I'm just doing a small implementation of TextDocumentContentProvider here; the Omnisharp side has a fuller implementation that does support eventing but will require a bit of refactoring to be able to reuse.
1c5daac
to
f4180b8
Compare
Will review tomorrow! |
); | ||
return result.text; | ||
} | ||
})() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so i personally really dont' like these sorts of anonymous inner classes. my pref is a distinct class with clear cpatures. but i don't feel strongly enough to block. if you feel like this is ok, then i'm ok with it.
Any chance this will get merged soon? |
@moander Hopefully in a week or two; unfortunately my attention is in a different direction for the rest of this week... |
I thought I'd leave a comment here thanking you for the great work, and also echoing the ask to merge this soon. |
@jasonmalinowski Apologies for the ping, but is there any update on this? It's painful working with source generators in VS Code. |
@glen-84 Thanks for the friendly ping! Hopefully can get this in a week or two -- I recognize that's what I said a few weeks go but that prior work turned into more than expected. But that work is now cleared so this is towards the top. |
f4180b8
to
cbda2b5
Compare
We had some code that slipped in that was calling ProtocolConversions.CreateAbsoluteUri(document.FilePath), which is fine for regular documents but skips the special handling around source generated files, so we'd break in those cases. This "fixes" the UriFormatExceptions people are getting, in the sense that you won't get a UriFormatException anymore. If you try to actually go to a source generated document, that'll still be broken until we merge dotnet/vscode-csharp#5858, but at least this means something like a find reference won't completely break if it has a location somewhere in a source generated file.
This allows the Roslyn LSP to return document references with a roslyn-source-generated URI, and we'll turn around and ask the LSP for those files when they're opened. For now we don't worry about refreshing files if they've changed, just to keep this simple.
cbda2b5
to
d344426
Compare
@jasonmalinowski any updates on this one? I'm really looking forward to this patch. Have a nice weekend! :) |
@moander I think I'm going to have the Roslyn side of the change today, assuming no other random fires come up. (They've been coming up a lot recently. 😢 ) |
Bump on this; I believe @jasonmalinowski has gone on leave. Are we able to reassign? |
Is there any progress? |
Will there be any further progress on this? cc @dibarbet |
Bump! |
Closing in favor of #7581 which merged this change. |
This depends on dotnet/roslyn#68771 for the server-side piece. But, if you don't have the server side piece, we won't return a URI that will trigger this code, so it's fine to have it in now.