lsp-layer additions: configuration and building blocks for derived layers.

[cormacc]

This adds some useful defaults to the lsp layer (in a new config.el), and adds some building block functions to facilitate development of derived layers.

It declares a number of major mode prefixes for typical classes of language server function, and provides the function spacemacs/lsp-bind-keys-for-mode which can be used by derived layers to provide a consistent set of keybindings common core language-server functions.

See README.org and the commit message for further details and a trail of the multiple amendments since initial submission.

A usage example can be found in the lsp-c-c++ layer, in my branch of the spacemacs repo:
https://github.com/cormacc/spacemacs/tree/lsp-c-c%2B%2B-layer
N.B. The lsp-c-c++ layer will not be the subject of a PR as is, but form the basis of a new PR against the c-c++ layer after this gets merged. Please do not provide any feedback relating to it in the discussion thread relating to this PR.

This is partially derived from @MaskRay's personal config and has benefited from some feedback from him and quite a number of other contributors to this discussion.

[MaskRay]

imenu is already bound at SPC s j and SPC j i

[cormacc]

Removed...

[MaskRay]

ffap does not use LSP.

[cormacc]

Also removed. Originally included that a binding like this was appropriate in a general purpose lsp layer, but maybe there's a better idiomatic equivalent, i.e an lsp method that would jump to a header file when invoked on a c source #include, or the appropriate ruby source file when invoked on a 'require...' statement etc.?

[MaskRay]

Great job! Hope this can be merged soon.

[cormacc]

Thanks Maskray -- amended the commit with your suggestions -- also some corrections to keybindings and file headers in line with CONVENTIONS.org.

You got any insight into the failing CI tests? They're all the same issue. Probably something daft I've done..

Test test-declare-layers--distribution-layer-is-second condition:
(ert-test-failed
((should
(eq 'spacemacs-base
(second configuration-layer--used-layers)))
:form
(eq spacemacs-base spacemacs-defaults)
:value nil))

[MaskRay]

No idea why CI fails... all recent PRs fail..

Regarding cquery, if the point is on a #include directive, you can invoke textDocument/definition (xref-find-definition, lsp-ui-peek-find-definitions, ...) to jump to the included file.

Implementation https://github.com/cquery-project/cquery/blob/master/src/messages/text_document_definition.cc#L116
Other cross reference candies https://github.com/cquery-project/cquery/wiki/FAQ#definitions

[sebastiencs]

I replied to @MaskRay in the comments, sorry for being late

[MaskRay]

I cannot see your comments in the web view but I remember I saw it somewhere...

For lsp-ui-peek performance issue, code bases of most languages wouldn't be large enough to have more than 1000 references and thus make Emacs slow. I think unfolding all files is a good default value.

[sebastiencs]

@MaskRay The issue is not the number of references but the number of files to open.
For each file it will call font-lock-ensure to fontify the whole file. With 10 big files it can takes +5 seconds to just open the peek view.

[bkchr]

@cormacc you are missing the spacemacs/lsp-format-buffer function. You declare a shortcut, but the function does not exist :)

[cormacc]

@bkchr Thanks -- corrected in an amendment to the commit (stripped the incorrect 'spacemacs/' prefix, which I must have added at some stage while refactoring the various changes between this PR and the new cquery layer). I haven't actually been using this keybinding, as believe cquery (the only lsp server I'm using) only includes the functionality if built using the --use-clang-cxx option, which the aur cquery-git package doesn't do out of the box.

https://github.com/cquery-project/cquery/wiki/Formatting
https://github.com/cquery-project/cquery/wiki/Build-%28Waf%29

[bkchr]

@cormacc Yeah thanks :) I'm using your changes with the rust language server and I wondered why it does not work. Today I finally got time to look into it :D

[cormacc]

Saw a recent commit to lsp-mode added lsp-goto-type-definition and lsp-goto-implementation so added bindings for these. Moved all the symbol browser type bindings under 'j' (jump), as 'g' was getting fairly crowded. So loose rule is that anything with a single target should go under g, whereas those with multiple targets (lsp-ui-imenu, lsp-ui-peek bindings) goes under j.

Amended my personal new-cquery-layer branch in the same way.
https://github.com/cormacc/spacemacs/tree/new-cquery-layer

@bkchr I've also raised a new PR against lsp-mode that adds a capability check to the lsp-format-buffer call. If that gets merged, you'll see an appropriate signal if the language server doesn't support the functionality.

[bkchr]

@cormacc, nice.

[franksn]

@cormacc can you set default cquery-sem-highlight-method to nil?
As people won't likely use it unless their theme support it, and the default spacemacs-theme hasn't supported it either.

[cormacc]

@franksn Semantic highlighting appears to me to work using the standard spacemacs theme -- at least with mode set to either 'font-lock or 'overlay I get colour coded tokens etc. -- or have I set my expectations too low? :)

