bug#41445: 26.3; Query-replace triggers "match data clobbered by..."

bug#41445: 26.3; Query-replace triggers "match data clobbered by..."

[Ture Pålsson]

1. Run macOS (Catalina, in my case, but I don't think it matters.)

2. Have a folder named "ä" (that's a single character, U+00E4)
   and a file in it.

3. Visit that file.

4. While the file is not modified, do a query-replace that will replace
   something.

5. When Emacs asks about the first replacement, type 'y'.

==> You get an error message, "Match data clobbered by buffer
modification hooks".

Setting before-change-functions, after-change-functions, and
first-change-hook all to nil does not make the problem go away.

However, setting inhibit-modification-hooks to t *does* make it go away.

Running Emacs in a debugger, I notice that the first modification of the
buffer calls lock_file, which calls code_convert_string which, through a
long series of Emacs Lisp calls end up calling re-search-forward. I have
not tried to unravel this call chain, but:

The function ucs-normalize-region calls re-search-forward. If I wrap
that call in save-match-data, the problem goes away!


In GNU Emacs 26.3 (build 1, x86_64-apple-darwin18.2.0, NS appkit-1671.20 Version 10.14.3 (Build 18D109))
 of 2019-09-02 built on builder10-14.porkrind.org
Windowing system distributor 'Apple', version 10.3.1894
Recent messages:
Mark saved where search started
Mark set


Mark saved where search started
Mark set
Mark saved where search started
Making completion list...
Quit
Auto-saving...
Saving file /Users/ture/Desktop/emacs/lisp/international/ucs-normalize.el...
Wrote /Users/ture/Desktop/emacs/lisp/international/ucs-normalize.el
Quit
Configured using:
 'configure --with-ns '--enable-locallisppath=/Library/Application
 Support/Emacs/${version}/site-lisp:/Library/Application
 Support/Emacs/site-lisp' --with-modules'

Configured features:
NOTIFY ACL GNUTLS LIBXML2 ZLIB TOOLKIT_SCROLL_BARS NS MODULES THREADS

Important settings:
  value of $LC_CTYPE: sv_SE.UTF-8
  value of $LANG: en_SE.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Emacs-Lisp

Minor modes in effect:
  shell-dirtrack-mode: t
  bug-reference-prog-mode: t
  diff-auto-refine-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny format-spec rfc822 mml
mml-sec epa derived epg gnus-util rmail rmail-loaddefs mm-decode
mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader
sendmail rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils
completion pulse find-dired semantic/fw xref project dired-aux dired
dired-loaddefs ibuf-ext ibuffer ibuffer-loaddefs loadhist mode-local
find-func apropos shell pcomplete grep compile comint ring bug-reference
map cc-mode cc-fonts cc-guess cc-menus cc-cmds cc-styles cc-align
cc-engine cc-vars cc-defs pp cl-print thingatpt help-fns radix-tree
jka-compr info misearch multi-isearch vc-git diff-mode easy-mmode view
elec-pair ansi-color iso-transl cl-extra help-mode cl finder-inf package
easymenu epg-config url-handlers url-parse auth-source cl-seq eieio
eieio-core cl-macs eieio-loaddefs password-cache url-vars seq byte-opt
gv bytecomp byte-compile cconv cl-loaddefs cl-lib time-date tooltip
eldoc electric uniquify ediff-hook vc-hooks lisp-float-type mwheel
term/ns-win ns-win ucs-normalize mule-util term/common-win tool-bar dnd
fontset image regexp-opt fringe tabulated-list replace newcomment
text-mode elisp-mode lisp-mode prog-mode register page menu-bar
rfn-eshadow isearch timer select scroll-bar mouse jit-lock font-lock
syntax facemenu font-core term/tty-colors frame cl-generic cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese eucjp-ms cp51932 hebrew greek romanian slovak czech european
ethiopic indian cyrillic chinese composite charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray minibuffer
cl-preloaded nadvice loaddefs button faces cus-face macroexp files
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget hashtable-print-readable backquote threads kqueue cocoa ns
multi-tty make-network-process emacs)

Memory information:
((conses 16 411912 23871)
 (symbols 48 28246 5)
 (miscs 40 341 2088)
 (strings 32 61990 7915)
 (string-bytes 1 1687293)
 (vectors 16 52073)
 (vector-slots 8 1712291 139060)
 (floats 8 71 567)
 (intervals 56 32656 600)
 (buffers 992 37))

bug#41445: 26.3; Query-replace triggers "match data clobbered by..."

[Mattias Engdegård]

[-- Attachment #1: Type: text/plain, Size: 110 bytes --]

tags 41445 patch
stop

Thank you! Clearly nobody expects normalisation functions to clobber match data.


[-- Attachment #2: 0001-Don-t-clobber-match-data-in-Unicode-normalisation-bu.patch --]
[-- Type: application/octet-stream, Size: 3695 bytes --]

From 851cbe5507e872d8649d457889c4f87395bd63c2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mattias=20Engdeg=C3=A5rd?= <mattiase@acm.org>
Date: Fri, 22 May 2020 12:21:28 +0200
Subject: [PATCH] Don't clobber match data in Unicode normalisation (bug#41445)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Suggested by Ture Pålsson.

* lisp/international/ucs-normalize.el (ucs-normalize-region):
Wrap regexp searches in save-match-data.
* test/lisp/international/ucs-normalize-tests.el
(ucs-normalize-save-match-data): New test.
---
 lisp/international/ucs-normalize.el           | 33 ++++++++++---------
 .../lisp/international/ucs-normalize-tests.el | 11 +++++++
 2 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/lisp/international/ucs-normalize.el b/lisp/international/ucs-normalize.el
index 201ff6b9b1..6bc08c247b 100644
--- a/lisp/international/ucs-normalize.el
+++ b/lisp/international/ucs-normalize.el
@@ -511,22 +511,23 @@ ucs-normalize-region
 COMPOSITION-PREDICATE will be used to compose region."
   (save-excursion
     (save-restriction
-      (narrow-to-region from to)
-      (goto-char (point-min))
-      (let (start-pos starter)
-        (while (re-search-forward quick-check-regexp nil t)
-          (setq starter (string-to-char (match-string 0)))
-          (setq start-pos (match-beginning 0))
-          (ucs-normalize-block
-           ;; from
-           (if (or (= start-pos (point-min))
-                   (and (= 0 (ucs-normalize-ccc starter))
-                        (not (memq starter ucs-normalize-combining-chars))))
-               start-pos (1- start-pos))
-           ;; to
-           (if (looking-at ucs-normalize-combining-chars-regexp)
-               (match-end 0) (1+ start-pos))
-           translation-table composition-predicate))))))
+      (save-match-data
+        (narrow-to-region from to)
+        (goto-char (point-min))
+        (let (start-pos starter)
+          (while (re-search-forward quick-check-regexp nil t)
+            (setq starter (string-to-char (match-string 0)))
+            (setq start-pos (match-beginning 0))
+            (ucs-normalize-block
+             ;; from
+             (if (or (= start-pos (point-min))
+                     (and (= 0 (ucs-normalize-ccc starter))
+                          (not (memq starter ucs-normalize-combining-chars))))
+                 start-pos (1- start-pos))
+             ;; to
+             (if (looking-at ucs-normalize-combining-chars-regexp)
+                 (match-end 0) (1+ start-pos))
+             translation-table composition-predicate)))))))
 
 ;; --------------------------------------------------------------------------------
 
