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

Add +tools/lsp layer for lsp-mode & lsp-ui #10211

Merged
merged 1 commit into from
Feb 19, 2018
Merged

Conversation

MaskRay
Copy link
Contributor

@MaskRay MaskRay commented Jan 21, 2018

This is a starting point to bring LSP ecosystem into spacemacs, as the initial step to address #10134

This PR adds a new layer, without changing anything default, so it is safe to merge. Based on it, we can integrate lsp-rust lsp-haskell+haskell-ide-engine and many others.

I recently raised a topic about C++ language server on Reddit. Many Emacs users expressed their interests. I am certainly confident this lsp + cquery combo works brilliantly well (refer to the Wiki definition/references and other cross references and fancy use of definition/references. But they also complain that LSP toolchain is difficult to set up. spacemacs is definitely a best place to collect these fragments and make the code navigation experience wonderful.

I also hope you appreciate references and other types of cross references as much as you do to find-definition #9911 The C++ language server cquery I am working on has some nice features. I'd also like to add a cquery layer based on this, after cquery.el is integrated into Melpa.

My elisp-fu is still too shabby to push forward everything. I'm seeking help from you.

@MaskRay
Copy link
Contributor Author

MaskRay commented Jan 21, 2018

I hope the language server protocol ecosystem can draw a bit more attention. Notably,

rely on this

))

(defun lsp/init-markdown-mode ()
(use-package markdown-mode))
Copy link
Owner

@syl20bnr syl20bnr Jan 23, 2018

Choose a reason for hiding this comment

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

You don't need to add markdown to this layer, package.el will install it implicitly for us.

On the init functions:
There can be only one init-<package> function, the layer defining this function is told to be the owner of <package>. For instance the markdown layer defines the function markdown/init-markdown-mode and it makes this layer the owner of the markdown-mode package.
Other layers that extend the configuration of <package> define function names pre-init-<package> and/or post-init-<package> which are respectively executed before and after the init-<package> function.
A package is installed and configured only if its init-<package> function is declared which means that the user added the layer owning this package in their dotfile, we say that the layer/package is used when it is explicitly listed in the dotfile.
Therefore if the layer owning a package is not used (not in the dotfile) then the init-<package> function is not declared and then pre-init and post-init functions are not called.
This is the basic principle in Spacemacs to achieve isolation of configuration.

Now in this particular case, even if the markdown-mode package is not used (i.e. the user has not added the markdown layer to the dotfile) package.el will still install the dependencies for lsp-ui so markdown-mode will be installed, but not configured by Spacemacs.

Copy link
Owner

@syl20bnr syl20bnr Jan 23, 2018

Choose a reason for hiding this comment

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

As a side note, a layer created by a user can redefine the init-<package> function and add this layer at the end of the layer list in the dotfile, we say that the layer steals the ownership of <package> as the last layer in the list to declare the init-<package> function wins the ownership.
This mechanism allows users to be able to reconfigure all pieces of configuration provided by Spacemacs.

More info in https://github.com/syl20bnr/spacemacs/blob/develop/doc/LAYERS.org

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thx. Removed lsp/init-markdown-mode... Yeah, I do not know much about spacemacs that was why I didn't create the layer earlier.

@Declspeck
Copy link

Thank you for your hard work - I installed this and have been using it with PHP language server.

I'm wondering where the code which enables this for specific languages should go - to get it running I had to comment out company-php initialization from the PHP layer and hook into php-mode in .spacemacs.

  • Add enablable / disablable sections for each language in +tools/lsp?
  • Add separate LSP layers for each language?
  • Or instantiate LSP in language-specific layers if it is enabled, while disabling "regular" language support?

@MaskRay
Copy link
Contributor Author

MaskRay commented Jan 24, 2018

Maybe a config var in individual +lang/ language layer to toggle LSP

@syl20bnr should make a decision here.

I don't add any default key bindings because I'm not clear what keys would not cause surprise.

Adding bindings for these function can be a good starting point.

