Introduce cquery support

[jaelsasser]

With these two patches I've got cquery up and running via eglot. Still have a few rough edges to hash out before this is ready to merge:

• Currently the per-process variables are accessed by internal methods (i.e. (cquery--init-opts)) rather than public functions.
• cquery seems fond of sending diagnostics for buffers that haven't been visited. I need to figure out whether or not this is part of the LSP spec or not and then write patches for the culprit. This seems to be allowed by the publishDiagnostics spec.
• I'd like minimal integration with hide-ifdef-mode, although that might end up needing a defadvice or upstream patch to the mode to avoid it clashing with the built-in parser. If that's the case I'll defer that to… well, either an external package or an init.el hack. Too invasive for eglot.el, I'll defer the to an add-on package after the process-local variable API is reworked.
• cquery is sending back an extra key for DocumentHighlight requests that doesn't seem to be in the language server spec. Fixed in cquery#682.

I have copyright attribution papers on file with the Emacs project. Apologies if I got the commit message format wrong, I had to read up on the style.

[joaotavora]

Thanks very much, I see you picked up on the new API a few hours after I released it. It looks good at first sight, but perhaps I'll let you trim those edges before merging, or before merging completely.

Apologies if I got the commit message format wrong, I had to read up on the style.

Seems OK at first sight. Don't worry about it and learn about C-x 4 a. It's a pain, but it's saved my a$$ many times, especially when merging.

[jaelsasser]

Been testing this out a bit. Just pushed up a quick v2 of the second commit ("Add cquery support for C/C++ projects") with a new eglot--debug to keep things usable with my Linux kernel checkout. I also fixed my usage of eglot-handle-notifications to actually work (and not just error out in the notifications log…).

And yeah, thanks for the quick turnaround on the new handler API. Was a nice surprise to see that this morning.

[jaelsasser]

Latest cquery also seems to be appending an extra role key to :textDocument/documentHighlight, which doesn't seem to be in accordance with the spec.

client-request (id:10):
(:jsonrpc "2.0" :id 10 :method :textDocument/documentHighlight :params
          (:textDocument
           (:uri "file:///home/josh/Upstream/net-next/net/core/dev.c")
           :position
           (:line 9114 :character 9)))

server-reply (id:10):
(:jsonrpc "2.0" :id 10 :result
          [(:range
            (:start
             (:line 9111 :character 2)
             :end
             (:line 9111 :character 21))
            :kind 1 :role 32)
           (:range
            (:start
             (:line 9112 :character 2)
             :end
             (:line 9112 :character 21))
            :kind 1 :role 32)
           (:range
            (:start
             (:line 9114 :character 2)
             :end
             (:line 9114 :character 21))
            :kind 1 :role 32)])

[joaotavora]

I asked you to split some things into two commits. If you're comfortable with splitting commits into two or more, feel free to rebase and force-push. If you aren't, I can do it for you if you grant me push access to this branch, or, when I finally merge it.

[joaotavora]

This is the trickiest part for an API: how to let specific clients announce different capabilities to the server as well as different initialization options.

I think these eventually become two public generic functions, eglot-client-capabilities (proc) and eglot-initialization-options (proc) that implementors can specialize on a proc object. This is the most natural way. Unfortunately, a process is not a class or even a symbol right now so that's kinda hard. We could specialize on the major-mode symbol which should, in Eglot's model have a 1-on-1 correspondence with the server, but not really, because more servers can exist that serve a specific major mode. We could hack a new property, a symbol, into the process and pass that. But that's just ugly. So perhaps we should just bite the bullet and make proc a cl-defclass. It's gonna be brutal, but in the end much cleaner. The only not so clean thing is how to let clients specify the class in eglot-server-programs. I'm thinking this (optional syntax) The syntax isn't bad at all:

