Split Roblox-specific functionality into dedicated interface

[voidedWarranties]

Closes #500

Outstanding issues

• Test Roblox-specific features, especially the studio plugin

[JohnnyMorganz]

Sorry for the delay, haven't had a chance to review this yet. Will try to over the next few days.

[JohnnyMorganz]

I went ahead and merged and resolved the conflicts between latest main and your branch. Will review in the next day or so.

[JohnnyMorganz]

Overall, I think this is pretty awesome. The boundaries seem quite clear to me and extendable. Most of my comments are more nits about the actual implementation, rather than the general proof-of-concept and separation. Thank you for taking the time!

I fear about that this is such a large change impacting a lot of places, but most of it is lift-and-shift so I hope nothing unexpected breaks. I wish I spent more time building a better test suite 😔.

A general comment: I'm curious on the use cases of having platform specific Go To Definition, Hover, Inlay Hints and Signature Help. These seem very generalized and only need Luau, rather than platform specific handling. The only reason I ask is because the handlers you added are called very early on in the request, meaning the platform specific code loses out on all the generic processing we have already implemented. If platform-specific code is copy-pasting from the generic code, then I think the boundary is not at the right place. Do you have an example in mind here?

[JohnnyMorganz]

Not sure if returning for the first workspace that matches is fine or if we should be sending the notification to all registered workspaces.

More of a note to self, I can check this.

[voidedWarranties]

A general comment: I'm curious on the use cases of having platform specific Go To Definition, Hover, Inlay Hints and Signature Help.

My logic behind these was mostly adding special "syntax" within comments, like the --- @ annotations I use for godot-luau-script. For greater control it may be better to move the handlers to the end rather than completely overriding default functionality. I'll try doing this when addressing the other reviews.

[JohnnyMorganz]

