Subject: Re: bug#76313: New function `replace-region`

From: Stefan Monnier <[email-masked]>
Date: Fri, 28 Mar 2025 11:38:19 -0400

> (the difference is just a mere `-contents` plus an extra `0` argument,
> but as argued here before, this sets a trap for the unwary user who
> may unwittingly end up calling a very expensive function).

The main part is not the `-contents` but the extra arg needed to avoid
the risk of spending half-an-hour waiting for the command to finish
looking for diffs.
Any chance we could consider a  breaking change and force a non-nil value of
one of MAX-SECS or MAX-COSTS before the function is allowed to use the
costly algorithm?

After all, it's a "soft breakage" since the algorithm doesn't make
any promises about what is recognized as "unchanged".

I took a look at the uses I could find "out there" (see below my sig)
and the overwhelming impression is that the change would have a very
minor impact, and of course would be easy to fix in
a backward-compatible manner.  [ Note that I do not suggest making
a similar change to `replace-buffer-contents`.  ]

        Stefan

There are currently no uses in Emacs itself.
I found one use in GNU ELPA:

    packages/sketch-mode/sketch-mode.el-      (with-current-buffer (print (marker-buffer s))
    packages/sketch-mode/sketch-mode.el-        (let ((inhibit-read-only t))
    packages/sketch-mode/sketch-mode.el:          (replace-region-contents s e (lambda () (concat "OPACITY: "
    packages/sketch-mode/sketch-mode.el-                                                     (format (if sketch-opacity
    packages/sketch-mode/sketch-mode.el-                                                                 "%.2f"
    packages/sketch-mode/sketch-mode.el-                                                               "%s"
    packages/sketch-mode/sketch-mode.el-                                                               )
    packages/sketch-mode/sketch-mode.el-                                                             sketch-opacity)
    packages/sketch-mode/sketch-mode.el-                                                     "\n ")))
    packages/sketch-mode/sketch-mode.el-      (put-text-property (1- e) e 'display (svg-image 
    packages/sketch-mode/sketch-mode.el-                                                (let* ((w 220)
    packages/sketch-mode/sketch-mode.el-                                                       (h (default-font-height))
    packages/sketch-mode/sketch-mode.el-                                                       (half-h (/ h 2))

Having tested this code, I know that careful preservation of markers and
such is of no importance so the proposed change would bring only a (minor)
improvement in performance.

I found one/two use in NonGNU ELPA:

    packages/parinfer-rust-mode/test/test-helper.el-    (goto-line (+ 1 (cdr (assoc 'lineNo change))))) ;; This is normally bad, but we're just doing this in a test
    packages/parinfer-rust-mode/test/test-helper.el-  (forward-char (cdr (assoc 'x change)))            ;; and we need to go to the line specified by the current change
    packages/parinfer-rust-mode/test/test-helper.el:  (replace-region-contents (point)
    packages/parinfer-rust-mode/test/test-helper.el-                           (+ (point) (length (cdr (assoc 'newText change))))
    packages/parinfer-rust-mode/test/test-helper.el-                           (lambda (&rest _args) (cdr (assoc 'oldText change)))))
    packages/parinfer-rust-mode/test/test-helper.el-
    packages/parinfer-rust-mode/test/test-helper.el-(defun apply-changes-in-buffer (change)
    packages/parinfer-rust-mode/test/test-helper.el-  "Given a list of CHANGE apply to the specified area of the buffer."
    packages/parinfer-rust-mode/test/test-helper.el-  (with-no-warnings
    packages/parinfer-rust-mode/test/test-helper.el-    (goto-line (+ 1 (cdr (assoc 'lineNo change)))))
    packages/parinfer-rust-mode/test/test-helper.el-  (forward-char (cdr (assoc 'x change)))
    packages/parinfer-rust-mode/test/test-helper.el-  (setq-local parinfer-rust--test-line-no (line-number-at-pos))
    packages/parinfer-rust-mode/test/test-helper.el:  (replace-region-contents (point)
    packages/parinfer-rust-mode/test/test-helper.el-                           (+ (point) (length (cdr (assoc 'oldText change))))
    packages/parinfer-rust-mode/test/test-helper.el-                           (lambda (&rest _args) (cdr (assoc 'newText change))))
    packages/parinfer-rust-mode/test/test-helper.el-  (setq-local parinfer-rust--test-line-no nil))

According to my tests, this would not be adversely affected either.
I found six/seven uses in MELPA:

    chroma-20240716.1131.tar-                 (match-end (match-end 0))
    chroma-20240716.1131.tar-                 (color-string (match-string-no-properties 0)))
    chroma-20240716.1131.tar:             (replace-region-contents (match-beginning 0) (match-end 0)
    chroma-20240716.1131.tar-                                      (lambda ()
    chroma-20240716.1131.tar-                                        (funcall color-conversion-fn
    chroma-20240716.1131.tar-                                                 color-string)))))))
    chroma-20240716.1131.tar-      (t
    chroma-20240716.1131.tar-       (cl-multiple-value-bind (color-string start end)
    chroma-20240716.1131.tar-           (funcall color-string-at-point-fn (point))
    chroma-20240716.1131.tar:         (replace-region-contents start end
    chroma-20240716.1131.tar-                                  (lambda ()
    chroma-20240716.1131.tar-                                    (funcall color-conversion-fn
    chroma-20240716.1131.tar-                                             color-string)))))))))

This is in a function that replaces one color name with another and
would not be significantly affected, AFAICT.

    ekg-20250313.436.tar-(defun ekg-edit-display-metadata ()
    ekg-20250313.436.tar-  "Create or edit the overlay to show metadata."
    ekg-20250313.436.tar-  (let ((o (ekg--metadata-overlay))
    ekg-20250313.436.tar-        (inhibit-read-only t))
    ekg-20250313.436.tar-    (buffer-disable-undo)
    ekg-20250313.436.tar:    (replace-region-contents (overlay-start o) (overlay-end o)
    ekg-20250313.436.tar-                             #'ekg--replace-metadata)
    ekg-20250313.436.tar-    (goto-char (overlay-end o))
    ekg-20250313.436.tar-    (insert "\n")
    ekg-20250313.436.tar-    (move-overlay o (point-min) (- (overlay-end o) 1))

AFAICT, again this wouldn't be significantly affected either.

    elisp-demos-20240128.810.tar-(with-temp-buffer
    elisp-demos-20240128.810.tar-  (insert "hello")
    elisp-demos-20240128.810.tar:  (replace-region-contents (point-min) (point-max) (lambda () '"world"))
    elisp-demos-20240128.810.tar-  (buffer-string))
    elisp-demos-20240128.810.tar-#+END_SRC
    elisp-demos-20240128.810.tar-
    elisp-demos-20240128.810.tar-#+RESULTS:
    elisp-demos-20240128.810.tar-: "world"
    elisp-demos-20240128.810.tar-

Not affected.

    hindent-20241128.1601.tar-                  (progn
    hindent-20241128.1601.tar-                    (if (fboundp 'replace-region-contents)
    hindent-20241128.1601.tar:                        (replace-region-contents
    hindent-20241128.1601.tar-                         beg end (lambda () temp))
    hindent-20241128.1601.tar-                      (let ((line (line-number-at-pos))
    hindent-20241128.1601.tar-                            (col (current-column)))
    hindent-20241128.1601.tar-                        (delete-region beg
    hindent-20241128.1601.tar-                                       end)
    hindent-20241128.1601.tar-                        (insert new-str)))
    hindent-20241128.1601.tar-                    (message "Formatted."))
    hindent-20241128.1601.tar-                  (message "Already formatted.")))))))))))

This one would be affected a bit: the package passes the region through
an external indenter/prettyprinter, so the use of careful replacement
to better preserves the markers in the mark-ring is relevant.
As the code shows, it's not the end of the world: not only it still has
the fallback insert+delete code but that fallback wasn't even careful to do
the insertion before the delete.

Also, this use case would be trivial to fix by passing the additional
MAX-SECS argument, of course (there is no backward compatibility in
this respect: all version of `replace-region-contents` have accepted
the MAX-SECS arg, contrary to `replace-buffer-contents`).

    jet-20240730.1228.tar-(defun jet-paste-cursor (thing &optional args)
    jet-20240730.1228.tar-  "Run jet for THING at cursor and ARGS pasting to current buffer."
    jet-20240730.1228.tar-  (interactive (jet-menu--interactive-args))
    jet-20240730.1228.tar-  (let ((result (string-trim (jet-menu--run thing args))))
    jet-20240730.1228.tar-    (if (use-region-p)
    jet-20240730.1228.tar:        (replace-region-contents (region-beginning) (region-end) (lambda () result))
    jet-20240730.1228.tar-      (insert result))))

This runs the `jet` command which converts data between different
representations (JSON/EDN/Transit?), so it's similar to hindent above:
it's nice to preserve things like mark-ring positions along the way, but
it's not the end of the world and shouldn't result in actual breakage,
just a slightly less refined behavior, which can be recovered by
a trivial change to the code.

    org-mpv-notes-20241222.1958.tar-(defun org-mpv-notes-replace-timestamp-with-link (begin end link)
    org-mpv-notes-20241222.1958.tar-  "Convert hh:mm:ss text within region to link with timestamp.
    org-mpv-notes-20241222.1958.tar-Region is between `BEGIN' and `END' points,
    org-mpv-notes-20241222.1958.tar-`LINK' is the new media url/path."
    org-mpv-notes-20241222.1958.tar-  (interactive "r\nsLink:")
    org-mpv-notes-20241222.1958.tar-  (save-excursion
    org-mpv-notes-20241222.1958.tar-    (let (timestamp)
    org-mpv-notes-20241222.1958.tar-      (setq link (org-link-escape link))
    org-mpv-notes-20241222.1958.tar-      (goto-char end)
    org-mpv-notes-20241222.1958.tar-      (while (re-search-backward "[^0-9]\\([0-9]+:[0-9]+:[0-9]+\\)" begin t)
    org-mpv-notes-20241222.1958.tar-        (setq timestamp (match-string 1))
    org-mpv-notes-20241222.1958.tar:        (replace-region-contents (match-beginning 1) (match-end 1)
    org-mpv-notes-20241222.1958.tar-                                 (lambda () (concat "[[mpv:" link "::" timestamp "][" timestamp "]]")))
    org-mpv-notes-20241222.1958.tar-        (search-backward "[[" begin t)))))

AFAICT this code should not affected by my proposed change.