bug#16981: 24.3.50; electric-pair-delete-adjacent-pairs broken in c-mode, python-mode, maybe-others

bug#16981: 24.3.50; electric-pair-delete-adjacent-pairs broken in c-mode, python-mode, maybe-others

[João Távora]

This variable, t by default, acts when `electric-pair-mode' is enabled.

It doesn't work in c-mode, python-mode and maybe other modes in
emacs. In also doesn't work in other modes outside of emacs, such as
markdown-mode.el [1]

All these modes rebind the backspace key to a command that does some
mode-specific action, then calls `backward-delete-char-untabify'.

elec-pair.el does mostly the same through remapping in its
`electric-pair-mode-map' but this binding is overriden by the
major-mode's.

elec-pair.el remaps two other commands similarly. Here's the value of
electric-pair-mode-map

    (keymap
     (remap keymap
            (delete-backward-char . electric-pair-backward-delete-char)
            (backward-delete-char . electric-pair-backward-delete-char)
            (backward-delete-char-untabify
               . electric-pair-backward-delete-char-untabify)))


Perhaps a hook in `backward-delete-char' is in order. Or the other major
modes can find other ways to overload the backspace key.

Thanks,
João

[1]: http://jblevins.org/git/markdown-mode.git/plain/markdown-mode.el

bug#16981: 24.3.50; electric-pair-delete-adjacent-pairs broken in c-mode, python-mode, maybe-others

[Alan Mackenzie]

Jo?o T?vora <joaotavora@gmail.com> wrote:
> This variable, t by default, acts when `electric-pair-mode' is enabled.

> It doesn't work in c-mode, python-mode and maybe other modes in
> emacs. In also doesn't work in other modes outside of emacs, such as
> markdown-mode.el [1]

> All these modes rebind the backspace key to a command that does some
> mode-specific action, then calls `backward-delete-char-untabify'.

> elec-pair.el does mostly the same through remapping in its
> `electric-pair-mode-map' but this binding is overriden by the
> major-mode's.

There's something funny going on here.  A minor mode's keymap should
normally take precedence over the major mode's keymap.  (See the page 
"Searching Keymaps" in the Elisp manual.)  So why is electric-pair-mode's
keymap being overridden by CC Mode's here?

> elec-pair.el remaps two other commands similarly. Here's the value of
> electric-pair-mode-map

>    (keymap
>     (remap keymap
>            (delete-backward-char . electric-pair-backward-delete-char)
>            (backward-delete-char . electric-pair-backward-delete-char)
>            (backward-delete-char-untabify
>               . electric-pair-backward-delete-char-untabify)))


> Perhaps a hook in `backward-delete-char' is in order. Or the other major
> modes can find other ways to overload the backspace key.

I would say it's legitimate for a major mode to bind the backspace key, but
it's more questionable for a minor mode to do the same.  Perhaps the minor
mode really ought to use defadvice rather than rebinding the key.

> Thanks,
> Jo?o

> [1]: http://jblevins.org/git/markdown-mode.git/plain/markdown-mode.el

-- 
Alan Mackenzie (Nuremberg, Germany).

bug#16959: bug#16981: 24.3.50; electric-pair-delete-adjacent-pairs broken in c-mode, python-mode, maybe-others

[Stefan]

>> Why do you think it is obsolete?
> Because every hint of its existence has been expunged from the documentation
> and the new advice seems to offer the same functionality.

Indeed, it's technically obsolete, but it's not declared officially
obsolete yet.  It will come, tho.  But it'll be with us for many years
to come.  OTOH its support is limited to keeping things working.

>>> BTW, the nadvice info could use some more examples and a note on how
>>> to transition from defadvice.
>> I'll leave this report open till someone does that.
> Some specifics:

I've installed some changes to try and address them.


        Stefan

bug#16959: bug#16981: 24.3.50; electric-pair-delete-adjacent-pairs broken in c-mode, python-mode, maybe-others

[Florian Beck]

> I've installed some changes to try and address them.

Very nice.

Some comments:

Advising Emacs Lisp Functions:

  - The third paragraph could link to "What Is a Function?"
  - also link "in order to trace the calls to the process filter of a
process" to "Filter Functions"
  - in the last paragraph ":above" should be ":around"; the formatting 
