Add lsp (cquery and ccls) backend support to c-c++ layer #11242

cormacc · 2018-08-30T20:55:20Z

N.B. This PR should be considered as on hold pending acceptance/rejection of #11351 . The current commit ~~is functional for evaluation purposes, however the keybinding logic will be updated to reflect the navigation style preferences in #11351 if that gets merged.~~ includes the lsp layer changes to facilitate evaluation in the interim

Migrated the cquery and ccls work from a separate lsp-c-c++ layer to the existing c-c++ layer.
Enabled by adding the following to your spacemacs dotfile.

  (setq-default dotspacemacs-configuration-layers
                '(; ... other layers ...
                  (c-c++ :variables c-c++-backend 'lsp-cquery))) ;or  'lsp-ccls

More detailed information has been added to the README.

The clang-format- keybindings will be added automatically if either lsp backend is selected.

This is just to get the ball rolling -- appears to work as the previous lsp-c-c++ layer based on some quick and dirty testing with both cquery and ccls backends.

The rough edges I'm aware of so far...

Setting c-c++-lsp-sem-highlight-rainbow to t results in an error. The macros <cquery/ccls>-use-default-rainbow-sem-highlight don't seem to be in scope when the layer is loaded, though they're provided by their respective packages -- suggestions welcome. This did work in some earlier incarnation of the lsp-c-c++ layer, but had stopped doing so before this most recent bit of integration. No idea whether it's something I've done or an upstream change affecting scope of package macros.
Fixed based on myrgy's suggestion
RTags is enabled by the existing mechanism of setting c-c++-enable-rtags-support to t. It would be neater to set c-c++-backend to 'rtags instead, but I don't know what the policy is on changing layer configuration mechanisms. Again, guidance welcome. The existing config var may be nil t or 'no-completion. If moving to using c-c++-backend desirable, would it be preferable to
(a) allow values of 'rtags and 'rtags-no-completion, or
(b) just rtags and add a new c-c++-rtags-completion variable with a default value of t?
RTags now enabled using the new c-c++-backend config.el variable. Completion enabled by default, disabled by setting new c-c-++-enable-rtags-completion config.el variable to nil

cormacc · 2018-08-30T21:07:41Z

@myrgy You might take a look when you get a chance?

@sdwolfz I've derived the backend selection/configuration logic from some of the recent examples you suggested (mostly some useful discussion in the python layer PR)

@MaskRay, @sebastiencs either of you have any enlightenment to offer on the -use-default-sem-rainbow-highlight issue? Also any useful backend-specific bindings I've missed?

Compro-Prasad · 2018-08-31T10:59:30Z

I think there should be an universal backend variable. This would prevent conflicts in between backends as the variable accepts only one symbol.

cormacc · 2018-08-31T11:10:43Z

@Compro-Prasad That's my own view too - just want to get confirmation from maintainers that that's acceptable before making the change.

myrgy · 2018-08-31T19:34:59Z

@cormacc , Thanks a lot for outstanding work!
Will take a look on Monday.
What consider ccls/cquery-use-default-sem-rainbow-highlight since it's macro we can make an if check and call appropriate method or create special macro, which will perform call like you c-c++-lsp-call-function, but I think since there is only one method if check would be enough.

layers/+lang/c-c++/README.org

layers/+lang/c-c++/funcs.el

sdwolfz · 2018-08-31T21:01:01Z

Some comments related to variables, docs and formatting. I have not tested it locally yet.

myrgy · 2018-08-31T21:14:46Z

@cormacc , works fine for me. (y)

cormacc · 2018-09-01T12:58:27Z

Amended the commit incorporating all the requested changes (I think).

@myrgy Your suggestion addressed the rainbow semantic highlighting issue -- I was trying to apply a macro as I would a function :S

layers/+lang/c-c++/config.el

MaskRay · 2018-09-02T19:53:36Z

layers/+lang/c-c++/config.el

+initialized. `c-c++-lsp-project-whitelist' is checked first, then this,
+if no pattern matches the project root, lsp-c-c++ will be initialized.")
+
+(defvar c-c++-lsp-sem-highlight-method 'font-lock


I would suggest removing this option and refer users to ccls/cquery wiki pages. Some people think semantic highlighting distracting.

MaskRay · 2018-09-02T19:57:00Z

layers/+lang/c-c++/funcs.el

+  (spacemacs//c-c++-lsp-call-function "spacemacs//c-c++-lsp-" "-customise-lsp-ui-peek")
+
+  (defun c-c++/callers () (interactive) (lsp-ui-peek-find-custom 'callers (spacemacs//c-c++-lsp-string "$" "/callers")))
+  (defun c-c++/derived () (interactive) (lsp-ui-peek-find-custom 'derived (spacemacs//c-c++-lsp-string "$" "/derived")))


$ccls/derived $cquery/derived have both been removed. Users should instead use lsp-goto-implementation

ccls has a flattened $ccls/inheritanceHierarchy to provide multi-level information but I think it is too implementation-specific and does not worth the common configuration. https://github.com/MaskRay/ccls/wiki/Emacs#ccls-cross-reference-extension

(defun ccls/derived () (interactive) (lsp-ui-peek-find-custom 'derived "$ccls/inheritanceHierarchy" (append (lsp--text-document-position-params) '(:flat t :level 3 :derived t))))

layers/+lang/c-c++/funcs.el

sdwolfz · 2018-09-03T19:38:01Z

I was thinking of merging #10454 before this layer, hope it won't cause to much trouble for this PR.

cormacc · 2018-09-04T07:57:41Z

Rebased
Incorporated feedback from @MaskRay (thanks!).
Corrected a projectile post-config hook that wasn't being applied.
Added c-c++-adopt-subprojects layer config variable (defaults to nil) implementing this recommendation from ccls and cquery wikis:
https://github.com/MaskRay/ccls/wiki/Emacs#projects-with-subprojects

@sdwolfz I'll rebase again when you've merged #10454

myrgy · 2018-09-05T06:13:05Z

Works fine for me!

rcoacci · 2018-09-05T12:22:13Z

I think I might have found a bug. I'm using cquery as backend, and when doing
projectile-find-other-file I get the following error:
helm-M-x: Wrong type argument: stringp, (".cquery_cached_index" ".ccls-cache")
The layer is configured as:

(c-c++ :variables
    c-c++-default-mode-for-headers 'c++-mode
    c-c++-backend 'lsp-cquery)

Edit: upon further investigation, it seems

(add-to-list 'projectile-globally-ignored-directories
        (if c-c++-lsp-cache-dir c-c++-lsp-cache-dir '(".cquery_cached_index" ".ccls-cache")))

Is not the correct way of doing it:

projectile-globally-ignored-directories is a variable defined in ‘projectile.el’.
Its value is
((".cquery_cached_index" ".ccls-cache")
".idea" ".ensime_cache" ".eunit" ".git" ".hg" ".fslckout" "FOSSIL" ".bzr" "_darcs" ".tox" ".svn" ".stack-work")
Original value was
(".idea" ".ensime_cache" ".eunit" ".git" ".hg" ".fslckout" "FOSSIL" ".bzr" "_darcs" ".tox" ".svn" ".stack-work")

cormacc · 2018-09-05T12:43:06Z

Thanks @rcoacci - yesterdays amendment resulted in appending a list to projectile-globally-ignored-directories rather than individual strings. Fixed/amended/pushed

rcoacci · 2018-09-05T13:32:58Z

Another thing: emacs is hanging a lot when the using the layer, it seems related to the server being busy (indexing probably, top shows a lot of cpu being used by cquery).
Also emacs seems to be doing a lot of background work even when its minimized.
It's a very large C++ project, with about 2k source files and lots of libraries.

cormacc · 2018-09-06T15:57:32Z

@rcoacci Can't comment there I'm afraid. My use to date has been almost entirely on an embedded C project, which has about 1300 source files, but very few external dependencies. Performance satisfactory here. Some discussion here that may be of some use to you jacobdufault/cquery#278

rcoacci · 2018-09-06T19:11:36Z

layers/+lang/c-c++/README.org

+To enable the rainbow semantic highlighting colour theme, set =c-c++-lsp-sem-highlight-rainbow= to =t=.
+
+***** Additional configuration options
+Both supported backends store their index cache in a subdirectory of the project dir by default. This can be overridden by


This doesn't seem correct. The default behavior of the layer is to use a subdirectory of spacemacs cache dir as index cache.

Good catch - thanks. Changed the default config based on feedback from sdwolfz and forgot to update the README. Corrected / commit amended

I don't think it is the right way to do it. The stuff in .cache is from .emacs.d and not from local project. It is nice to have an option but I don't think it is mandatory to do it.

rcoacci · 2018-09-06T20:11:55Z

@cormacc I'm willing to blame my project config for now. If I remove the compile_commands.json from the project, it seems to work better.
I noticed from the logs that it keeps reindexing(?) files with "Arguments have changed for ..."
I'll test a bit.

MaskRay · 2018-09-08T21:34:31Z

@rcoacci Give ccls a try? It leverages some preamble optimization which should accelerate diagnostics,completion a bit (libclang cannot use this trick). I've also just pushed a commit to optimize indexing when the current file is opened.

Completion and diagnostics are done in two separate threads (in both ccls and cquery). In ccls, the preamble (the #include part of your main source file) is shared by the two threads.
If you can live without diagnostics, set diagnostics.onChange: false (cquery uses a different name).

For a simple project that you don't want to save cache to the disk, and if you want to re-index for every change (cautious: it may hurt performance), use the following initialization options:

{
  "cacheDirectory": "",
  "index": {"onChange": true}
}

In Emacs:

(setq ccls-cache-dir "")
(setq ccls-extra-init-params '(:index (:onChange t)))

There is another ccls-specific feature semantic navigation for which I do not recommend adding default key bindings. (ccls-navigate "D") ;; and "L" "R" "U"
https://www.reddit.com/r/emacs/comments/9dg13i/cclsnavigate_semantic_navigation_for_cc/

kvaneesh · 2018-09-10T16:07:28Z

Is there a way to disable lsp-ui-peek features/keybindings and only use xref-* interfaces provided by lsp-mode ? Right now I am adding lsp-ui to dotspacemacs-excluded-packages. But things like xref-find-definitions and all don't have any key binding.

d12frosted · 2018-10-18T16:40:35Z

Wow. What a PR.

@sdwolfz what do you think about it?

sdwolfz · 2018-10-18T16:53:31Z

I'm trying to find the time to review this properly and merge, but I'm optimistic about it.

cormacc · 2018-10-20T17:58:06Z

@sdwolfz I've rebased the blocking #11351 on develop if you want to take a look at that first. Think it should be a pretty safe merge as the changes are in default core keybinding functions I contributed myself and no other layers are using. I'll rebase and push a new commit of this PR once that one gets resolved either way.

iromise · 2018-10-29T13:32:27Z

layers/+lang/c-c++/README.org

+
+***** Additional configuration options
+Both lsp backends are configured to store their index cache in a subdirectory of =.emacs.d/cache=. This can be overridden by
+specifying an explicit =lsp-c-c++-cache-dir=. Setting this value to a relative path will cause the index cache to be placed in a


Hi, lsp-c-c++-cache-dir should be c-c++-lsp-cache-dir because

;; c-c++-lsp-backend variables (defvar c-c++-lsp-cache-dir nil "Cache directory. Absolute and relative paths supported.")

Thanks iromise -- corrected

cormacc · 2018-11-05T10:44:45Z

@myrgy I've pushed an updated version of this based on #11351 that I've been sitting on for a while - you might let me know if it resolves the ccls issues you were patching around locally if you get a chance to evaluate? I haven't had any issues in the last few weeks, though admittedly have been doing more python and planning/scoping than C in that period...

myrgy · 2018-11-06T09:18:08Z

@cormacc , thank you! I'll give a try on thursday

svenihoney

@cormacc I have the current version in productive development since yesterday and so far it is working properly. Replaces my own local ccls integration and I am more than happy with your current solution.

svenihoney · 2018-11-05T11:25:54Z

layers/+lang/c-c++/README.org

+  (setq-default dotspacemacs-configuration-layers
+                '((c-c++ :variables
+                         c-c++-adopt-subprojects t
+                         c-c++-backend 'ccls


Above the backend variable reads 'lsp-ccls, here just 'ccls.

Suggested change

c-c++-backend 'ccls

c-c++-backend 'lsp-ccls

Thanks svenihoney - updated the readme. @sdwolfz , you happy enough to use the 'Apply suggestion' feature as a general rule? Looks very convenient for this sort of change (oversights in the README). I'd imagine it probably results in a new commit though rather than an amendment (which appears to be the recommended approach in the beginner's guide to PRs)? Regardless, went with the traditional git commit --amend, git push --force in this instance, pending enlightenment :)

It's a nice feature but I would like to have as few commits as possible at the end. So if they are small fixups then I'd preffer they be squashed anyway.

sdwolfz

Looks good, I'll merge it this evening. Thanks everybody for the effort, and sorry it took so long to review.

sdwolfz · 2018-11-06T22:20:51Z

@cormacc Looks like you need to rebase on top of the current develop.

<<Amendment 1 01/09/2018>> Incorporated initial feedback from multiple sources (PR discussion) <<Amendment 2 04/09/2018>> Incorporated feedback from MaskRay. Fixed projectile post-config hooks. Added `c-c++-adopt-subprojects' layer option. <<Amendment 3 07/09/18>> Corrected cache section in README to describe new cache defaults introduced by Amendment 2. <<Amendment 4 13/09/18>> Corrected config var names in readme. Rebased on current develop branch tip. <<Amendment 5 14/09/18>> Minor adaptations for compatibility with upstream ccls changes. <<Amendment 6 21/09/18>> Bindings updated for consistency with lsp layer lsp-navigation config variable. <<Amendment 7 1/11/18>> Rebased <<Amendment 8 06/11/18>> Incorporated readme correction from svenihoney. <<Amendment 9 06/11/18>> Updated c-c++/refresh-index wrapper for ccls function renamed upstream. Moved 2 bindings from 'l' to 'b' to reflect updated lsp-layer mnemonic

cormacc · 2018-11-07T00:30:26Z

@sdwolfz Rebased / pushed after merge of #11351

myrgy · 2018-11-08T09:30:39Z

@cormacc , I gave a try - seems to work fine for me 👍. (ccls)

sdwolfz · 2018-11-08T09:56:08Z

@JAremko the Validate Documentation (required) job is failing for this build and I have no idea why, can you please take a look?

JAremko · 2018-11-08T15:03:32Z

layers/+lang/c-c++/README.org

+                '((c-c++ :variables c-c++-backend `lsp-cquery))) ;or 'lsp-ccls
+#+END_SRC
+
+N.B. The [[../../+tools/lsp/README.org][LSP layer]] will be loaded automatically if either backend is selected.


@sdwolfz Should be [[file:../../+tools/lsp/README.org][LSP layer]]
Actually the current one is also valid but we a bit strict about it. Mb too strict 🤔

I'll add it to autofixes.

sdwolfz · 2018-11-08T23:06:20Z

Thank you ❤️!

There was a problem with the c-c++-backend variable when it was unbound, an error was thrown since the layers.el file is loaded before the config.el file, but I addressed that and amended it to the commit: 445f6af.

Cherry-picked into develop branch, you can safely delete your branch.

cormacc · 2018-11-09T21:22:18Z

Great! Thanks @sdwolfz

MaskRay · 2018-11-11T21:18:40Z

@sdwolfz Please also take a look at #11583

I didn't mention it as this was already taking too long.

iddm · 2020-01-21T11:18:37Z

Sorry, I am kinda new to all of this LSP thing. How can I setup it easily for my spacemacs?

Compro-Prasad · 2020-01-21T16:11:00Z

@vityafx Please open an issue if you find something missing or open a PR if you have a solution or discuss about this somewhere else.

cormacc force-pushed the c-c++-lsp-backend branch 2 times, most recently from 5cb6114 to dcae0cc Compare August 30, 2018 21:04

cormacc changed the title ~~Added cquery and ccls backend support to c-c++ layer~~ Add lsp (cquery and ccls) backend support to c-c++ layer Aug 30, 2018

sdwolfz suggested changes Aug 31, 2018

View reviewed changes

cormacc force-pushed the c-c++-lsp-backend branch from dcae0cc to 1873303 Compare September 1, 2018 12:56

MaskRay reviewed Sep 2, 2018

View reviewed changes

layers/+lang/c-c++/config.el Show resolved Hide resolved

MaskRay reviewed Sep 2, 2018

View reviewed changes

layers/+lang/c-c++/funcs.el Outdated Show resolved Hide resolved

cormacc force-pushed the c-c++-lsp-backend branch from 1873303 to 499649a Compare September 4, 2018 07:53

cormacc force-pushed the c-c++-lsp-backend branch from 499649a to fb150a9 Compare September 5, 2018 12:41

rcoacci reviewed Sep 6, 2018

View reviewed changes

cormacc force-pushed the c-c++-lsp-backend branch from fb150a9 to f89d121 Compare September 6, 2018 22:09

iromise reviewed Oct 29, 2018

View reviewed changes

ristomet mentioned this pull request Oct 31, 2018

company-lsp + cquery does not worked well doomemacs/doomemacs#935

Closed

cormacc force-pushed the c-c++-lsp-backend branch from a4032ed to 0635b4f Compare November 5, 2018 10:06

cormacc force-pushed the c-c++-lsp-backend branch from 0635b4f to b671053 Compare November 6, 2018 10:09

svenihoney reviewed Nov 6, 2018

View reviewed changes

cormacc force-pushed the c-c++-lsp-backend branch from b671053 to 1cfd3ff Compare November 6, 2018 13:30

sdwolfz approved these changes Nov 6, 2018

View reviewed changes

cormacc force-pushed the c-c++-lsp-backend branch from 1cfd3ff to 37f658b Compare November 6, 2018 16:03

cormacc force-pushed the c-c++-lsp-backend branch from 37f658b to be587cc Compare November 7, 2018 00:27

JAremko reviewed Nov 8, 2018

View reviewed changes

sdwolfz closed this Nov 8, 2018

MaskRay mentioned this pull request Nov 11, 2018

Some tweak to c-c++ layer #11583

Merged

This was referenced Nov 13, 2018

Add ccls/cquery (language server) to c-c++ layer #10134

Closed

How do I use LSP with Java/Python/Rust/C++? #11161

Closed

cormacc deleted the c-c++-lsp-backend branch May 11, 2020 09:01

Add lsp (cquery and ccls) backend support to c-c++ layer #11242

Add lsp (cquery and ccls) backend support to c-c++ layer #11242

Conversation

cormacc commented Aug 30, 2018 • edited Loading

cormacc commented Aug 30, 2018

Compro-Prasad commented Aug 31, 2018 • edited Loading

cormacc commented Aug 31, 2018

myrgy commented Aug 31, 2018 • edited Loading

sdwolfz commented Aug 31, 2018

myrgy commented Aug 31, 2018

cormacc commented Sep 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdwolfz commented Sep 3, 2018

cormacc commented Sep 4, 2018

myrgy commented Sep 5, 2018

rcoacci commented Sep 5, 2018 • edited Loading

cormacc commented Sep 5, 2018

rcoacci commented Sep 5, 2018

cormacc commented Sep 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Compro-Prasad Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

rcoacci commented Sep 6, 2018

MaskRay commented Sep 8, 2018 • edited Loading

kvaneesh commented Sep 10, 2018

d12frosted commented Oct 18, 2018

sdwolfz commented Oct 18, 2018

cormacc commented Oct 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cormacc commented Nov 5, 2018

myrgy commented Nov 6, 2018

svenihoney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdwolfz left a comment

Choose a reason for hiding this comment

sdwolfz commented Nov 6, 2018

cormacc commented Nov 7, 2018

myrgy commented Nov 8, 2018

sdwolfz commented Nov 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdwolfz commented Nov 8, 2018

cormacc commented Nov 9, 2018

MaskRay commented Nov 11, 2018

iddm commented Jan 21, 2020

Compro-Prasad commented Jan 21, 2020

cormacc commented Aug 30, 2018 •

edited

Loading

Compro-Prasad commented Aug 31, 2018 •

edited

Loading

myrgy commented Aug 31, 2018 •

edited

Loading

rcoacci commented Sep 5, 2018 •

edited

Loading

Compro-Prasad Sep 7, 2018 •

edited

Loading

MaskRay commented Sep 8, 2018 •

edited

Loading