Re: [patch] make electric-pair-mode smarter/more useful

[Stefan Monnier]

> - `electric-pair-pairs` now defaults to nil.

>   See its updated docstring for the reasoning and the new (akwardly
>   named) variables `electric-pair-non-code-pairs' and
>   `electric-pair-non-code-syntax-table' for slightly better, more
>   flexible alternatives to it, in my opinion.

I think this change is for the worst.  I often rely on ".." pairing in
things like text-mode where they don't have `string' syntax.

The explanation for removing (?\" . ?\") doesn't make much sense: you
seem to say that the list should be kept empty because balance can't be
provided, but until now balance wasn't provided either.

Of course, when the syntax-table gives string syntax to ", then this
entry should be ignored since the syntax table gives more info, which
allows balancing.

I suggest you use "text" rather than "non-code" in the variable names
(and indeed, the behavior in strings and comments should largely be the
same as in text-mode).

>   get autopair.el's backtick-and-quote pairing for elisp comments and
>   strings...

This is good, yes, thanks.

>   I read the other FIXME notes but didn't understand them so well. Can
>   you provide examples of the conflicts between `electric-indent-mode'
>   and other `electric-modes' that you mention there?

I can't remember off-hand, sorry.

> - I simplififed the previous `electric-pair--up-list' function and
>   renamed it `electric-pair--balance-info'. But I still couldn't make it
>   use `up-list' or get rid of some `forward-sexp'. `up-list' can
>   probably be done with enough care, but I think replacing
>   `forward-sexp' with `syntax-ppss' is only possible in the "pair", not
>   the "skip" case.

We can use syntax-ppss to get the START of all the parens, but indeed,
to find the overall balance, we need to look at least for the close-paren
matching the outermost open paren, which syntax-ppss doesn't give us.

>   And only when outside comments or strings.

The important case is the "pathological" case where we have to scan from
"almost point-min" to "almost point-max", and that should hopefully
never happen inside strings or comments.

>   In this implementation, I think the the number of `forward-sexp''s is
>   at most proportional to the nesting depth, which is less
>   dangerous.

Why do you need more than a fixed number of calls?  My naive
understanding is that we only need to know if we have more openers than
closers, or more closers than openers, or the same number of each; and
for that we basically need to do:

   (goto-char (car (last (nth 9 (syntax-ppss)))))
   (forward-sexp 1)  => failure means more openers than closers
   (up-list 1)       => failure means the parens are balanced.

Tho maybe even simpler is

   (save-excursion (car (syntax-ppss (point-max))))

this will also have the side effect of applying syntax-propertize over
the whole buffer, which could lead to performance problems, but would
also eliminate bugs when a sole-open paren is inside a special comment
only recognized by syntax-propertize.

This said, this dependence on correctly parsing the whole buffer makes
this feature brittle.  Both for the case where the major mode has a bug
(so it fails to parse things properly), or when it knowingly doesn't
handle all cases (same thing, tho it's not considered as a bug,
e.g. "#ifdef FOO \n { \n #else \n {x \n #fi"), or when some (remote)
part of the buffer is simply incorrect/incomplete.

So I think we need a "electric-pair-preserve-balance" flag to control
those features.

> - some helper functions might be reinventing the wheel, such as
>   `electric-pair--looking-at-mismatched-string-p' and

IIUC if the next string is "mismatched" that means it extends til EOB,
so you can test it with (nth 3 (syntax-ppss (point-max))).

>   `electric-pair--inside-comment-string'.

I think
   (nth 3 (parse-partial-sexp (nth 8 (syntax-ppss)) (point)))
will do.

But of course, this parses the content of the comment as if it were
code, which is naive.  You could use another syntax-table during the
call to parse-partial-sexp (but not the call to syntax-ppss), but it'd
still be naive.  IOW the problem is not in the implementation but in the
idea of detecting a string inside a comment.

> - I'm also trying my luck and changed lisp-mode.el defaults to
>   accomodate some of my preferred defaults in variables
>   `electric-pair-skip-whitespace' and `electric-pair-non-code-pairs').

See comments below.

> +(defun electric--sort-post-self-insertion-hook ()
> +  "Ensure order of electric functions in `post-self-insertion-hook'.
> +
> +Hooks in this variable interact in non-trivial ways, so a
> +relative order must be maintained within it."
> +  (let ((relative-order '(electric-pair-post-self-insert-function
> +                          electric-layout-post-self-insert-function
> +                          electric-indent-post-self-insert-function
> +                          blink-paren-post-self-insert-function)))
> +    (setq post-self-insert-hook
> +          (sort post-self-insert-hook
> +                #'(lambda (fn1 fn2)
> +                    (let ((fn1-tail (memq fn1 relative-order))
> +                          (fn2-tail (memq fn2 relative-order)))
> +                      (cond ((and fn1-tail fn2-tail)
> +                             (> (length fn1-tail)
> +                                (length fn2-tail)))
> +                            (fn1-tail t)
> +                            (fn2-tail nil)
> +                            (t nil))))))))

I think the idea is OK, but be careful: these functions are
supposed to be placed on the global part of the hook, so you'll want to
use `default-value' and `setq-default'.

BTW: I resisted the temptation to do such a `sort', but I can't give
a good reason why.  I just wish we had a better way to handle such
order-dependencies.  The part I don't like is the lack of modularity.

I generally tend to dislike "priorities", but maybe adding priorities
would make sense here: blink-paren-post-self-insert-function would get
a priority of 100 since it should "come last because it does a sit-for,
electric-indent-post-self-insert-function would get a priority of 90
because "it just does some post-processing".

The priorities can be added as symbol properties, so the "sort" function
doesn't need to know about the existing hooks.

> +(make-variable-buffer-local 'electric-pair-non-code-pairs)

I think we don't want/need that.  We can and do use setq-local when
needed, which should be sufficient.

> +(defcustom electric-pair-skip-whitespace t
> +  "If non-nil skip whitespace when skipping over closing parens.

Unclear.  IIUC from the code, what this doesn't just skip whitespace but
deletes it.  It should also say which whitespace is skipped/deleted
(before/after the skipped paren).

> +(defvar electric-pair-non-code-syntax-table prog-mode-syntax-table

Why prog-mode-syntax-table, rather than (say) text-mode-syntax-table?

BTW, syntax-ppss will get majorly confused if you call it while
a different syntax-table is temporarily installed.

> +  :keymap (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)

Don't use this :keymap arg; instead defvar electric-pair-mode-map.

> -  (setq-local prettify-symbols-alist lisp--prettify-symbols-alist))
> +  (setq-local prettify-symbols-alist lisp--prettify-symbols-alist)
> +  ; electric
> +  (when elisp
> +    (setq-local electric-pair-non-code-pairs '((?\` . ?\'))))

Good.

> +  (setq-local electric-pair-skip-whitespace 'chomp))

Hmm... lemme think... well... maybe we can't keep it tentatively.
I suspect it will bite me sometimes, but let's give it a chance.

> --- a/lisp/simple.el
> +++ b/lisp/simple.el
> @@ -607,7 +607,7 @@ In some text modes, where TAB inserts a tab, this command 
> indents to the
>  column specified by the function `current-left-margin'."
>    (interactive "*")
>    (delete-horizontal-space t)
> -  (newline)
> +  (newline 1 (not (or executing-kbd-macro noninteractive)))
>    (indent-according-to-mode))

Could you explain this change?

> +;;; electric-tests.el --- tests for electric.el -*- lexical-binding: t; -*-

Great!


        Stefan