"_advice_" is strange
  - mention that in the first example the advising function takes the 
same arguments as the original, and in the second the original function 
as an additional argument (this is the most important info, IMO)

Core Advising Primitives:
  - Should the WHERE specifications have their own node or section? In 
any case, I would move the second example (his-tracing-function) to the 
end of the list and explain it a little bit. Like

"In this case a call to `display-buffer' works like calling

(lambda (BUFFER-OR-NAME &optional ACTION FRAME)
   (apply 'his-tracing-fun 'display-buffer BUFFER-OR-NAME ACTION FRAME))

That is, instead of `display-buffer', the new function `his tracing-fun' 
is called with display-buffer as its first argument."

  - depth: clarify what happens when you have several pieces of 
":override" or ":around" advice that doesn't call the original function 
at the depth 0.

Advising named functions:
  - "Macros can also be advised": this doesn't work when the macro is 
used in a compiled function.
-- 
Florian Beck

bug#16959: bug#16981: 24.3.50; electric-pair-delete-adjacent-pairs broken in c-mode, python-mode, maybe-others

[Stefan]

>  - also link "in order to trace the calls to the process filter of a
> process" to "Filter Functions"

I find Texinfo links to be a bit heavy, so I prefer not to put too many
of them, so the reader can concentrate on the subject.  Process filters
are not really relevant.

>  - in the last paragraph ":above" should be ":around";

Fixed.

> the formatting "_advice_" is strange

That's how Texinfo renders @emph in Info.

> Core Advising Primitives:
>  - Should the WHERE specifications have their own node or section?

Sizewise, I fully agree, so I moved it to its own node/section.  I'm not
completely happy with the result, but I think it's better.

> In any case, I would move the second example (his-tracing-function) to
> the end of the list and explain it a little bit.

I'd rather keep the advice-add in the first node alongside the
add-function example, since as you say in many cases advice-add is what
the reader is really after.

>  - depth: clarify what happens when you have several pieces of ":override"
> or ":around" advice that doesn't call the original function at the depth 0.

I tried to clarify the notion of depth by spelling out what it means
for :after, :before and :override.

> Advising named functions:
>  - "Macros can also be advised": this doesn't work when the macro is used
> in a compiled function.

Of course it works.  The restriction is that the macro needs to be
advised before it is expanded.  I spelled it out now.


        Stefan

bug#16959: bug#16981: 24.3.50; electric-pair-delete-adjacent-pairs broken in c-mode, python-mode, maybe-others

[Florian Beck]

Looks very nice now. Thank you.

>>  - depth: clarify what happens when you have several pieces of ":override"
>> or ":around" advice that doesn't call the original function at the depth 0.
>
> I tried to clarify the notion of depth by spelling out what it means
> for :after, :before and :override.

The depth metaphor actually works pretty well.
The use case I had in mind was this: (1) Someone defines a :override
advice. (2) There already is an :override advice. (3) Neither advice
specifies depth, so it defaults to 0.

The result is that the advice SEEMS to work (the user put it in her
.emacs and presses C-x C-e), but on restarting emacs it depends on the
order advice defintions.

>
>> Advising named functions:
>>  - "Macros can also be advised": this doesn't work when the macro is used
>> in a compiled function.
>
> Of course it works.  The restriction is that the macro needs to be
> advised before it is expanded.  I spelled it out now.

Of course, from a user's point of view, macros usually are already expanded.

How would a user go about advising, say, byte-compile-log in her .emacs?

-- 
Florian Beck

bug#16959: bug#16981: 24.3.50; electric-pair-delete-adjacent-pairs broken in c-mode, python-mode, maybe-others

[Stefan Monnier]

> The result is that the advice SEEMS to work (the user put it in her
> .emacs and presses C-x C-e), but on restarting emacs it depends on the
> order advice defintions.

That's why the doc says:

   [..]
   should stay as the outermost advice.  When two advices specify the
   same depth, the most recently added advice will be outermost.
   [..]

> Of course, from a user's point of view, macros usually are already expanded.
> How would a user go about advising, say, byte-compile-log in her .emacs?

She can't do it and hope for it to affect already compiled code, indeed.

