bug#19400: 24.4.50; `completion-separator-self-insert-command' calls `undefined' with error

bug#19400: 24.4.50; `completion-separator-self-insert-command' calls `undefined' with error

[Nicolas Richard]

Hello,

From:
emacs -Q -f dynamic-completion-mode

Open a vc controlled directory with vc-dir

Hit ^

Instead of "^ is undefined", I get
"completion-separator-self-insert-command: Wrong number of arguments: (0
. 0), 1"

backtrace is :

Debugger entered--Lisp error: (wrong-number-of-arguments (0 . 0) 1)
  undefined(1)
  completion-separator-self-insert-command(1)
  call-interactively(completion-separator-self-insert-command nil nil)
  command-execute(completion-separator-self-insert-command)

Have the function "undefined" accept (and ignore) any argument would be
an easy workaround. WDYT ?

-- 
Nicolas Richard

bug#19400: 24.4.50; `completion-separator-self-insert-command' calls `undefined' with error

[Stefan Monnier]

> emacs -Q -f dynamic-completion-mode

[ Yuck!  completion.el is one of the packages which does not following
  the prefix convention *at all*.  ]

> Instead of "^ is undefined", I get
> "completion-separator-self-insert-command: Wrong number of arguments: (0
> . 0), 1"

Does the patch below fix this problem (I'm pretty sure it does), and
more importantly, does it seem to preserve the expected behavior?


        Stefan


diff --git a/lisp/completion.el b/lisp/completion.el
index d2d94e7..3e8b6aa 100644
--- a/lisp/completion.el
+++ b/lisp/completion.el
@@ -2156,26 +2156,27 @@ Patched to remove the most recent completion."
 ;; to work)
 
 ;; All common separators (eg. space "(" ")" """) characters go through a
-;; function to add new words to the list of words to complete from:
-;;  COMPLETION-SEPARATOR-SELF-INSERT-COMMAND (arg).
+;; function to add new words to the list of words to complete from.
 ;; If the character before this was an alpha-numeric then this adds the
 ;; symbol before point to the completion list (using ADD-COMPLETION).
 
-(defun completion-separator-self-insert-command (arg)
-  (interactive "p")
-  (if (command-remapping 'self-insert-command)
-      (funcall (command-remapping 'self-insert-command) arg)
-    (use-completion-before-separator)
-    (self-insert-command arg)))
-
-(defun completion-separator-self-insert-autofilling (arg)
-  (interactive "p")
-  (if (command-remapping 'self-insert-command)
-      (funcall (command-remapping 'self-insert-command) arg)
-    (use-completion-before-separator)
-    (self-insert-command arg)
-    (and auto-fill-function
-	 (funcall auto-fill-function))))
+(defvar completion-separator-chars
+  (append " !%^&()=`|{}[];\\'#,?"
+          ;; We include period and colon even though they are symbol
+          ;; chars because :
+          ;;  - in text we want to pick up the last word in a sentence.
+          ;;  - in C pointer refs. we want to pick up the first symbol
+          ;;  - it won't make a difference for lisp mode (package names
+          ;;    are short)
+          ".:"))
+
+(defun completion--post-self-insert ()
+  (when (memq last-command-event completion-separator-chars)
+    (let ((after-pos (electric--after-char-pos)))
+      (when after-pos
+        (save-excursion
+          (goto-char (1- after-pos))
+          (use-completion-before-separator))))))
 
 ;;-----------------------------------------------
 ;; Wrapping Macro
@@ -2244,9 +2245,8 @@ TYPE is the type of the wrapper to be added.  Can be :before or :under."
 (completion-def-wrapper 'electric-c-semi :separator)
 (defun completion-c-mode-hook ()
   (setq completion-syntax-table completion-c-syntax-table)
-  (local-set-key "+" 'completion-separator-self-insert-command)
-  (local-set-key "*" 'completion-separator-self-insert-command)
-  (local-set-key "/" 'completion-separator-self-insert-command))
+  (setq-local completion-separator-chars
+              (append "+*/" completion-separator-chars)))
 
 ;; FORTRAN mode diffs. (these are defined when fortran is called)
 
@@ -2259,10 +2259,8 @@ TYPE is the type of the wrapper to be added.  Can be :before or :under."
 
 (defun completion-setup-fortran-mode ()
   (setq completion-syntax-table completion-fortran-syntax-table)
-  (local-set-key "+" 'completion-separator-self-insert-command)
-  (local-set-key "-" 'completion-separator-self-insert-command)
-  (local-set-key "*" 'completion-separator-self-insert-command)
-  (local-set-key "/" 'completion-separator-self-insert-command))
+  (setq-local completion-separator-chars
+              (append "+-*/" completion-separator-chars)))
 \f
 ;; Enable completion mode.
 
@@ -2281,15 +2279,16 @@ if ARG is omitted or nil."
   ;; This is always good, not specific to dynamic-completion-mode.
   (define-key function-key-map [C-return] [?\C-\r])
 
-  (dolist (x '((find-file-hook		. completion-find-file-hook)
-               (pre-command-hook	. completion-before-command)
+  (dolist (x `((find-file-hook		. ,#'completion-find-file-hook)
+               (pre-command-hook	. ,#'completion-before-command)
                ;; Save completions when killing Emacs.
-               (kill-emacs-hook		. kill-emacs-save-completions)
-
+               (kill-emacs-hook		. ,#'kill-emacs-save-completions)
+               (post-self-insert-hook	. ,#'completion--post-self-insert)
+               
                ;; Install the appropriate mode tables.
-               (lisp-mode-hook		. completion-lisp-mode-hook)
-               (c-mode-hook		. completion-c-mode-hook)
-               (fortran-mode-hook	. completion-setup-fortran-mode)))
+               (lisp-mode-hook		. ,#'completion-lisp-mode-hook)
+               (c-mode-hook		. ,#'completion-c-mode-hook)
+               (fortran-mode-hook	. ,#'completion-setup-fortran-mode)))
     (if dynamic-completion-mode
         (add-hook (car x) (cdr x))
       (remove-hook (car x) (cdr x))))
@@ -2315,44 +2314,7 @@ if ARG is omitted or nil."
                ;; cumb
 
                ;; Patches to standard keymaps insert completions
-               ([remap kill-region] . completion-kill-region)
-
-               ;; Separators
-               ;; We've used the completion syntax table given  as a guide.
-               ;;
-               ;; Global separator chars.
-               ;;  We left out <tab> because there are too many special
-               ;; cases for it.  Also, in normal coding it's rarely typed
-               ;; after a word.
-               (" " . completion-separator-self-insert-autofilling)
-               ("!" . completion-separator-self-insert-command)
-               ("%" . completion-separator-self-insert-command)
-               ("^" . completion-separator-self-insert-command)
-               ("&" . completion-separator-self-insert-command)
-               ("(" . completion-separator-self-insert-command)
-               (")" . completion-separator-self-insert-command)
-               ("=" . completion-separator-self-insert-command)
-               ("`" . completion-separator-self-insert-command)
-               ("|" . completion-separator-self-insert-command)
-               ("{" . completion-separator-self-insert-command)
-               ("}" . completion-separator-self-insert-command)
-               ("[" . completion-separator-self-insert-command)
-               ("]" . completion-separator-self-insert-command)
-               (";" . completion-separator-self-insert-command)
-               ("\"".  completion-separator-self-insert-command)
-               ("'" . completion-separator-self-insert-command)
-               ("#" . completion-separator-self-insert-command)
-               ("," . completion-separator-self-insert-command)
-               ("?" . completion-separator-self-insert-command)
-
-               ;; We include period and colon even though they are symbol
-               ;; chars because :
-               ;;  - in text we want to pick up the last word in a sentence.
-               ;;  - in C pointer refs. we want to pick up the first symbol
-               ;;  - it won't make a difference for lisp mode (package names
-               ;;    are short)
-               ("." . completion-separator-self-insert-command)
-               (":" . completion-separator-self-insert-command)))
+               ([remap kill-region] . completion-kill-region)))
       (push (cons (car binding) (lookup-key global-map (car binding)))
             completion-saved-bindings)
       (global-set-key (car binding) (cdr binding)))

bug#19400: 24.4.50; `completion-separator-self-insert-command' calls `undefined' with error

[Drew Adams]

> > emacs -Q -f dynamic-completion-mode
> 
> > Instead of "^ is undefined", I get
> > "completion-separator-self-insert-command: Wrong number of
> > arguments: (0 . 0), 1"

FWIW, I don't see that problem at all, with this build (the latest
one I have):

In GNU Emacs 25.0.50.1 (i686-pc-mingw32)
 of 2014-11-30 on LEG570
Repository revision: 3517da701ea5d16c296745d6678988b06bee615d
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --without-dbus --enable-checking=yes,glyphs
 CPPFLAGS=-DGLYPH_DEBUG=1'

With emacs -Q -f dynamic-completion-mode I just get that mode turned on in *scratch*, and dynamic completion works fine.  And I see no such problem with official releases through Emacs 24.4 either.

> Does the patch below fix this problem (I'm pretty sure it does), and
> more importantly, does it seem to preserve the expected behavior?

I haven't tried it. But why rewrite so much, just to fix this problem (if it is indeed a problem and a fix)?

If this is a regression introduced sometime after the build I mentioned above, then why not concentrate on the code change that led to the breakage? It surely wasn't a change in the completions.el code that introduced a regression here.

FWIW, just looking at the patch I wonder what happened to the autofill behavior of SPC...

bug#19400: 24.4.50; `completion-separator-self-insert-command' calls `undefined' with error

[Nicolas Richard]

Hi,

Drew Adams <drew.adams@oracle.com> writes:
> FWIW, I don't see that problem at all, with this build (the latest
> one I have):

"Problem" is a bit too strong to describe the situation. Pressing ^
doesn't make sense in vc-dir anyway. It's only a minor annoyance that
the error is wrong.

> With emacs -Q -f dynamic-completion-mode I just get that mode turned
> on in *scratch*, and dynamic completion works fine.

It's a global mode : it's turned on everywhere.

The next step was to open a vc-dir buffer. Did you do that ?

-- 
Nicolas Richard

bug#19400: 24.4.50; `completion-separator-self-insert-command' calls `undefined' with error

[Nicolas Richard]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> Instead of "^ is undefined", I get
>> "completion-separator-self-insert-command: Wrong number of arguments: (0
>> . 0), 1"
>
> Does the patch below fix this problem (I'm pretty sure it does), and

It does, thanks. Though, you need to append nil at the end of the
definition of completion-separator-chars to make it a list : (append
"foo" ".:") is not a list.

> more importantly, does it seem to preserve the expected behavior?

It seems to, but my vote doesn't really count : I rarely use
dynamic-completion-mode in fact. (I mostly use hippie-expand these
days.)

-- 
Nicolas Richard

bug#19400: 24.4.50; `completion-separator-self-insert-command' calls `undefined' with error

[Andreas Schwab]

Nicolas Richard <theonewiththeevillook@yahoo.fr> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>> Instead of "^ is undefined", I get
>>> "completion-separator-self-insert-command: Wrong number of arguments: (0
>>> . 0), 1"
>>
>> Does the patch below fix this problem (I'm pretty sure it does), and
>
> It does, thanks. Though, you need to append nil at the end of the
> definition of completion-separator-chars to make it a list : (append
> "foo" ".:") is not a list.

s/append/concat/?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#19400: 24.4.50; `completion-separator-self-insert-command' calls `undefined' with error

[Nicolas Richard]

Andreas Schwab <schwab@linux-m68k.org> writes:
>> It does, thanks. Though, you need to append nil at the end of the
>> definition of completion-separator-chars to make it a list : (append
>> "foo" ".:") is not a list.
>
> s/append/concat/?

IIRC the code later uses memq on the result, hence my guess he wanted a
list.

-- 
Nicolas

bug#19400: 24.4.50; `completion-separator-self-insert-command' calls `undefined' with error

[Stefan Monnier]

> It does, thanks. Though, you need to append nil at the end of the
> definition of completion-separator-chars to make it a list : (append
> "foo" ".:") is not a list.

Thanks for spotting this.

>> more importantly, does it seem to preserve the expected behavior?
> It seems to, but my vote doesn't really count : I rarely use
> dynamic-completion-mode in fact. (I mostly use hippie-expand these
> days.)

I don't know anyone else who uses it, so you're the only vote I have
so far.


        Stefan

bug#19400: 24.4.50; `completion-separator-self-insert-command' calls `undefined' with error

[Stefan Monnier]

Version:25.1

>> It does, thanks. Though, you need to append nil at the end of the
>> definition of completion-separator-chars to make it a list : (append
>> "foo" ".:") is not a list.
> Thanks for spotting this.

Fixed and installed into master.


        Stefan