[shuwens]

@cormacc Looks like we are blocked here for a while, any plan to move forward? If it is the problem in circleci, can we get some help from the spacemacs community? I assume we can gently ask for help in the gitter chat room?

[cormacc]

I think (purely on the basis of eyeballing recent commit history) that the principal spacemacs developers have been working on a large-ish reorganisation of the core layers and that's whats causing the circle CI failures and holding up the merge of all/most recent PRs. Seem to have merged this work to develop ?today? -- anyway I've just rebased my branch on the latest commit to develop and all the circle CI tests are passing now.

As a heads-up, the recent upstream change-set renamed spacemacs/lsp-set-default-jump-handlers to spacemacs//setup-lsp-jump-handler (note the double forward-slash), so you'll need to update any derived layers accordingly.

[cormacc]

@syl20bnr Just rebased this branch on latest commit on develop (i.e. 7aca5ae) before seeing you'd started work on it.

[cormacc]

@myrgy Incorporated your and @MaskRay's suggestions. Also tidied up some hasty crap left in Friday. Thank you both

[cormacc]

Greatly reduced the duplication between configuration for cquery and ccls backends by introducing a couple of helper functions at the top of funcs.el

I've a couple of niggly bits annoying me -- see the excerpt from funcs.el below. The two commented out regions of lsp-c-c++//common-config are more elegant/concise, but result in the errors mentioned in the comments. Any ideas?
edit resolved the issue with evil-make-overriding-map using (symbol value ...)
edit d'oh -- commas in a lisp list :)

