bug#33794: 26.1; electric-pair-mode breaks auto-newline minor mode of cc-mode

[João Távora]

Hi again Beatrix,

In the latest version of the patch I am preparing, if you do

    (add-hook 'c-mode-hook
              (lambda ()
                (setq-local electric-layout-rules
                            '((?\{ . before})
                              (?\{ . after)
                              (?\{ . after-stay)))))

You should arive at Allman C-style electric layouts.  So typing
int main () <spaces> then brace should give you:

int main ()
{
  <cursor here>
}

I attach the two patches for convenience, which you can also find in the
scratch/fix-33794-extend-electric-layout-mode branch.

João

>From ab036bdedbb49ecc96d550b5e883e43bb03eaccc Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20T=C3=A1vora?= <joaotavora@gmail.com>
Date: Fri, 21 Dec 2018 18:00:08 +0000
Subject: [PATCH 1/2] Extend electric-layout-mode to handle more complex
 layouts

Also, have it play nice with electric-pair-mode.

Multiple matching entries in `electric-layout-rules' are executed in
order of appearance.  When inserting a newline in the 'after-stay
rule, ensure electric-pair-open-newline-between-pairs is nil.

Arguably the logic behind electric-pair-open-newline-between-pairs
should be moved to electric-layout-mode, but the current rule-matching
engine doesn't allow for it.  The current solution seems to be good
enough for the situations reported in bug#33794.

* lisp/electric.el (electric-layout-rules): Adjust docstring.
(electric-layout-post-self-insert-function): Loop through rules.  Bind
electric-pair-open-newline-between-pairs to nil when handling
after-stay.
---
 lisp/electric.el | 61 ++++++++++++++++++++++++++----------------------
 1 file changed, 33 insertions(+), 28 deletions(-)

diff --git a/lisp/electric.el b/lisp/electric.el
index 6dbf46b80c..6a307a49b9 100644
--- a/lisp/electric.el
+++ b/lisp/electric.el
@@ -370,38 +370,43 @@ electric-layout-rules
 
 The symbols specify where in relation to CHAR the newline
 character(s) should be inserted. `after-stay' means insert a
-newline after CHAR but stay in the same place.")
+newline after CHAR but stay in the same place.
+
+If multiple rules match, they are all executed in order of
+appearance.")
 
 (defun electric-layout-post-self-insert-function ()
-  (let* ((rule (cdr (assq last-command-event electric-layout-rules)))
-         pos)
-    (when (and rule
-               (setq pos (electric--after-char-pos))
+  (let (pos)
+    (when (and (setq pos (electric--after-char-pos))
                ;; Not in a string or comment.
                (not (nth 8 (save-excursion (syntax-ppss pos)))))
-      (let ((end (point-marker))
-            (sym (if (functionp rule) (funcall rule) rule)))
-        (set-marker-insertion-type end (not (eq sym 'after-stay)))
-        (goto-char pos)
-        (pcase sym
-          ;; FIXME: we used `newline' down here which called
-          ;; self-insert-command and ran post-self-insert-hook recursively.
-          ;; It happened to make electric-indent-mode work automatically with
-          ;; electric-layout-mode (at the cost of re-indenting lines
-          ;; multiple times), but I'm not sure it's what we want.
-          ;;
-          ;; FIXME: check eolp before inserting \n?
-          ('before (goto-char (1- pos)) (skip-chars-backward " \t")
-                   (unless (bolp) (insert "\n")))
-          ('after  (insert "\n"))
-          ('after-stay (save-excursion
-                         (let ((electric-layout-rules nil))
-                           (newline 1 t))))
-          ('around (save-excursion
-                     (goto-char (1- pos)) (skip-chars-backward " \t")
-                     (unless (bolp) (insert "\n")))
-                   (insert "\n")))      ; FIXME: check eolp before inserting 
\n?
-        (goto-char end)))))
+      (goto-char pos)
+      (dolist (rule electric-layout-rules)
+        (when (eq last-command-event (car rule))
+          (let* ((end (point-marker))
+                 (rule (cdr rule))
+                 (sym (if (functionp rule) (funcall rule) rule)))
+            (set-marker-insertion-type end (not (eq sym 'after-stay)))
+            (pcase sym
+              ;; FIXME: we used `newline' down here which called
+              ;; self-insert-command and ran post-self-insert-hook recursively.
+              ;; It happened to make electric-indent-mode work automatically 
with
+              ;; electric-layout-mode (at the cost of re-indenting lines
+              ;; multiple times), but I'm not sure it's what we want.
+              ;;
+              ;; FIXME: check eolp before inserting \n?
+              ('before (goto-char (1- pos)) (skip-chars-backward " \t")
+                       (unless (bolp) (insert "\n")))
+              ('after  (insert "\n"))
+              ('after-stay (save-excursion
+                             (let ((electric-layout-rules nil)
+                                   (electric-pair-open-newline-between-pairs 
nil))
+                               (newline 1 t))))
+              ('around (save-excursion
+                         (goto-char (1- pos)) (skip-chars-backward " \t")
+                         (unless (bolp) (insert "\n")))
+                       (insert "\n")))      ; FIXME: check eolp before 
inserting \n?
+            (goto-char end)))))))
 
 (put 'electric-layout-post-self-insert-function 'priority  40)
 
-- 
2.20.0

>From dfc19a04f5f455051d158648a963cb1fd8b91c67 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20T=C3=A1vora?= <joaotavora@gmail.com>
Date: Sat, 22 Dec 2018 01:52:08 +0000
Subject: [PATCH 2/2] Rework electric-layout-post-self-insert-function
 (bug#33794)

This should now fix more problems reported in (bug#33794) regarding
insertion of newlines before and after the opening brace.  Write two
automated tests.

Also provide a new electric-layout-local-mode for testing.

* lisp/electric.el (electric-layout-post-self-insert-function-1):
New function that does the work for
electric-layout-post-self-insert-function-1.
(electric-layout-local-mode): New minor mode.

* test/lisp/electric-tests.el (electric-layout-int-main-kernel-style)
(electric-layout-int-main-allman-style): Add two tests.
---
 lisp/electric.el            | 49 +++++++++++++++++++++++++------------
 test/lisp/electric-tests.el | 34 +++++++++++++++++++++++++
 2 files changed, 68 insertions(+), 15 deletions(-)

diff --git a/lisp/electric.el b/lisp/electric.el
index 6a307a49b9..24c040d05b 100644
--- a/lisp/electric.el
+++ b/lisp/electric.el
@@ -376,17 +376,25 @@ electric-layout-rules
 appearance.")
 
 (defun electric-layout-post-self-insert-function ()
-  (let (pos)
+  (when electric-layout-mode
+    (electric-layout-post-self-insert-function-1)))
+
+;; for edebug's sake a separate function
+(defun electric-layout-post-self-insert-function-1 ()
+  (let (pos end)
     (when (and (setq pos (electric--after-char-pos))
                ;; Not in a string or comment.
                (not (nth 8 (save-excursion (syntax-ppss pos)))))
       (goto-char pos)
+      (setq end (point-marker))
       (dolist (rule electric-layout-rules)
         (when (eq last-command-event (car rule))
-          (let* ((end (point-marker))
-                 (rule (cdr rule))
-                 (sym (if (functionp rule) (funcall rule) rule)))
-            (set-marker-insertion-type end (not (eq sym 'after-stay)))
+          (let* ((rule (cdr rule))
+                 (sym (if (functionp rule) (funcall rule) rule))
+                 (nl (lambda ()
+                       (let ((electric-layout-mode nil)
+                             (electric-pair-open-newline-between-pairs nil))
+                               (newline 1 t)))))
             (pcase sym
               ;; FIXME: we used `newline' down here which called
               ;; self-insert-command and ran post-self-insert-hook recursively.
@@ -395,18 +403,16 @@ electric-layout-post-self-insert-function
               ;; multiple times), but I'm not sure it's what we want.
               ;;
               ;; FIXME: check eolp before inserting \n?
-              ('before (goto-char (1- pos)) (skip-chars-backward " \t")
-                       (unless (bolp) (insert "\n")))
-              ('after  (insert "\n"))
-              ('after-stay (save-excursion
-                             (let ((electric-layout-rules nil)
-                                   (electric-pair-open-newline-between-pairs 
nil))
-                               (newline 1 t))))
+              ('before (save-excursion
+                         (goto-char (1- pos)) (skip-chars-backward " \t")
+                         (unless (bolp) (funcall nl))))
+              ('after  (funcall nl))
+              ('after-stay (save-excursion (funcall nl)))
               ('around (save-excursion
                          (goto-char (1- pos)) (skip-chars-backward " \t")
-                         (unless (bolp) (insert "\n")))
-                       (insert "\n")))      ; FIXME: check eolp before 
inserting \n?
-            (goto-char end)))))))
+                         (unless (bolp) (funcall nl)))
+                       (funcall nl)))      ; FIXME: check eolp before 
inserting \n?
+            ))))))
 
 (put 'electric-layout-post-self-insert-function 'priority  40)
 
@@ -424,6 +430,19 @@ electric-layout-mode
          (remove-hook 'post-self-insert-hook
                       #'electric-layout-post-self-insert-function))))
 
+;;;###autoload
+(define-minor-mode electric-layout-local-mode
+  "Toggle `electric-layout-mode' only in this buffer."
+  :variable (buffer-local-value 'electric-layout-mode (current-buffer))
+  (cond
+   ((eq electric-layout-mode (default-value 'electric-layout-mode))
+    (kill-local-variable 'electric-layout-mode))
+   ((not (default-value 'electric-layout-mode))
+    ;; Locally enabled, but globally disabled.
+    (electric-layout-mode 1)             ; Setup the hooks.
+    (setq-default electric-layout-mode nil) ; But keep it globally disabled.
+    )))
+
 ;;; Electric quoting.
 
 (defcustom electric-quote-comment t
diff --git a/test/lisp/electric-tests.el b/test/lisp/electric-tests.el
index a665d2eb28..971c3e9567 100644
--- a/test/lisp/electric-tests.el
+++ b/test/lisp/electric-tests.el
@@ -812,5 +812,39 @@ electric-quote-markdown-in-code
   :bindings '((comment-start . "<!--") (comment-use-syntax . t))
   :test-in-comments nil :test-in-strings nil)
 
+
+;;; tests for `electric-layout-mode'
+
+(ert-deftest electric-layout-int-main-kernel-style ()
+  (save-electric-modes
+    (ert-with-test-buffer ()
+      (c-mode)
+      (electric-layout-local-mode 1)
+      (electric-pair-local-mode 1)
+      (electric-indent-local-mode 1)
+      (setq-local electric-layout-rules
+              '((?\{ . after)
+                (?\{ . after-stay)))
+      (insert "int main () ")
+      (let ((last-command-event ?\{))
+        (call-interactively (key-binding `[,last-command-event])))
+      (should (equal (buffer-string) "int main () {\n  \n}")))))
+
+(ert-deftest electric-layout-int-main-allman-style ()
+  (save-electric-modes
+    (ert-with-test-buffer ()
+      (c-mode)
+      (electric-layout-local-mode 1)
+      (electric-pair-local-mode 1)
+      (electric-indent-local-mode 1)
+      (setq-local electric-layout-rules
+              '((?\{ . before)
+                (?\{ . after)
+                (?\{ . after-stay)))
+      (insert "int main () ")
+      (let ((last-command-event ?\{))
+        (call-interactively (key-binding `[,last-command-event])))
+      (should (equal (buffer-string) "int main ()\n{\n  \n}")))))
+
 (provide 'electric-tests)
 ;;; electric-tests.el ends here
-- 
2.20.0





João Távora <joaotavora@gmail.com> writes:

> Beatrix Klebe <beeuhtricks@gmail.com> writes:
>
>> I know how to do hotpaches but that doesn't appear to solve the
>> problem I'm having here, unless I've missed something. The problem is
>> with moving the opening bracket, not the insertion point.
>
> Beatrix,
>
> I think you may have missed the fact that I am suggesting alternatives
> that:
>
> * involve cc-mode, or one of its derived modes;
>
> * don't involve M-x c-toggle-auto-newline (turning on what you call
>   auto-newline-mode);
>
> * involve turning on the global electric-layout-mode and a thin
>   customization for it in the buffers where you think it's relevant
>   (presumably cc-mode);
>
> * may involve multiple fixed/patched versions of lisp/electric.el as I
>   understand your problem(s);
>
> As it stands, the last patch I sent you passes my only test which is
> this:  given a file 33794.el which is just:
>
>    (electric-pair-mode)
>    (electric-layout-mode)
>     
>    (add-hook 'c-mode-hook
>              (lambda ()
>                (setq-local electric-layout-rules
>                            '((?\{ . after)
>                              (?\{ . after-stay)))))
>
>
> then running this from a shell:
>
>    $ emacs -Q -l 33794.el something.c
>
> Opens a new c-mode buffer.  Type 'int main ()' and then an opening
> brace.  You should get:
>
>    int main () {
>        <cursor>
>    }
>
> Can you reproduce these results?  If you can come up with more of these
> tests written in this or a similarly simple and exact manner it's easier
> for me to understand what's going on (it's also easier to write
> automated tests).
>
> João
>
>>
>>
>> On Fri, Dec 21, 2018 at 2:20 PM João Távora <joaotavora@gmail.com> wrote:
>>>
>>> Hi Beatrix,
>>>
>>> The solution I propose involves introducing the hotpatch I attached
>>> to fix electric-layout-mode in your emacs, so I wouldn't expect it
>>> to work if you haven't done that.
>>>
>>> Do you know how to do it?
>>>
>>> Though Alan will probably suggest otherwise, I'd also steer away
>>> from c-specific functionality and keep to the triad
>>> electric-indent-mode, electric-pair-mode and electric-indent-mode,
>>> at least while we try to extend/fix these modes to accommodate your
>>> needs.
>>>
>>> After such a solution is evaluated, you can select to keep it or move to 
>>> something else.
>>>
>>> João
>>>
>>> On Fri, Dec 21, 2018, 19:06 Beatrix Klebe <beeuhtricks@gmail.com wrote:
>>>>
>>>> Here's the link, I believe it was Stefan that answered it:
>>>> https://emacs.stackexchange.com/questions/2837/automatically-formatting-brackets/2853#2853
>>>>
>>>> I have tried this with emacs -Q and it does not fix the issue, which
>>>> is as follows.
>>>>
>>>> Ordinarily in cc-mode when you have auto-newline-mode activated, and
>>>> as far as I can tell, a cc-mode configuration that supports it, (which
>>>> csharp-mode contains), the following happens when opening a block
>>>> (pipe is the cursor):
>>>>
>>>> void Main() {| // opening bracket is typed
>>>>
>>>> becomes
>>>>
>>>> void Main
>>>> {
>>>>     |
>>>>
>>>> when c-toggle-auto-newline is activated. However, if you also want
>>>> your braces automatically paired, with electric-pair-mode, instead the
>>>> following occurs:
>>>>
>>>> void Main() {| // opening bracket is typed
>>>>
>>>> void Main() {|} // electric-pair-mode closes the open bracket, but
>>>> auto-newline-mode does not appear to do anything.
>>>>
>>>> void Main() {
>>>>     |
>>>> } // user hits return, inserting the cursor at the correct indent
>>>> level, but leaving the opening brace where it is.
>>>>
>>>> The ideal/desired behavior is:
>>>>
>>>> void Main() {| // opening bracket is typed
>>>>
>>>> void Main()
>>>> {
>>>>     |
>>>> } // user hits return key, electric-pair-mode pairs up the brackets,
>>>> and auto-newline-mode formats the braces correctly
>>>>
>>>> It would also probably suffice to format with the newline before
>>>> hitting enter as well, although I think I prefer hitting enter to open
>>>> the block. I'm quite curious as to the internals of these formatting
>>>> systems and would be happy to help with a fix/feature if that would be
>>>> desired, I am mostly an OCaml programmer but C# is my day job and I've
>>>> just recently gotten deeper into Emacs Lisp.
>>>>
>>>> On Fri, Dec 21, 2018 at 1:49 PM João Távora <joaotavora@gmail.com> wrote:
>>>> >
>>>> > Beatrix Klebe <beeuhtricks@gmail.com> writes:
>>>> >
>>>> > > I believe I saw your Stack Overflow answer about this while searching
>>>> > > for the solution. electric-layout-mode works with some quirks, such as
>>>> > > that if you put a space after parens in a function definition, the
>>>> > > space gets carried on to the newline with that method, which is a bit
>>>> > > annoying. What would be ideal, and what I'm looking for, is to get
>>>> > > auto-pairing of brackets with braces being placed where they should be
>>>> > > automatically and the insertion point getting put in between them at
>>>> > > the correct indent level, such as what happens with Visual Studio, or
>>>> > > Visual Studio Code, or several other editors with this functionality.
>>>> > > Perhaps it is not emacslike to have such behavior be totally
>>>> > > automated, but I am used to it and finds it decreases my ordinary
>>>> > > levels of frustration when working with verbose and imperative
>>>> > > languages. I am currently trying to write some insert specifiers for
>>>> > > smartparens to do this, but it is proving more difficult to find an
>>>> > > elegant solution than I had expected.
>>>> >
>>>> > It is quite emacslike (though maybe not activated by default): you just
>>>> > have to report the bugs to the Emacs developers as efficiently as
>>>> > possible.
>>>> >
>>>> > 1. Though Alan possibly has already, I still cannot understand the
>>>> >    original problem.  Can you start by describing what the buffer looked
>>>> >    like before, what you did, what it looked like afterwards, and what
>>>> >    you expected it to look like?  If possible start with a clean Emacs
>>>> >    -Q recpe.
>>>> >
>>>> > 2. I have experimented with nicer-playing like alternatives like
>>>> >    electric-layout-mode.  I came across a few quirks myself (though I'm
>>>> >    not sure if they are the same as yours). So I prepared a patch (in
>>>> >    branch scratch/fix-33794-extend-electric-layout-mode) and attached
>>>> >    it after the sig.
>>>> >
>>>> > After loading this patch, in a simple Emacs -Q the configuration:
>>>> >
>>>> >    (electric-pair-mode)
>>>> >    (electric-layout-mode)
>>>> >
>>>> >    (add-hook 'c-mode-hook
>>>> >           (lambda ()
>>>> >             (setq-local electric-layout-rules
>>>> >                         '((?\{ . after)
>>>> >                           (?\{ . after-stay)))))
>>>> >
>>>> > And, when visiting a C file, if I press `{' I get the expected
>>>> > pair+layout+indent behaviour.  Sor example opening a brace after
>>>> > int main () gives me:
>>>> >
>>>> >     int main () {
>>>> >         <cursor here>
>>>> >     }
>>>> >
>>>> > I, like Stefan, think cc-mode could/should set electric-layout-rules
>>>> > buffer-locally to reflect whatever c-style the user has selected.
>>>> >
>>>> > Thanks,
>>>> > João
>>>> >
>>>> > PS: Also, can you link to the the relevant to the stack overflow answer 
>>>> > you
>>>> > mentioned?
>>>> >
>>>> > commit ab036bdedbb49ecc96d550b5e883e43bb03eaccc
>>>> > Author: João Távora <joaotavora@gmail.com>
>>>> > Date:   Fri Dec 21 18:00:08 2018 +0000
>>>> >
>>>> >     Extend electric-layout-mode to handle more complex layouts
>>>> >
>>>> >     Also, have it play nice with electric-pair-mode.
>>>> >
>>>> >     Multiple matching entries in `electric-layout-rules' are executed in
>>>> >     order of appearance.  When inserting a newline in the 'after-stay
>>>> >     rule, ensure electric-pair-open-newline-between-pairs is nil.
>>>> >
>>>> >     Arguably the logic behind electric-pair-open-newline-between-pairs
>>>> >     should be moved to electric-layout-mode, but the current 
>>>> > rule-matching
>>>> >     engine doesn't allow for it.  The current solution seems to be good
>>>> >     enough for the situations reported in bug#33794.
>>>> >
>>>> >     * lisp/electric.el (electric-layout-rules): Adjust docstring.
>>>> >     (electric-layout-post-self-insert-function): Loop through rules.  
>>>> > Bind
>>>> >     electric-pair-open-newline-between-pairs to nil when handling
>>>> >     after-stay.
>>>> >
>>>> > diff --git a/lisp/electric.el b/lisp/electric.el
>>>> > index 6dbf46b80c..6a307a49b9 100644
>>>> > --- a/lisp/electric.el
>>>> > +++ b/lisp/electric.el
>>>> > @@ -370,38 +370,43 @@ electric-layout-rules
>>>> >
>>>> >  The symbols specify where in relation to CHAR the newline
>>>> >  character(s) should be inserted. `after-stay' means insert a
>>>> > -newline after CHAR but stay in the same place.")
>>>> > +newline after CHAR but stay in the same place.
>>>> > +
>>>> > +If multiple rules match, they are all executed in order of
>>>> > +appearance.")
>>>> >
>>>> >  (defun electric-layout-post-self-insert-function ()
>>>> > -  (let* ((rule (cdr (assq last-command-event electric-layout-rules)))
>>>> > -         pos)
>>>> > -    (when (and rule
>>>> > -               (setq pos (electric--after-char-pos))
>>>> > +  (let (pos)
>>>> > +    (when (and (setq pos (electric--after-char-pos))
>>>> >                 ;; Not in a string or comment.
>>>> >                 (not (nth 8 (save-excursion (syntax-ppss pos)))))
>>>> > -      (let ((end (point-marker))
>>>> > -            (sym (if (functionp rule) (funcall rule) rule)))
>>>> > -        (set-marker-insertion-type end (not (eq sym 'after-stay)))
>>>> > -        (goto-char pos)
>>>> > -        (pcase sym
>>>> > -          ;; FIXME: we used `newline' down here which called
>>>> > -          ;; self-insert-command and ran post-self-insert-hook 
>>>> > recursively.
>>>> > -          ;; It happened to make electric-indent-mode work 
>>>> > automatically with
>>>> > -          ;; electric-layout-mode (at the cost of re-indenting lines
>>>> > -          ;; multiple times), but I'm not sure it's what we want.
>>>> > -          ;;
>>>> > -          ;; FIXME: check eolp before inserting \n?
>>>> > -          ('before (goto-char (1- pos)) (skip-chars-backward " \t")
>>>> > -                   (unless (bolp) (insert "\n")))
>>>> > -          ('after  (insert "\n"))
>>>> > -          ('after-stay (save-excursion
>>>> > -                         (let ((electric-layout-rules nil))
>>>> > -                           (newline 1 t))))
>>>> > -          ('around (save-excursion
>>>> > -                     (goto-char (1- pos)) (skip-chars-backward " \t")
>>>> > -                     (unless (bolp) (insert "\n")))
>>>> > -                   (insert "\n")))      ; FIXME: check eolp before 
>>>> > inserting \n?
>>>> > -        (goto-char end)))))
>>>> > +      (goto-char pos)
>>>> > +      (dolist (rule electric-layout-rules)
>>>> > +        (when (eq last-command-event (car rule))
>>>> > +          (let* ((end (point-marker))
>>>> > +                 (rule (cdr rule))
>>>> > +                 (sym (if (functionp rule) (funcall rule) rule)))
>>>> > +            (set-marker-insertion-type end (not (eq sym 'after-stay)))
>>>> > +            (pcase sym
>>>> > +              ;; FIXME: we used `newline' down here which called
>>>> > +              ;; self-insert-command and ran post-self-insert-hook 
>>>> > recursively.
>>>> > +              ;; It happened to make electric-indent-mode work 
>>>> > automatically with
>>>> > +              ;; electric-layout-mode (at the cost of re-indenting lines
>>>> > +              ;; multiple times), but I'm not sure it's what we want.
>>>> > +              ;;
>>>> > +              ;; FIXME: check eolp before inserting \n?
>>>> > +              ('before (goto-char (1- pos)) (skip-chars-backward " \t")
>>>> > +                       (unless (bolp) (insert "\n")))
>>>> > +              ('after  (insert "\n"))
>>>> > +              ('after-stay (save-excursion
>>>> > +                             (let ((electric-layout-rules nil)
>>>> > +                                   
>>>> > (electric-pair-open-newline-between-pairs nil))
>>>> > +                               (newline 1 t))))
>>>> > +              ('around (save-excursion
>>>> > +                         (goto-char (1- pos)) (skip-chars-backward " 
>>>> > \t")
>>>> > +                         (unless (bolp) (insert "\n")))
>>>> > +                       (insert "\n")))      ; FIXME: check eolp before 
>>>> > inserting \n?
>>>> > +            (goto-char end)))))))
>>>> >
>>>> >  (put 'electric-layout-post-self-insert-function 'priority  40)
>>>> >