Package: emacs;
Reported by: Stefan Monnier <monnier <at> iro.umontreal.ca>
Date: Sat, 15 Feb 2025 22:19:02 UTC
Severity: normal
Tags: patch
Message #120 received at 76313 <at> debbugs.gnu.org (full text, mbox):
From: Stefan Monnier <monnier <at> iro.umontreal.ca> To: Eli Zaretskii <eliz <at> gnu.org> Cc: phst <at> google.com, 76313 <at> debbugs.gnu.org, Stefan Kangas <stefankangas <at> gmail.com>, tsdh <at> gnu.org Subject: Re: bug#76313: New function `replace-region` 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.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.