But we use advice-add in cl.el on the `declare', `dolist', and `dotimes'
macros and it works very well.  Obviously code that uses those needs to
have a (require 'cl) somewhere to make sure the advice is installed
before macro-expansion.


        Stefan

bug#16981: 24.3.50; electric-pair-delete-adjacent-pairs broken in c-mode, python-mode, maybe-others

[João Távora]

Hi Alan,

Sorry for the delay, I only check emacs-devel regularly, do cc me next
time please if you remember.

> There's something funny going on here.  A minor mode's keymap should
> normally take precedence over the major mode's keymap.  (See the page 
> "Searching Keymaps" in the Elisp manual.)  So why is electric-pair-mode's
> keymap being overridden by CC Mode's here?

You're right, but I can't figure out what. Probably has to do with
remapping and translation. But if it were the other way around, you'd
probably be reporting the bug :-) so it doesn't matter.

>> Perhaps a hook in `backward-delete-char' is in order. Or the other major
>> modes can find other ways to overload the backspace key.
> I would say it's legitimate for a major mode to bind the backspace key, but
> it's more questionable for a minor mode to do the same.  Perhaps the minor
> mode really ought to use defadvice rather than rebinding the key.

Yes, I think so too. Or both can use a (possibly excessively
complicated) system of backspace-related hooks.  Advice would probably
work too, but do we want that in emacs? Stefan, when you suggested the
rebinding originally, was it with any particular concern in mind?

Why not this, just the single `delete-backward-char-before-hook'. Seems
to work. Maybe also add a "don't use this in programs" to
`delete-backward-char'...

=== modified file 'lisp/elec-pair.el'
*** lisp/elec-pair.el	2014-04-04 23:31:02 +0000
--- lisp/elec-pair.el	2014-04-05 11:28:42 +0000
***************
*** 166,176 ****
  quotes or comments.  If lookup fails here, `electric-pair-text-pairs' will
  be considered.")
  
! (defun electric-pair-backward-delete-char (n &optional killflag untabify)
!   "Delete characters backward, and maybe also two adjacent paired delimiters.
! 
! Remaining behavior is given by `backward-delete-char' or, if UNTABIFY is
! non-nil, `backward-delete-char-untabify'."
    (interactive "*p\nP")
    (let* ((prev (char-before))
           (next (char-after))
--- 166,173 ----
  quotes or comments.  If lookup fails here, `electric-pair-text-pairs' will
  be considered.")
  
! (defun electric-pair-delete-pair-maybe (killflag)
!   "Forward-delete pair of soon-to-be-deleted char before point."
    (interactive "*p\nP")
    (let* ((prev (char-before))
           (next (char-after))
***************
*** 184,200 ****
                   electric-pair-delete-adjacent-pairs)
                 (memq syntax '(?\( ?\" ?\$))
                 (eq pair next))
!       (delete-char 1 killflag))
!     (if untabify
!         (backward-delete-char-untabify n killflag)
!         (backward-delete-char n killflag))))
! 
! (defun electric-pair-backward-delete-char-untabify (n &optional killflag)
!   "Delete characters backward, and maybe also two adjacent paired delimiters.
  
! Remaining behavior is given by `backward-delete-char-untabify'."
!   (interactive "*p\nP")
!   (electric-pair-backward-delete-char n killflag t))
  
  (defun electric-pair-conservative-inhibit (char)
    (or
--- 181,189 ----
                   electric-pair-delete-adjacent-pairs)
                 (memq syntax '(?\( ?\" ?\$))
                 (eq pair next))
!       (delete-char 1 killflag))))
  
