New API: Add single-server document executor

[ahmedneilhussain]

This covers the main uses for DocumentInfo: following a server request, bundle the document and the server that responded into an object that can follow up further requests.
Extracted at reviewer's request from an API port into a standalone PR

[ahmedneilhussain]

@rubenporras as requested! I even added a test ;-)

[rubenporras]

I think that to be consistent with the names of the other classes, it should be named SingleLanguageServerDocumentExecutor, in the name of the other classes we use LanguageServer and not just Server

[ahmedneilhussain]

Done. The names are becoming a bit cumbersome but I guess that's what var was invented for ;-)

[rubenporras]

I think it not right that this executor does not respect the withFilter or withPredicate (even though one could call it) and instead offers a new method to do the filter

[ahmedneilhussain]

I have amended it so that the inherited methods collectAll() etc will respect it, but kept execute() as a method for cases where the code already knows what the server is and what capabilities it has and just wants a 'get it done' request method.

See summary of revisions below

[rubenporras]

the LanguageServerDocumentExecutor uses the document to find out the wrappers and connect them if needed, but I do not think this executor uses the document for anything.

[ahmedneilhussain]

You are (partly) right - one of the motivations for this class was as a replacement for DocumentInfo, which effectively bundles a reference to a single LS with a document - it's virtually a POJO with no brains. In some of the code I was trying to port, the existing logic passed through a DocumentInfo as a single field. My first attempt replaced this with passing around a LanguageServerWrapper and an IDocument as individual arguments. But then I thought of taking your LanguageServerWrapperExecutor idea and marrying it with LanguageServerDocumentExecutor.

I think this gives a nicer way of rewriting the existing code, and also means versioning support is available.

[rubenporras]

@ahmedneilhussain, I have prepared #435, it is only the API in LanguageServers extracted out of #396, it have added a getServer method as you propose and have also taken care of handling the filter if given.
If you like it I can take over your test and merge. If not we can keep the discussion and see where we converge.

[ahmedneilhussain]

HI @rubenporras I think you raise some good points. I proposed this class for various reasons, but it still feels a bit unsatisfactory. Although I was following your model for an executor for a single server/wrapper, this was something I had considered myself anyway as part of the original design for the new API.

The general need for this sort of class is for following up an initial request/response with further requests. There are quite a few existing cases within the old codebase where the language server is stored on e.g. a UI class for subsequent requests. We obviously don't want this, which is why I initially proposed using the wrapper (with a public execute) method on it.
Having an executor based around this wrapper takes it one step further but it does have some drawbacks:

• The core executor methods collectAll computeAll computeFirst don't make a lot of sense: they are there to handle the complexities that more than one LS could respond to the request - and the calling code cannot know in advance how many might apply. With an executor for a single LS we already know there will be exactly one LS, so using those three methods for followup requests adds a lot of extra baggage for no gain.
• If we expose a single execute method to avoid the pointless complexity of the core executor methods, then what exactly does a single-ls executor offer over using the wrapper itself? The answer might be versioning and cancel support - if and when they get added.
• I like your idea of taking the filter into account. Very often in the codebase we can see that after the initial request, some of the follow up requests depend on the server offering additional capabilities. However that doesn't play very well with having the executor as a final field on the class and being reused - since setting the filter changes it for subsequent requests.

[ahmedneilhussain]

I think there are two issues to resolve:

• Perhaps the single-server executors should not inherit from LanguageServers directly - either they should be a separate (but similar) class hierarchy, or perhaps we should refactor so that there is a common root with e.g. the filtering support:

            LanguageServers
              /                          \
MultiLSExecutors             SingleLSExecutors
              |                                                               |
existing leaf classes                           SingleLanguageServerDocumentExecutor, SingleLanguageServerExecutor

• Lifecycle: versioning and cancel support makes reusing an executor (or changing the filter for a one-off use) problematic. We could solve these problems either by offering a clone() method (create a new LS with the same type) or a reset() method.

What do you think?

[rubenporras]

HI @rubenporras I think you raise some good points. I proposed this class for various reasons, but it still feels a bit unsatisfactory. Although I was following your model for an executor for a single server/wrapper, this was something I had considered myself anyway as part of the original design for the new API.

Yes, it has its drawbacks, but it has the upside that you can actually tell that the pattern of how to do a remote call to one or many LS, just use LanguageServers and the methods in it. Then you can even remove the public method from the LanguageServerWrapper as I did in #396. That is to me very appealing.

