nimlsp
nimlsp copied to clipboard
PMunch
Clean
Why did you remove the whenValid template?
The crashing on error is done on purpose, a proper LSP client implementation should take care of restarting the server, so we don't try to recover from crashes by design. This also makes it easier to debug nimlsp and won't leave the user with a LSP server in a bad state or spinning for no reason. If this is to be added it should at least be placed behind a user-passed switch to be enabled.
old code have whenValid notification whenValid request in same indent, that will cause a request is not a valid notification, a notification is not a valid request, you will get many "Unable to parse data".
also during developing nimlsp self, it can't work well with nimlsp project itself , will get error when compileProject, so I decide to reduce the code complexity.
vscode will try restart server in minutes, later it will not retry, that's annoying when you developing project suddenly extension not work. also I dont like one specific file cause language server not work for whole project. for now the error message also catched and print to stderr, and write to log file, same as old behivor.
This doesn't really decrease complexity though, it just makes it easier to do mistakes. The "notification is not a valid request" messages are intentional to aid in debugging and I'd like them to stay. I will not accept this cleanup (which is otherwise quite sensible) if you remove whenValid as it is a nice pattern.
But having the server crash and report that to the user is better than not crashing but getting into an illegal state and start doing weird stuff, or get stuck in some kind of weird loop because we try to handle errors. The bug should be fixed in NimSuggest, or if there are some Exceptions that we should catch then we should catch those instead of all Exceptions.
what do you mean " illegal state" , it just run nimsuggest commands (or maybe one command) fails for single file , I dont think it should make server shutdown. further more if I work in workspace for multiple projects, it will shutdown for single file fails.
I've been poking at the LSP code and trying various experiments and have found the following things that would help in general for a bit more robustness of the server, I don't have a clean PR, here are my quick notes in the meantime:
- add a guard to whenValid for notifications so it doesn't match objects with an id field
- then embed the whenValid notification handling as the else body of the whenValid for requests
- both of the above continue the loop, so if neither are valid they'll echo a single message by way of the notification whenValid, this else body can be modified to make a nicer message
- in the main loop after requests and notifications, respond with error because none of the above continued, it must have been an invalid request as the notification with id guard never matched
As for try/catch and error handling for nimsuggest, this feels like a must as the server is effectively working in a multiplexing scenario 1-client:1-server:n-projects and as @bung87 points out errors in one shouldn't cross the failure domain into another. Errors should of course be reported to the user but crashing the server on any occurrence sounds like a rate limiting or bulkheading issue?
Maybe a simple guard could be keeping a count of exceptions caught and crash the server after 10(?) to act as a rate limit. This can increase in sophistication by decreasing the count as time passes so a very small number of exceptions (1 ever 20 seconds?) are tolerated at any one time but a few over a large period of time do nothing. One could go further by tracking the percentile (p99) of nimsuggest responses for the server process and using that to both determine if something is off with repeated requests being far outside that range and/or using that to determine how long to wait to reduce the count. Personally, I'd start with the simple counter and a ceiling of 5.
Take a look at https://github.com/bung87/nimlsp/commits/myver if you want something new, I build my version runs on local ly work with https://github.com/bung87/vscode-nim-lsp, working fine so far, I'd like to take some part from your project @saem, maybe signatureHelp's param position matching part first.
@bung87 I'm actually looking at the language server because I'd like to move as much as possible into one from the existing extensions. Mostly inspired by what the Haxe community did with VS Code Haxe and friends.
I went looking for a vscode extension in your github account to see if I could find it got a bit distracted in the meantime, thanks for the link I'll dive into that -- I wouldn't mind collaborating if you're up for it?
@saem yeah, I'd like to , contact me if you get any news, I'll continue this field until I feel comfortable during developing nim project.
@bung87:
what do you mean " illegal state" , it just run nimsuggest commands (or maybe one command) fails for single file , I dont think it should make server shutdown. further more if I work in workspace for multiple projects, it will shutdown for single file fails.
This might not be the case any longer, but in the scenario where nimsuggest fails it isn't necesarilly the case that the same instance can be used again without always throwing an error. It could of course refresh that specific instance though, instead of crashing. But I've focused on keeping the functionality of NimLSP to be as simple as possible as to not implement another layer of orchestration as it is already under the control of your editor/plugin which should handle all these failure scenarios. This also means that there is no case in which the NimLSP plug-in can get itself into a state where it isn't working properly but doesn't crash so the editor won't restart it.
@saem:
I've been poking at the LSP code and trying various experiments and have found the following things that would help in general for a bit more robustness of the server, I don't have a clean PR, here are my quick notes in the meantime:
1. add a guard to whenValid for notifications so it doesn't match objects with an id field
whenValid uses the JSON schema and the isValid procedure from the jsonschema module to detect which kind of object it is the valid kind for. If notifications shouldn't have an id field either it's just a matter of fixing the schema or there is something wrong with the specification. We shouldn't impose rules on the schema that doesn't exist.
2. then embed the whenValid notification handling as the else body of the whenValid for requests 3. both of the above continue the loop, so if neither are valid they'll echo a single message by way of the notification whenValid, this else body can be modified to make a nicer message 4. in the main loop after requests and notifications, respond with error because none of the above continued, it must have been an invalid request as the notification with id guard never matched
This wasn't done to avoid having half of the code indented by another level, but I can see your point in that the error messages would look better. I think it is better to track them another way though. As for the rest of the comment see what I replied to bung further up in this comment. I agree that it isn't ideal that this crashes as often as it does, but this should be fixed in nimsuggest and not merely hidden by NimLSP. As I said to bung earlier though I will accept a simple tracking thing that does opt-in error recovery.
@bung87 I'm actually looking at the language server because I'd like to move as much as possible into one from the existing extensions. Mostly inspired by what the Haxe community did with VS Code Haxe and friends.
Why do you want to do that? The entire point of LSP is to move code out of existing extensions into a single extension that everyone can help maintain for the benefit of users of all IDEs.
@PMunch please see section 4 and 4.1, by definition the id field most not be present for a notification request, this is precisely in spec for json-rpc 2.0.
The indentation should be avoidable by having the start of the else body, via parentheses/do notation, and the when on the same line. Alternatively, simply discard in the else body of the when valid request as the continue should have dealt with avoiding fall through.
In regards to crashes, I agree that nimsuggest should have fixes against it but it's a matter of practicality. The number of folks who can implement such a fix and then the release turn around time of said fix is incredibly impractical. Without handling it here it then pushes out to individual clients and their maturity.
I'd recommend it as the default, as the trigger could be as simple as a user have breaking code as they're typing their way to non-breaking code, smoothing that out seems like a good user experience. Bugs are always going to happen, eventually they'll be less often, but especially for newcomers setting up, I'm worried they won't even bother reporting let alone fixing.
Don't want to make a mountain out of a molehill, thanks for taking the time to read and also maintain this project.
Why do you want to do that? The entire point of LSP is to move code out of existing extensions into a single extension that everyone can help maintain for the benefit of users of all IDEs
I think my phrasing might have been confusing, but I want to move capabilities out of the extension and into a language server.
@PMunch please see section 4 and 4.1, by definition the id field most not be present for a notification request, this is precisely in spec for json-rpc 2.0.
Oh, just looked into this and it is actually a regression. As you can see here the schema specifies that they shouldn't have an id: https://github.com/PMunch/nimlsp/blob/master/src/nimlsppkg/messages.nim#L28. But because some of the LSP clients out there are misbehaving and sending fields that shouldn't exist the allowExtra = true was added to the isValid check in whenValid. This is possibly causing some severe bugs, good catch!
In regards to crashes, I agree that nimsuggest should have fixes against it but it's a matter of practicality. The number of folks who can implement such a fix and then the release turn around time of said fix is incredibly impractical. Without handling it here it then pushes out to individual clients and their maturity.
I'd recommend it as the default, as the trigger could be as simple as a user have breaking code as they're typing their way to non-breaking code, smoothing that out seems like a good user experience. Bugs are always going to happen, eventually they'll be less often, but especially for newcomers setting up, I'm worried they won't even bother reporting let alone fixing.
Don't want to make a mountain out of a molehill, thanks for taking the time to read and also maintain this project.
Don't worry about making a mountain out of this, it's an issue we've discussed in length before and it's not an easy one. I totally agree that it's not a great user experience. But I'm afraid of the "nimlsp does nothing" scenario you might run into if it just throws an error that gets written to a log and puts the program itself in an illegal state which means it will never be able to resolve anything. That would arguably be worse because then they aren't sure if they have set it up incorrectly or if something else is the matter.
Why do you want to do that? The entire point of LSP is to move code out of existing extensions into a single extension that everyone can help maintain for the benefit of users of all IDEs
I think my phrasing might have been confusing, but I want to move capabilities out of the extension and into a language server.
Ah yes, I thought you meant the opposite!
Fixed the regression in whenValid, hopefully the LSP clients have stopped misbehaving.
I fixed the tiny regression in whenValid that does the allowExtra = true thing. This PR still has a lot of nice cleanup stuff in it..
Just having a think and I don't believe removing allow extra is necessarily the correct fix, probably a subtle violation of the LSP spec. Specifically, except for the id field in notifications others are allowed, here and elsewhere.
I think it's actually an issue where the jsonschema library needs to allow for specifying explicit non-presence and to have the allow extra check to account for that. Or alternatively check for it as I suggested, which can be done without nesting as well, either by:
- whenValid check for notification and then the first thing inside that block being a check off non-presence and continuing thereafter
- Update the whenValid template to special case (when) notification and do the non-presence check
Just having a think and I don't believe removing allow extra is necessarily the correct fix, probably a subtle violation of the LSP spec. Specifically, except for the id field in notifications others are allowed, here and elsewhere.
Are they? As far as I can tell from the specification you aren't allowed to add your own fields (apart from a couple positions that takes arbitrary objects).
I think it's actually an issue where the jsonschema library needs to allow for specifying explicit non-presence and to have the allow extra check to account for that.
Or add a allowExtraExcept option that allows you to have extra fields, except for a list of named ones.
Are they? As far as I can tell from the specification you aren't allowed to add your own fields (apart from a couple positions that takes arbitrary objects).
I believe so, at least for forwards compatibility.
This also allows for clients that might ship extra data that can be safely ignored unless it's a server implementation in the know.