bug#72765: Eglot + Clangd + Company + non-empty suffix = duplicate text
On 01/09/2024 12:43, João Távora wrote: It seems the difference comes from bug#70968 as well (which came up recently). Okay, but that presumed bug has nothing to do with Eglot, AFAICT. One could argue that the current definition of the style (in Eglot) relies on buggy (or suboptimal) behavior in completion-at-point. -(defun eglot--dumb-allc (pat table pred _point) (funcall table pat pred t)) +(defun eglot--dumb-allc (pat table pred point) + (funcall table (substring pat 0 point) pred t)) + (defun eglot--dumb-tryc (pat table pred point) (let ((probe (funcall table pat pred nil))) (cond ((eq probe t) t) That fixes the scenario in Company, with seemingly no change with completion-at-point. Like in that other recent bug, if you can add some Eglot test that demonstrates the bug and then apply this fix and verify that it passes the new tests and all the other tests you added recently, I'm fine with the change. Sure. Or if we want 100% compatibility, we can use 'or': (or (funcall table pat pred t) (funcall table (substring pat 0 point) pred t)) I don't understand what 100% compatibility this refers to, but if it is a better change that also passes the aforementioned tests, I'm also fine with it. One patch simply doesn't filter by the suffix, and another first tries filtering by prefix+suffix and if nothing matches falls back to filtering by prefix only. But in any case this doesn't help with the completion-at-point behavior described at the end of the report (where foo_|bar_2 turns into foo_bar_2bar_2|). If we consider it okay - then the above patch fixes the discrepancy with Company completion, and done. But if we think it a problem, then the fix might be required somewhere in the area of (cond (textEdit ;; Revert buffer back to state when the edit ;; was obtained from server. If a `proxy' After (and if) that is done, we might not need to change the completion style in the end. Same criteria as above. Alas, I have a fix which works for Company but not so well for completion-at-point: diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el index 59d9c346424..197e7d9869d 100644 --- a/lisp/progmodes/eglot.el +++ b/lisp/progmodes/eglot.el @@ -3353,7 +3353,6 @@ eglot-completion-at-point ;; "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))) It fixes the main scenario with both UIs - but when the suffix is not matching, exit-function can delete too much text. E.g. v.count|123.123456789 turns into v.count_ones()3456789 That's the example from the recently added test. What's currently working shall continue working. I would advise generally to be conservative here: the bugs you're fixing seem to be somewhat academic edge cases and not reports by actual Eglot users. I agree that this report is not very critical (and so can wait), but I don't think I'll be the only person to trigger it. Just hopefully it won't happen too often. The only thing I'd like to add is the following two notes: * before any of this, you showed earlier a way to completely forbid partial completions in Eglot. That's a good change for reasons we've already discussed and it prevents a number of bugs. I'd like that change to be commited first (presuming it does what you expect it to). Said reasons were also more of "academic" nature, right? That change would be removing a certain bit of functionality from the completion UIs, so I'd rather only do that in the face of hard evidence. * the rust-analyzer test you added recently -- and which you said was very brittle -- is indeed very brittle: I cannot get it to pass. We should fix it, or just delete it and do those rust-analyzer tests manually each time we touch this area. Could you give more details? It is indeed more brittle in theory, but on my machine it's passing every time. No failures from our CIs have been reported either, although that might not be saying much.
bug#60321: 29.0.60; ruby-mode indentation of hash or array as first arg in multiline method call
On 01/09/2024 22:28, Aaron Jensen wrote:
Here's a corrected patch for that particular example. Thank you for
finding that. I think I missed it because as long as you type the code
in, it indents fine. I still have a lot to understand about SMIE, so if
anything looks off in my patch, please let me know.
Thanks! Just being thorough. We can add this example to the args-indent
test file, too.
Here's a bonus example which looks off but would be more difficult to
fix (and it's not urgent, given the expression is in mixed styles):
foo([{
a: 2
},
{
b: 3
},
4
])
It would be nice to at least handle the last arg correctly - maybe we'll
just get that supported in the ts mode at some later date.
I didn't change the default. I wasn't sure if you wanted to change the
defaults of all of the variables you added in the last round or just
this one, so I'll let you handle that the way you want to.
Sure. I think we can add this into Emacs 30 too, while the change is off
by default.
A few other things:
* I think the docstring should say "Set it to nil to align to the line
with the open bracket" - it doesn't necessarily align to the beginning
of the statement (seems like a good thing).
* Let's change the first example to this, for less ambiguity?
foo
.update({
key => value,
other_key:
}, {
key => value,
other_key:
})
* Support for the new option in ruby-ts-mode (it's good to have parity).
Could you take the attached patch for a spin? Seems to work here, but
I'd like to have an extra confirmation.diff --git a/lisp/progmodes/ruby-ts-mode.el b/lisp/progmodes/ruby-ts-mode.el
index 5f4e11e0b4c..adcdf15c7ad 100644
--- a/lisp/progmodes/ruby-ts-mode.el
+++ b/lisp/progmodes/ruby-ts-mode.el
@@ -842,6 +842,16 @@ ruby-ts--parent-call-or-bol
;; No paren/curly/brace found on the same line.
((< (treesit-node-start found) parent-bol)
parent-bol)
+ ;; Nesting of brackets args.
+ ((and
+ (not (eq ruby-bracketed-args-indent t))
+ (string-match-p "\\`array\\|hash\\'" (treesit-node-type parent))
+ (equal (treesit-node-parent parent) found)
+ ;; Grandparent is not a parenless call.
+ (or (not (equal (treesit-node-type found) "argument_list"))
+ (equal (treesit-node-type (treesit-node-child found 0))
+ "(")))
+ parent-bol)
;; Hash or array opener on the same line.
((string-match-p "\\`array\\|hash\\'" (treesit-node-type found))
(save-excursion
diff --git a/test/lisp/progmodes/ruby-ts-mode-tests.el b/test/lisp/progmodes/ruby-ts-mode-tests.el
index 61ef80eb610..05d98974acf 100644
--- a/test/lisp/progmodes/ruby-ts-mode-tests.el
+++ b/test/lisp/progmodes/ruby-ts-mode-tests.el
@@ -326,6 +326,7 @@ "ruby-block-indent.rb"
(ruby-ts-deftest-indent "ruby-method-call-indent.rb")
(ruby-ts-deftest-indent "ruby-method-params-indent.rb")
(ruby-ts-deftest-indent "ruby-parenless-call-arguments-indent.rb")
+(ruby-ts-deftest-indent "ruby-bracketed-args-indent.rb")
(provide 'ruby-ts-mode-tests)
bug#60321: 29.0.60; ruby-mode indentation of hash or array as first arg in multiline method call
On 02/09/2024 03:49, Aaron Jensen wrote:
Here's a bonus example which looks off but would be more difficult to
fix (and it's not urgent, given the expression is in mixed styles):
foo([{
a: 2
},
{
b: 3
},
4
])
Yes, that's connected to the case I mentioned... how do you think it
should be indented? I wonder if it should just be 2 spaces in (rather
than aligned with the opening bracket)
It seems to me that anything other than 0 spaces would look inconsistent
with the first element (the hash), and its closing brace in particular.
Anyway, it's not urgent and like you said in the first message, people
formatting code this way might have other unusual requirements as well.
* Support for the new option in ruby-ts-mode (it's good to have parity).
Could you take the attached patch for a spin? Seems to work here, but
I'd like to have an extra confirmation.
I don't have the treesitter stuff installed at the moment, will try
this out shortly.
Thanks in advance.
bug#60321: 29.0.60; ruby-mode indentation of hash or array as first arg in multiline method call
Hi Aaron!
On 01/09/2024 03:54, Aaron Jensen wrote:
Updated patch with more precise variables in the new test.
Thanks for taking the initiative.
Here's an example which seems to get worse with the new variable set to nil:
def foo
foo.update(
{
key => value,
other_key: foo
}
)
end
I'd like to flip the default value (now or in Emacs 31), so it would be
great to deal with examples like this.
bug#60321: 29.0.60; ruby-mode indentation of hash or array as first arg in multiline method call
On 02/09/2024 04:56, Aaron Jensen wrote: It seems to me that anything other than 0 spaces would look inconsistent with the first element (the hash), and its closing brace in particular. Yeah, that's my sense as well, even if it looks awful, but you get what you get. * Support for the new option in ruby-ts-mode (it's good to have parity). Could you take the attached patch for a spin? Seems to work here, but I'd like to have an extra confirmation. I don't have the treesitter stuff installed at the moment, will try this out shortly. Thanks in advance. I installed it and gave it a run on a few things. I didn't observe any issues with it. Great! I've pushed both patches to emacs-30 (6c15b7710d4 and 24f12bdd77e) and now marking the issue as done. Thanks again for the patch. To summarize for future readers: the default behavior doesn't change - at least not now - you need to customize the option for different indentation.
bug#72765: Eglot + Clangd + Company + non-empty suffix = duplicate text
On 01/09/2024 17:28, Dmitry Gutov wrote: * the rust-analyzer test you added recently -- and which you said was very brittle -- is indeed very brittle: I cannot get it to pass. We should fix it, or just delete it and do those rust-analyzer tests manually each time we touch this area. Could you give more details? It is indeed more brittle in theory, but on my machine it's passing every time. Yeah, I see it now - it succeeds in an interactive session and fails in batch mode. Not sure it was the same when the patch was committed (hopefully not). Might be due to window configuration being different...
bug#73044: [PATCH] Add project-find-file-in-root
Version: 31.1 Hi Spencer, On 05/09/2024 17:01, Spencer Baugh via Bug reports for GNU Emacs, the Swiss army knife of text editors wrote: Several users have asked me for a command which is just find-file, but starting from the project root. In large projects, where project-files is expensive, this will have substantially better performance than project-find-file. Also, it allows opening files which aren't included in project-files without paying the further cost of running project--files-in-directory (which is what happens when passing INCLUDE-ALL=t to project-find-file). Also, it may help with user confusion about why project-find-file doesn't behave like find-file. (which I've encountered a few times) This command is equivalent to C-x p o C-x C-f, but it's nice to be able to bind it to a specific key. Overall, this is easy enough to provide, so let's just do that. Makes sense, thanks, pushed to master (adding the 'interactive' form and a NEWS entry). The name is a little verbose - if anybody has a better idea later, they're welcome to suggest it, there is some time to do a change.
bug#73044: [PATCH] Add project-find-file-in-root
On 06/09/2024 20:44, Eli Zaretskii wrote: Resent-To:bug-gnu-emacs@gnu.org Date: Fri, 6 Sep 2024 19:20:16 +0300 From: Dmitry Gutov This command is equivalent to C-x p o C-x C-f, but it's nice to be able to bind it to a specific key. Overall, this is easy enough to provide, so let's just do that. Makes sense, thanks, pushed to master (adding the 'interactive' form and a NEWS entry). Thanks, but why only NEWS? I think this should be in the user manual as well. I don't know, to me it seems like a niche enough feature, having lived without this addition for a number of major releases. Spencer seems to agree, having submitted the patch without a default binding. But if someone wants to describe it in the manual, I of course wouldn't mind. They can mention the situations described in this report.
bug#43086: [PATCH] Allow tags backend to not query for TAGS file
On 03/09/2024 19:39, Philip Kaludercic wrote: I could imagine this might be extended to allow an auto-generate option, but that feature seems out of scope of this patch, and probably would require some interoperation with project.el. Indeed. Actually, I have an old, WIP patch for tag file auto-generation which, yes, uses project.el. I can post it again if you're curious. Hasn't this issue been resolved by `etags-regen-mode'? The part quoted above was, I think. What might still be missing, is functioning better without having a tags table generated - after all etags-regen-mode is off by default, and it might not work for certain projects anyway. Maybe just like this? This makes Xref identifier completion not query for TAGS unless already loaded. In many cases that would be TRT, although `C-u M-.` seems to regress (seems like we *would* want to query eagerly there). Adding a user option is still... an option. diff --git a/lisp/progmodes/etags.el b/lisp/progmodes/etags.el index d3eb0d46e9b..a4e9abe9b7a 100644 --- a/lisp/progmodes/etags.el +++ b/lisp/progmodes/etags.el @@ -2102,7 +2102,9 @@ xref-backend-identifier-at-point (cl-defmethod xref-backend-identifier-completion-table ((_backend (eql 'etags))) - (tags-lazy-completion-table)) + (and (or tags-file-name + tags-table-list) + (tags-lazy-completion-table))) (cl-defmethod xref-backend-identifier-completion-ignore-case ((_backend (eql 'etags)))
bug#70968: 29.2.50; choose-completion on an emacs22-style completion deletes text after point
On 07/09/2024 10:30, Eli Zaretskii wrote: Date: Mon, 26 Aug 2024 03:08:56 +0300 Cc:sba...@janestreet.com,70...@debbugs.gnu.org,j...@linkov.net, monn...@iro.umontreal.ca From: Dmitry Gutov On 16/05/2024 21:25, Eli Zaretskii wrote: I don't think that would be required exactly. The problem here (IIUC) is that completion behaves differently with the emacs22 style depending on whether the execution path went through choose-completion (which is not a method of completion style but a common subroutine) or not (when completion--do-completion performed expansion). I understand that much. But what did these two (or their then-equivalents) do in Emacs 22 and Emacs 23? I'm guessing they behaved incorrectly (or however we want to call the inconsistent behavior), but I don't have a compiled Emacs 22/23 around, and they might be difficult to build. Note that we fixed bug#48356 not too long ago, which is from the same general area, and it probably originated from before Emacs 22/23 too. It's worth looking for edge cases where we'd strongly prefer the current behavior, and they might exist, but so far I only know of situations where the change would be for the better, or the user might be okay with either (example at the end ofhttps://debbugs.gnu.org/72705#35). Ping! How should we proceed with this bug report? I suggest we come up with a fix (which Stefan might have some ideas for), then see which reasonable scenarios get broken, if any. The one edge case in Eglot could be fixed in the same Emacs release, I believe. If any larger scope problems, we could add a variable/option to switch to the previous behavior.
bug#72768: [PATCH] Keep local keymap out of vc-git-stash-get-at-point
Version: 31.1 On 07/09/2024 10:33, Eli Zaretskii wrote: Date: Tue, 27 Aug 2024 03:37:54 +0530 From: James Thomas via "Bug reports for GNU Emacs, the Swiss army knife of text editors" James Thomas wrote: emacs -Q (define-key key-translation-map [?\C-j] (kbd "RET")) M-x vc-dir (select a git repo) (with point on a 'stash' entry) C-x v ! = C-j (fails) Of course, if a 'stash' doesn't exist, it may be created, so that the steps are: emacs -Q C-x C-f (open a git-tracked file, make changes), C-x C-s (and save it) C-x p v (select the above repo) z c (create stash) M-: (define-key key-translation-map [?\C-j] (kbd "RET")) (with point on the new 'stash' entry) C-x v ! = C-j (fails) Dmitry, any comments or suggestions? Makes sense. Pushed to master, thanks!
bug#73044: [PATCH] Add project-find-file-in-root
On 07/09/2024 09:10, Eli Zaretskii wrote: If you consider this not important enough to be in the manual, please mark the NEWS entry with "---", to convey your opinion. Added, thanks. Also split the entry into two lines.
bug#72701: eglot crash when project-files-relative-names t
On 07/09/2024 10:20, Eli Zaretskii wrote: Ping! Is this issue resolved and can be closed, or do we need to do anything else here? I suggest installing the following. Not a hard necessity, but seems like an improvement: diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el index acc197754db..e5b14ce9f80 100644 --- a/lisp/progmodes/eglot.el +++ b/lisp/progmodes/eglot.el @@ -3813,6 +3813,7 @@ eglot--code-action (cl-defmethod eglot-register-capability (server (method (eql workspace/didChangeWatchedFiles)) id &key watchers) "Handle dynamic registration of workspace/didChangeWatchedFiles." + (defvar project-files-relative-names) (eglot-unregister-capability server method id) (let* (success (globs (mapcar @@ -3823,6 +3824,7 @@ eglot-register-capability ;; (2), WatchKind.Delete (4) (or kind 7))) watchers)) + (project-files-relative-names nil) (dirs-to-watch (delete-dups (mapcar #'file-name-directory (project-files diff --git a/lisp/progmodes/project.el b/lisp/progmodes/project.el index c38d3f0048a..78f5c127900 100644 --- a/lisp/progmodes/project.el +++ b/lisp/progmodes/project.el @@ -331,7 +331,10 @@ project-files-relative-names The file names should be relative to the project root. And this can only happen when all returned files are in the same directory. In other words, the DIRS argument of `project-files' has to be nil or a -list of only one element.") +list of only one element. + +This variable is only meant to be set by Lisp code, not customized by +the user.") (cl-defgeneric project-files (project &optional dirs) "Return a list of files in directories DIRS in PROJECT.
bug#72765: Eglot + Clangd + Company + non-empty suffix = duplicate text
On 03/09/2024 16:43, João Távora wrote: On Tue, Sep 3, 2024 at 2:20 PM Dmitry Gutov wrote: On 01/09/2024 17:28, Dmitry Gutov wrote: * the rust-analyzer test you added recently -- and which you said was very brittle -- is indeed very brittle: I cannot get it to pass. We should fix it, or just delete it and do those rust-analyzer tests manually each time we touch this area. Could you give more details? It is indeed more brittle in theory, but on my machine it's passing every time. Yeah, I see it now - it succeeds in an interactive session and fails in batch mode. Not sure it was the same when the patch was committed (hopefully not). Might be due to window configuration being different... Yes, I was trying batch mode. make -C test eglot-tests or something similar. Please fix it or delete it (or disable it). Looking at minibuffer-tests.el, the above might be a solution, but it gets me a core dump instead: diff --git a/test/lisp/progmodes/eglot-tests.el b/test/lisp/progmodes/eglot-tests.el index e0168baee54..fa3b63b38dc 100644 --- a/test/lisp/progmodes/eglot-tests.el +++ b/test/lisp/progmodes/eglot-tests.el @@ -711,7 +711,8 @@ eglot-test-rust-completion-exit-function (search-forward "v.count_on") (let ((minibuffer-message-timeout 0) ;; Fail at (ding) if completion fails. -(executing-kbd-macro t)) +(executing-kbd-macro t) +(redisplay-skip-initial-frame nil)) (when (buffer-live-p "*Completions*") (kill-buffer "*Completions*")) ;; The design is pretty brittle, we'll need to monitor the Will follow up later if nobody beats me to it (can others reproduce the crash?)
bug#72701: eglot crash when project-files-relative-names t
On 08/09/2024 13:56, João Távora wrote: On Sun, Sep 8, 2024 at 3:24 AM Dmitry Gutov wrote: On 07/09/2024 10:20, Eli Zaretskii wrote: Ping! Is this issue resolved and can be closed, or do we need to do anything else here? I suggest installing the following. Not a hard necessity, but seems like an improvement: Let's not, for all the reasons enunciated up-thread. Very well - I've just installed the docstring update. Thanks, closing.
bug#72765: Eglot + Clangd + Company + non-empty suffix = duplicate text
On 08/09/2024 18:51, João Távora wrote: Looking at minibuffer-tests.el, the above might be a solution, but it gets me a core dump instead: diff --git a/test/lisp/progmodes/eglot-tests.el b/test/lisp/progmodes/eglot-tests.el index e0168baee54..fa3b63b38dc 100644 --- a/test/lisp/progmodes/eglot-tests.el +++ b/test/lisp/progmodes/eglot-tests.el @@ -711,7 +711,8 @@ eglot-test-rust-completion-exit-function (search-forward "v.count_on") (let ((minibuffer-message-timeout 0) ;; Fail at (ding) if completion fails. -(executing-kbd-macro t)) +(executing-kbd-macro t) +(redisplay-skip-initial-frame nil)) (when (buffer-live-p "*Completions*") (kill-buffer "*Completions*")) ;; The design is pretty brittle, we'll need to monitor the Will follow up later if nobody beats me to it (can others reproduce the crash?) This now aborts (segfault?). At least something different. So, for the record, before this patch with the latest emacs-30, I get the results in failure1.txt and with your last redisplay-skip-initial-frame patch I get failure2.txt. I've produced these files with make -C test eglot-tests SELECTOR=\"rust-completion\" 2>&1 | tee failure1.txt So it's reproducible. Great! Could someone look into the segfault? The repro steps are simple: 1) apply the patch above, 2) run 'make -C test eglot-tests' or the longer command above which executes just one test from that file. The backtrace that I managed to generate is attached.Core was generated by `../src/emacs --module-assertions --no-init-file --no-site-file --no-site-lisp -'. Program terminated with signal SIGABRT, Aborted. #0 __pthread_kill_implementation (no_tid=0, signo=6, threadid=) at ./nptl/pthread_kill.c:44 Download failed: Invalid argument. Continuing without source file ./nptl/./nptl/pthread_kill.c. 44 ./nptl/pthread_kill.c: No such file or directory. (gdb) bt #0 __pthread_kill_implementation (no_tid=0, signo=6, threadid=) at ./nptl/pthread_kill.c:44 #1 __pthread_kill_internal (signo=6, threadid=) at ./nptl/pthread_kill.c:78 #2 __GI___pthread_kill (threadid=, signo=signo@entry=6) at ./nptl/pthread_kill.c:89 #3 0x794b524428e6 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26 #4 0x557cbe936774 in terminate_due_to_signal (sig=sig@entry=6, backtrace_limit=backtrace_limit@entry=40) at emacs.c:480 #5 0x557cbe959e9f in emacs_abort () at sysdep.c:2391 #6 0x557cbe8793fb in try_window_id (w=0x557cc05f09e8) at xdisp.c:22092 #7 redisplay_window (window=0x557cc05f09ed, just_this_one_p=just_this_one_p@entry=false) at xdisp.c:20444 #8 0x557cbe87d13e in redisplay_window_0 (window=window@entry=0x557cc05f09ed) at xdisp.c:18020 #9 0x557cbe9bff71 in internal_condition_case_1 (bfun=bfun@entry=0x557cbe87d10b , arg=arg@entry=0x557cc05f09ed, handlers=, hfun=hfun@entry=0x557cbe835eac ) at eval.c:1637 #10 0x557cbe833d83 in redisplay_windows (window=0x557cc05f09ed) at xdisp.c:17989 #11 0x557cbe867eeb in redisplay_internal () at xdisp.c:17388 #12 0x557cbe86853d in redisplay () at xdisp.c:16563 #13 0x557cbe947be0 in read_char (commandflag=commandflag@entry=0, map=map@entry=0x0, prev_event=prev_event@entry=0x30, used_mouse_menu=used_mouse_menu@entry=0x0, end_time=0x7fff70418740) at keyboard.c:2678 #14 0x557cbe9ebec0 in read_filtered_event (no_switch_frame=no_switch_frame@entry=false, ascii_required=ascii_required@entry=false, error_nonascii=error_nonascii@entry=false, input_method=input_method@entry=false, seconds=seconds@entry=0x794b4e67024f) at lread.c:791 #15 0x557cbe9ec0fa in Fread_event (prompt=0x0, inherit_input_method=0x0, seconds=0x794b4e67024f) at lread.c:941 #16 0x557cbe9c47e8 in eval_sub (form=) at eval.c:2604 #17 0x557cbe9c4df0 in Fprogn (body=) at eval.c:439 #18 0x557cbe9c55a9 in Fcond (args=0x794b4da7fd93) at eval.c:419 #19 0x557cbe9c45b2 in eval_sub (form=) at eval.c:2549 #20 0x557cbe9c4df0 in Fprogn (body=) at eval.c:439 #21 0x557cbe9c45b2 in eval_sub (form=) at eval.c:2549 #22 0x557cbe9c5545 in Fif (args=0x794b4d932be3) at eval.c:394 #23 0x557cbe9c45b2 in eval_sub (form=) at eval.c:2549 #24 0x557cbe9c4df0 in Fprogn (body=, body@entry=0x794b4d932e53) at eval.c:439 #25 0x557cbe9c597f in prog_ignore (body=0x794b4d932e53) at eval.c:450 #26 Fwhile (args=) at eval.c:1130 #27 0x557cbe9c45b2 in eval_sub (form=) at eval.c:2549 #28 0x557cbe9c4df0 in Fprogn (body=) at eval.c:439 #29 0x557cbe9c6210 in FletX (args=) at /home/dgutov/vc/emacs/src/lisp.h:1539 #30 0x557cbe9c45b2 in eval_sub (form=) at eval.c:2549 #31 0x557cbe9c5d5b in Flet (args=) at eval.c:1080 #32 0x557cbe9c45b2 in eval_sub (form=) at eval.c:2549 #33 0x557cbe9c4df0 in Fprogn (body=) at eval.c:439 #34 0x557cbe9c517b in funcall_lambda (fun=, nargs=nargs@entry=0, a
bug#43086: [PATCH] Allow tags backend to not query for TAGS file
On 07/09/2024 09:18, Eli Zaretskii wrote: Maybe just like this? This makes Xref identifier completion not query for TAGS unless already loaded. In many cases that would be TRT, although `C-u M-.` seems to regress (seems like we *would* want to query eagerly there). I don't understand why the obvious way of asking the user whether they would like to generate the tags table is not the solution here. What did I miss? I don't know if it's obvious, given that the optimal scenario at the beginning of the report describes ... allow the backend to never query a TAGS file Adding a query to avoid querying might not be ideal. And if we did query, that would raise a question of where to store the answer (should it be global, or per-project, or temporary somehow?). As it is, we already have a hint of the user preference from the fact that they have visited a TAGS file already (or not), or enabled etags-regen-mode (or not). It's not ironclad, but we could rely on these indicators to decide.
bug#43086: [PATCH] Allow tags backend to not query for TAGS file
On 09/09/2024 14:54, Eli Zaretskii wrote: Date: Mon, 9 Sep 2024 03:29:05 +0300 Cc: phil...@posteo.net, 43...@debbugs.gnu.org From: Dmitry Gutov On 07/09/2024 09:18, Eli Zaretskii wrote: Maybe just like this? This makes Xref identifier completion not query for TAGS unless already loaded. In many cases that would be TRT, although `C-u M-.` seems to regress (seems like we *would* want to query eagerly there). I don't understand why the obvious way of asking the user whether they would like to generate the tags table is not the solution here. What did I miss? I don't know if it's obvious, given that the optimal scenario at the beginning of the report describes ... allow the backend to never query a TAGS file But what do you expect from a backend that depends on TAGS to do when TAGS is not there? You yourself just noticed the regression. Why would we want that? I'm thinking of the xref-find-references case - where the scanner doesn't depend on the tags table being available. Just the identifier completion step. Perhaps Philip had other some situations in mind, too. AFAIU, the problem here is that the backend can avoid querying when the TAGS file exists. But what do you expect it to do when it does _not_ exist? > We have the regeneration feature now, so I suggested to ask the user whether to regenerate the file, after which it could be read without any further prompts. We have an existing way to enable etags-regen-mode. And it's a global mode, so it's not just an issue of using it that one time - the naive solution will make stay on until the end of the session. Also, if the tags file is not loaded, we're not quite sure whether the user wants an auto-generated file, or an existing one. As it is, we already have a hint of the user preference from the fact that they have visited a TAGS file already (or not), or enabled etags-regen-mode (or not). It's not ironclad, but we could rely on these indicators to decide. Then regenerate TAGS without asking, if you think it's reasonable. But letting the backend continue without TAGS is not a reasonable solution, IMO. How do you feel about etags-regen-mode being on by default in some next Emacs release? It shouldn't conflict with the manual invocations of 'M-x visit-tags-file' - and of course if any cases come up we'll work on fixing those.
bug#72765: Eglot + Clangd + Company + non-empty suffix = duplicate text
On 09/09/2024 14:46, Eli Zaretskii wrote: Date: Mon, 9 Sep 2024 03:20:01 +0300 Cc: Eli Zaretskii , 72...@debbugs.gnu.org From: Dmitry Gutov This now aborts (segfault?). At least something different. So, for the record, before this patch with the latest emacs-30, I get the results in failure1.txt and with your last redisplay-skip-initial-frame patch I get failure2.txt. I've produced these files with make -C test eglot-tests SELECTOR=\"rust-completion\" 2>&1 | tee failure1.txt So it's reproducible. Great! Could someone look into the segfault? The repro steps are simple: 1) apply the patch above, 2) run 'make -C test eglot-tests' or the longer command above which executes just one test from that file. The backtrace that I managed to generate is attached. Thanks. Please try the patch below. Thanks! The patch takes care of the crash AFAICS (no core dump now), but alas not of the original test failure. P.S. I'm not at all sure this is the last segfault you will see because you are playing with fire: you are not supposed to reset redisplay-skip-initial-frame to nil in batch-mode tests that test redisplay-related features! Isn't that the main/only purpose of this variable? That's how the docstring reads to me, and it's also seems why minibuffer-test.el uses it. In any case, this var is neither necessary nor sufficient (see my next email), so sorry if that wasted you time. The fix might still be worth installing, though. diff --git a/src/xdisp.c b/src/xdisp.c index bf7d84c..a1319e7 100644 --- a/src/xdisp.c +++ b/src/xdisp.c @@ -22089,7 +22089,8 @@ #define GIVE_UP(X) return 0 /* Window must either use window-based redisplay or be full width. */ if (!FRAME_WINDOW_P (f) - && (!FRAME_LINE_INS_DEL_OK (f) + && (FRAME_INITIAL_P (f) + || !FRAME_LINE_INS_DEL_OK (f) || !WINDOW_FULL_WIDTH_P (w))) GIVE_UP (4);
bug#72765: Eglot + Clangd + Company + non-empty suffix = duplicate text
Hi Joao,
On 08/09/2024 18:51, João Távora wrote:
So, for the record, before this patch with the latest emacs-30, I get the
results in failure1.txt
I've taken some more looks at the test output.
[eglot-tests] contents of `*EGLOT (cmpl-project/(rust-mode
rust-ts-mode)) output*':
[eglot-tests] contents of `nil':
[eglot-tests] Killing (main.rs), wiping /tmp/eglot--fixture-XCmCqo
Test eglot-test-rust-completion-exit-function backtrace:
set-buffer(#) eglot--call-with-fixture((("cmpl-project" ("main.rs" .
"fn test() ->Test eglot-test-rust-completion-exit-function condition:
(error "Selecting deleted buffer")
This error comes from eglot--call-with-fixture, where one of the last
(with-current-buffer buffer (buffer-string))
calls results in "Selecting deleted buffer" error. I'm not sure where
that comes from (for the duration of the log the contents of stdout
buffer are printed twice, and stderr and events just once -- somehow
there are two elements inside the 'new-servers' var).
That hid the original problem, which was just that in the bare session
yasnippet is not available. Not sure what to do about it -
monkeypatching a replacements does not seem wise - but if we adjust the
"expected" at the end not to have parens, then the test finally passes.
It will fail interactively, though, when yasnippet is installed.
I've pushed that fix as
https://git.savannah.gnu.org/cgit/emacs.git/commit/?h=emacs-30&id=818c0cc9a51a1d678749404cdacdf640d6f32d6e
now.
It makes that test more similar to
'eglot-test-try-completion-inside-symbol', but testing rust-analyzer
separately still seems like a plus.
bug#72765: Eglot + Clangd + Company + non-empty suffix = duplicate text
On 10/09/2024 14:47, Eli Zaretskii wrote: Date: Tue, 10 Sep 2024 03:58:58 +0300 Cc:joaotav...@gmail.com,72...@debbugs.gnu.org From: Dmitry Gutov The backtrace that I managed to generate is attached. Thanks. Please try the patch below. Thanks! The patch takes care of the crash AFAICS (no core dump now), but alas not of the original test failure. I installed the patch on the emacs-30 branch. P.S. I'm not at all sure this is the last segfault you will see because you are playing with fire: you are not supposed to reset redisplay-skip-initial-frame to nil in batch-mode tests that test redisplay-related features! Isn't that the main/only purpose of this variable? You mean, playing with fire? Yes, but you need to be aware of that. This variable was introduced relatively recently for use by test/src/xdisp-tests.el; anywhere else you are on your own 😉 Thanks!
bug#43086: [PATCH] Allow tags backend to not query for TAGS file
On 10/09/2024 14:41, Eli Zaretskii wrote: But what do you expect from a backend that depends on TAGS to do when TAGS is not there? You yourself just noticed the regression. Why would we want that? I'm thinking of the xref-find-references case - where the scanner doesn't depend on the tags table being available. Just the identifier completion step. Completion is also important, IMO. Just not always worth the extra query or wait time. We have an existing way to enable etags-regen-mode. And it's a global mode, so it's not just an issue of using it that one time - the naive solution will make stay on until the end of the session. We could in this particular case enable it once, then disable it after regenerating TAGS. I'm not sure I'd want a one-time generation of tags which never gets updated afterward. Not for me, nor for an inexperienced user who would likely get puzzled at some point about why the index not updating. Also, if the tags file is not loaded, we're not quite sure whether the user wants an auto-generated file, or an existing one. The query should allow the user to tell us his/her preference, no? For that we need to decide on the options and the possible lifetimes of the answer in advance. That's all I'm saying: it's not an obvious "just ask the user". How do you feel about etags-regen-mode being on by default in some next Emacs release? It shouldn't conflict with the manual invocations of 'M-x visit-tags-file' - and of course if any cases come up we'll work on fixing those. As long as there's a way of turning it off, I don't think I will mind too much. Great! As long as nobody objects in the coming days I'll switch the default value.
bug#43086: [PATCH] Allow tags backend to not query for TAGS file
On 10/09/2024 15:45, Eli Zaretskii wrote: Cc: phil...@posteo.net, 43...@debbugs.gnu.org Date: Tue, 10 Sep 2024 14:41:43 +0300 From: Eli Zaretskii How do you feel about etags-regen-mode being on by default in some next Emacs release? It shouldn't conflict with the manual invocations of 'M-x visit-tags-file' - and of course if any cases come up we'll work on fixing those. As long as there's a way of turning it off, I don't think I will mind too much. Come to think about this: this mode runs 'etags' without any special switches, right? Then what will turning this mode ON do in the Emacs repository, where our 'etags' command is very heavily customized (see src/Makefile.in)? etags-regen-mode doesn't know how to parse Makefile.in, and it doesn't seem feasible, so it needs the user to customize `etags-regen-regexp-alist` for any special rules. That's already done in the Emacs repo, see our .dir-locals.el.
bug#73172: [PATCH] Move to start of current header in diff-{file, hunk}-prev
Hi Spencer and Stefan, On 10/09/2024 21:40, Spencer Baugh via Bug reports for GNU Emacs, the Swiss army knife of text editors wrote: (The following change is split across two patches; the first one, "move easy-mmode", fixes an unrelated FIXME, which makes the diff in the second patch simpler) If point was after a file or hunk header, the diff-file-prev and diff-hunk-prev commands would move to the start of that header. But if point was *within* the header, they would not move, and would report "No previous file" or "No previous hunk". This differs from the behavior of most other movement commands, e.g. backward-sexp or backward-sentence. This commit fixes diff-file-prev and diff-hunk-prev, as well as other easy-mmode-define-navigation BASE-prev commands. Now these commands move to the start of the containing "thing" just like other movement commands. * lisp/emacs-lisp/easy-mmode.el (easy-mmode--prev): Move to start of current match first. Also discussed here: https://lists.gnu.org/archive/html/help-gnu-emacs/2024-08/msg00367.html Patch#1 seems unequivocally a good thing (easier code iteration) and patch#2 seems good on balance. It does introduce some backward incompatibility in rare cases where I have probably internalized the current behavior already -- for example in the vc-print-root-log output pressing 'p' while on the first line somewhere between the initial '*' and the end of the date dings with "No previous log message", and how will move to bol. But it might be more consistent anyway, given that the there is no ding already if you start out inside the summary text. There aren't many in-tree callers of easy-mmode-define-navigation (diff-mode, log-view, smerge and, uh, cvs-status), and few 3rd party callers, so this seems low-impact. diff-hl-mode is not affected either because it only uses diff-hunk-next, not diff-hunk-prev.
bug#72426: 29.2.50; comint-pager doesn't affect async-shell-command
On 03/08/2024 18:38, Eli Zaretskii wrote: comint-terminfo-terminal affects async-shell-command, why not this? Ugh! A mistake, IMNSHO. But that ship sailed a long time ago, so we cannot fix the mistake. We can avoid enlarging the mistake, though. If the fact that the variable is in comint is the problem, I can rename it and move it elsewhere. I don't think functions that are almost primitives should pay attention to application-level features such as this one. FWIW async-shell-command's behavior is affected by a bunch of other comint-* variables as well because it uses a major mode which inherits from comint-mode (previously it was shell-mode, now shell-command-mode, but that aspect didn't change).
bug#72652: 31.0.50; url-retrieve on non-existent domain gives no indication of error
On 17/08/2024 15:54, Greg Minshall wrote: Patches to report an error in the (mytry 2) case are welcome. that'd be great. thanks. For those who'd like to try their hand at writing the fix, https://debbugs.gnu.org/cgi/bugreport.cgi?bug=26835#18 might be relevant, or at least point to the problem area.
bug#72765: Eglot + Clangd + Company + non-empty suffix = duplicate text
On 31/08/2024 15:03, João Távora wrote: Eglot aims primarily at that, since it's what's in Emacs proper. But Eglot also aims at supporting Company in particular as fully as possible. Anyway, I don't have time to investigate this. The :exit-function in eglot.el should be stepped through to understand exactly who's at fault. And I don't think differences between servers matter: clangd is likely following the spec correctly here. It seems the difference comes from bug#70968 as well (which came up recently). When the completion style emacs22 is used, Company doesn't delete the suffix text before inserting completion. Which is an improvement for some other completion sources, but not Eglot, so far. To to fix this here, we can avoid a fail-over to emacs22 by only matching the prefix in eglot--dumb-allc like this: diff --git a/lisp/progmodes/eglot.el b/lisp/progmodes/eglot.el index 59d9c346424..20c15584d2d 100644 --- a/lisp/progmodes/eglot.el +++ b/lisp/progmodes/eglot.el @@ -3138,7 +3138,9 @@ eglot--dumb-flex nil comp) finally (cl-return comp))) -(defun eglot--dumb-allc (pat table pred _point) (funcall table pat pred t)) +(defun eglot--dumb-allc (pat table pred point) + (funcall table (substring pat 0 point) pred t)) + (defun eglot--dumb-tryc (pat table pred point) (let ((probe (funcall table pat pred nil))) (cond ((eq probe t) t) That fixes the scenario in Company, with seemingly no change with completion-at-point. Or if we want 100% compatibility, we can use 'or': (or (funcall table pat pred t) (funcall table (substring pat 0 point) pred t)) But in any case this doesn't help with the completion-at-point behavior described at the end of the report (where foo_|bar_2 turns into foo_bar_2bar_2|). If we consider it okay - then the above patch fixes the discrepancy with Company completion, and done. But if we think it a problem, then the fix might be required somewhere in the area of (cond (textEdit ;; Revert buffer back to state when the edit ;; was obtained from server. If a `proxy' After (and if) that is done, we might not need to change the completion style in the end.
