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.