The general need for this sort of class is for following up an initial request/response with further requests. There are quite a few existing cases within the old codebase where the language server is stored on e.g. a UI class for subsequent requests. We obviously don't want this, which is why I initially proposed using the wrapper (with a public execute) method on it. Having an executor based around this wrapper takes it one step further but it does have some drawbacks:

• The core executor methods collectAll computeAll computeFirst don't make a lot of sense: they are there to handle the complexities that more than one LS could respond to the request - and the calling code cannot know in advance how many might apply. With an executor for a single LS we already know there will be exactly one LS, so using those three methods for followup requests adds a lot of extra baggage for no gain.

Yes, agree with that. I did not look at the overall code, so I do not know how many of these consumers do we actually have. If they are only a few, I think it is fine, they will just not use these APIs. To me it would not be worth having a new interface. It is similar to using Collections.singletoList, Collections.singletoSet, Collections.singletoMap. Yes, there is no way of getting one element out of them, you still need to iterate or just call get(0) or similar. It is not perfect, yet it is good enough.

• If we expose a single execute method to avoid the pointless complexity of the core executor methods, then what exactly does a single-ls executor offer over using the wrapper itself? The answer might be versioning and cancel support - if and when they get added.

Yes, I agree with that, that is why in my original version in #396 I did not have it, to me to use computeFirst() or iterate through collectAll() as one would do with Collections.singletoList. I added it now to my proposal because I imagine you would like it ;)

• I like your idea of taking the filter into account. Very often in the codebase we can see that after the initial request, some of the follow up requests depend on the server offering additional capabilities. However that doesn't play very well with having the executor as a final field on the class and being reused - since setting the filter changes it for subsequent requests.

Okay, I see, then we could add to the forWrapper method, so that it can only be provided only once, and the implement withFilter and withCapability to throw an UnsupportedOperationException (Like again SingletonList does).

I can adapt my PR with any of this changes if you like.

[rubenporras]

I think there are two issues to resolve:

• Perhaps the single-server executors should not inherit from LanguageServers directly - either they should be a separate (but similar) class hierarchy, or perhaps we should refactor so that there is a common root with e.g. the filtering support:

            LanguageServers
              /                          \
MultiLSExecutors             SingleLSExecutors
              |                                                               |
existing leaf classes                           SingleLanguageServerDocumentExecutor, SingleLanguageServerExecutor

What do you think?

I do not know how much code would profit from a better API, but my feeling now is that not many (as I said above), and that we provide more value by porting the code to the API we have (with minor improvements here and there like what we are doing).

[rubenporras]

• I like your idea of taking the filter into account. Very often in the codebase we can see that after the initial request, some of the follow up requests depend on the server offering additional capabilities. However that doesn't play very well with having the executor as a final field on the class and being reused - since setting the filter changes it for subsequent requests.

Actually now that I think about it, is this not a problem with the existing executors ?

I can already write

		LanguageServerDocumentExecutor ex = LanguageServers.forDocument(document);
		ex = ex.withFilter(x -> true);
		ex.computeAll(...);
		ex = ex.withFilter(x -> false);
		ex.computeAll(...);

and it is undefined what will happen. I think the LanguageServerProjectExecutor will behave as the LanguageServerWrapperExecutor, the document one will ignore it. Something we need to improve already aparently :(

[ahmedneilhussain]

An interesting point about Collections.singletonList()!

Just to let you know I have to work on something else for a couple of days, so will follow this up later in the week.

[rubenporras]

I do not remember the details, but I remember that Effective Java (2nd Edition) recommends avoiding clone and use copy constructors instead, and I think it is a usually accepted pattern.
For example
public static LanguageServerDocumentExecutor forExecutor(final @NonNull LanguageServerDocumentExecutor executor) {

and similar for the other executor types.

Could we follow that recommendation?

[ahmedneilhussain]

If you want, but that's partly because clone and Clonable predate generics and are thus broken. I've not actually implemented the 'official' Clonable interface, just provided a method called clone() that is generic and thus properly covariantly typed.

[rubenporras]

Alright, I see, I have overlooked that. I still think the copy constructor is preferable, as then there is still only pattern to get new executors (constructor) vs two pattern (close + constructor)

[rubenporras]

I do not think we need the reset method, for the number of times we will actually want to reuse executors, I think just creating a new object should be good enough.

[ahmedneilhussain]

You are perhaps right but I can foresee getting the reverse complaint as well. Certainly it's more of a convenience thing than a necessity thing if clone/copy is implemented.

[rubenporras]

I agree, but since adding is easier than removing, I think we should start small. It is safer :)

[rubenporras]

I think we should remove this three methods for the time being and add then later if needed, it is really weird that they promote different patterns, like an executor which does not respect the filter.

I would propose that we go without execute, supports and getServer, change a consumers, and the create a new PR showing how those consumers would be indeed simplified.

Could we do that? Adding the methods is indeed simple if really needed.

[ahmedneilhussain]

The methods were all driven by necessity - the design choices were prompted by the issues I encountered porting code. See for example CodeActionCompletionProposal.apply().

I like the idea that if you want to follow up your initial request with a further action - but only if the server supports the more refined capability - then you can call clone().withFilter(...).computeFirst() and get a result only if the server supports it.

In the case of trying to port the apply method above, there are two criteria for proceeding, only one of which is a server predicate, so just using the filter method doesn't work.

[rubenporras]

thanks for the newer version of the PR, I have left some comments.

[rubenporras]

This I would also remove, by the way, and keep the API small. It is only a bit of syntactic sugar, and we should only add those if it really pays. I am not sure yet it does.

[ahmedneilhussain]

Yeah, but it does make the api more convenient, especially as a lot of these methods will get used in lambdas. LSP4e is already somewhat rich in lambda functions that ramble on for multiple lines and are hard to read..!

[ahmedneilhussain]

Hi @rubenporras you started re-reviewing before I had a chance to finish typing up my summary of what I had changed and why :-)

