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

[João Távora]

Stefan Monnier <address@hidden> writes:

> 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.

But if you "rely" on those in text-mode, isn't the correct thing to give
them that syntax there?

> 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.

I suggested in the docstring that before adding to this list, since it
has priority, the lower priority buffer's syntax table should be used
instead, as it also helps balancing.

> 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.

So you mean inverting the lookup order? Lookup electric-pair-pairs and
electric-pair-text-pairs only if no relevant syntax is found in
(syntax-table) and `electric-pair-text-syntax-table' respectively.

If so, it seems like a good idea. I got the opposite idea from the
original implementation of `electric-pair-syntax' and the mention of
"regardless of major mode" in `electric-pair-pairs''s docstring.

> 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).

I disagree, but won't insist. "strings" and "comments" exist when
programming, in my experience these typically are places where
programmatic syntax is useful, to write error messages, explain
structure, etc. But since `electric-pair-text-syntax-table' stays I'll
just set it globablly to prog-mode-syntax-table in my config. This way I
can get half-decent balancing for it.

>>   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 think I just found one when implementing the `electric-layout-rules'
suggestion in the nearby thread.

> 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.

OK, so I'm already using `syntax-ppss' where possible right?

>>   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.

I tried to make this into a function and failed again.

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

This works... but only for one kind of parenthesis. It incorrectly
reports 0 for a buffer with '[)', the mixed parenthesis case. I think
none of these alternatives will make the "mixed parens" test cases pass,
i.e. the ones where one kind is unbalanced but you still want to keep
balancing the other kind. The motivation to invest in electric.el came
after I fixed one outstanding such bug in autopair.el.

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

OK, but I would set it to t. Or maybe run `check-parens' in
prog-mode-hook and set it to nil if that errors out. Maybe issue a
warning to the user.

By the way, even with that evil #ifdef that breaks balancing of the `{}'
parens, we should strive to to still have `()' pair as if nothing
happened. (currently this is still shaky).

>> - 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))).

Looks much simpler, but weirdly fails some automated tests while
passing the same tests when run interactively. Will investigate.

>  IOW the problem is not in the implementation but in the idea of
> detecting a string inside a comment.

Yes. This was always slightly shaky in autopair.el, but good enough for
most uses. It's seldom a problem. Anyway I'll try your suggestion.

>> +(defun electric--sort-post-self-insertion-hook ()

> 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'.

OK, good idea.

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

Sounds a bit overkill for this particular case, unless we
make it more generic, like maybe adding this to add-hook's understanding
of its APPEND arg. But OK.

>> +(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.

OK.

>> +(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).

OK. Only the defcustom docs explain the whole story: it only deletes it
when set to 'chomp, the default is t.

>> +(defvar electric-pair-non-code-syntax-table prog-mode-syntax-table
> Why prog-mode-syntax-table, rather than (say) text-mode-syntax-table?

Explained above, but I don't object to text-mode-syntax-table.

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

Never bit me, but thanks for the heads-up. I'll probably end up calling
parse-partial-sexp with the correct table in comments or
strings.

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

OK.

>> +  (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.

I don't undestand: we "can" or we "can't" keep it? I think we should
give it a chance for lisp based modes. elecric-mode is always going to
be a surprise, might as well make it a good one. And when could it
possibly bite you?

>> -  (newline)
>> +  (newline 1 (not (or executing-kbd-macro noninteractive)))
>>    (indent-according-to-mode))
>
> Could you explain this change?

No, it doesn't belong here, sorry :-) Some earlier attempt at making the
related newline-between-pairs feature. But since you spotted it,
shoudn't newline-and-indent also call the post-self-insertion hooks? Or
should we leave that job to electric-indent-mode?

João