arjanadriaanse / pascal-language-server

LSP server implementation for Pascal
GNU General Public License v3.0
56 stars 20 forks source link

Completion additions #11

Closed genericptr closed 1 year ago

genericptr commented 4 years ago

Ok Arjan I've moved all my changes to a "trunk" branch and I guess I'll start moving them over in pieces to this "pending" branch so we can talk about it before merging them in. I've implemented a large number of requests but there's too much to just throw at you now (and some is in dev states, like document/workspace symbols)

For this I've improved completions and did some tests with inserting as snippets (for functions) which are currently buggy in Sublime Text but they'll address that later.

Across my changes I've decided we need some settings which the user can provide with initializationOptions. Completions for example shouldn't be snippets unless the user actually wants that. Currently however I've just added them in a global record (not sure if that's ok with you) and I haven't actually streamed them so they're off until I get to that point.

There's probably better ways to use code tools but I couldn't figure everything out so I included a codeUtils unit which is helping me do additional parsing (yes, we shouldn't have do this extra step). I'll do that right later once I know how. :)

genericptr commented 4 years ago

another thing:

1) I usually don't do unit headers but do you want me to use my name in the header for units I created? I didn't know if you wanted your name on those or not.

2) I went to test this today I'm having regressions, something causing an exception which is sending an invalid payload and crashing the LSP plugin in Sublime Text. Nasty thing that I haven't even figured out yet. Maybe you want to test this and see if it even works.

genericptr commented 4 years ago

I just tried your latest branch and I think there was a regression. try logging all payloads to stderr. I get this error after a completion:

::  -> pascal-language-server textDocument/didOpen
::  -> pascal-language-server textDocument/didChange
:: --> pascal-language-server textDocument/completion(2): {'position': {'character': 4, 'line': 72}, 'textDocument': {'uri': 'file:///Users/ryanjoseph/Desktop/FPCLS-Test/parser_test.pas'}}
pascal-language-server: {"jsonrpc":"2.0","error":{"code":-32603,"message":"Access violation"},"id":null}
arjanadriaanse commented 4 years ago

Thank you for making the effort of breaking up your changes! I will test this with my Emacs client soon.

1) I usually don't do unit headers but do you want me to use my name in the header for units I created? I didn't know if you wanted your name on those or not.

Feel free to fill in your name for new units or add yours to units you change significantly.

2) I went to test this today I'm having regressions, something causing an exception which is sending an invalid payload and crashing the LSP plugin in Sublime Text. Nasty thing that I haven't even figured out yet. Maybe you want to test this and see if it even works.

The wrapper for optional properties was not used anywhere yet, but it might have impacted the way null values are handled. I will also look into this.

genericptr commented 4 years ago

What I found is that we were sending {"jsonrpc":"2.0","error":{"code":-32603,"message":"Access violation"},"id":null} messages in a number of situations. Nasty bugs that ate up hours and even the LSP plugin in Sublime Text crashed on it. We need to handle this gracefully but I'll have to look in to it tomorrow.

genericptr commented 4 years ago

LSP was sending workspace/didChangeConfiguration and textDocument/didSave (by mistake) but our server just gave back bad messages that were very hard to debug given the client/server nature of the program. I'll find a fix tomorrow but if you can test in emacs without it failing please do so.

As for the unit headers I'll change my name in them. Are you ok with the server settings? I just threw that together for testing but it may need to be ported to a class so we can stream it using dicts like "optionName: true".

genericptr commented 4 years ago

The wrapper for optional properties was not used anywhere yet, but it might have impacted the way null values are handled. I will also look into this.

I think the problem is the main loop with Dispatcher.Execute. I thought it would give a message about an invalid handler but instead we get back {"jsonrpc":"2.0","error":{"code":-32603,"message":"Access violation"},"id":null} which we just passed right on to stdout.

Ideally we should fail before this but maybe that's what the JSON serialization does and we should just check for this message and log it to stdErr instead. Unless you have a better idea I'll probably go that route.

genericptr commented 4 years ago

Here are the changes I'm recommending. I don't like that we need to make this special error check because it should be captured earlier but I'll leave it like this for now since it's breaking otherwise.

I also added a LSP macro which we need to set for the API version. This shouldn't be needed but I've already found that Sublime Text crashes by sending unknown values in newer versions of the API. If it's not too much work I think we should just wrap the newer methods in ifdefs:

    // If a server provides both `allCommitCharacters` and commit
    // characters on an individual completion item the ones on the
    // completion item win.
    {$if LSP >= 3015}
    property allCommitCharacters: TStrings read fAllCommitCharacters write fAllCommitCharacters;
    {$endif}
genericptr commented 4 years ago

sorry for all the noise. The Sublime Text LSP people say I shouldn't use compile time macros but rather rely on client capabilities so I'll need to remove that. So far we've been ignoring those but I guess they need to be considered other we may crash various clients (in rare cases but still possible).

genericptr commented 4 years ago

I've made the settings streamable and I will integrate them into this pull request later so please don't apply it yet.