Just for the record:

• I have made the single-use aspect of the executors explicit, so the executors throw if used more than once without an intervening called to reset(). This was already the case because of versioning support, and cancel support will only make them more stateful, so better to make this aspect of the contract explicit.
• I didn't split the class hierarchy into two (as I initially proposed) with separate executors for a single server. I've kept them with the multi-server collectAll() methods and so on, but also kept the single-server execute() method. The answer as to what the executor offers over just calling wrapper.execute() would be versioning (and shortly, cancellation) support.

I quite like this as a compromise. I know you've already come back with some comments - can I ask you take a look at the other two PRs I have in flight porting stuff to the new API, as that might give some better rationale for the choices?

[rubenporras]

Hi @ahmedneilhussain , I think we should discuss the check that deals with the executor classes being stateful in a different thread/PR, I understand the problem but it is indeed unrelated the single-server-document executor, that should make easier to reach a conclusion. I have the feeling that making the executors support versioning looked great, but now that I realize we made the executors stateful I have the feeling it was not good.

Regarding the single-server-document executor, I have the feeling that if it inherits from LanguageServers as the LanguageServerDocumentExecutor and the LanguageServerProjectExecutor, it should have the same API, either that is good enough or have a structure like the one you propose in #434 (comment)

In order to understand better the usage of the new class, I have looked at some code we would migrate and just did that. If we continue doing so I expect at the end I will see how many consumers would profit from the API and think better about what is worth doing in these area.

[ahmedneilhussain]

I don't think statefulness is a big deal if it's made explicit, and if you get something in return. It's more a problem if it's implicit and sporadic and so people introduce bugs by misusing the class because they're not aware of that aspect of the contract. Lots of classes like eclipse Job implementations and e.g. the LSSearchQuery classes we have in this codebase are essentially 'one-shot' classes that should only be run once: you don't rerun them, you create a new object.

Versioning and cancel support will IMHO make the trade off well worth it, especially as it's trivial to implement clone and reset methods (or copy constructors!) that make the statefulness no inconvenience. Cancel support could be done outside the executors directly (but less conveniently), but versioning makes sense as built in. I think it is worth having a rich API that makes common stuff easy and concise, then that means consumers are less likely to make mistakes and can also focus more on their domain-level logic. I think the fact that most of the time the code is shrinking when we port it to the new API is a good sign.

[rubenporras]

Hi @ahmedneilhussain , I see your last PR also uses this executor, but only with computeFirst(), which I think it is good enough. Could it be a way forward for the time being that we remove from this PR the checkHasRun, clone, reset logic and getServer, execute?, the we have the slave executor that we can use and continue the rest of the review later. I think we can follow the piecemeal approach here without loosing much on the way, since we agree on the other half of the API.

What do you think of that?

[rubenporras]

Hi @ahmedneilhussain , I have the felling we have done quite well without an executor like this one or the one I proposed in another PR. I already closed my PR, shall we close this one as well?

[ahmedneilhussain]

Hi @ahmedneilhussain , I have the felling we have done quite well without an executor like this one or the one I proposed in another PR. I already closed my PR, shall we close this one as well?

Sure @rubenporras I think we can bin this and reinstate one or other idea if a real need arises in future.