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

Allow to report syntax errors using LSP error and/or "window/showMessage" notification #2

Merged
merged 6 commits into from
Nov 17, 2022

Conversation

michaliskambi
Copy link

Looking for a perfect way to report Pascal syntax errors (easily caused by missing Pascal units), this PR gives users 2 new ways to see error:

  • window/showMessage . This is supported by VS Code nicely, by Emacs too (but poorly). It is by default true (as it seems to do no harm, in the worst case clients that don't support it will just ignore it).

  • by returning LSP error instead of a "fake completion item". The fake completion item works poorly in Emacs, the LSP error is visible nicer. It is by default false to not break previous behavior. Emacs users may likely prefer to turn this on.

Both these can be controlled by new syntaxErrorCausesLspError, syntaxErrorCausesShowMessage flags that clients can pass as LSP initialization options.

The options are documented in README.md.

See michaliskambi/elisp#1 for various details about Emacs.

Codewise, I have introduced 2 new classes: TRpcNotification and TRpcMessageToClient (common ancestor for TRpcNotification and TRpcResponse). window/showMessage is a "notification" in JSON-RPC sense (see https://www.jsonrpc.org/specification#notification ) and it seemed cleanest to express it as a new class (rather then overusing TRpcResponse). Following JSON-RPC ver 2 spec, "notifications" differ from "responses" in that:

  • Notifications do not pass any id. (Trying to pass id with window/showMessage was leading to weirdest errors from both VS Code and Emacs :) ).

  • The other side (LSP client in this case) is not supposed to reply. This is nice, it means we don't have to handle anything extra (no need to dispatch some LSP client answer) to support window/showMessage.

BTW 2 fixes:

  • fake completion item contains isIncomplete, as required by LSP spec. Without it, Emacs throws scary Lisp error.

  • I noticed that TRpcPeer.Receive has accidentally Result.Reader := Reader assignment 2 times.

window/showMessage

Also, fixed the fake completion item: needs isIncomplete for Emacs to
not raise Lisp errors.
Copy link
Owner

@Isopod Isopod left a comment

Choose a reason for hiding this comment

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

Thanks for the merge request. I added a few comments. Other than that it looks good.

server/utextdocument.pas Outdated Show resolved Hide resolved
server/uinitialize.pas Outdated Show resolved Hide resolved
Copy link
Owner

@Isopod Isopod left a comment

Choose a reason for hiding this comment

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

Yep, looks good. Is this ready to be merged?

@michaliskambi
Copy link
Author

Yep, looks good. Is this ready to be merged?

Yes, all done, including last docs (README) improvements :)

@Isopod Isopod merged commit 13d298e into Isopod:master Nov 17, 2022
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