emacs-lsp / lsp-mode

Emacs client/library for the Language Server Protocol
https://emacs-lsp.github.io/lsp-mode
GNU General Public License v3.0
4.8k stars 892 forks source link

lsp-prefer-capf doesnt work well #1451

Closed glepnir closed 4 years ago

glepnir commented 4 years ago

Describe the bug Set the lsp-prefer-capf t, It performs exact matching and does not perform filtering well. Bug 1: I can get completion items , but when input p cant get any completion items. image input p image

Bug 2: doesnt filter for gopls. image

To Reproduce

  1. open a go file
  2. use this lsp config and company config
    
    (use-package lsp-mode
    :commands (lsp-install-server lsp lsp-deferred)
    :hook (prog-mode . (lambda ()
                          (unless (derived-mode-p 'emacs-lisp-mode 'lisp-mode)
                            (lsp-deferred))))
    :init
    (setq  lsp-auto-guess-root t
        lsp-prefer-capf  t
        lsp-print-io t
        lsp-keep-workspace-alive nil)
    :config
    (when lsp-auto-configure
    (mapc (lambda (package) (require package nil t))
          lsp-client-packages)))
    (use-package company
    :commands company-complete-common company-manual-begin company-grab-line
    :after-call pre-command-hook after-find-file
    :init
    (setq company-tooltip-align-annotations t
        company-tooltip-limit 14
        company-idle-delay 0
        company-echo-delay (if (display-graphic-p) nil 0)
        company-minimum-prefix-length 2
        company-require-match 'never
        company-dabbrev-ignore-case nil
        company-dabbrev-downcase nil
        company-global-modes '(not erc-mode message-mode help-mode gud-mode eshell-mode shell-mode)
        company-backends '(company-capf)
        company-frontends '(company-pseudo-tooltip-frontend
                            company-echo-metadata-frontend))
    :config
    (global-company-mode +1))


**Expected behavior**
1. doesn't  exact match
2. filter the response.

**Which Language Server did you use**
lsp-go

**OS**
macos 10.15.3

**Error callstack**
kiennq commented 4 years ago

Can you provide the language server log according to this? I cannot repro bug 1: image

For bug 2: I can kind of repro it, the problem is that the gopls return candidates with no prefix (the request.position == response.textEdit.range.start). Thus lsp-capf cannot detect the prefix and cannot filter the completion candidates.

[Trace - 09:08:40 PM] Sending request 'textDocument/completion - (1303)'.
Params: {
  "textDocument": {
    "uri": "file:///home/kienn/projects/go/src/github.com/kiennq/test/a.go"
  },
  "position": {
    "line": 20,
    "character": 4
  },
  "context": {
    "triggerKind": 1
  }
}

[Trace - 09:08:40 PM] Received response 'textDocument/completion - (1303)' in 1ms.
Result: {
  "items": [
    {
      "textEdit": {
        "newText": "bufio",
        "range": {
          "end": {
            "character": 4,
            "line": 20
          },
          "start": {
            "character": 4,
            "line": 20
          }
        }
      },
      "insertTextFormat": 2,
      "filterText": "bufio",
      "sortText": "00000",
      "preselect": true,
      "detail": "\"bufio\"",
      "kind": 9,
      "label": "bufio"
    },...
  ]
}  

What's the behavior of VsCode in this case?

yyoncho commented 4 years ago

For bug 2: I can kind of repro it, the problem is that the gopls return candidates with no prefix (the request.position == response.textEdit.range.start).

Why having empty prefix breaks us?

kiennq commented 4 years ago

Why having empty prefix breaks us?

No prefix means we do no filtering. We just display whatever language server returns.

yyoncho commented 4 years ago

It returns empty prefix even if you type after .?

kiennq commented 4 years ago

No, it seems return empty prefix only when you define new function name.

kiennq commented 4 years ago

@taigacute Not that, but the communication log

set lsp-log-io to t to inspect communication between client and the server. Use lsp-workspace-show-log to switch to the corresponding log buffer.

Also, what's the value of your completion-styles?

glepnir commented 4 years ago

@kiennq i upload the log .please check it .the completion values

completion-styles is a variable defined in minibuffer.el.gz.

Value
(basic partial-completion emacs22)

Original Value
(basic partial-completion emacs22)

log.txt

kiennq commented 4 years ago

Thanks, you can set the completion-styles to the following to archive fuzzy candidate filtering for Emacs 27.

(setq completion-styles `(basic partial-completion emacs22 initials
                                ,(if (version<= emacs-version "27.0") 'helm-flex 'flex)))
kiennq commented 4 years ago

The returns value from your gopls server all have filterText set to Errorf, that's why you see no candidate after typing p. Since the filtering is done against filterText. What's version of your gopls? I've tried with latest gopls (v0.3.2) and it seems return correct filterText.

glepnir commented 4 years ago

Dont know why it doesnt work ,I just updated 0.3.2.

completion-styles is a variable defined in minibuffer.el.gz.

Value
(basic partial-completion emacs22 initials flex)

gopls version

golang.org/x/tools/gopls v0.3.2
    golang.org/x/tools/gopls@v0.3.2 h1:eP1aj1AvT6ynElQH6KP0mmOT2gnWa1gYclHL4wGUbMo=
kiennq commented 4 years ago

Could you run M-x lsp-shutdown-workspace and M-x lsp in the main.go. Then take a trace again?

glepnir commented 4 years ago

this is log ,i found something .. when input . image but when i input p image

log2.txt

kiennq commented 4 years ago

Hmm, the trace still showing the same filter Text for all of returned completion. Can you check if you're actually using the correct gopls from Emacs? You can take a look at *lsp-log* buffer and looking for something similar to

2020/02/25 00:45:02 Build info
----------
golang.org/x/tools/gopls v0.3.2
    golang.org/x/tools/gopls@v0.3.2 h1:eP1aj1AvT6ynElQH6KP0mmOT2gnWa1gYclHL4wGUbMo=
    github.com/BurntSushi/toml@v0.3.1 h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ=
    github.com/sergi/go-diff@v1.0.0 h1:Kpca3qRNrduNnOQeazBd0ysaKrUJiIuISHxogkT9RPQ=
    golang.org/x/mod@v0.1.1-0.20191105210325-c90efee705ee h1:WG0RUwxtNT4qqaXX3DPA8zHFNm/D9xaBpxzHt1WcA/E=
    golang.org/x/sync@v0.0.0-20190423024810-112230192c58 h1:8gQV6CLnAEikrhgkHFbMAEhagSSnXWGV915qUMm9mrU=
    golang.org/x/tools@v0.0.0-20200212213342-7a21e308cf6c h1:D2X+P0Z6ychko7xn2jvd38yxQfdU0eksO4AHfd8AWFI=
    golang.org/x/xerrors@v0.0.0-20191011141410-1b5146add898 h1:/atklqdjdhuosWIl6AIbOeHJjicWYPqR9bpxqxYG2pA=
    honnef.co/go/tools@v0.0.1-2019.2.3 h1:3JgtbtFHMiCmsznwGVTUWbgGov+pVqnlf1dEJTNAXeM=
    mvdan.cc/xurls/v2@v2.1.0 h1:KaMb5GLhlcSX+e+qhbRJODnUUBvlw01jt4yrjFIHAuA=
glepnir commented 4 years ago

I can reproduce use this min config

(setq
      straight-cache-autoloads nil
      straight-check-for-modifications nil
      straight-enable-package-integration nil
      straight-vc-git-default-clone-depth 1
      autoload-compute-prefixes nil)
;; Install and load straight.el
;; https://github.com/raxod502/straight.el#getting-started
(defvar bootstrap-version)
(let ((bootstrap-file
       (expand-file-name "straight/repos/straight.el/bootstrap.el"
                         user-emacs-directory))
      (bootstrap-version 5))
  (unless (file-exists-p bootstrap-file)
    (with-current-buffer
        (url-retrieve-synchronously
         "https://raw.githubusercontent.com/raxod502/straight.el/develop/install.el"
         'silent 'inhibit-cookies)
      (goto-char (point-max))
      (eval-print-last-sexp)))
  ;; (benchmark 1 `(load ,bootstrap-file nil 'nomessage))
  (load bootstrap-file nil 'nomessage))

(straight-use-package 'use-package)

(require 'use-package)

(setq use-package-always-defer t)

(straight-use-package 'company)
(use-package company)

(use-package company
  :init
  (setq completion-styles `(basic partial-completion emacs22 initials
                                ,(if (version<= emacs-version "27.0") 'helm-flex 'flex)))
  (setq company-tooltip-align-annotations t
        company-tooltip-limit 14
        company-idle-delay 0
        company-echo-delay (if (display-graphic-p) nil 0)
        company-minimum-prefix-length 2
        company-require-match 'never
        company-dabbrev-ignore-case nil
        company-dabbrev-downcase nil
        company-global-modes '(not erc-mode message-mode help-mode gud-mode eshell-mode shell-mode)
        company-backends '(company-capf)
        company-frontends '(company-pseudo-tooltip-frontend
                            company-echo-metadata-frontend))
  :config
  (global-company-mode +1))

(straight-use-package 'lsp-mode)
(use-package lsp-mode
  :commands (lsp-install-server lsp lsp-deferred)
  :hook (prog-mode . (lambda ()
                          (unless (derived-mode-p 'emacs-lisp-mode 'lisp-mode)
                            (lsp-deferred))))
  :init
  (setq
        lsp-prefer-flymake nil
        lsp-prefer-capf  t
        lsp-log-io  t
        lsp-keep-workspace-alive nil)
         ;; For `lsp-clients'
 :config
 (when lsp-auto-configure
    (mapc (lambda (package) (require package nil t))
          lsp-client-packages)))

(straight-use-package 'go-mode)
(add-hook 'go-mode #'lsp-deferred)
(straight-use-package 'exec-path-from-shell)

(when (memq window-system '(mac ns x))
  (exec-path-from-shell-initialize))
glepnir commented 4 years ago

i found this

2020/02/24 23:59:01 Build info
----------
golang.org/x/tools/gopls v0.3.2
    golang.org/x/tools/gopls@v0.3.2 h1:eP1aj1AvT6ynElQH6KP0mmOT2gnWa1gYclHL4wGUbMo=
    github.com/BurntSushi/toml@v0.3.1 h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ=
    github.com/sergi/go-diff@v1.0.0 h1:Kpca3qRNrduNnOQeazBd0ysaKrUJiIuISHxogkT9RPQ=
    golang.org/x/mod@v0.1.1-0.20191105210325-c90efee705ee h1:WG0RUwxtNT4qqaXX3DPA8zHFNm/D9xaBpxzHt1WcA/E=
    golang.org/x/sync@v0.0.0-20190423024810-112230192c58 h1:8gQV6CLnAEikrhgkHFbMAEhagSSnXWGV915qUMm9mrU=
    golang.org/x/tools@v0.0.0-20200212213342-7a21e308cf6c h1:D2X+P0Z6ychko7xn2jvd38yxQfdU0eksO4AHfd8AWFI=
    golang.org/x/xerrors@v0.0.0-20191011141410-1b5146add898 h1:/atklqdjdhuosWIl6AIbOeHJjicWYPqR9bpxqxYG2pA=
    honnef.co/go/tools@v0.0.1-2019.2.3 h1:3JgtbtFHMiCmsznwGVTUWbgGov+pVqnlf1dEJTNAXeM=
    mvdan.cc/xurls/v2@v2.1.0 h1:KaMb5GLhlcSX+e+qhbRJODnUUBvlw01jt4yrjFIHAuA=
sebastiansturm commented 4 years ago

at the risk of muddying the waters, I've seen the following issue today (may not be related to the issue described above, but at least it's related to the capf backend): when trying to complete the following line in a TypeScript buffer,

let some_set : Set<string> = new Set();
some_set.|

the language server decides I might be interested in a completion item with "filterText": ".Symbol":

[Trace - 06:19:08 PM] Received response 'textDocument/completion - (41)' in 7ms.
[
  {
    "insertTextFormat": 2,
    "data": {
      "entryNames": [
        "add"
      ],
      <offset/line/file removed>
    },
    "commitCharacters": [
      ".",
      ",",
      "("
    ],
    "sortText": "0",
    "kind": 2,
    "label": "add"
  },
  ... some more items of interest here ...
  {
    "textEdit": {
      "newText": "[Symbol]",
      "range": {
        ...
      }
    },
    "filterText": ".Symbol", <-- entirely uninteresting to me, but unfortunately starts with '.'
    "data": {
      "entryNames": [
        "Symbol"
      ],
      ...
    },
    "commitCharacters": [
      ".",
      ",",
      "("
    ],
    "sortText": "0",
    "kind": 6,
    "label": "Symbol"
  },
  ... more interesting items ...
]

Since lsp-completion-at-point thinks (quite reasonably, IMO) that its current prefix is '.', it disregards all the interesting items and offers me a single-item menu with [Symbol] as the only candidate to choose. Not sure what's the right way to fix this

sebastiansturm commented 4 years ago

should trigger characters generally be removed from prefix strings?

kiennq commented 4 years ago

Since lsp-completion-at-point thinks (quite reasonably, IMO) that its current prefix is '.'

lsp-completion-at-point will try to find the prefix based on response from language server. Specifically, it will look at items[0].textEdit.range.start or (car (bounds-of-thing-at-point 'symbol)) or (point) as prefix start position.

Can you try this

(advice-add #'completion-all-completions :around
            (lambda (orig string table pred point &optional metadata)
              (let* ((result (funcall orig string table pred point metadata))
                     (last (last result))
                     (base-size (if (consp last) (cdr last))))
                (if (consp last) (setcdr last nil))
                (message "completion-all-completions string=%s table-len=%s length=%s"
                         string
                         (if (and (not (functionp table)) (listp table)) (length table))
                         (length result))
                (append result base-size)))
            '((name . --a1)))

Repro the problem and see what did message spill?

kiennq commented 4 years ago

@taigacute I think I found the problem why you don't get any completions. Can you try to type fmt.P instead of fmt.p, and see if you get any completion? The filtering is done case-sensitive depends on completion-ignore-case.

sebastiansturm commented 4 years ago

right after the some_set.|, I'm always getting those two lines of output:

completion-all-completions string=. table-len=10 length=1
completion-all-completions string= table-len=nil length=1
yyoncho commented 4 years ago

@kiennq is this caused by the double filtering? If yes, we may try to fix it the way I described in the PR - by declaring custom category and have it bound only while we are filtering.

kiennq commented 4 years ago

@yyoncho No, it's not due to double filtering. The second filtering in the double filtering is basically noop if completion-styles include basic. The problem here is that an completion item returned from language server contains a different prefix than the rest, its prefix start point (decided by textEdit.range.start) is one character before others. I'm thinking of having to go through all completion items and heuristically getting prefix start point as most right side prefix start point of all items. With that we can still get other completion items after filtering

yyoncho commented 4 years ago

@kiennq wouldn't that mean that we might lose the first item?

I am looking at the eclipse lsp implementation:

https://git.eclipse.org/c/lsp4e/lsp4e.git/tree/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/completion/LSIncompleteCompletionProposal.java#n378

It seems like the completion prefix per item and the filtering is performed per item. If we do what you are suggesting we will lose the items with most left prefix start point when we are doing basic filtering.

We may also consider fixing this upstream.

s-kostyaev commented 4 years ago

With tips from this thread capf works better, but still there is some issues. For example with this simple file:

package main

import (
    "fmt"
)

func main() {
    fmt.Println("Results:")
}

func DoSome(a interface{}, b interface{}) {}

snippet expansion works well with company-lsp but not with capf:

Снимок экрана 2020-02-25 в 15 28 53
glepnir commented 4 years ago

@kiennq yes . P is worked. use this solved

(setq completion-styles `(basic partial-completion emacs22 initials
                                ,(if (version<= emacs-version "27.0") 'helm-flex 'flex))
      completion-ignore-case t)

by the way i can get snippet.. image the filter doesnt work on gopls 0.3.2. What do we need to do to make the filter work for gopls image

kiennq commented 4 years ago

the filter doesnt work on gopls 0.3.2. What do we need to do to make the filter work for gopls

I think you should create an issue upstream in gopls repo, even if we tweak for the filter to work, when you insert, you will get text being inserted doubly.

The problem is that in the mentioned case, gopls returns current cursor position as textEdit range. So, for example if you type int| (| is cursor position), and the filter shows [int int8 int16 int32] as completion results. Now if you select int16, what will be inserted will be intint16, since basically that's what gopls told LSP client to do

glepnir commented 4 years ago

but it works fine on vim (coc.nvim).I don't know if there is a good way in emacs

yyoncho commented 4 years ago

but it works fine on vim (coc.nvim).I don't know if there is a good way in emacs

The fact that it works somewhere else does not mean that the server is implemented correctly.

Is 0.3.2 the latest version? I am testing with master and it works fine. Can you provide a test file which does not work with gopls master?

glepnir commented 4 years ago

@yyoncho hmm master gopls ? mean you get gopls bygo get golang.org/x/tools/gopls@master? a simple main.go .

yyoncho commented 4 years ago

Yes. When I start typing outside of the main method it works fine.

glepnir commented 4 years ago

doesnt work for me . i tried the gopls@master and gopls@latest .. image

yyoncho commented 4 years ago

Please include the test file, not screenshots hiding the content of the file.

glepnir commented 4 years ago

sry . a simple file, a go.mod project

package main

import "fmt"

func main() {
    fmt.Println("test")
}
//method here
yyoncho commented 4 years ago

@muirdm @stamblerre willing to take a look? I looks like the gopls breaks when the completion is invoked outside of the method like that:

package main

import "fmt"

func main() {
    fmt.Println("test")
}
main|

(cursor at | ) - it seems like gopls is disregarding the prefix and it is returning everything that is in the global scope with insert textEdit with no replace range, like this:

{
      "command": {
        "command": "",
        "title": ""
      },
      "textEdit": {
        "newText": "fmt",
        "range": {
          "end": {
            "character": 0,
            "line": 16
          },
          "start": {
            "character": 0,
            "line": 16
          }
        }
      },
      "insertTextFormat": 2,
      "filterText": "fmt",
      "sortText": "00000",
      "preselect": true,
      "detail": "\"fmt\"",
      "kind": 9,
      "label": "fmt"
    }
muirdm commented 4 years ago

Completion at file level should be fixed on gopls@master. The only completions offered now are the keywords "const", "func", "import", "type", and "var". Writing arbitrary code at the file level is a syntax error so completion is not supported.

As for completion-at-point, gopls by default performs server side fuzzy matching and always returns completion results with "isIncomplete: true", intending to disable client-side filtering. gopls offers up to 3 "deep completion" candidates which we want to update as you continue to type, so client side filtering is not possible. gopls also gives all candidates an identical "filterText" as a workaround to prevent VSCode from re-ordering the candidates.

An example of deep completion:

package main
import "context"
func main() {
    var _ context.Context = cb // completes straight to "context.Background()"
}

company-capf doesn't seem to work in above case.

If you want to use company-capf with only client side filtering, you should set "gopls.matcher" to "default", which will disable server side fuzzy matching, causing gopls to return full results. Note that deep completion candidates won't be able to update as you type since the filtering is client side, so disabling "gopls.deepCompletion" also probably makes sense. However, I feel the default gopls config paired with company mode gives the best completion experience ATM.

(lsp-register-custom-settings
 '(("gopls.matcher" "default")
   ("gopls.deepCompletion" nil t)))
yyoncho commented 4 years ago

Thank you @muirdm .

Completion at file level should be fixed on gopls@master.

AFAICS this is not the case(although I might be wrong).

As a side note company-capf works fine if you switch to flex matching:

Screenshot at 2020-02-25 21-06-35

@kiennq - I think that @muirdm's suggestion to do not filter client-side makes sense when server has returned isIncomplete = t. If the server has returned partial result this automatically means that we should not filter client-side because the server has already filtered the data. WDYT?

kiennq commented 4 years ago

If the server has returned partial result this automatically means that we should not filter client-side because the server has already filtered the data. WDYT?

It makes sense to do that.

glepnir commented 4 years ago

@kiennq Update the lsp-mode .still not work.

yyoncho commented 4 years ago

@taigacute refer to @muirdm's comment about completion on top level. ATM the original issue report is solved from lsp-mode point of view. The only outstanding work is on @sebastiansturm report about typescript language server.

glepnir commented 4 years ago

@yyoncho aha ok.

yyoncho commented 4 years ago

There is one more issue when completing imports: see https://github.com/emacs-lsp/lsp-mode/issues/1459

When the server no filter text, no textEdit, no insertText we should not filter those items.

  {
    "data": {
      "entryNames": [
        "@angular-devkit/build-angular"
      ],
      "offset": 43,
      "line": 2,
      "file": "/home/kyoncho/Sources/my-first-app/src/main.ts"
    },
    "sortText": "0",
    "kind": 9,
    "label": "@angular-devkit/build-angular"
  },

Also, it seems like it will behave better if we use the algorithm from https://git.eclipse.org/c/lsp4e/lsp4e.git/tree/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/completion/LSIncompleteCompletionProposal.java#n378 instead calling back to thing-at-point for the cases when we do not have textEdit and when we have to guess the prefix.

The alternative is to try to fix typescript server but I believe it won't be easy since the data comes from the tsserver.

s-kostyaev commented 4 years ago

There is another problem: if completion triggered during snippet expansion of other completion, snippet expansion will be stopped, so I can't expand snippet of first completion:

glepnir commented 4 years ago

@s-kostyaev cant reproduce. it works fine on my emacs.

rrudakov commented 4 years ago

@s-kostyaev try (setq yas-inhibit-overlay-modification-protection t)

glepnir commented 4 years ago

maybe dont set (setq yas-inhibit-overlay-modification-protection t) test

s-kostyaev commented 4 years ago

@rrudakov, @taigacute thanks! (setq yas-inhibit-overlay-modification-protection t) fixed it for me.

I think we should collect tips from this topic to default configuration and/or docs.

glepnir commented 4 years ago

It ’s weird. I didn’t set it, but it still works.

yyoncho commented 4 years ago

@taigacute based on the screenshots you are using company-box which does not use overlays but posframe - thus there is no problem.

glepnir commented 4 years ago

@yyoncho thanks for your reply. got it.

kiennq commented 4 years ago

When the server no filter text, no textEdit, no insertText we should not filter those items.

Indeed, The algorithm you said may provide a better heuristic to determine the prefix when there's no textEdit. JFYI, I'm working on a change to divide the completion items into groups based on prefix offset position and later combine all filtered items into a final list. Just that we need a ranking algorithm. For now I will rely on emacs built-in one and heuristically combine the first item of each group first then the rest, going from the left most prefix offset group.

And thought on how we should rank the filtered candidate is welcome.