bug#21766: 25.0.50; delete-trailing-whitespace sometimes deletes non-whitespace

bug#21766: 25.0.50; delete-trailing-whitespace sometimes deletes non-whitespace

[Markus Triska]

Steps to reproduce:

1) wget http://www.metalevel.at/ei/fault1.html

2) emacs -Q fault1.html

3) M-x delete-trailing-whitespace RET

This removes several non-white-space characters, including the whole
line containing "and use it like" (line 83 in the original file),
yielding the following diff against the original file:

   http://www.metalevel.at/ei/fault1.patch

Subtle changes in different sections of the file cause different
behaviour of `delete-trailing-whitespace'. For example, if I remove the
initial paragraph (patch: http://www.metalevel.at/ei/fault12.patch) to
obtain the slightly altered file:

   http://www.metalevel.at/ei/fault2.html

Then the analogous steps, using fault2.html ($ emacs -Q fault2.html,
M-x delete-trailing-whitespace RET) work as expected, removing only
trailing whitespace characters and leading to the diff:

   http://www.metalevel.at/ei/fault2.patch

All steps work as expected in Emacs 24.5, removing only trailing
whitespace characters in all cases.

Thank you for looking into this!
Markus


In GNU Emacs 25.0.50.5 (x86_64-apple-darwin14.1.0, X toolkit, Xaw3d scroll bars)
 of 2015-10-04
Repository revision: acfb5cd0353406784f085ddb6edfb0d0587048c8
Windowing system distributor 'The X.Org Foundation', version 11.0.11502000
Configured using:
 'configure --without-ns CFLAGS=-I/opt/local/include
 LDFLAGS=-L/opt/local/lib'

Configured features:
XAW3D XPM JPEG TIFF GIF PNG IMAGEMAGICK GSETTINGS NOTIFY ACL GNUTLS
LIBXML2 FREETYPE XFT ZLIB TOOLKIT_SCROLL_BARS LUCID X11

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

bug#21766: 25.0.50; delete-trailing-whitespace sometimes deletes non-whitespace

[Juanma Barranquero]

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

On Tue, Oct 27, 2015 at 12:07 AM, Markus Triska <triska@metalevel.at> wrote:
>
>
> Steps to reproduce:
>
> 1) wget http://www.metalevel.at/ei/fault1.html
>
> 2) emacs -Q fault1.html
>
> 3) M-x delete-trailing-whitespace RET
>
> This removes several non-white-space characters, including the whole
> line containing "and use it like" (line 83 in the original file),

Apparently, the call to skip-syntax-backward is altering the match data. If
you try this

--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -609,7 +609,7 @@ delete-trailing-whitespace
             (start (or start (point-min))))
         (goto-char start)
         (while (re-search-forward "\\s-$" end-marker t)
-          (skip-syntax-backward "-" (line-beginning-position))
+          (save-match-data (skip-syntax-backward "-"
(line-beginning-position)))
           ;; Don't delete formfeeds, even if they are considered
whitespace.
           (if (looking-at-p ".*\f")
               (goto-char (match-end 0)))


or, alternatively,

@@ -609,11 +609,12 @@ delete-trailing-whitespace
             (start (or start (point-min))))
         (goto-char start)
         (while (re-search-forward "\\s-$" end-marker t)
-          (skip-syntax-backward "-" (line-beginning-position))
-          ;; Don't delete formfeeds, even if they are considered
whitespace.
-          (if (looking-at-p ".*\f")
-              (goto-char (match-end 0)))
-          (delete-region (point) (match-end 0)))
+          (let ((end (match-end 0)))
+            (skip-syntax-backward "-" (line-beginning-position))
+            ;; Don't delete formfeeds, even if they are considered
whitespace.
+            (if (looking-at-p ".*\f")
+                (goto-char end))
+            (delete-region (point) end)))
         ;; Delete trailing empty lines.
         (goto-char end-marker)
         (when (and (not end)

it works. Now, the question is, should skip-syntax-backward preserve the
match data, or must delete-trailing-whitespace be coded defensively? The
usual policy has been that code that needs the match data preserved should
make sure itself that it is so.

[-- Attachment #2: Type: text/html, Size: 2981 bytes --]

bug#21766: 25.0.50; delete-trailing-whitespace sometimes deletes non-whitespace

[Andreas Röhler]

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

  On 27.10.2015 01:15, Juanma Barranquero wrote:
> On Tue, Oct 27, 2015 at 12:07 AM, Markus Triska <triska@metalevel.at 
> <mailto:triska@metalevel.at>> wrote:
> >
> >
> > Steps to reproduce:
> >
> > 1) wget http://www.metalevel.at/ei/fault1.html
> >
> > 2) emacs -Q fault1.html
> >
> > 3) M-x delete-trailing-whitespace RET
> >
> > This removes several non-white-space characters, including the whole
> > line containing "and use it like" (line 83 in the original file),
>
> Apparently, the call to skip-syntax-backward is altering the match 
> data. If you try this
>
> --- a/lisp/simple.el
> +++ b/lisp/simple.el
> @@ -609,7 +609,7 @@ delete-trailing-whitespace
>              (start (or start (point-min))))
>          (goto-char start)
>          (while (re-search-forward "\\s-$" end-marker t)
> -          (skip-syntax-backward "-" (line-beginning-position))
> +          (save-match-data (skip-syntax-backward "-" 
> (line-beginning-position)))
>            ;; Don't delete formfeeds, even if they are considered 
> whitespace.
>            (if (looking-at-p ".*\f")
>                (goto-char (match-end 0)))
>
>
> or, alternatively,
>
> @@ -609,11 +609,12 @@ delete-trailing-whitespace
>              (start (or start (point-min))))
>          (goto-char start)
>          (while (re-search-forward "\\s-$" end-marker t)
> -          (skip-syntax-backward "-" (line-beginning-position))
> -          ;; Don't delete formfeeds, even if they are considered 
> whitespace.
> -          (if (looking-at-p ".*\f")
> -              (goto-char (match-end 0)))
> -          (delete-region (point) (match-end 0)))
> +          (let ((end (match-end 0)))
> +            (skip-syntax-backward "-" (line-beginning-position))
> +            ;; Don't delete formfeeds, even if they are considered 
> whitespace.
> +            (if (looking-at-p ".*\f")
> +                (goto-char end))
> +            (delete-region (point) end)))
>          ;; Delete trailing empty lines.
>          (goto-char end-marker)
>          (when (and (not end)
>
> it works.

First fix looks cleaner.

> Now, the question is, should skip-syntax-backward preserve the match 
> data, or must delete-trailing-whitespace be coded defensively? The 
> usual policy has been that code that needs the match data preserved 
> should make sure itself that it is so.

Such a basic routine should preserve matches by its own virtue.

A couple of regression tests should pay here.

Thanks

[-- Attachment #2: Type: text/html, Size: 4541 bytes --]

bug#21766: 25.0.50; delete-trailing-whitespace sometimes deletes non-whitespace

[Juanma Barranquero]

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

On Tue, Oct 27, 2015 at 8:07 AM, Andreas Röhler <
andreas.roehler@easy-emacs.de> wrote:

> First fix looks cleaner.

Shorter, indeed, though it does more work, and inside a loop. I really have
no preference.

> Such a basic routine should preserve matches by its own virtue.

Are you talking about delete-trailing-whitespace or skip-syntax-backward?

Anyway, I've always thought that functions should preserve the match data,
but I was told that doing so is a (relatively) expensive operation, and
functions that use match data should be the ones to make sure that it is
valid when they need it.

    J

[-- Attachment #2: Type: text/html, Size: 817 bytes --]

bug#21766: 25.0.50; delete-trailing-whitespace sometimes deletes non-whitespace

[martin rudalics]

 > Now, the question is, should skip-syntax-backward preserve the
 > match data, or must delete-trailing-whitespace be coded defensively?

The latter.

 > The
 > usual policy has been that code that needs the match data preserved should
 > make sure itself that it is so.

Shouldn't we remove that "\\s-$" rigmarole?  Something like the largely
untested

       (let ((end-marker (copy-marker (or end (point-max))))
	    old-bol new-eol new-bol)
         (goto-char (or start (point-min)))
         (while (re-search-forward "\\\n" end-marker t)
	  (setq new-bol (point))
	  (goto-char (setq new-eol (match-beginning 0)))
           (if (or (zerop (skip-syntax-backward
			  "-" (or old-bol (line-beginning-position))))
		  (looking-at-p ".*\f"))
	      (goto-char new-bol)
	    (delete-region (point) new-eol)
	    (forward-char))
	  (setq old-bol (point)))

It's still not very clean (should line-beginning-position be allowed to
go before START?) so if someone wants to polish it up ....

martin

bug#21766: 25.0.50; delete-trailing-whitespace sometimes deletes non-whitespace

[Juanma Barranquero]

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

On Tue, Oct 27, 2015 at 9:05 AM, martin rudalics <rudalics@gmx.at> wrote:

> The latter.

Yep, that's the policy.

> Shouldn't we remove that "\\s-quot; rigmarole?  Something like the largely
> untested

Hmm, what's the gain for the added complexity?

    J

[-- Attachment #2: Type: text/html, Size: 488 bytes --]

bug#21766: 25.0.50; delete-trailing-whitespace sometimes deletes non-whitespace

[martin rudalics]

 > Hmm, what's the gain for the added complexity?

Maybe none.  I have no idea how the regexp machine works when looking
for "\\s-$".  If it looks for whitespace followed by a line end and not
for whitespace preceding a line end, the old version will be slow with
indented code.

martin

bug#21766: 25.0.50; delete-trailing-whitespace sometimes deletes non-whitespace

[Juanma Barranquero]

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

On Tue, Oct 27, 2015 at 9:25 AM, martin rudalics <rudalics@gmx.at> wrote:

> If it looks for whitespace followed by a line end and not
> for whitespace preceding a line end, the old version will be slow with
> indented code.

I don't think we've ever had a bug report / complaint about
delete-trailing-whitespace's performance. It's usually used either
interactively or as a hook before saving the file, and in both cases I'd
bet the time it takes is dwarfed by human reaction times or by the time
spent saving the file to disk.

    J

[-- Attachment #2: Type: text/html, Size: 692 bytes --]

bug#21766: 25.0.50; delete-trailing-whitespace sometimes deletes non-whitespace

[Andreas Schwab]

Juanma Barranquero <lekktu@gmail.com> writes:

> Apparently, the call to skip-syntax-backward is altering the match
> data.

No, skip-syntax-backward is safe.  There must be something in
line-beginning-position that does this, but only indirectly.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

bug#21766: 25.0.50; delete-trailing-whitespace sometimes deletes non-whitespace

[Juanma Barranquero]

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

On Tue, Oct 27, 2015 at 10:01 AM, Andreas Schwab <schwab@suse.de> wrote:

> No, skip-syntax-backward is safe.  There must be something in
> line-beginning-position that does this, but only indirectly.

  (skip-syntax-backward "-"
        (save-match-data
(line-beginning-position)))

still shows the bug, while

  (save-match-data
    (skip-syntax-backward "-" (line-beginning-position)))

fixes it.

[-- Attachment #2: Type: text/html, Size: 776 bytes --]

bug#21766: 25.0.50; delete-trailing-whitespace sometimes deletes non-whitespace

[martin rudalics]

 >> No, skip-syntax-backward is safe.  There must be something in
 >> line-beginning-position that does this, but only indirectly.
 >
 >    (skip-syntax-backward "-"
 >          (save-match-data
 > (line-beginning-position)))
 >
 > still shows the bug, while
 >
 >    (save-match-data
 >      (skip-syntax-backward "-" (line-beginning-position)))
 >
 > fixes it.

I doubt that ‘syntax-propertize’ is always safe.

martin

bug#21766: 25.0.50; delete-trailing-whitespace sometimes deletes non-whitespace

[Andreas Schwab]

Juanma Barranquero <lekktu@gmail.com> writes:

> On Tue, Oct 27, 2015 at 10:01 AM, Andreas Schwab <schwab@suse.de> wrote:
>
>> No, skip-syntax-backward is safe.  There must be something in
>> line-beginning-position that does this, but only indirectly.
>
>   (skip-syntax-backward "-"
>         (save-match-data
> (line-beginning-position)))
>
> still shows the bug, while
>
>   (save-match-data
>     (skip-syntax-backward "-" (line-beginning-position)))
>
> fixes it.

Ok, I see it now, it is the new parse_sexp_propertize stuff which is
called by SETUP_SYNTAX_TABLE.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

bug#21766: 25.0.50; delete-trailing-whitespace sometimes deletes non-whitespace

[Juanma Barranquero]

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

On Tue, Oct 27, 2015 at 10:46 AM, Andreas Schwab <schwab@suse.de> wrote:

> Ok, I see it now, it is the new parse_sexp_propertize stuff which is
> called by SETUP_SYNTAX_TABLE.

Then, can I assume this is not a bug in skip-syntax-backward or
parse_sexp_propetize, and fix delete-trailing-whitespace?

[-- Attachment #2: Type: text/html, Size: 422 bytes --]

bug#21766: 25.0.50; delete-trailing-whitespace sometimes deletes non-whitespace

[Juanma Barranquero]

[-- Attachment #1.1: Type: text/plain, Size: 177 bytes --]

This will be my first ERT test, so, does the following (attached, because
Gmail) patch look acceptable?

And, it is possible to commit a file with trailing whitespace?
​

[-- Attachment #1.2: Type: text/html, Size: 234 bytes --]

[-- Attachment #2: bug-21766.patch --]
[-- Type: application/octet-stream, Size: 2277 bytes --]

diff --git c/lisp/simple.el i/lisp/simple.el
index 338a060..f6c580f 100644
--- c/lisp/simple.el
+++ i/lisp/simple.el
@@ -609,7 +609,8 @@ delete-trailing-whitespace
             (start (or start (point-min))))
         (goto-char start)
         (while (re-search-forward "\\s-$" end-marker t)
-          (skip-syntax-backward "-" (line-beginning-position))
+          (save-match-data
+            (skip-syntax-backward "-" (line-beginning-position)))
           ;; Don't delete formfeeds, even if they are considered whitespace.
           (if (looking-at-p ".*\f")
               (goto-char (match-end 0)))
diff --git c/test/automated/data/simple/bug-21766.py i/test/automated/data/simple/bug-21766.py
new file mode 100644
index 0000000..118719a
--- /dev/null
+++ i/test/automated/data/simple/bug-21766.py
@@ -0,0 +1,4 @@
+query = """WITH filtered AS 
+WHERE      
+""".format(fv_)
+
diff --git c/test/automated/simple-test.el i/test/automated/simple-test.el
index 8da575d..62fe17c 100644
--- c/test/automated/simple-test.el
+++ i/test/automated/simple-test.el
@@ -21,6 +21,10 @@
 
 (require 'ert)
 
+(defvar simple-tests-data-directory
+  (expand-file-name "data/simple" (getenv "EMACS_TEST_DIRECTORY"))
+  "Directory containing simple.el test data.")
+
 (defmacro simple-test--dummy-buffer (&rest body)
   (declare (indent 0)
            (debug t))
@@ -180,5 +184,23 @@ simple-test--dummy-buffer
           (should (= x 2)))
       (remove-hook 'post-self-insert-hook inc))))
 
+\f
+;;; `delete-trailing-whitespace'
+(ert-deftest simple-delete-trailing-whitespace ()
+  (defvar python-indent-guess-indent-offset)  ; to avoid a warning
+  (let ((python (featurep 'python))
+        (python-indent-guess-indent-offset nil)
+        (delete-trailing-lines t))
+    (unwind-protect
+        (with-temp-buffer
+          (python-mode)
+          (insert-file-contents (expand-file-name "bug-21766.py"
+                                                  simple-tests-data-directory))
+          (delete-trailing-whitespace)
+          (should (equal (count-lines (point-min) (point-max)) 3)))
+      ;; Let's clean up if running interactive
+      (unless (or noninteractive python)
+        (unload-feature 'python)))))
+
 (provide 'simple-test)
 ;;; simple-test.el ends here

bug#21766: 25.0.50; delete-trailing-whitespace sometimes deletes non-whitespace

[Stephen Leake]

Juanma Barranquero <lekktu@gmail.com> writes:

> This will be my first ERT test, so, does the following (attached, because
> Gmail) patch look acceptable?

Looks good to me, except see below.

> And, it is possible to commit a file with trailing whitespace?

Yes, but it's also very easy to lose it, thus breaking the test. I
suggest instead to save the file without extra whitespace, and add code
to the test to insert the extra whitespace. That way you can also add
comments about why this test exists, etc.

-- 
-- Stephe

bug#21766: 25.0.50; delete-trailing-whitespace sometimes deletes non-whitespace

[Juanma Barranquero]

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

On Wed, Oct 28, 2015 at 2:51 AM, Stephen Leake <
stephen_leake@stephe-leake.org> wrote:

> I suggest instead to save the file without extra whitespace, and add code
> to the test to insert the extra whitespace.

The file is quite short, so in this case is better to get rid of it and
just insert its contents into the buffer.

Thanks,

     Juanma



        Fix bug#21766 and add test.

        * lisp/simple.el (delete-trailing-whitespace): Save match data when
        calling `skip-syntax-backward'.
        * test/automated/simple-test.el (simple-delete-trailing-whitespace):
        New test.


diff --git a/lisp/simple.el b/lisp/simple.el
index 338a060..f6c580f 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -609,7 +609,8 @@ delete-trailing-whitespace
             (start (or start (point-min))))
         (goto-char start)
         (while (re-search-forward "\\s-$" end-marker t)
-          (skip-syntax-backward "-" (line-beginning-position))
+          (save-match-data
+            (skip-syntax-backward "-" (line-beginning-position)))
           ;; Don't delete formfeeds, even if they are considered
whitespace.
           (if (looking-at-p ".*\f")
               (goto-char (match-end 0)))
diff --git a/test/automated/simple-test.el b/test/automated/simple-test.el
index 8da575d..5bfb746 100644
--- a/test/automated/simple-test.el
+++ b/test/automated/simple-test.el
@@ -180,5 +180,27 @@ simple-test--dummy-buffer
           (should (= x 2)))
       (remove-hook 'post-self-insert-hook inc))))

+
+;;; `delete-trailing-whitespace'
+(ert-deftest simple-delete-trailing-whitespace ()
+  "Test bug#21766: delete-whitespace sometimes deletes non-whitespace."
+  (defvar python-indent-guess-indent-offset)  ; to avoid a warning
+  (let ((python (featurep 'python))
+        (python-indent-guess-indent-offset nil)
+        (delete-trailing-lines t))
+    (unwind-protect
+        (with-temp-buffer
+          (python-mode)
+          (insert (concat "query = \"\"\"WITH filtered AS \n"
+                          "WHERE      \n"
+                          "\"\"\".format(fv_)\n"
+                          "\n"
+                          "\n"))
+          (delete-trailing-whitespace)
+          (should (equal (count-lines (point-min) (point-max)) 3)))
+      ;; Let's clean up if running interactive
+      (unless (or noninteractive python)
+        (unload-feature 'python)))))
+
 (provide 'simple-test)
 ;;; simple-test.el ends here
-- 
2.6.2.windows.1

[-- Attachment #2: Type: text/html, Size: 3668 bytes --]

bug#21766: 25.0.50; delete-trailing-whitespace sometimes deletes non-whitespace

[Juanma Barranquero]

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

Fixed in commit 1f02cbea8b489ed7676110431aa36ad5abc47d9b
​

[-- Attachment #2: Type: text/html, Size: 334 bytes --]

bug#21766: 25.0.50; delete-trailing-whitespace sometimes deletes non-whitespace

[Stefan Monnier]

>> (save-match-data
>> (skip-syntax-backward "-" (line-beginning-position)))
[...]
> Ok, I see it now, it is the new parse_sexp_propertize stuff which is
> called by SETUP_SYNTAX_TABLE.

Indeed, internal--syntax-propertize needs to be fixed so it preserves
the match-data!


        Stefan

bug#21766: 25.0.50; delete-trailing-whitespace sometimes deletes non-whitespace

[Andreas Röhler]

  On 28.10.2015 20:19, Stefan Monnier wrote:
>>> (save-match-data
>>> (skip-syntax-backward "-" (line-beginning-position)))
> [...]
>> Ok, I see it now, it is the new parse_sexp_propertize stuff which is
>> called by SETUP_SYNTAX_TABLE.
> Indeed, internal--syntax-propertize needs to be fixed so it preserves
> the match-data!
>
>
>          Stefan
>
>
>
>
>

When internal--syntax-propertize is fixed, the current fix will be 
useless, i.e. redundant.

Is there a task-keeper for this scenario?

bug#21766: 25.0.50; delete-trailing-whitespace sometimes deletes non-whitespace

[Juanma Barranquero]

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

On Thu, Oct 29, 2015 at 7:30 AM, Andreas Röhler <
andreas.roehler@easy-emacs.de> wrote:

> When internal--syntax-propertize is fixed, the current fix will be
useless, i.e. redundant.

Stefan already reverted the fix to delete-trailing-whitespace and fixed
internal--syntax-propertize in d7a67c5a2fe63b6f087d6cae24c8f3b3c09eb57a

[-- Attachment #2: Type: text/html, Size: 474 bytes --]