bug#50236: 27.2; electric-pair-mode is inconvenient in comint
Andrew Hyatt <[email protected]> Sun, 02 Aug 2026 21:34:56 -0400
| Newsgroups | gmane.emacs.bugs |
|---|---|
| Message-ID | <[email protected]> |
--=-=-= Content-Type: multipart/alternative; boundary="==-=-=" --==-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Eli Zaretskii <[email protected]> writes: > Ping! Any further comments or suggestions? In case there is not, I'm attaching a patch for the complete change, now including a mention of the behavior in the manual, and two new tests. > >> From: Andrew Hyatt <[email protected]> >> Cc: Ihor Radchenko <[email protected]>, Lars Ingebrigtsen >> <[email protected]>, [email protected], [email protected], >> [email protected] >> Date: Sun, 19 Jul 2026 17:28:18 -0400 >>=20 >> Augusto Stoffel <[email protected]> writes:=20 >>=20 >> Hi Andrew,=20 >>=20 >> in your proposed patch, why did you choose to change electric-pair-post= -self-insert-function directly and >> not electric-pair-default-skip-self (or even define a skip-self functio= n specifically for comint)?=20 >>=20 >> Isn't skip-self for avoiding two closing parens in a row? This doesn't s= eem related to the problem.=20 >>=20 >> Also, I should mention over another solution that Jo=C3=A3o had previous= ly implemented for Sly, discussed on the >> bug I merged into this one, this issue can be partially solved on `comin= t` or other mode side by marking the >> non-user generated text with a comment syntax, which electric-pair alrea= dy skips. That solves the issue that >> program output in comint can mess up balance, but not that two separate = inputs should have independent >> balance.=20 >>=20 >> Maybe that's okay, but then it would force other modes to solve similar= issues by using field properties.=20 >>=20 >> I think that's probably a good idea; using fields to represent different= provenance of input is a good general >> practice that electric-pair and perhaps other modes may come to rely on.= =20 >>=20 >> I've added Ihor to the discussion since the issue affects Org mode as w= ell (see my email of 22 Aug 2022 >> in this thread). WDYT?=20 >>=20 >> On Sat, 18 Jul 2026, Andrew Hyatt wrote:=20 >>=20 >> Augusto Stoffel <[email protected]> writes:=20 >>=20 >> Is the search bound (and attending local variable) really necessary? Te= xt property search uses an >> interval tree so it's better than linear time in the character counts.= =20 >>=20 >> I was able to construct a buffer that, without the bound, took tens of = milliseconds to get the previous >> field. Basically, lots of different faces, etc, which requires a lot of= iteration. I'm not sure what the >> normal expectations are, but I thought it best to err on the side of ma= king sure everything stays >> optimally fast.=20 >>=20 >> The 1000 char limit is more than enough for normal comint use, in my ex= perience.=20 >>=20 >> Okay, if we use this approach then I think the default should ensure at= least one screenful is considered, >> so maybe 10000.=20 --==-=-= Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: quoted-printable <p> Eli Zaretskii <[email protected]> writes: </p> <p> <blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1p= x #ccc solid;padding-left:1ex"> <div>Ping! Any further comments or suggestions? </div></blockquote> </p> <p> In case there is not, I'm attaching a patch for the complete change, now including a mention of the behavior in the manual, and two new tests. </p> <p> <blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1p= x #ccc solid;padding-left:1ex"> <div> <blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1p= x #ccc solid;padding-left:1ex"> <div>From: Andrew Hyatt <[email protected]> Cc: Ihor Radchenko <[email protected]>, Lars Ingebrigtsen <[email protected]>, [email protected], [email protected], [email protected] Date: Sun, 19 Jul 2026 17:28:18 -0400 </div> <div> <br /></div> <div>Augusto Stoffel <[email protected]> writes:=20 </div> <div> <br /></div> <div>Hi Andrew,=20 </div> <div> <br /></div> <div>in your proposed patch, why did you choose to change electric-pair-pos= t-self-insert-function directly and not electric-pair-default-skip-self (or even define a skip-self function sp= ecifically for comint)?=20 </div> <div> <br /></div> <div>Isn't skip-self for avoiding two closing parens in a row? This doesn't= seem related to the problem.=20 </div> <div> <br /></div> <div>Also, I should mention over another solution that Jo=C3=A3o had previo= usly implemented for Sly, discussed on the bug I merged into this one, this issue can be partially solved on `comint` = or other mode side by marking the non-user generated text with a comment syntax, which electric-pair already = skips. That solves the issue that program output in comint can mess up balance, but not that two separate inp= uts should have independent balance.=20 </div> <div> <br /></div> <div>Maybe that's okay, but then it would force other modes to solve simila= r issues by using field properties.=20 </div> <div> <br /></div> <div>I think that's probably a good idea; using fields to represent differe= nt provenance of input is a good general practice that electric-pair and perhaps other modes may come to rely on.=20 </div> <div> <br /></div> <div>I've added Ihor to the discussion since the issue affects Org mode as = well (see my email of 22 Aug 2022 in this thread). WDYT?=20 </div> <div> <br /></div> <div>On Sat, 18 Jul 2026, Andrew Hyatt wrote:=20 </div> <div> <br /></div> <div>Augusto Stoffel <[email protected]> writes:=20 </div> <div> <br /></div> <div>Is the search bound (and attending local variable) really necessary? T= ext property search uses an interval tree so it's better than linear time in the character counts.=20 </div> <div> <br /></div> <div>I was able to construct a buffer that, without the bound, took tens of= milliseconds to get the previous field. Basically, lots of different faces, etc, which requires a lot of ite= ration. I'm not sure what the normal expectations are, but I thought it best to err on the side of making= sure everything stays optimally fast.=20 </div> <div> <br /></div> <div>The 1000 char limit is more than enough for normal comint use, in my e= xperience.=20 </div> <div> <br /></div> <div>Okay, if we use this approach then I think the default should ensure a= t least one screenful is considered, so maybe 10000.=20 </div></blockquote> </div></blockquote> </p> --==-=-=-- --=-=-= Content-Type: text/x-patch Content-Disposition: attachment; filename=0001-electric-pair-fix-v2.patch Content-Description: Complete patch for electric-pair fix diff --git a/doc/emacs/programs.texi b/doc/emacs/programs.texi index 7cf7b2ff96f..9be597eea29 100644 --- a/doc/emacs/programs.texi +++ b/doc/emacs/programs.texi @@ -1129,7 +1129,9 @@ Matching buffer. And typing a closing delimiter immediately before another closing delimiter of the same type does not insert that character but moves point as described above, even when there is an unpaired matching -opening delimiter earlier in the buffer. +opening delimiter earlier in the buffer. Matching does not consider the +whole buffer, only the current field, so for example, previous input or +non-user input in comint buffers is not included. If there is an active region, this variable has no effect. diff --git a/lisp/elec-pair.el b/lisp/elec-pair.el index 54fbc960b0f..5f338f686f5 100644 --- a/lisp/elec-pair.el +++ b/lisp/elec-pair.el @@ -228,6 +228,16 @@ electric-pair-skip-whitespace-chars (const :tag "Newline" ?\n)) (repeat (character :value " ")))) +(defvar-local electric-pair-max-field-size 1000 + "Maximum number of characters to search for a field boundary. + +This is used to limit how far we search the buffer to find a field +boundary to restrict the search for matching pairs to. If there is no +field boundary within this many characters, then we will not attempt to +restrict the matching, otherwise we only match within the field. + +This is buffer local because different modes may want to tweak this.") + (defvar-local electric-pair-skip-whitespace-function #'electric-pair--skip-whitespace "Function to use to move point forward over whitespace. @@ -620,88 +630,100 @@ electric-pair-post-self-insert-function (both as defined by the major mode's syntax table). This is done by looking up the variables `electric-pair-inhibit-predicate', `electric-pair-skip-self' - and `electric-pair-skip-whitespace' (which see)." - (let* ((pos (and electric-pair-mode (electric--after-char-pos))) - (num (when pos (prefix-numeric-value current-prefix-arg))) - (beg (when num (- pos num))) - (skip-whitespace-info)) - (pcase (electric-pair-syntax-info last-command-event) - (`(,syntax ,pair ,unconditional ,space) - (cond - ((null pos) nil) - ((zerop num) nil) - ;; Wrap a pair around the active region. - ;; - ((and (memq syntax '(?\( ?\) ?\" ?\$)) (use-region-p)) - ;; FIXME: To do this right, we'd need a post-self-insert-function - ;; so we could add-function around it and insert the closer after - ;; all the rest of the hook has run. - (if (or (eq syntax ?\") - (and (eq syntax ?\)) - (>= (point) (mark))) - (and (not (eq syntax ?\))) - (>= (mark) (point)))) - (save-excursion - (goto-char (mark)) - (electric-pair--insert pair num)) - (delete-region beg pos) - (electric-pair--insert pair num) - (goto-char (mark)) - (electric-pair--insert last-command-event num))) - ;; Backslash-escaped: no pairing, no skipping. - ((save-excursion - (goto-char beg) - (not (evenp (skip-syntax-backward "\\")))) - (let ((current-prefix-arg (1- num))) - (electric-pair-post-self-insert-function))) - ;; Skip self. - ((and (memq syntax '(?\) ?\" ?\$)) - (and (or unconditional - (if (functionp electric-pair-skip-self) - (electric-pair--save-literal-point-excursion - (goto-char pos) - (funcall electric-pair-skip-self - last-command-event)) - electric-pair-skip-self)) - (save-excursion - (when (and - (not (and unconditional (eq syntax ?\"))) - (setq skip-whitespace-info - (if (and - (not - (eq electric-pair-skip-whitespace - 'chomp)) - (functionp electric-pair-skip-whitespace)) - (funcall electric-pair-skip-whitespace) - electric-pair-skip-whitespace))) - (funcall electric-pair-skip-whitespace-function)) - (eq (char-after) last-command-event)))) - ;; This is too late: rather than insert&delete we'd want to only - ;; skip (or insert in overwrite mode). The difference is in what - ;; goes in the undo-log and in the intermediate state which might - ;; be visible to other post-self-insert-hook. We'll just have to - ;; live with it for now. - (when skip-whitespace-info - (funcall electric-pair-skip-whitespace-function)) - (delete-region beg (if (eq skip-whitespace-info 'chomp) - (point) - pos)) - (forward-char num)) - ;; Insert matching pair. - ;; String pairs - ((and (eq syntax 'str) (not overwrite-mode)) - (if space (insert " ")) - (save-excursion - (insert pair))) - ;; Char pairs - ((and (memq syntax '(?\( ?\" ?\$)) - (not overwrite-mode) - (or unconditional - (not (electric-pair--save-literal-point-excursion - (goto-char pos) - (funcall electric-pair-inhibit-predicate - last-command-event))))) - (save-excursion (electric-pair--insert pair num)))))))) + and `electric-pair-skip-whitespace' (which see). + +If possible, this function will attempt to match delimiters for the +current field only. However, we limit how much we scan for the +beginning and end of the field to limit possible computation, so in +large fields this falls back to using the entire buffer for matching." + (with-restriction + (let* ((subtracted-point (- (point) electric-pair-max-field-size)) + (beginning (field-beginning nil nil (max (point-min) subtracted-point)))) + (if (or (use-region-p) (= beginning subtracted-point)) (point-min) beginning)) + (let* ((added-point (+ (point) electric-pair-max-field-size)) + (end (field-end nil nil (min (point-max) added-point)))) + (if (or (use-region-p) (= end added-point)) (point-max) end)) + (let* ((pos (and electric-pair-mode (electric--after-char-pos))) + (num (when pos (prefix-numeric-value current-prefix-arg))) + (beg (when num (- pos num))) + (skip-whitespace-info)) + (pcase (electric-pair-syntax-info last-command-event) + (`(,syntax ,pair ,unconditional ,space) + (cond + ((null pos) nil) + ((zerop num) nil) + ;; Wrap a pair around the active region. + ;; + ((and (memq syntax '(?\( ?\) ?\" ?\$)) (use-region-p)) + ;; FIXME: To do this right, we'd need a post-self-insert-function + ;; so we could add-function around it and insert the closer after + ;; all the rest of the hook has run. + (if (or (eq syntax ?\") + (and (eq syntax ?\)) + (>= (point) (mark))) + (and (not (eq syntax ?\))) + (>= (mark) (point)))) + (save-excursion + (goto-char (mark)) + (electric-pair--insert pair num)) + (delete-region beg pos) + (electric-pair--insert pair num) + (goto-char (mark)) + (electric-pair--insert last-command-event num))) + ;; Backslash-escaped: no pairing, no skipping. + ((save-excursion + (goto-char beg) + (not (evenp (skip-syntax-backward "\\")))) + (let ((current-prefix-arg (1- num))) + (electric-pair-post-self-insert-function))) + ;; Skip self. + ((and (memq syntax '(?\) ?\" ?\$)) + (and (or unconditional + (if (functionp electric-pair-skip-self) + (electric-pair--save-literal-point-excursion + (goto-char pos) + (funcall electric-pair-skip-self + last-command-event)) + electric-pair-skip-self)) + (save-excursion + (when (and + (not (and unconditional (eq syntax ?\"))) + (setq skip-whitespace-info + (if (and + (not + (eq electric-pair-skip-whitespace + 'chomp)) + (functionp electric-pair-skip-whitespace)) + (funcall electric-pair-skip-whitespace) + electric-pair-skip-whitespace))) + (funcall electric-pair-skip-whitespace-function)) + (eq (char-after) last-command-event)))) + ;; This is too late: rather than insert&delete we'd want to only + ;; skip (or insert in overwrite mode). The difference is in what + ;; goes in the undo-log and in the intermediate state which might + ;; be visible to other post-self-insert-hook. We'll just have to + ;; live with it for now. + (when skip-whitespace-info + (funcall electric-pair-skip-whitespace-function)) + (delete-region beg (if (eq skip-whitespace-info 'chomp) + (point) + pos)) + (forward-char num)) + ;; Insert matching pair. + ;; String pairs + ((and (eq syntax 'str) (not overwrite-mode)) + (if space (insert " ")) + (save-excursion + (insert pair))) + ;; Char pairs + ((and (memq syntax '(?\( ?\" ?\$)) + (not overwrite-mode) + (or unconditional + (not (electric-pair--save-literal-point-excursion + (goto-char pos) + (funcall electric-pair-inhibit-predicate + last-command-event))))) + (save-excursion (electric-pair--insert pair num))))))))) (defun electric-pair-open-newline-between-pairs-psif () "Honor `electric-pair-open-newline-between-pairs'. diff --git a/test/lisp/electric-tests.el b/test/lisp/electric-tests.el index 9209e1739fd..256fcd0e7dd 100644 --- a/test/lisp/electric-tests.el +++ b/test/lisp/electric-tests.el @@ -703,7 +703,30 @@ autowrapping-multi-2 (goto-char (point-max)) (skip-chars-backward "\"") (mark-sexp -1))) - + +(define-electric-pair-test preserve-balance + "\"\n\n" "---\"" + :expected-string "\"\n\n\"" + :expected-point 5 + :modes '(fundamental-mode) + :test-in-comments nil + :test-in-strings nil + :bindings '((electric-pair-preserve-balance . t)) + :fixture-fn (lambda () + (electric-pair-mode 1))) + +(define-electric-pair-test preserve-balance-not-across-fields + "\"\n\n" "---\"" + :expected-string "\"\n\n\"\"" + :expected-point 5 + :modes '(fundamental-mode) + :test-in-comments nil + :test-in-strings nil + :bindings '((electric-pair-preserve-balance . t)) + :fixture-fn (lambda () + (electric-pair-mode 1) + (add-text-properties (point-min) (+ (point-min) 2) + '(field t)))) ;;; Electric quotes (define-electric-pair-test electric-quote-string @@ -904,7 +927,6 @@ electric-quote-markdown-in-code nil :local)) :bindings '((comment-start . "<!--") (comment-use-syntax . t)) :test-in-comments nil :test-in-strings nil) - ;;; tests for `electric-layout-mode' --=-=-=--