;; you get an instance of a plain proc class, say `eglot-server`
(add-to-list 'eglot-server-programs `(foo-mode . ("foo" "--bar"))) 
 ;; you get an instance of `experimental-server`, which inherits from `eglot-server` 
(add-to-list 'eglot-server-program `(foo-mode . (experimental-server "exper")))

What do you think? Do you follow me? If you don't have any great objections, get ready to rebase or merge your PR onto this. It's byebye to the process-local variables thingy, they all become CL slots with proper acessors. But the acessors names stay the same so hopefully a minimum of lines are changed.

[jaelsasser]

Yeah, I follow. I'd rather deal with more cl-defmethods instead of the buffer-local hook dance I've got in place for the cache directory right now. (Which will also break as soon as we've got multiple language server implementations for a given major mode…)

I'll keep an eye on master and rebase my changes whenever you're ready. There's not that much code to manage right now, and I kinda expected as much from following the emacs-devel thread.

[joaotavora]

Good, I'm trying to rush it a bit so you still catch it today. It's a mostly mechanical thing, but I have to be careful with certain details. Almost every "proc" is being changed into "server", so it's a lot of code changed. Fortunately there are a few integration tests.

[jaelsasser]

Awesome. I'm going to hold off pushing a v3 of this until that's ready, as it sounds like just rebasing along will handle most of the review comments.

[joaotavora]

good idea. But I'm running into some eieio.el problems. Nothing serious, but I doubt I'll finish tonight it's almost 2AM over here.

[jaelsasser]

No worries then. I can hop back onto this whenever, and it doesn't have to be today. Thanks for putting in the time to restructure the user API.

[joaotavora]

Actually, I just pushed the monster refactoring to a branch called use-a-eglot-server-defclass. It passes all tests (there aren't many tho). Feel free to play around. I didn't add the two API methods yet (init options and capabilities), maybe you want to do that.

[jaelsasser]

Doesn't look like that branch got pushed to GitHub. I can for sure grab the init options work when that makes its way up here, though. Not sure how you're envisioning the capabilities API so I'll probably leave that alone.

[joaotavora]

I messed up the push, silly. I was too tired. The branch name is use-eieio-server-defclass. I'll add the new API methods eglot-server-initialization-options and eglot-server-capabilities sorry I meant eglot-client-capabilities, with blank defaults.

[joaotavora]

Nice catch, this was a bug. Though I'd put in all in one line, but that's style.

[jaelsasser]

I'll collapse it onto one line with the next rebase.

[joaotavora]

No need, I rewrote that function to run it in the correct buffer (the proc buffer change was useless anyway, a remnant from an old version).

[joaotavora]

This is where you'd call the eglot-initialization-options generic function.

[joaotavora]

Please make this a separate commit. I have to rethink the logging structure. There are two many outlets: Messages, Warnings, events... We should have at most 2, perhaps only Messages for user visible stuff and events.

[jaelsasser]

Wilco. Maybe it would make sense to have some form of eglot--debug that pushes to the events buffer instead of *Messages*? A lot of the errors it makes sense to log at the debug level are probably going to be pretty closely tied to recently received language server messages.

[joaotavora]

Yes it would make sense. Look for :internal calls to eglot-log-event. It's basically that already, tho ugly. It should really be :debug not :internal. Come up with some solid minimalist solution that adds few lines (or better yet, deletes some) and I'll merge it gladly.

[jaelsasser]

Rewrote eglot--debug to call through to eglot-log-event.

[joaotavora]

This would become simply (cl-defmethod eglot-initialization-options ((server cquery)) ...)

[joaotavora]

Please make the defgroup and defcustom a separate commit. I'd like to keep eglot's usage of these to a bare bare minimum. I'd rather hardcode the default path, and suggest a advice-add hack for those who really really to change the default.

[joaotavora]

Actually, if eglot-initialization-options is a proper method, it wouldn't really be a hack. Just have the user add an :around or subclass cquery to cquery-with-the-superspecial-cache-directory.

[jaelsasser]

Sounds good. I only added in the defcustom out of reluctance to hardcode something that users might want to change, but subclassing would give users (and, more importantly, me) a way to set it to something nonstandard like ~/.cache/cquery.

I'll drop this when I respin these patches. Being able to specialize also opens up an interesting possibility for splitting the implementation of cquery-specific stuff in two: have the bare minimum necessary to let some generate a compile_commands.json out-of-the-box and go, plus an eglot-cquery package on ELPA/MELPA that subclasses the cquery server and implements its extensions.

[joaotavora]

plus an eglot-cquery package on ELPA/MELPA that subclasses the cquery server and implements its extensions.

Exactly what I was thinking of.

[jaelsasser]

Dropped from the V3 respin.

[joaotavora]

Because you wouldn't need to add to the connect hook, this would be nuked as well.

[jaelsasser]

Thanks for the new API, the cquery setup is much cleaner now.

[joaotavora]

This would stay. By the way I noticed that you changed this from a request. Makes more sense now :-)

[joaotavora]

Latest cquery also seems to be appending an extra role key to :textDocument/documentHighlight, which doesn't seem to be in accordance with the spec.

Hmmm. I see. I think we should add that key to the eglot--lambda success handler and ignore it. Maybe issue a warning, and/or ask the cquery developers why they are sending it.

[jaelsasser]

I've got a bit of familiarity with the cquery codebase. Looks like jacobdufault/cquery@a402813 added it to track a bit more information about the type of reference, then jacobdufault/cquery@ff4ff86#diff-958fe22aed1ef4ce3bb1394f0ccd9bb6R103 included it in their textDocument:documentHighlight reply.

Opened jacobdufault/cquery#682 for this. I'll also push up an eglot commit with the unused role key as suggested, which I can revert if/when cquery returns conforming documentHighlight replies.

[jaelsasser]

@joaotavora: V3 is up with the new class-based implementation. Dropped the defcustom groups, changed the PR base to keep the diffstat clean, and implemented a spinner for cquery's indexing progress messages. With this I've got completion-at-point, symbol highlights, Flymake diagnostics, and xref.

I also rewrote 23d2b65 ("Introduce elgot--debug for unimportant message") to call into eglot--log-event with the implicit :internal level. Keeps the minibuffer usable when opening large C projects, and I've found it makes the events log easier to parse:

server-notification:
(:jsonrpc "2.0" :method "textDocument/publishDiagnostics" :params
          (:uri "file:///home/josh/Upstream/cquery/src/messages/text_document_type_definition.cc" :diagnostics
                []))

:internal-message:
(:message "Diagnostics received for unvisited (file:///home/josh/Upstream/cquery/src/messages/text_document_type_definition.cc)")

server-notification:
(:jsonrpc "2.0" :method "textDocument/publishDiagnostics" :params
          (:uri "file:///home/josh/Upstream/cquery/src/messages/workspace_did_change_configuration.cc" :diagnostics
                []))

:internal-message:
(:message "Diagnostics received for unvisited (file:///home/josh/Upstream/cquery/src/messages/workspace_did_change_configuration.cc)")

I still need to push up a respin with a working spinner, as right now its not behaving at all like it should (i.e. only setting it to a string instead of the expected form).

[joaotavora]

It would be nice if you could change all the eglot--log-event calls that aren't of the client or server type to also use this helper. But I can do that later.

[jaelsasser]

I dropped my attempt at implementing eglot--spinner for cquery. The progress messages don't really map onto eglot's view of a spinner, and cquery's pretty responsive before its initial indexing is done. They weren't buying much.

Also, I went through and converted eglot--log-event calls to use eglot--debug. There's only one non-internal user of eglot--log-event left aside from the debug function, the timeout handler, which wasn't sure about shoehorning into a pure debug message.

[kahatlen]

In my environment, the problem is not so much that cquery sends diagnostics, but more that eglot warns even though the diagnostics message says there is no problem. Example diagnostics message:
{"jsonrpc":"2.0","method":"textDocument/publishDiagnostics","params":{"uri":"file:///usr/include/setjmp.h","diagnostics":[]}}
Note that the "diagnostics" array is empty.

An alternative fix might be to keep eglot--warn, but only call it if the length of the diagnostics array is greater than zero. Then we'll still see the warnings for the files that actually have a problem, but silence it for the vast majority of files that don't have a problem.

[kahatlen]

The code in the pull request seems to work fine in my environment. Thank you!

I see these warnings when I byte compile eglot.el, though:

eglot.el:1633:1:Warning: Unused lexical variable ‘activeThreads’
eglot.el:1633:1:Warning: Unused lexical argument ‘server’
eglot.el:1638:1:Warning: Unused lexical variable ‘inactiveRegions’
eglot.el:1638:1:Warning: Unused lexical variable ‘uri’
eglot.el:1638:1:Warning: Unused lexical argument ‘server’
eglot.el:1643:1:Warning: Unused lexical variable ‘symbols’
eglot.el:1643:1:Warning: Unused lexical variable ‘uri’
eglot.el:1643:1:Warning: Unused lexical argument ‘server’

Also, if I hit C-u M-. and type the name of a symbol that doesn't exist and hit enter, the cquery process crashes. But I doubt that it's your fault.

[joaotavora]

@kahatlen thanks for testing this, it's added confidence for a merge.

I see these warnings when I byte compile eglot.el, though:

I'll take care of this later today when I merge this PR.

Also, if I hit C-u M-. and type the name of a symbol that doesn't exist and hit enter, the cquery process crashes. But I doubt that it's your fault.

Indeed it isn't. But it's worth investigating all the same.

In my environment, the problem is not so much that cquery sends diagnostics, but more that eglot warns even though the diagnostics message says there is no problem. Note that the "diagnostics" array is empty.

Thanks for pointing that out. I'll probably perform the fix you suggest after I merge this.

[joaotavora]

So I merged this. Thanks very much. Here are the comments on the merge commit.

Merge branch 'cquery-support' into master
    
    The conflicts in eglot.el where fixed by calling the new eglot--debug
    helper coming from 'cquery-support'. This helper was converted to
    allow a non-string format passed directly to eglot--log-event.
    
    Also fixed some compilation warnings.
    
    * eglot.el (eglot--debug): Allow non-string FORMAT to be a JSON
    object.  (eglot-handle-notification :$cquery/progress)
    (eglot-handle-notification :$cquery/setInactiveRegions)
    (eglot-handle-notification :$cquery/publishSemanticHighlighting):
    Solve compilation warnings.

The handle-notification methods do little more than repeat what the default behaviour for ignoring notifications already does. But I'm gonna let them stay for a little more while in case you want to implement something in them.

[jaelsasser]

Awesome, thanks for merging! Sorry about the unused lexical variable warnings -- I didn't realize that _key would still match on key.

Feel free to drop the three no-op defmethods, too. I'm going to try and get a PR merged against cquery proper that lets users disable the frequent $cquery/setInactiveRegions and $cquery/publishSemanticHighlighting notifications. They make following the events buffer a pain -- the semantic highlighting notifications are pretty beefy -- and there's no real need to go through the whole nooo-to-supress-warning dance if we can just avoid them.

(I'm still looking at hide-ifdef-mode integration via $cquery/setInactiveRegions, too, but that'll become a small separate package if it ends up being too hacky.)

[joaotavora]

(I'm still looking at hide-ifdef-mode integration via $cquery/setInactiveRegions, too, but that'll become a small separate package if it ends up being too hacky.)

Yeah, try it here as a PR and if it's too hacky we can move it away.

[joaotavora]

@jaelsasser , BTW, i've been trying cquery (not the master version though) and recently fixed some bugs.

Be sure to try it out. I've also been running into another more serious bug: after renaming a symbol, cquery appears to lose the correct state of the buffer's contents. I don't know if this is cquery's or eglot's fault. Can you confirm this? Can you help me ask cquery for its version of a buffer's contents?

[jaelsasser]

Oh, nice, you got the zero-length diagnostic bug. I was trying to figure out whether to blame cquery, clang, or flymake for that before raising it on here. (I think its squarely clang's fault -- cquery just reports back what libclang has to say about the diagnostics). Flymake is running a lot smoother after that commit -- thanks!

I can definitely confirm that cquery seems to end up with corrupted file contents after applying workspace edits. I renamed a priv variable to adapter and got a weird document state. I then added :enableIndexOnChange t to cquery's eglot-initialization-options and got better results (apparently its disabled by default) and started getting back pter: struct ctx_priv *priv = NULL in eldoc and highlights that only covered pter.

Seems like it could be one of two things:

• eglot is incorrectly sending textDocument/didChange notifications after applying edits that cquery has already applied to its in-memory file copy, causing cquery's view of the file to go out of sync.
• One or more of the clang-backed indexing components in cquery is incorrectly using the on-disk file rather than the in-memory working copy.

All the weirdness goes away after a textDocument/didSave hits cquery, though, which causes some reloads and re-indexes.

I was originally going to suggest using the $cquery/fileInfo extension to get the server's view of the file, but that doesn't look like it includes a dump of the directory (response definition is here). I'll instead implement something like $cquery/fileDump to get cquery's working copy of the buffer.