Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always return something on request #113

Merged
merged 1 commit into from
Mar 2, 2020

Conversation

giltho
Copy link
Collaborator

@giltho giltho commented Mar 2, 2020

Changed the signature of request to return either a success (store * resp) or a Response.Error
The server should always answer requests, even in case of error, it should return an error response.

Copy link
Member

@rgrinberg rgrinberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@giltho giltho merged commit 5978636 into ocaml:master Mar 2, 2020
@giltho giltho deleted the request-error-handling branch March 2, 2020 16:57
@rgrinberg
Copy link
Member

rgrinberg commented Mar 3, 2020

One issue this PR introduced is the mass spamming of errors like this in Emacs:

LSP :: Invalid_argument("get_every_pattern: [{\"filename\":\"ocaml.ml\",\"start\":{\"line\":379,\"col\":32},\"end\":{\"line\":379,\"col\":40},\"ghost\":false,\"attrs\":[],\"kind\":\"value_binding\",\"children\":[{\"filename\":\"ocaml.ml\",\"start\":{\"line\":379,\"col\":36},\"end\":{\"line\":379,\"col\":40},\"ghost\":false,\"attrs\":[],\"kind\":\"pattern (ocaml.ml[379,10448+36]..ocaml.ml[379,10448+40])\\n  Tpat_var \\\"poly/17813\\\"\\n\",\"children\":[]},{\"filename\":\"ocaml.ml\",\"start\":{\"line\":379,\"col\":40},\"end\":{\"line\":379,\"col\":40},\"ghost\":true,\"attrs\":[{\"start\":{\"line\":379,\"col\":40},\"end\":{\"line\":379,\"col\":40},\"name\":\"merlin.hole\"}],\"kind\":\"expression\",\"children\":[]}]}]") [8 times]

and like this:

LSP :: Failure("No node at given range") [7 times]

@giltho
Copy link
Collaborator Author

giltho commented Mar 3, 2020

urgh
I'm trying to understand if these errors are legitimate.
This is the server answering every request, as it should, and not silencing any issue. I guess Emacs is showing the errors ?
Now, are these bugs that I introduced or are those just errors that were there before but were invisible ?

@giltho
Copy link
Collaborator Author

giltho commented Mar 3, 2020

The "get_every_pattern" error seems to be coming from a wrong call to get_every_pattern in merlin/src/analysis/destruct. This happens after calls to code_actions being fired
I think the source is the same for the "No node given at range" thing which is due to the case_analysis call in code actions

Let's track this in an issue

@giltho giltho mentioned this pull request Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants