From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: sbaugh@catern.com Newsgroups: gmane.emacs.devel Subject: Re: Eglot cannot work with default completion-in-region? Date: Sat, 27 Jan 2024 15:37:35 +0000 Message-ID: <87ttmy7pog.fsf@catern.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="26962"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) To: emacs-devel@gnu.org Cancel-Lock: sha1:hTK3BR0Jb7wcPMQBgLtbz4tlCLs= Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Sun Jan 28 06:15:16 2024 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1rTxVk-0006n0-3s for ged-emacs-devel@m.gmane-mx.org; Sun, 28 Jan 2024 06:15:16 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1rTxUq-0005HL-4h; Sun, 28 Jan 2024 00:14:20 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rTkmX-0008FF-SB for emacs-devel@gnu.org; Sat, 27 Jan 2024 10:39:45 -0500 Original-Received: from ciao.gmane.io ([116.202.254.214]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rTkmW-00047T-9N for emacs-devel@gnu.org; Sat, 27 Jan 2024 10:39:45 -0500 Original-Received: from list by ciao.gmane.io with local (Exim 4.92) (envelope-from ) id 1rTkmS-000AOf-Cg for emacs-devel@gnu.org; Sat, 27 Jan 2024 16:39:40 +0100 X-Injected-Via-Gmane: http://gmane.org/ Received-SPF: pass client-ip=116.202.254.214; envelope-from=ged-emacs-devel@m.gmane-mx.org; helo=ciao.gmane.io X-Spam_score_int: -16 X-Spam_score: -1.7 X-Spam_bar: - X-Spam_report: (-1.7 / 5.0 requ) BAYES_00=-1.9, HEADER_FROM_DIFFERENT_DOMAINS=0.249, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=no autolearn_force=no X-Spam_action: no action X-Mailman-Approved-At: Sun, 28 Jan 2024 00:14:17 -0500 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.devel:315523 Archived-At: João Távora writes: >> So Eglot has this somewhat hacky code which runs in :exit-function to >> delete the completion after completion-in-region inserts it, and insert >> a different string instead: >> >> (cond (textEdit >> ;; Revert buffer back to state when the edit >> ;; was obtained from server. If a `proxy' >> ;; "bar" was obtained from a buffer with >> ;; "foo.b", the LSP edit applies to that >> ;; state, _not_ the current "foo.bar". >> (delete-region orig-pos (point)) >> (insert (substring bounds-string (- orig-pos (car bounds)))) >> (eglot--dbind ((TextEdit) range newText) textEdit >> (pcase-let ((`(,beg . ,end) >> (eglot-range-region range))) >> (delete-region beg end) >> (goto-char beg) >> (funcall (or snippet-fn #'insert) newText)))) >> (snippet-fn >> ;; A snippet should be inserted, but using plain >> ;; `insertText'. This requires us to delete the >> ;; whole completion, since `insertText' is the full >> ;; completion's text. >> (delete-region (- (point) (length proxy)) (point)) >> (funcall snippet-fn (or insertText label)))) >> >> This is code which is often broken, especially with default >> completion-in-region. > > If you know of broken cases, contact bug-gnu-emacs@gnu.org and > provide a full reproducible error recipe according to Eglot's > manual (clangd, pyright, or rust-analyzer, gopls preferred, > other servers must come with installation instructions. If > installation too complex/bloaty it may take a while.) > >> However, the common case is that sortText==insertText/textEdit. > > Is it? The above all return textEdits, I think. How can sortText, > a JSON string, equal textEdit, a JSON object? If textEdit.range==the completion region and textEdit.newText==sortText, then the textEdit is trivial and the default completion insertion behavior is correct and needs no special handling. So we can skip the textEdit behavior in the exit-function. >> In that case, this code is not necessary, and Eglot doesn't need an >> :exit-function at all. > > Indeed. But how do you plan to know this in advance? And if for all the completion items, we see that we can skip the textEdit behavior, then we don't need an :exit-function at all. >> Can Eglot detect this and avoid this somewhat hacky code in that case? > > Not easily or cleanly... What's the issue with the approach I described above? Seems like it would work? >> It should be a performance improvement and simplification anyway for all >> completion frameworks. > > .. but you can try your hand at a patch. I'd love to be proven wrong. > >> (BTW, besides the issue with default completion-in-region, I also ran >> into this while trying to write an OCaml-specific wrapper for >> eglot-completion-at-point function which adds some additional >> completions to those returned from the LSP. But if those completions >> are chosen, then the Eglot exit-function breaks when it tries to look up >> the insertText/textEdit. My LSP doesn't use textEdit at all, though, so >> this is another unnecessary breakage) > > What contract exactly is eglot-completion-at-point breaking? No contract, my modification isn't staying within the bounds of any explicit contract. So of course I shouldn't expect it to work. But nevertheless I'd like to get it to work, and the exit-function is what doesn't work, so I'd like to figure out a way to make it work. We could add a new extra property to completion-at-point-functions, :text-function, which gets the completion item after it's been selected, before text properties are stripped, and returns the actual text which should be inserted into the buffer. Then Eglot could have a text-function which would hopefully handle most insertText/textEdit cases (any insertText and any textEdit where the range is the completion region). If a completion item requires any additional changes, the text-function could communicate that to Eglot somehow and then Eglot could do it in exit-function. That would be useful to Eglot, right? It would let Eglot avoid some hacks to work around the fact that default completion-in-region strips the text properties before calling exit-function. I would also find this useful elsewhere too - the fact that exit-function strips the properties is quite annoying, and the ability to transform the completion (possibly preserving text properties when inserted) would be handy. And then my wrapper around eglot-completion-at-point could just provide its own text-function which calls Eglot's text-function whenever it sees an Eglot completion, and runs its own logic on its own completions, and avoid any breakage.