(defun lsp-c-c++//backend-string (prefix suffix) (concat prefix (symbol-name lsp-c-c++-backend) suffix))
(defun lsp-c-c++//backend-symbol (prefix suffix) (intern (lsp-c-c++//backend-string prefix suffix)))
(defun lsp-c-c++//call-backend-function (prefix suffix &rest args) (apply (lsp-c-c++//backend-symbol prefix suffix) args))
;; (defun lsp-c-c++//funcall-interactively (prefix suffix) (funcall-interactively (lsp-c-c++//backend-symbol prefix suffix)))
(defun lsp-c-c++//funcall-interactively (prefix suffix &rest args) (funcall-interactively (lsp-c-c++//backend-symbol prefix suffix) args))
(defun lsp-c-c++//funcall-interactively-no-args (prefix suffix) (funcall-interactively (lsp-c-c++//backend-symbol prefix suffix)))
(defun lsp-c-c++//set-backend-symbol (prefix suffix value) (set (lsp-c-c++//backend-symbol prefix suffix) (symbol-value value)))
(defun lsp-c-c++//set-backend-config (param prefix suffix) (when (symbol-value param) (lsp-c-c++//set-backend-symbol prefix suffix param)))
(defun lsp-c-c++//apply-backend-config (suffix)
  (lsp-c-c++//set-backend-config (intern (concat "lsp-c-c++-" suffix)) nil (concat "-" suffix)))

(defun lsp-c-c++//enable ()
  (condition-case nil
    (lsp-c-c++//call-backend-function "lsp-" "-enable")
    (user-error nil)))

(defun lsp-c-c++//common-config ()
  (lsp-c-c++//customise-lsp-ui-peek)
  (lsp-c-c++//wrap-backend-functions)
  (setq-default flycheck-disabled-checkers '(c/c++-clang c/c++-gcc))

  (dolist (param '("executable" "extra-init-params" "cache-dir" "project-whitelist" "project-blacklist" "sem-highlight-method"))
    (lsp-c-c++//apply-backend-config param))
  (when lsp-c-c++-sem-highlight-rainbow
    (lsp-c-c++//call-backend-function nil "-use-default-rainbow-sem-highlight"))

  (dolist (mode c-c++-modes)
    (spacemacs/lsp-bind-keys-for-mode mode)
    (lsp-c-c++//bind-keys-for-mode mode))

  (evil-set-initial-state '(lsp-c-c++//backend-symbol nil "-tree-mode") 'emacs)
  ;;evil-record-macro keybinding clobbers q in cquery-tree-mode-map for some reason?
  (evil-make-overriding-map (symbol-value (lsp-c-c++//backend-symbol nil "-tree-mode-map"))))

[rcoacci]

Is there anything preventing this from being merged?
Anything we can do to help?

[sdwolfz]

This PR is a bit overwhelming due to it's size, both code and thread, but I feel like I can focus my effort on it and guide you to how to get it merged.

The main thing I want is to have the lsp functionality inside the current c-c++ layer, instead of having another layer.

Let me know if there are other changes or PRs I can focus on that will simplify your work on this one and I will get right to it (to the limits of my ability).

[cormacc]

@sdwolfz That would be great. This PR contained just the lsp layer additions for most of its life -- I'd been maintaining the lsp-c-c++ layer in a branch of my own fork of the spacemacs repo, but added it recently as I was wondering whether the lack of a usage example was (one of) the reason(s) it wasn't getting any love.

The intent with the lsp-layer improvements was to provide a consistent set of of prefixes for core keybindings for all/most of the lsp layers for different languages. And a few of the other guys/gals working on lsp-based layers seem to think that's useful. So how does this sound as a plan of action...

1. Remove the lsp-c-c++ layer from this PR and work with you on getting the lsp layer changes merged
2. Integrate the lsp-c-c++ layer with the c-c++ layer and raise a new PR. This should (famous last words) be pretty straightforward as lsp-c-c++ is pretty orthogonal to c-c++, which it declares as a layer dependency anyway

This sound like a reasonable approach?

Re. 2 -- my original reason for creating a separate layer rather than taking this route originally were some responses to @MaskRay 's original suggestion of adding cquery to the c-c++ layer here: #10143
@quicknir was working on a some tidy-up of the c-c++ layer at the time and appeared to have progressed quite a bit, but don't know what the current status of that work is.

[sdwolfz]

@cormacc sounds like a good plan.

I recently merged the lsp support for Golang and Java so you can take a look there for hints on how to add your changes to the c-c++ layer.

Let me know when you are ready and I will start reviewing the code.

[cormacc]

@sdwolfz OK - that latest commit amendment has stripped all the c-c++ stuff from this PR. So anyone who's actually using the lsp-c-c++ layer shouldn't pull this update :)

edit I've moved the lsp-c-c++ layer into a new branch of my own fork, here: https://github.com/cormacc/spacemacs/tree/lsp-c-c%2B%2B-layer

N.B. this branch won't receive any updates -- just a placeholder til we get the lsp-layer PR merged and I can rework lsp-c-c++ as a PR against the c-c++ layer.

[sdwolfz]

The CONVENTIONS.org file requires the SPC m e prefix to contain code evaluation functionality. Is this what lsp-execute-code-action does? As far as I can tell it's a different thing.
Here's the definition of the SPC m e prefix:

Live evaluation of code is under the prefix ~SPC m e~.

| Key     | Description                                   |
|---------+-----------------------------------------------|
| ~m e $~ | put point at the end of the line and evaluate |
| ~m e b~ | evaluate buffer                               |
| ~m e e~ | evaluate last expression                      |
| ~m e f~ | evaluate function                             |
| ~m e l~ | evaluate line                                 |
| ~m e r~ | evaluate region                               |

[cormacc]

Yep -- added that one at someone else's suggestion. Perhaps @yyoncho? Believe it does things like syntax corrections etc. if suggested by language server. Support seems to be pretty recent in cquery, though was able to execute a code action of some description during my brief time playing with ccls.

I've moved it under m l e for now, though open to other suggestions...

[yyoncho]

Execute code action is applying quick fix, e. g. if you are on a string a potential code action is "Extract constant". Possible bindin is "mrea", which will stands for Refactoring -> Execute Action.

[sdwolfz]

What is the difference between jump and goto? I feel like all the jump keybindings should go under goto.

[cormacc]

Yeah you're right. At some point I felt goto was in danger of getting overcrowded and made an artificial distinction between actions with only one possible target (goto) and those with multiple potential targets (jump - lsp-ui-peek). Moved all the j bindings back under g (had to rebind lsp-imenu under m g m as m g i was already bound to goto implementation).

[cormacc]

Made requested changes and rebased on develop branch tip.

[sdwolfz]

Thank you ❤️!
Cherry-picked into develop branch, you can safely delete your branch.

[cormacc]

Thanks @sdwolfz! Minor issue though - just realised a few minutes ago that moving the binding for spacemacs/lsp-avy-document-symbol from m j a to m g a was a bad idea, as it conflicts with the m g a standard for alternate file used in c layers. I'd pushed another commit amendment before noticing you'd already merged -- rebound under m g k (k for keyboard navigation). Can you pull that amendment across, or would you rather I raised a new PR?

[sdwolfz]

@cormacc It's better if you open up a new PR.

By the way it looks like you associated the commit with the cormaccannon user, is that intentional? There's no problem about it but you won't get credit for it in GitHub's statistics.

[cormacc]

Grand -- raised #11203
No idea how I associated the commit with that other account. Had forgotten it existed (was empty), but I've deleted it now :)
Thanks for the help -- it'll be next week before I get back to the c-c++ layer -- house guests arriving tomorrow.

[yyoncho]

IMO this shouldn't be here since only small part of the LSP server provide hierarchy support. Also, as per CONVENTIONS.org "mh" stands for help. I am voting to remove this line out of base layer and maybe use "mH" for hierarchy in C++ mode. @sdwolfz what do you think?

[cormacc]

I've no objections, but maybe move this discussion over to #11203 (which I'll re-title) as this PR is now closed