;; (define-key lsp-ui-mode-map [remap xref-find-definitions] #'lsp-ui-peek-find-definitions)
;; (define-key lsp-ui-mode-map [remap xref-find-references] #'lsp-ui-peek-find-references)
There is a window-local jump list dedicated to cross references:

(lsp-ui-peek-jump-backward)
(lsp-ui-peek-jump-forward)
Other cross references:

(lsp-ui-peek-find-workspace-symbol "pattern 0")
;; If the server supports custom cross references
(lsp-ui-peek-find-custom 'base "$cquery/base")

...... I just want to make this and cquery checked in earlier so that others can pick up and polish them.

My personal key bindings:

    (dolist (mode c-c++-modes)
      (spacemacs/set-leader-keys-for-major-mode mode
        "la" #'lsp-ui-find-workspace-symbol
        "lA" #'lsp-ui-peek-find-workspace-symbol
        "lb" #'cquery/base
        "lc" #'cquery/callers
        "ld" #'cquery/derived
        ;; Formatting requires waf configure --use-clang-cxx
        ;; https://github.com/jacobdufault/cquery/wiki/Formatting
        "lf" #'lsp-format-buffer
        "ll" #'lsp-ui-sideline-mode
        "lD" #'lsp-ui-doc-mode
        "ln" #'lsp-ui-find-next-reference
        "lp" #'lsp-ui-find-previous-reference
        "lr" #'lsp-rename
        "lv" #'cquery/vars
       ))

@Declspeck
Copy link

Awesome - I didn't know some of those functions in your keybindings even existed. I'll try and see how they work with php-language-server, if at all.

Adding configurable LSP-interfacing parts to each +lang layer seems like the best option to me too, at least among the ones I presented. But I've never contributed to spacemacs so my opinion probably won't matter much. When we have a decision and I have released lsp-php, I might make a PR to add PHP support.

@CestDiego
Copy link
Contributor

I think it would be helpful to add the completion provide by the language service to the head of thee list of company-backends instead of overriding a language's Spacemacs configuration.

However, there are some features that interfere with what's provided by lsp and lsp-ui, so I encourage discussion to know whether we disable certain functionality that interferes with lsp functionality.

As per how should we enable this per language: We have language specific company-backends configuration and that seems to work out pretty great so far.

@myrgy
Copy link
Contributor

myrgy commented Jan 25, 2018

Guys, what do you think about c-c++-lsp, c-c++-rtags layers.
This layers may use c-c++ as a core layer and then extend it. As a result user will be able to choose required things.
And we will keep that layers as a small modules.

@CestDiego
Copy link
Contributor

CestDiego commented Jan 25, 2018 via email

@MaskRay
Copy link
Contributor Author

MaskRay commented Jan 25, 2018

For c-c++, we may add c-c++-enable-cquery-support to the list of defvar's in layers/+lang/c-c++/config.el:

(defvar c-c++-enable-clang-support nil
  "If non nil Clang related packages and configuration are enabled.")

(defvar c-c++-enable-rtags-support nil
  "If non nil Rtags related packages and configuration are enabled.")

Yes, lsp-ui-flycheck.el conflicts with lsp-flycheck.el, and the latter is a bit out-of-date that's why I disable the latter.

@MaskRay
Copy link
Contributor Author

MaskRay commented Jan 30, 2018

Ping?

:distant-foreground (face-attribute 'highlight :foreground))
(set-face-attribute 'lsp-ui-peek-header nil
:background (face-attribute 'highlight :background)
:foreground (face-attribute 'default :foreground))

Choose a reason for hiding this comment

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

I would set the INHERIT parameter of face-attribute to t

- Each language server may support a subset of these features and may have their extension, check =M-x lsp-capabilities= in a LSP buffer and the language server's website for details.

* Configuration
The LSP ecosystem is based on two packages: [[https://github.com/emacs-lsp/lsp-mode][lsp-mode]] and [[https://github.com/emacs-lsp/lsp-mode][lsp-ui]].

Choose a reason for hiding this comment

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

The link to lsp-ui is wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

(add-hook 'lsp-mode-hook #'lsp-ui-mode)

(require 'lsp-imenu)
(add-hook 'lsp-after-open-hook #'lsp-enable-imenu)

Choose a reason for hiding this comment

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

This isn't necessary anymore, lsp-ui-imenu-enable does it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@MaskRay
Copy link
Contributor Author

MaskRay commented Feb 4, 2018

Ping

I am not qualified to write a LSP layer because my elisp-fu is just not decent. But I believe people will be very active to contribute C++/Python/Rust/Haskell plugins, pick up and improve this common layer, and make the language support of spacemacs even better. @syl20bnr

You also don't need to worry that current language settings will be deprecated. The LSP language-specific support can be selectively enabled like:

(defvar c-c++-enable-cquery-support nil  ...)
(defvar python-enable-pyls-support nil  ...)

I am very proud of cquery because I see it attracts GCC developer(s), Linux kernel developer(s), the author of irony-mode (an alternative to ycmd)

These are collective efforts. the main author of lsp-ui, the author of company-lsp, +tools/lsp-python they will all be willing to push forward the LSP ecosystem.

@tadfisher
Copy link
Contributor

This also would be very useful for Java, Scala, Kotlin, and the rest of the JVM ecosystem. I am working on a standalone LSP server using IntelliJ IDEA as a base, which would finally bring advanced JVM language support to Emacs without heavy-handed and incompatible approaches like eclim and meghanda.

@CestDiego
Copy link
Contributor

I would love to see this too :)

@iromise
Copy link

iromise commented Feb 17, 2018

I think this is a new generation design for emacs configure of different Ianguage, and I would like to see it soon :)

@@ -0,0 +1,18 @@
(defun lsp//sync-peek-face ()
Copy link
Owner

Choose a reason for hiding this comment

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

What does this function do exactly ? Can you add a docstring ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@syl20bnr
Copy link
Owner

Does it make sense to have both helm-xref and ivy-xref owned by the LSP layer ? Would it make sense to have them owned by helm and ivy layer respectively and have the LSP layer configure them via post-init functions ?

@MaskRay
Copy link
Contributor Author

MaskRay commented Feb 18, 2018

Thanks for paying attention to this. You can decide

@syl20bnr
Copy link
Owner

syl20bnr commented Feb 18, 2018

Flycheck package needs to appear in the package list of the LSP layer and the flycheck specific setup for LSP should be done in a post-init-flycheck function.

We could even invent an imaginary package flycheck-lsp to mirror company-lsp, would be:

(defconst lsp-packages
  '(
    (company-lsp :requires company)
    # this package does not exists so we flag it as built-in
    # we use this package to hook flycheck LSP specific configuration in language layers
    (flycheck-lsp :requires flycheck :location built-in)
    (helm-xref :requires helm)
    (ivy-xref :requires ivy)
    lsp-mode
   lsp-ui
    ))

With this dummy package we can hook flycheck specific config for LSP in language layers. Exactly like we can do it by using the company-lsp package. For instance in the python layer we can then hook company LSP backends in a post-init-company-lsp function. We can also hook flycheck checkers powered by LSP in post-init-flycheck-lsp function.

If LSP layer is not used then all the company-lsp and flycheck-lsp configuration will be ignored as both packages are not used.
Similarly to activate auto-completion with LSP, both auto-completion and lsp layers must be activated, if only lsp layer is activated then auto-completion will be automatically disabled in LSP as company-lsp will be declared not used because company package is not used.

Not sure it is clear but both company-lsp and flycheck-lsp will represent the auto-completion and syntax-checking feature of LSP respectively in Spacemacs config.

@MaskRay
Copy link
Contributor Author

MaskRay commented Feb 18, 2018

Thanks for the comment but I think I am incapable (in terms of elisp-fu and familiarity with spacemacs) to sort these out and make it perfect. But I believe there will be improvement from other contributors after this is checked in.

@syl20bnr
Copy link
Owner

No problem, I can do it. Will work on it and the python integration tomorrow. From there, we will have an example of how to integrate a language.

@syl20bnr
Copy link
Owner

To be clear, I'll merge your PR tomorrow as well as the python one and will work from this state. Don't close this PR please :-)

@MaskRay
Copy link
Contributor Author

MaskRay commented Feb 18, 2018

Thanks! I am not good at elisp but I will be very happy to improve and answer these LSP questions (as being the maintainer of the C++ language server cquery (its PR), its Emacs plugin emacs-cquery, lsp-ui, and having commits to other clients)

https://github.com/cquery-project/cquery/wiki/Emacs contains some settings for cquery but many concepts are useful for other language servers. But I'm unclear how to set default keybindings properly so I do not do that in this PR.

This is a starting point to bring LSP ecosystem into spacemacs.

Theme by 0bl.blwl
@syl20bnr syl20bnr self-assigned this Feb 19, 2018
@syl20bnr syl20bnr merged commit d99cb00 into syl20bnr:develop Feb 19, 2018
@syl20bnr syl20bnr removed the Merged label Feb 19, 2018
@syl20bnr
Copy link
Owner

Thank you ! 💜

I added LSP support for python layer in commit 81a931f4. You an check this commit to see how it is done. Don't hesitate if you have questions.

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

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.

8 participants