Re-enable stdio clients by fixing initialization sequence #1801
Conversation
Otherwise this is incredibly noisy with diagnostic logging.
ghost
added
This fixes a whole confusing mess of problems. Previously our initialize delegate just set some options, but didn’t actually initialize the host (which is required for the server to work). Instead, we relied on a particular behavior of VS Code’s client to immediately send us an `OnDidChangeConfiguration` request, where we then actually started the host’s command loop. The logic was there because that’s where we received the client’s configuration on loading profiles and initial working directory. However, not all clients immediately sent this request, resulting in PSES v3 not working in Vim or Emacs etc. This behavior was also confusing! Instead, we now request that the client send initialization configuration via the initialize request’s parameters (in fact, the spec has an `initializationOptions` field just for this). Based on these settings, we can now start the host at the correct time. We also no longer respect `rootUri` as it has been long-since deprecated. Note that we can “no longer” (but we weren’t currently) respect a change to `EnableProfileLoading` nor `InitialWorkingDirectory` after the server has initialized. But this makes sense, you must restart it if you want to disable/enable profile loading or change the initial working directory.
andyleejordan
force-pushed
the
andschwa/stdio
branch
from
0d139e3
to
20d7e37
Compare
andyleejordan
force-pushed
the
andschwa/stdio
branch
from
20d7e37
to
a688be7
Compare
We could set the `Label` to `CompletionText` as the fallback, but that then makes the list of completions in fancier clients not as accurate (since PowerShell explicitly provides `ListItemText` for this purpose). Instead, we set usually unused field `InsertText` with the same information we provided for more advanced clients in `TextEdit`.
andyleejordan
force-pushed
the
andschwa/stdio
branch
from
a688be7
to
c1dd1db
Compare
| @@ -98,7 +98,7 @@ public sealed class EditorServicesLoader : IDisposable | |||
|
|
|||
| AssemblyLoadContext.Default.Resolving += (AssemblyLoadContext _, AssemblyName asmName) => | |||
| { | |||
| #if DEBUG | |||
| #if ASSEMBLY_LOAD_STACKTRACE | |||
There was a problem hiding this comment.
Was really bugging me, but didn't want to throw this away either.
| @@ -184,7 +185,10 @@ public override async Task<CompletionItem> Handle(CompletionItem request, Cancel | |||
| // Retain PowerShell's sort order with the given index. | |||
| SortText = $"{sortIndex:D4}{result.ListItemText}", | |||
| FilterText = result.CompletionText, | |||
| TextEdit = textEdit // Used instead of InsertText. | |||
| // Used instead of Label when TextEdit is unsupported | |||
| InsertText = result.CompletionText, | |||
There was a problem hiding this comment.
Ah so it just falls back then? That's handy!
How does InsertText work with things like type names where the completion text can be the whole type name? e.g. [System.Management.Automation.<tab>. Though even if that explodes it's better than not working at all so I'm cool with rollin' with it.
There was a problem hiding this comment.
I think it works. Just tested [System.Management.Automation.<tab> using eglot in Emacs and got [System.Management.Automation.Job, but also as I was tabbing through SMA it erroneously at some point doubled up the SM part, which IDK, this also isn't rebased on top of your branch, and dealing with other client's idiosyncrasies is hard. I want to get this out the door and then see what happens in the real world.
There was a problem hiding this comment.
but also as I was tabbing through SMA it erroneously at some point doubled up the SM part
That's what I was expecting. My PR won't fix that, I'm even not 100% sure we can fix it if the client doesn't support edits. Either way I'm cool with it just to get those clients at least working and we can take a pass at figuring something out later
|
YES, I can finally remove my hacky onDidChnageConfiguration workaround to get PSES to initialize! |
|
@dkattan I saw that and just cringed. Don't ask me how it happened! |
I'm pretty sure it's been like this since before the re-write. I first encountered the issue on 2021-06-03 and I stared at TypeFox/monaco-languageclient#201 forever trying to figure out how to make it work. This was what I had to do: const monacoLanguageClient = new MonacoLanguageClient({
name: "Monaco PowerShell",
clientOptions: {
// use a language id as a document selector
documentSelector: ["powershell"],
middleware: {
workspace: {
didChangeConfiguration: sections => {
console.debug(
this.$options.name,
"Language Server: Overriding workspace/didChangeConfiguration response; Sections: ",
sections
);
monacoLanguageClient.sendNotification(
"workspace/didChangeConfiguration",
sections
);
},
},
}, |
In addition to a few clean-ups, the primary fix here is to start the PowerShell host in the
initializerequest handler, instead of theonDidChangeConfigurationhandler, which only worked as it relied on a quirk of the VS Code client. In order to do this, we now expect to receive the settingsEnableProfileLoading: boolandInitialWorkingDirectory: stringas initialization parameters.Fixes #1695.