! (add-hook 'delete-backward-char-before-hook 'electric-pair-delete-pair-maybe)
  
  (defun electric-pair-conservative-inhibit (char)
    (or
***************
*** 546,562 ****
         (memq (car (electric-pair-syntax-info last-command-event))
               '(?\( ?\) ?\" ?\$))))
  
- (defvar electric-pair-mode-map
-   (let ((map (make-sparse-keymap)))
-     (define-key map [remap backward-delete-char-untabify]
-       'electric-pair-backward-delete-char-untabify)
-     (define-key map [remap backward-delete-char]
-       'electric-pair-backward-delete-char)
-     (define-key map [remap delete-backward-char]
-       'electric-pair-backward-delete-char)
-     map)
-   "Keymap used by `electric-pair-mode'.")
- 
  ;;;###autoload
  (define-minor-mode electric-pair-mode
    "Toggle automatic parens pairing (Electric Pair mode).
--- 535,540 ----

=== modified file 'lisp/simple.el'
*** lisp/simple.el	2014-04-02 15:14:50 +0000
--- lisp/simple.el	2014-04-05 11:27:17 +0000
***************
*** 949,954 ****
--- 949,959 ----
  is undefined.  If DELETE is nil, just return the content as a string.
  If anything else, delete the region and return its content as a string.")
  
+ (defvar delete-backward-char-before-hook nil
+   "Hook run just before `delete-backward-char' actually deletes.
+ Each function in this list is passed the KILLFLAG arg to
+ `delete-backward-char' call.")
+ 
  (defun delete-backward-char (n &optional killflag)
    "Delete the previous N characters (following if N is negative).
  If Transient Mark mode is enabled, the mark is active, and N is 1,
***************
*** 984,990 ****
  	   (save-excursion
  	     (insert-char ?\s (- ocol (current-column)) nil))))
  	;; Otherwise, do simple deletion.
! 	(t (delete-char (- n) killflag))))
  
  (defun delete-forward-char (n &optional killflag)
    "Delete the following N characters (previous if N is negative).
--- 989,997 ----
  	   (save-excursion
  	     (insert-char ?\s (- ocol (current-column)) nil))))
  	;; Otherwise, do simple deletion.
! 	(t
!          (run-hook-with-args 'delete-backward-char-before-hook killflag)
!          (delete-char (- n) killflag))))
  
  (defun delete-forward-char (n &optional killflag)
    "Delete the following N characters (previous if N is negative).

bug#16981: 24.3.50; electric-pair-delete-adjacent-pairs broken in c-mode, python-mode, maybe-others

[Stefan Monnier]

>> There's something funny going on here.  A minor mode's keymap should
>> normally take precedence over the major mode's keymap.  (See the page 
>> "Searching Keymaps" in the Elisp manual.)  So why is electric-pair-mode's
>> keymap being overridden by CC Mode's here?
> You're right, but I can't figure out what.

That's because it's how remapping is works: first we lookup the key
(minor-maps, local-map, global-map), then we lookup the command to see
if it's remapped (minor-maps, local-map, global-map).
So the local-map and global-map's normal bindings take precedence over the
minor-map's remapping.

>>> Perhaps a hook in `backward-delete-char' is in order. Or the other major
>>> modes can find other ways to overload the backspace key.
>> I would say it's legitimate for a major mode to bind the backspace key, but
>> it's more questionable for a minor mode to do the same.  Perhaps the minor
>> mode really ought to use defadvice rather than rebinding the key.

How 'bout

  (define-key electric-pair-mode-map "\177"
    `(menu-item "" electric-pair-delete-pair
      :filter
      ,(lambda (cmd)
         (let* ((prev (char-before))
                (next (char-after))
                (syntax-info (and prev
                                  (electric-pair-syntax-info prev)))
                (syntax (car syntax-info))
                (pair (cadr syntax-info)))
           (and next pair
                (memq syntax '(?\( ?\" ?\$))
                (eq pair next)
                (if (functionp electric-pair-delete-adjacent-pairs)
                    (funcall electric-pair-delete-adjacent-pairs)
                  electric-pair-delete-adjacent-pairs)
                cmd))))))


-- Stefan

bug#16981: 24.3.50; electric-pair-delete-adjacent-pairs broken in c-mode, python-mode, maybe-others

[João Távora]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>>> There's something funny going on here.  A minor mode's keymap should
>> You're right, but I can't figure out what.
> That's because it's how remapping is works: first we lookup the key
> So the local-map and global-map's normal bindings take precedence over the
> minor-map's remapping.

Yeah that's it I remembered that this afternoon.

>   (define-key electric-pair-mode-map "\177"
>     `(menu-item "" electric-pair-delete-pair
>       :filter

>                   electric-pair-delete-adjacent-pairs)
>                 cmd))))))

Aha, the famed :filter trick I had heard about. Looks good enough while
we wait for that multiple-commands-per-keybinding refactoring. Not as
clean as remapping, but probably good enough, and it's arguable we want
this on any other key than backspace.

Hmmm now I remember that's it's even harder than defadvice to find the
extra code, docstring-wise I mean. How would a hacker discover we're
using the filter trick. describe-key doesn't help. .

Anyway should I install it? To emacs-24 only? I need a little help
VC-wise:

- I got two commits that are in trunk, r116926 and r116940, and should
be in (cherry-picked to?) emacs-24

- Three different fixes (bugs 16981,17192 and 17183) not yet pushed that
I think you want me to only push to emacs-24, since that will be merged
back to trunk later.

Is this correct? I also don't know much bzr to do this, especially the
first item. Is there a wiki as clear as BzrForEmacsDevs on this
particular workflow? I just did a "bzr switch emacs-24" from what I
believe to be a heavyweight checkout or something. It seems to have
worked.

bug#16981: 24.3.50; electric-pair-delete-adjacent-pairs broken in c-mode, python-mode, maybe-others

[Stefan Monnier]

> Hmmm now I remember that's it's even harder than defadvice to find the
> extra code, docstring-wise I mean. How would a hacker discover we're
> using the filter trick. describe-key doesn't help. .

Right.  C-h k will give you the binding, but only when
electric-pair-delete-pair would be run.  To make this trick more widely
usable, we'd need to change C-h k so that it shows all "possible bindings".

> Anyway should I install it?

If that's OK with you, yes.

> To emacs-24 only?

Yes.

> - I got two commits that are in trunk, r116926 and r116940, and should
> be in (cherry-picked to?) emacs-24

Just take the corresponding diffs (IIRC you can use
"bzr merge -r116925..r116926 .../trunk" for that) and commit them into
emacs-24.

Bzr doesn't really know about cherry picking, but if you put
"backported" into your commit message, our bzrmerge.el script
will know that this commit is already in the trunk and will know how to
avoid the corresponding conflicts when merging emacs-24 back into trunk.

> - Three different fixes (bugs 16981,17192 and 17183) not yet pushed that
> I think you want me to only push to emacs-24, since that will be merged
> back to trunk later.

Right.


        Stefan

bug#16981: 24.3.50; electric-pair-delete-adjacent-pairs broken in c-mode, python-mode, maybe-others

[João Távora]

On Sun, Apr 6, 2014 at 1:48 PM, Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>> Hmmm now I remember that's it's even harder than defadvice to find the
>> extra code, docstring-wise I mean. How would a hacker discover we're
>> using the filter trick. describe-key doesn't help. .
>
> Right.  C-h k will give you the binding, but only when
> electric-pair-delete-pair would be run.  To make this trick more widely
> usable, we'd need to change C-h k so that it shows all "possible bindings".

Yes, I realized that later on.

In the meantime, we can use the trick that yasnippet uses
(documentation-function if I'm not mistaken) to show what other command
electric-pair-delete-pair is shadowing in that situation.

>> Anyway should I install it?
> If that's OK with you, yes.

Yes, I think it's cleaner than all alternatives, unless add-function is
the way to go for these situations.

> Bzr doesn't really know about cherry picking, but if you put
> "backported" into your commit message, our bzrmerge.el script
> will know that this commit is already in the trunk and will know how to
> avoid the corresponding conflicts when merging emacs-24 back into trunk.

Alright, I'll include "backported from trunk, revs x and y".

bug#16981: 24.3.50; electric-pair-delete-adjacent-pairs broken in c-mode, python-mode, maybe-others

[João Távora]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> Just take the corresponding diffs (IIRC you can use
> "bzr merge -r116925..r116926 .../trunk" for that) and commit them into
> emacs-24.
>
> Bzr doesn't really know about cherry picking, but if you put
> "backported" into your commit message, our bzrmerge.el script
> will know that this commit is already in the trunk and will know how to
> avoid the corresponding conflicts when merging emacs-24 back into trunk.
>
>> - Three different fixes (bugs 16981,17192 and 17183) not yet pushed that
>> I think you want me to only push to emacs-24, since that will be merged
>> back to trunk later.
>
> Right.

These three I did successfully, but failed miserably in the
backporting. Oh git where art thou... Now I'm downloading a new repo
(with sharing properly setup hopefully), since I must have done
something that messed up my bzr conf, apparently I shouldn't have moved
my repo dir.

Should be done tomorrow morning, so I'll backport r116926 and r116940
then.