I see (or maybe I'll see even better after looking at your godot implementation 😄)

What I would say is maybe we should start with a minimal interface that we extend on later, once we better understand how it all fits together. If you don't need to customise all of those operations yet, then maybe let's leave them out for now. But if you do, feel free to keep it in!

[voidedWarranties]

I've tried to address the review items as best I can—I've removed the LSPPlatform methods for the functionality I don't currently forsee handling, and I've also split sourcemaps from the main LSP as I believe it's in scope for this PR (unfortunately that seems to have doubled the size of the diff). As the dependency on LSPPlatform has grown more complex, I've added a system which should guarantee the LSP doesn't attempt to respond to requests requiring the platform before configuration has been received.

As before, I haven't had a chance to test Roblox functionality in-depth as I don't have an ideal setup to do so (I don't run Windows). If you could help testing the studio plugin, sourcemap, and any other relevant functionality that would be great.

[JohnnyMorganz]

unfortunately that seems to have doubled the size of the diff

Yeah, unfortunately this is quite difficult to review. I trust that the code is mostly just copy-paste movements, rather than actual logic changes.

As the dependency on LSPPlatform has grown more complex, I've added a system which should guarantee the LSP doesn't attempt to respond to requests requiring the platform before configuration has been received.

The reasoning behind this makes sense, but the postponed messages system seems a bit off for me. I already dislike how we postpone diagnostics until configuration, and now to do this for (almost) all LSP messages isn't great. Unfortuantely we do need a solution for it though.

I wonder if we should just abuse initializationOptions for this, to receive (at least the platform) config on startup. It is not technically correct, but I think it is important so that we don't have this complex logic here (and we might even be able to clean up the isConfigured check for diagnostics, but lets leave that for later). Of course, this requires work for clients, and we will need to handle default when this is not provided. (it looks like rust-analyzer does this already. relevant: microsoft/language-server-protocol#1006, microsoft/language-server-protocol#567)

I will also try to test out the code myself soon.

[voidedWarranties]

I wonder if we should just abuse initializationOptions for this

I did consider this; I believe the reason I didn't is that it seems impossible to use this method for more than one workspace with different platforms. Do you have any suggestion on this?

[JohnnyMorganz]

Oh yeah... that is a valid point. Ok, then let's leave it with the postponing mechanism. Not sure if there is a nicer way to do it.

Although at this point, is it worth the ensureConfigured call inside of every method - maybe we should do the postponing directly in the on request loop? And postpone everything until configured.

My only fear is what if the client never responds with config, and we are stuck forever. But tbh, maybe that just means the client is buggy

[JohnnyMorganz]

Sorry to keep delaying this. I think I am happy with the general setup, and we can always refactor it later.

My only concern still lies with the postponing mechanism. I think there is a potential issue here with the ordering of events. Consider the following sequence:

• client requests document diagnostics. Server puts it into postponing queue as not received config
• client sends textDocument/didUpdate notification to change document text
• client sends configuration
• server responds to the original diagnostics message. However, the text document is not in the state when the message was sent (it got updated). Now the diagnostics provided are not in sync with the request sent.

I think we might be able to get around this just by also postponing notifications as well as requests (making sure we still allow any relevant notifications?). It is somewhat disappointing that the server completely halts whilst we wait for the config though.

I did consider this; I believe the reason I didn't is that it seems impossible to use this method for more than one workspace with different platforms. Do you have any suggestion on this?

Also I was rethinking this. Can't we just send the config for each workspace in initialisation options?

Maybe we can structure it as follows:

initializationOptions: {
    "config": {
        "global": /* non-workspace specific config. We can use this if client doesn't support workspace folders */
        [workspaceUri]: /* workspace config */
    }
}

Other than this though, I'm happy to proceed. I will try to find some time to test everything that I can think of works on my end (the cons of manual testing...)

[voidedWarranties]

I think there is a potential issue here with the ordering of events.

I am not so familiar with the LSP spec, but does this mean that the client is responsible for handling this scenario and not the server?

Can't we just send the config for each workspace in initialisation options?

Isn't it possible to add another workspace after initialization?

[JohnnyMorganz]

The potentially problematic part comes from the second part:

keep the request running if the client can still make use of the result by, for example, transforming it to a new result by applying the state change to the result.
servers should therefore not decide by themselves to cancel requests simply due to that fact that a state change notification is detected in the queue. As said the result could still be useful for the client.

Although I do not know of any requests that do this in practice.

Isn't it possible to add another workspace after initialization?

Yeah..., that is also valid. Maybe this concern is for nothing. Let's leave the postponing mechanism for now and I can revisit it if necessary

[JohnnyMorganz]

We should look into merging this ASAP to prevent it from diverging to much. Are you happy with everything (can it exit draft mode?)

I still need to do my own local testing, especially the points you mentioned in OP

[voidedWarranties]

The thing blocking moving out of draft is making appropriate changes to the VSCode extension. If that's best done in another PR then I think the server side is basically ready to exit draft.

[voidedWarranties]

I've finally addressed the extension and split it in a way I think makes sense. The second commit I added updates config and config usage; I think this needs some attention because the resolution between deprecated and new config options is somewhat dissatisfying (sometimes, the old options are ignored entirely and I don't see a better way to resolve it).

Again, things are mostly lift-and-shift, but again, I haven't tested functionality in-depth.

This PR is pretty much feature-complete, so I marked as ready for review.

[JohnnyMorganz]

Hey, thank you!

Very sorry for the delay. I've been meaning to dedicate some time to finish reviewing this but haven't got round to it yet.

I will try to pick it up next weekend.

[JohnnyMorganz]

I am incredibly sorry in the delay for reviewing this code. Unfortunately, my time to work on this project has become severely limited recently.

I still have not yet had a chance to test this thoroughly locally, as I do not have access to my dev machine for a couple more weeks. However, I really do want to get this merged ASAP so it doesn't become stale any longer.

I am going to cut a new release of the server in its current state, then merge this code in. Then, we can fix-forward any issues we find.

Thank you again for taking the time for doing this, and sorry to make it feel like it was wasted!

[JohnnyMorganz]

I think platform here might be too broad - we don't need to reload the whole server if a user decides to enable / disable the Studio plugin, or configure the way the sourcemap works.

[JohnnyMorganz]

I think this needs some attention because the resolution between deprecated and new config options is somewhat dissatisfying (sometimes, the old options are ignored entirely and I don't see a better way to resolve it).

I am wondering two things here:

1. Is it worth moving all the configs under platform? It is definitely nice to have, but seems purely cosmetic. I do have more configs I want to move around too, so maybe it could be deferred to a breaking change release.

2. If we do switch over the configs, one thing that I saw rust-analyzer do is to perform a one-time transformation of any deprecated configs to the new ones. Maybe we could do something similar here, to minimise bad config settings. (Of course, won't help non-VSCode users)

[voidedWarranties]

I am incredibly sorry in the delay for reviewing this code.

No worries, I think we both have lives outside of this.

I am wondering two things here:

I think a one-time migration would be nice, and since there isn't a great resolution for configuration right now I've gone ahead and reverted the configuration changes. Those two can be left to another PR.

There are a lot of additional code movements that could happen (e.g. Utils and many of the tests), but in the interest of not growing this PR any more I've skipped them.

If you are still interested in reviewing the changes (and I understand if not) it might help (a bit) to color code moved code: git diff --color-moved upstream/main..HEAD.

[JohnnyMorganz]

it might help (a bit) to color code moved code: git diff --color-moved upstream/main..HEAD.

TIL, thanks for the tip! This might be pretty useful

[JohnnyMorganz]

It's been a while, but I've finally had a chance to sit down and spend some time re-reading this PR. Even though there are a lot of files changed, it actually doesn't seem that bad. The only actual behaviour change I see is the postponing of messages until we are configured.

I've also tested it locally and it seems to be all good. Studio plugin still works as well. I don't have the chance to test in my day-to-day right now, but I think it's fine to move forward.

Just a few final queries (if you remember the context! It's been a long time, so totally fine if not). Then I think we are good to merge.

[JohnnyMorganz]

I've cut a release, so we are ready to go. It's been a long time, but your work is much appreciated. Thank you!