diff --git a/test/lisp/international/ucs-normalize-tests.el b/test/lisp/international/ucs-normalize-tests.el
index c36808ad72..2c60bd318a 100644
--- a/test/lisp/international/ucs-normalize-tests.el
+++ b/test/lisp/international/ucs-normalize-tests.el
@@ -341,4 +341,15 @@ ucs-normalize-check-failing-lines
           (display-buffer (current-buffer)))
       (message "No changes to failing lines needed"))))
 
+(ert-deftest ucs-normalize-save-match-data ()
+  "Verify that match data isn't clobbered (bug#41445)"
+  (string-match (rx (+ digit)) "a47b")
+  (should (equal (match-data t) '(1 3)))
+  (should (equal
+           (decode-coding-string
+            (encode-coding-string "Käsesoßenrührlöffel" 'utf-8-hfs)
+            'utf-8-hfs)
+           "Käsesoßenrührlöffel"))
+  (should (equal (match-data t) '(1 3))))
+
 ;;; ucs-normalize-tests.el ends here
-- 
2.21.1 (Apple Git-122.3)

bug#41445: 26.3; Query-replace triggers "match data clobbered by..."

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Fri, 22 May 2020 12:46:03 +0200
> Cc: 41445@debbugs.gnu.org
> 
> Thank you! Clearly nobody expects normalisation functions to clobber match data.

Is that the right place to save-match-data, though?  Should we perhaps
do that where utf-8-hfs file names are encoded?

Thanks.

bug#41445: 26.3; Query-replace triggers "match data clobbered by..."

[Mattias Engdegård]

22 maj 2020 kl. 13.11 skrev Eli Zaretskii <eliz@gnu.org>:

> Is that the right place to save-match-data, though?  Should we perhaps
> do that where utf-8-hfs file names are encoded?

What location did you have in mind, more precisely?

bug#41445: 26.3; Query-replace triggers "match data clobbered by..."

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Fri, 22 May 2020 13:16:04 +0200
> Cc: ture@turepalsson.se, 41445@debbugs.gnu.org
> 
> 22 maj 2020 kl. 13.11 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > Is that the right place to save-match-data, though?  Should we perhaps
> > do that where utf-8-hfs file names are encoded?
> 
> What location did you have in mind, more precisely?

I think in ucs-normalize-hfs-nfd-post-read-conversion and
ucs-normalize-hfs-nfd-pre-write-conversion.  IOW, so that this only
affects encoding and decoding macOS file names.  WDYT?

bug#41445: 26.3; Query-replace triggers "match data clobbered by..."

[Mattias Engdegård]

22 maj 2020 kl. 14.07 skrev Eli Zaretskii <eliz@gnu.org>:

> I think in ucs-normalize-hfs-nfd-post-read-conversion and
> ucs-normalize-hfs-nfd-pre-write-conversion.  IOW, so that this only
> affects encoding and decoding macOS file names.  WDYT?

Both of these call ucs-normalize-region, which makes it natural to save match data in that function. Conversely, anything that calls ucs-normalize-region would have to be wrapped, not just the two functions you mentioned. In other words, there seems to be no advantage in saving the match data in those functions, only disadvantages.

bug#41445: 26.3; Query-replace triggers "match data clobbered by..."

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Fri, 22 May 2020 14:21:04 +0200
> Cc: Ture Pålsson <ture@turepalsson.se>, 41445@debbugs.gnu.org
> 
> 22 maj 2020 kl. 14.07 skrev Eli Zaretskii <eliz@gnu.org>:
> 
> > I think in ucs-normalize-hfs-nfd-post-read-conversion and
> > ucs-normalize-hfs-nfd-pre-write-conversion.  IOW, so that this only
> > affects encoding and decoding macOS file names.  WDYT?
> 
> Both of these call ucs-normalize-region, which makes it natural to save match data in that function. Conversely, anything that calls ucs-normalize-region would have to be wrapped, not just the two functions you mentioned. In other words, there seems to be no advantage in saving the match data in those functions, only disadvantages.

My line of reasoning was that only the callers of ENCODE_FILE and
DECODE_FILE will not expect the match-data to be clobbered.  Code that
calls ucs-normalize-region directly may or may not be bothered by the
clobbering, so we should leave that to the caller.

The advantage of not doing this unconditionally is that we don't
unnecessarily punishing callers that don't need match-data to be
saved.

bug#41445: 26.3; Query-replace triggers "match data clobbered by..."

[Mattias Engdegård]

> My line of reasoning was that only the callers of ENCODE_FILE and
> DECODE_FILE will not expect the match-data to be clobbered.  Code that
> calls ucs-normalize-region directly may or may not be bothered by the
> clobbering, so we should leave that to the caller.
> 
> The advantage of not doing this unconditionally is that we don't
> unnecessarily punishing callers that don't need match-data to be
> saved.

For callers of the ucs-normalize- functions, correctness should come first; their names, semantics or descriptions do not lead the user to suspect them of clobbering the match data. It is an implementation leakage which can be quite unexpected, as is witnessed by this bug.

For example, in one of the few calls I could find at all, this might be classified as a near-miss, only saved by the left-to-right argument evaluation order:


                (lookup-new-entry 
                 'regular dictionary (match-string 0)
                 (ucs-normalize-HFS-NFC-string (match-string 1))))

bug#41445: 26.3; Query-replace triggers "match data clobbered by..."

[Eli Zaretskii]

> From: Mattias Engdegård <mattiase@acm.org>
> Date: Sat, 23 May 2020 13:36:37 +0200
> Cc: ture@turepalsson.se, 41445@debbugs.gnu.org
> 
> > My line of reasoning was that only the callers of ENCODE_FILE and
> > DECODE_FILE will not expect the match-data to be clobbered.  Code that
> > calls ucs-normalize-region directly may or may not be bothered by the
> > clobbering, so we should leave that to the caller.
> > 
> > The advantage of not doing this unconditionally is that we don't
> > unnecessarily punishing callers that don't need match-data to be
> > saved.
> 
> For callers of the ucs-normalize- functions, correctness should come first; their names, semantics or descriptions do not lead the user to suspect them of clobbering the match data. It is an implementation leakage which can be quite unexpected, as is witnessed by this bug.

I thought we had some advice to Lisp programs not to assume that
match-data will be preserved, but maybe I'm misremembering.  Stefan,
do you remember something along these lines?

Anyway, if you feel strongly about doing this on the lowest level, I
won't fight.

bug#41445: 26.3; Query-replace triggers "match data clobbered by..."

[Philipp Stephani]

Am Sa., 23. Mai 2020 um 14:29 Uhr schrieb Eli Zaretskii <eliz@gnu.org>:
>
> > From: Mattias Engdegård <mattiase@acm.org>
> > Date: Sat, 23 May 2020 13:36:37 +0200
> > Cc: ture@turepalsson.se, 41445@debbugs.gnu.org
> >
> > > My line of reasoning was that only the callers of ENCODE_FILE and
> > > DECODE_FILE will not expect the match-data to be clobbered.  Code that
> > > calls ucs-normalize-region directly may or may not be bothered by the
> > > clobbering, so we should leave that to the caller.
> > >
> > > The advantage of not doing this unconditionally is that we don't
> > > unnecessarily punishing callers that don't need match-data to be
> > > saved.
> >
> > For callers of the ucs-normalize- functions, correctness should come first; their names, semantics or descriptions do not lead the user to suspect them of clobbering the match data. It is an implementation leakage which can be quite unexpected, as is witnessed by this bug.
>
> I thought we had some advice to Lisp programs not to assume that
> match-data will be preserved, but maybe I'm misremembering.  Stefan,
> do you remember something along these lines?

That's at least what the manual says
(https://www.gnu.org/software/emacs/manual/html_node/elisp/Match-Data.html):
"Notice that all functions are allowed to overwrite the match data
unless they're explicitly documented not to do so."
(Not that I like that statement, but it is current reality.)

bug#41445: 26.3; Query-replace triggers "match data clobbered by..."

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Sat, 23 May 2020 14:37:17 +0200
> Cc: Mattias Engdegård <mattiase@acm.org>, 
> 	Stefan Monnier <monnier@iro.umontreal.ca>, ture@turepalsson.se, 41445@debbugs.gnu.org
> 
> > I thought we had some advice to Lisp programs not to assume that
> > match-data will be preserved, but maybe I'm misremembering.  Stefan,
> > do you remember something along these lines?
> 
> That's at least what the manual says
> (https://www.gnu.org/software/emacs/manual/html_node/elisp/Match-Data.html):
> "Notice that all functions are allowed to overwrite the match data
> unless they're explicitly documented not to do so."
> (Not that I like that statement, but it is current reality.)

Right, thanks.  I missed that.

bug#41445: 26.3; Query-replace triggers "match data clobbered by..."

[Mattias Engdegård]

23 maj 2020 kl. 14.37 skrev Philipp Stephani <p.stephani2@gmail.com>:

> "Notice that all functions are allowed to overwrite the match data
> unless they're explicitly documented not to do so."
> (Not that I like that statement, but it is current reality.)

Thanks for the reference. Nevertheless, functions do use save-match-data for the benefit of their callers every now and then. The practice is fairly widespread, more so for functions that are otherwise side-effect-free. It's a matter of reasonable expectation, not following the manual to the letter.

For that matter, there are few functions explicitly documented not to clobber the match data, not counting the automatically inserted statement for functions marked pure or side-effect-free.

bug#41445: 26.3; Query-replace triggers "match data clobbered by..."

[Stefan Monnier]

>> > The advantage of not doing this unconditionally is that we don't
>> > unnecessarily punishing callers that don't need match-data to be
>> > saved.
>> 
>> For callers of the ucs-normalize- functions, correctness should come
>> first; their names, semantics or descriptions do not lead the user to
>> suspect them of clobbering the match data. It is an implementation leakage
>> which can be quite unexpected, as is witnessed by this bug.
>
> I thought we had some advice to Lisp programs not to assume that
> match-data will be preserved, but maybe I'm misremembering.  Stefan,
> do you remember something along these lines?

Right, we follow a convention where, by default, any function can
clobber the match-data, with just some exceptions (typically,
small/trivial functions).

From that point of view, I see no reason why ucs-normalize-* should be
careful to preserve the match data.

This said, *if* it is the case that many/most calls to a given function
need to preserve the match-data around calls to it, it's of course
OK to simply move the match-data-saving into that function, especially
if that function's work dwarfs that of save-match-data.

I just added the following to `save-match-data`'s docstring, to try and
clarify:

    NOTE: The convention in Elisp is that any function, except for a few
    exceptions like car/assoc/+/goto-char, can clobber the match data,
    so `save-match-data' should normally be used to save *your* match
    data rather than your caller's match data.


-- Stefan

bug#41445: 26.3; Query-replace triggers "match data clobbered by..."

[Drew Adams]

>     NOTE: The convention in Elisp is that any function, except for a
>     few exceptions like car/assoc/+/goto-char, can clobber the match
>     data, so `save-match-data' should normally be used to save *your* 
>     match data rather than your caller's match data.

+1.  I like that, especially the last phrase.
Clear; important to get across.

bug#41445: 26.3; Query-replace triggers "match data clobbered by..."

[Mattias Engdegård]

23 maj 2020 kl. 15.36 skrev Stefan Monnier <monnier@iro.umontreal.ca>:

> From that point of view, I see no reason why ucs-normalize-* should be
> careful to preserve the match data.

Very well, since there appears to be a consensus for that view, I've pushed a change to the utf-8-hfs coding only.

bug#41445: 26.3; Query-replace triggers "match data clobbered by..."

[Eli Zaretskii]

> Feedback-ID:mattiase@acm.or
> From: Mattias Engdegård <mattiase@acm.org>
> Date: Wed, 27 May 2020 16:31:42 +0200
> Cc: Eli Zaretskii <eliz@gnu.org>,
>         Ture Pålsson <ture@turepalsson.se>,
>         41445-done@debbugs.gnu.org
> 
> 23 maj 2020 kl. 15.36 skrev Stefan Monnier <monnier@iro.umontreal.ca>:
> 
> > From that point of view, I see no reason why ucs-normalize-* should be
> > careful to preserve the match data.
> 
> Very well, since there appears to be a consensus for that view, I've pushed a change to the utf-8-hfs coding only.

Thanks.